nisse@lysator.liu.se (Niels Möller) writes:
However, I suppose this complexity could be hidden with a utility function 'pbkdf2_hmac' that is similar to my original function signature.
Do you think a "half-general" pbkdf2_hmac function really is useful? I think specific wrapper functions, pbkdf2_hmac_sha256, etc, would be more useful.
I agree. PBKDF2-HMAC-SHA1 is the most important. If there is also support for PBKDF2-HMAC-SHA256 then all usual cases are handled. I would actually hesistate to add convenience functions for the rest since it may lead people to use them without thinking that there are costs in using a new variant. I feel even PBKDF2-HMAC-SHA256 is wasteful, there is nothing cryptographically wrong with PBKDF2-HMAC-SHA1, but that is an uphill battle to fight...
This is a problem shared with almost every function in nettle accepting function pointer arguments. I think we have to live with casts one way or the other; I don't see any good solution.
Aha. I hadn't realized that.
Casting the parameter would solve this. Is there any other way to resolve the warning? Do you think casting is an acceptable solution here? The problem seems to be that the casting needs to happen in the application not in the library.
At least specific functions like pbkdf2_hmac_sha256 functions will not expose the problem to users. It's also possible to write some macros to do the cast but preserve some type checking, something like
#define PBKDF2(ctx, update, digest, ...) \ (0 ? (update(ctx, 0, NULL), digest(ctx, 0, NULL)) \ : pbkdf(ctx, (nettle_hash_update_func *) update, (nettle_hash_digest_func *) digest, ..,))
The CBC_ENCRYPT and CBC_DECRYPT macros do this, and it seems to work fine, but it's not particularly pretty.
Any other ideas?
This approach seems fine to me.
Thanks for the implementation, I'll try to get it integrated soon. Some minor comments (which I can fix):
I can submit an updated patch if that would speed things up. Recall that the ChangeLog, manual etc was not updated for the new interface in my most recent patch, so there is some cleaning up to do.
--- a/nettle-internal.h +++ b/nettle-internal.h @@ -48,6 +48,7 @@ do { if (size > (sizeof(name) / sizeof(name[0]))) abort(); } while (0) #define NETTLE_MAX_BIGNUM_SIZE ((NETTLE_MAX_BIGNUM_BITS + 7)/8) #define NETTLE_MAX_HASH_BLOCK_SIZE 128 #define NETTLE_MAX_HASH_DIGEST_SIZE 64 +#define NETTLE_MAX_HASH_CONTEXT_SIZE 216 #define NETTLE_MAX_SEXP_ASSOC 17 #define NETTLE_MAX_CIPHER_BLOCK_SIZE 32
This is no longer needed, right?
Once the next issue is fixed, agreed.
+void +pbkdf2 (void *mac_ctx, unsigned digest_size,
- nettle_hash_update_func *update,
- nettle_hash_digest_func *digest,
- unsigned length, uint8_t *dst,
- unsigned iterations,
- unsigned salt_length, const uint8_t *salt)
+{
- char U[NETTLE_MAX_HASH_DIGEST_SIZE];
- char T[NETTLE_MAX_HASH_DIGEST_SIZE];
It would make sense to use TMP_ALLOC for those. And I think I'd use uint8_t rather than char (unless there's some reason for char I'm missing?).
Agreed. It was copy'n'paste from my old implementation.
tmp[0] = (i & 0xff000000) >> 24;tmp[1] = (i & 0x00ff0000) >> 16;tmp[2] = (i & 0x0000ff00) >> 8;tmp[3] = (i & 0x000000ff) >> 0;There's WRITE_UINT32 for this.
Great. Same reason here.
/Simon