[Dovecot] [PATCH] Generalize HMAC implementation
Hello everyone and Timo in particular,
about a year ago I implemented a SHA-1 variant of the HMAC(-MD5) present in Dovecot. I had always disliked this a bit, because it replicates a lot of code. This patch generalizes the HMAC function to take a hash_method struct as parameter, and changes existing code which uses the "old" HMAC function to use this new one.
I'm not really sure this is actually a good idea, but I still felt I should provide the code in case you would want to merge it upstream.
Attached is the patch as a hg export based on the revision of dovecot-2.2 current at the time of writing.
Regards, Florian Zeitz
On 4.9.2012, at 21.25, Florian Zeitz wrote:
Hello everyone and Timo in particular,
about a year ago I implemented a SHA-1 variant of the HMAC(-MD5) present in Dovecot. I had always disliked this a bit, because it replicates a lot of code. This patch generalizes the HMAC function to take a hash_method struct as parameter, and changes existing code which uses the "old" HMAC function to use this new one.
I'm not really sure this is actually a good idea, but I still felt I should provide the code in case you would want to merge it upstream.
It's otherwise good, but this isn't safe:
- ctx->ctx = t_malloc(meth->context_size);
- ctx->ctxo = t_malloc(meth->context_size);
It assumes that the hmac_init() + hmac_final() is called close to each others. I think we could simply #define the largest allowed context_size, use it for these buffers' sizes and then add i_assert(meth->context_size <= HMAC_MAX_CONTEXT_SIZE)
Am 11.09.2012 18:02, schrieb Timo Sirainen:
On 4.9.2012, at 21.25, Florian Zeitz wrote:
Hello everyone and Timo in particular,
about a year ago I implemented a SHA-1 variant of the HMAC(-MD5) present in Dovecot. I had always disliked this a bit, because it replicates a lot of code. This patch generalizes the HMAC function to take a hash_method struct as parameter, and changes existing code which uses the "old" HMAC function to use this new one.
I'm not really sure this is actually a good idea, but I still felt I should provide the code in case you would want to merge it upstream.
It's otherwise good, but this isn't safe:
- ctx->ctx = t_malloc(meth->context_size);
- ctx->ctxo = t_malloc(meth->context_size);
It assumes that the hmac_init() + hmac_final() is called close to each others. I had in fact noticed that. The assumption is currently true for all occurrences, and probably will remain such, but I agree it's better to be safe then sorry.
I think we could simply #define the largest allowed context_size, use it for these buffers' sizes and then add i_assert(meth->context_size <= HMAC_MAX_CONTEXT_SIZE)
Well, either that, or we could use a union of all known context structs there. Possibly plus an i_assert(meth->context_size <= sizeof(union hmac_ctxts)). Or we could use i_malloc() and i_free() under the assumption hmac_init()
- hmac_final() calls are always matched.
I've a certain preference for the union variant, but it's your call.
Regards, Florian
participants (2)
-
Florian Zeitz
-
Timo Sirainen