590515 - Implement SHA-224/256/384/512 key derivation in Softoken

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This includes:

Mechanisms CKM_SHA224_KEY_DERIVATION, CKM_SHA256_KEY_DERIVATION, CKM_SHA384_KEY_DERIVATION, and CKM_SHA512_KEY_DERIVATION.

Support for CKD_SHA224_KDF, CKD_SHA256_KDF, CKD_SHA384_KDF, and CKD_SHA512_KDF support for the mechanisms that take these parameters.

I filed Bug 590514 for the iterative versions as NSS may soon need them internally:
CKD_SHA224_CONCATENATE_KDF, CKD_SHA256_CONCATENATE_KDF, CKD_SHA384_CONCATENATE_KDF, and CKD_SHA512_CONCATENATE_KDF.

Comment 1

[David Cooper]

Attachment: Implementation of SHA-224/256/384/512 key derivation in Softoken.

This is an implementation of CKM_SHA224_KEY_DERIVATION, CKM_SHA256_KEY_DERIVATION, CKM_SHA384_KEY_DERIVATION, and CKM_SHA512_KEY_DERIVATION.  It is needed for the implementation of CKD_SHA224_KDF, CKD_SHA256_KDF, CKD_SHA384_KDF, and CKD_SHA512_KDF in bug #676114.

Comment 2

[Robert Relyea]

Comment on attachment 635902 [details] [diff] [review]
Implementation of SHA-224/256/384/512 key derivation in Softoken.

r+ rrelyea

Comment 3

[David Cooper]

Attachment: Cleanup of SHA-2 key derivation implementations

While working on another patch, I discovered that the patch that I submitted for this bug does not compile cleanly.  The problem is that I reused the context variable "sha" to implement each of the key derivation mechanisms, but "sha" is defined as type "SHA1Context *" and so should only be used to implement the SHA-1 key derivation mechanism.  I believe that this mistake will only lead to compile time type mismatch warnings, since the issue just relates to different types of pointers, but I created this patch to correct the problem so that the compiler doesn't create any warnings.

Since the original patch has already been approved and is awaiting check-in, I created this as a patch to be applied against the first patch, but if you'd prefer I could create a single patch that combines the first patch and this one.

Comment 4

[Robert Relyea]

Comment on attachment 641979 [details] [diff] [review]
Cleanup of SHA-2 key derivation implementations

r+ rrelyea. This patch is a diff on the other approved patch.

bob

Comment 5

[Kai Engert [:KaiE:]]

Comment on attachment 635902 [details] [diff] [review]
Implementation of SHA-224/256/384/512 key derivation in Softoken.

Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.180; previous revision: 1.179
done
Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.127; previous revision: 1.126
done

Comment 6

[Kai Engert [:KaiE:]]

Comment on attachment 641979 [details] [diff] [review]
Cleanup of SHA-2 key derivation implementations

Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.128; previous revision: 1.127
done

Comment 7

[Wan-Teh Chang]

Attachment: Use the SHAx_HashBuf functions

David: could you review and test this patch?  Thanks.

I found that the Begin,Update,End sequences can be replaced
by the HashBuf functions.  Note that MD2_HashBuf doesn't exist,
so the CKM_MD2_KEY_DERIVATION case is left unchanged.

This code assumes the 'key_block' buffer is large enough to
hold the largest hash result (SHA-512).  I verified it manually.
It might be good to add a static assertion.

Also, I ignore the return value of SHAx_HashBuf because
those functions always return SECSuccess.  Because of
the freebl loader, they may actually fail, but the original
code has the same issue (SHAx_Begin, etc., which return void,
may fail silently).

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/freebl/loader.c&rev=1.57&mark=749-750,747#746

Comment 8

[David Cooper]

Comment on attachment 643104 [details] [diff] [review]
Use the SHAx_HashBuf functions

Review of attachment 643104 [details] [diff] [review]:
-----------------------------------------------------------------

I tested out the code in conjunction with my patch to implement the ANSI X9.63 key derivation function (bug #676114) and it seems to work fine.

In comparing the code, however, I did notice one difference.  After computing the hash, the current code calls the destroy context function (e.g., SHA256_DestroyContext), which zeros out the context variable.  Since both the input and the output of the hash is keying material, would it be a good idea to zero out the context variable?  If so, then this issue isn't limited to this patch.  My implementation of sftk_ANSI_X9_63_kdf (in softoken/pkcs11c.c) has the same issue since it uses (SHA1_HashBuf, SHA224_HashBuf, etc.) to implement the hash.  Would it make sense to address this in a general way by changing the *_HashBuf functions by adding a line at the end to zero out the context variable (e.g., memset(&ctx, 0, sizeof ctx))?

Comment 9

[David Cooper]

Comment on attachment 643104 [details] [diff] [review]
Use the SHAx_HashBuf functions

When combined with the patch submitted in bug #775337, I think this is an improvement over the current code.

Thanks,

Dave

Comment 10

[Wan-Teh Chang]

Comment on attachment 643104 [details] [diff] [review]
Use the SHAx_HashBuf functions

Patch checked in on the NSS trunk (NSS 3.14).

Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.129; previous revision: 1.128
done