Bug 554827 - Support additional pseudorandom functions for PKCS #5 PBKDF2

Bug 554827 - Support additional pseudorandom functions for PKCS #5 PBKDF2

[mt, 2015-10-19 16:24:42]

Where are the tests?

https://codereview.appspot.com/270310043/diff/1/lib/softoken/pkcs11c.c
File lib/softoken/pkcs11c.c (right):

https://codereview.appspot.com/270310043/diff/1/lib/softoken/pkcs11c.c#newcod...
lib/softoken/pkcs11c.c:3753: }
Nice

[ttaubert, 2015-10-20 10:47:14]

On 2015/10/19 16:24:42, mt wrote:
> Where are the tests?

Ah, sorry. Assured myself everything's working by writing tests for the
WebCrypto API but you're right, we of course need NSS tests too. Will update the
patch when I'm done.

[mt, 2015-11-09 05:55:21]

LGTM

https://codereview.appspot.com/270310043/diff/20001/external_tests/pk11_gtest...
File external_tests/pk11_gtest/pk11_pbkdf2_unittest.cc (right):

https://codereview.appspot.com/270310043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_pbkdf2_unittest.cc:31: const uint8_t
kTestVector2[] = {
Move this to the function that uses it.

https://codereview.appspot.com/270310043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_pbkdf2_unittest.cc:43: class Pkcs11Pbkdf2Test :
public ::testing::Test {
I don't see any testing of error paths here.

https://codereview.appspot.com/270310043/diff/20001/external_tests/pk11_gtest...
external_tests/pk11_gtest/pk11_pbkdf2_unittest.cc:92: Derive();
Pass the value in to Derive instead.  Then remove Init().  Pass as a const
reference as well.

https://codereview.appspot.com/270310043/diff/20001/lib/pk11wrap/pk11pbe.c
File lib/pk11wrap/pk11pbe.c (right):

https://codereview.appspot.com/270310043/diff/20001/lib/pk11wrap/pk11pbe.c#ne...
lib/pk11wrap/pk11pbe.c:808: switch (prfAlgTag) {
Hmm, tabs or spaces?

[wtc1, 2015-11-11 02:35:14]

Review comments on patch set 3:

Thanks a lot for writing this patch!

I found a bug in pkcs11t.h. I didn't review the tests.

https://codereview.appspot.com/270310043/diff/40001/lib/pk11wrap/pk11pbe.c
File lib/pk11wrap/pk11pbe.c (right):

https://codereview.appspot.com/270310043/diff/40001/lib/pk11wrap/pk11pbe.c#ne...
lib/pk11wrap/pk11pbe.c:641: prfAlg = SEC_OID_HMAC_SHA1;

Should we change the default to SEC_OID_HMAC_SHA256?
Perhaps we can't because we need to be backward
compatible?

https://codereview.appspot.com/270310043/diff/40001/lib/softoken/lowpbe.h
File lib/softoken/lowpbe.h (right):

https://codereview.appspot.com/270310043/diff/40001/lib/softoken/lowpbe.h#new...
lib/softoken/lowpbe.h:81: int iterator);
Nit: this is a pre-existing problem, so you can fix it
separately.

|iterator| is a poor parameter name. This should be named
|iteration| or |iterationCount|.

https://codereview.appspot.com/270310043/diff/40001/lib/softoken/pkcs11c.c
File lib/softoken/pkcs11c.c (right):

https://codereview.appspot.com/270310043/diff/40001/lib/softoken/pkcs11c.c#ne...
lib/softoken/pkcs11c.c:3738: HASH_HashType hashType = HASH_AlgSHA1;

Should we initialize this to HASH_AlgNULL (an invalid value)?
It does seem like this needs to be initialized to HASH_AlgSHA1
to preserve the original behavior in the non-PBKDF2 case.

https://codereview.appspot.com/270310043/diff/40001/lib/util/pkcs11t.h
File lib/util/pkcs11t.h (right):

https://codereview.appspot.com/270310043/diff/40001/lib/util/pkcs11t.h#newcod...
lib/util/pkcs11t.h:1797: #define CKP_PKCS5_PBKD2_HMAC_SHA512_256 0x00000007
IMPORTANT/BUG: these values are all off by one. See
http://docs.oasis-open.org/pkcs11/pkcs11-curr/v2.40/cos01/pkcs11-curr-v2.40-c...

0x00000002 is already taken. It is assigned to CKP_PKCS5_PBKD2_HMAC_GOSTR3411.

[ttaubert, 2015-11-11 10:26:03]

https://codereview.appspot.com/270310043/diff/40001/lib/pk11wrap/pk11pbe.c
File lib/pk11wrap/pk11pbe.c (right):

https://codereview.appspot.com/270310043/diff/40001/lib/pk11wrap/pk11pbe.c#ne...
lib/pk11wrap/pk11pbe.c:641: prfAlg = SEC_OID_HMAC_SHA1;
On 2015/11/11 02:35:13, wtc1 wrote:
> 
> Should we change the default to SEC_OID_HMAC_SHA256?
> Perhaps we can't because we need to be backward
> compatible?

There is at least PK11_CreatePBEAlgorithmID() that ends up passing
SEC_OID_UNKNOWN and thus defaults to HMAC_SHA1 for everyone using it. It would
certainly be nice to upgrade the default but I don't know how that would affect
consumers.

https://codereview.appspot.com/270310043/diff/40001/lib/softoken/lowpbe.h
File lib/softoken/lowpbe.h (right):

https://codereview.appspot.com/270310043/diff/40001/lib/softoken/lowpbe.h#new...
lib/softoken/lowpbe.h:81: int iterator);
On 2015/11/11 02:35:13, wtc1 wrote:
> Nit: this is a pre-existing problem, so you can fix it
> separately.
> 
> |iterator| is a poor parameter name. This should be named
> |iteration| or |iterationCount|.

Will file a follow-up for that.

https://codereview.appspot.com/270310043/diff/40001/lib/softoken/pkcs11c.c
File lib/softoken/pkcs11c.c (right):

https://codereview.appspot.com/270310043/diff/40001/lib/softoken/pkcs11c.c#ne...
lib/softoken/pkcs11c.c:3738: HASH_HashType hashType = HASH_AlgSHA1;
On 2015/11/11 02:35:13, wtc1 wrote:
> 
> Should we initialize this to HASH_AlgNULL (an invalid value)?
> It does seem like this needs to be initialized to HASH_AlgSHA1
> to preserve the original behavior in the non-PBKDF2 case.

Yeah, I think we should leave SHA-1 as the initial value, it's the only
supported value for PBKDF1 and there seem to be more cases that require it.

https://codereview.appspot.com/270310043/diff/40001/lib/util/pkcs11t.h
File lib/util/pkcs11t.h (right):

https://codereview.appspot.com/270310043/diff/40001/lib/util/pkcs11t.h#newcod...
lib/util/pkcs11t.h:1797: #define CKP_PKCS5_PBKD2_HMAC_SHA512_256 0x00000007
On 2015/11/11 02:35:14, wtc1 wrote:
> IMPORTANT/BUG: these values are all off by one. See
>
http://docs.oasis-open.org/pkcs11/pkcs11-curr/v2.40/cos01/pkcs11-curr-v2.40-c...
> 
> 0x00000002 is already taken. It is assigned to CKP_PKCS5_PBKD2_HMAC_GOSTR3411.

Thanks for spotting this! Will fix.

[wtc1, 2015-11-11 17:12:32]

Patch set 4 LGTM. (I didn't review the tests.)