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

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Wan-Teh Chang]

In PKCS #5 v2.0, only one pseudorandom function, HMAC-SHA-1,
is specified.  This is why PKCS #11 v2.20 defines only one
CKP_PKCS5_PBKD2_xxx constant, CKP_PKCS5_PBKD2_HMAC_SHA1.

As a result, NSS's PBKDF2 code limits itself to HMAC-SHA-1,
even though it seems to support any hash functions:
http://mxr.mozilla.org/security/ident?i=CKP_PKCS5_PBKD2_HMAC_SHA1

PKCS #5 v2.1 draft specifies additional pseudorandom functions
such as HMAC-SHA-256, but it is still a draft after more than 3
years.

PKCS #11 v2.30 draft added CKP_PKCS5_PBKD2_HMAC_GOSTR3411
(0x00000002).  So we need to propose the addition of several new
PKCS #5 PBKDF2 pseudorandom functions to PKCS #11 v2.30:

CKP_PKCS5_PBKD2_HMAC_SHA224      0x0000003
CKP_PKCS5_PBKD2_HMAC_SHA256      0x0000004
CKP_PKCS5_PBKD2_HMAC_SHA384      0x0000005
CKP_PKCS5_PBKD2_HMAC_SHA512      0x0000006

and then change NSS to allow them.

Comment 1

[Richard Newman [:rnewman]]

Any update on this, folks? We're planning to use PBKDF2-HMAC-SHA256 for PICL, and it'd be nice to not have to ship OpenSSL to do it!

Comment 2

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

To fix this, we'd just need to search for CKP_PKCS5_PBKD2_HMAC_SHA1 and then expand that logic to CKP_PKCS5_PBKD2_HMAC_SHA256 and the others that Wan-Teh listed in comment 0.

It seems there is no spec for CKP_PKCS5_PBKD2_HMAC_SHA256, etc. yet. Should we define a separate mechanism in the NETSCAPE namespace to deal with this?

Comment 3

[Wan-Teh Chang]

Yes. I should also submit a proposal to the PKCS #11 Technical Committee
to add CKP_PKCS5_PBKD2_HMAC_SHA256, etc. officially.

Comment 4

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

1. Define a new mechanism named CKM_NSS_PKCS5_PBKD2, based on how CKM_NSS_JPAKE_ROUND2_SHA512, et al., were defined. See
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=609076&attachment=495196

2. Define new flags CKP_NSS_PKCS5_PBKD2_HMAC_SHA224...CKP_NSS_PKCS5_PBKD2_HMAC_SHA512 with values 0x0000003...0x0000006.

3. Modify nsc_SetupPBEKeyGen to consider the new flags.

4. Search for CKM_PKCS5_PBKD2 everywhere in NSS, and add CKM_NSS_PKCS5_PBKD2 cases.

5. Do the same for SEC_OID_PKCS5_PBKDF2.

6. Probably do other things I didn't mention.

Easy as pie.

Comment 5

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

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #4)
> 4. Search for CKM_PKCS5_PBKD2 everywhere in NSS, and add CKM_NSS_PKCS5_PBKD2
> cases.

Wan-Teh, this step adds quite a bit of work to this bug.

nstead, we should
#define CKP_NSS_PKCS5_PBKD2_HMAC_SHA224 (NSSCK_VENDOR_NSS + 3)
...
#define CKP_NSS_PKCS5_PBKD2_HMAC_SHA224 (NSSCK_VENDOR_NSS + 6).

And then keep using CKM_PKCS5_PBKD2 without creating a new CKM_NSS_PKCS5_PBKD2 mechanism.

This would cut down the amount of boilerplate significantly.

Comment 6

[Wan-Teh Chang]

Brian: I agree that we should not add a new CKM_NSS_PKCS5_PBKD2 mechanism.
We just need to add the new CKP_NSS_PKCS5_PBKD2_HMAC_SHA* PRFs in the NSS
vendor space, and use them with the standard CKM_PKCS5_PBKD2 mechanism.

Comment 7

[Ryan Sleevi]

(In reply to Wan-Teh Chang from comment #6)
> Brian: I agree that we should not add a new CKM_NSS_PKCS5_PBKD2 mechanism.
> We just need to add the new CKP_NSS_PKCS5_PBKD2_HMAC_SHA* PRFs in the NSS
> vendor space, and use them with the standard CKM_PKCS5_PBKD2 mechanism.

There is no assigned vendor space for CKPs, up to and including the draft 2.30 space.

Sounds like that should be an item to bring back to the PKCS#11 WG?

Specifically, Section 9.4/9.5 establishes the CK*_VENDOR_DEFINED, but no specific list of CKP_VENDOR_DEFINED is added. There is also no CKP_VENDOR_DEFINED in the 2.30 includes.

Comment 8

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

pkcs11n.h says that the top half of every number space is the vendor space. It would be good to get that codified into the spec though.

/*
 * NSSCK_VENDOR_NSS
 *
 * Cryptoki reserves the high half of all the number spaces for
 * vendor-defined use.  I'd like to keep all of our NSS-
 * specific values together, but not in the oh-so-obvious
 * 0x80000001, 0x80000002, etc. area.  So I've picked an offset,
 * and constructed values for the beginnings of our spaces.
 *
 * Note that some "historical" Netscape values don't fall within
 * this range.
 */
#define NSSCK_VENDOR_NSS 0x4E534350 /* NSCP */

Comment 9

[Wan-Teh Chang]

Attachment: Proposal for PKCS 11 TC: add four HMAC-SHA-2 PRFs for PBKDF2

I just submitted this proposal to the PKCS 11 Technical Committee.

Comment 10

[Wan-Teh Chang]

Attachment: Proposal for PKCS 11 TC: add four HMAC-SHA-2 PRFs for PBKDF2, v2

I fixed a typo in the value of CKP_PKCS5_PBKD2_HMAC_SHA512.
The value should be 0x00000006UL rather than 0x000000026UL.

Comment 11

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Requesting this as a blocker to b2g 1.4 since Firefox Accounts depends on it.

Comment 12

[Wan-Teh Chang]

Attachment: Proposal for PKCS 11 TC: add six HMAC-SHA-2 PRFs for PBKDF2

Also added HMAC SHA-512-224 and HMAC SHA-512-256 PRFs for completeness.

Comment 13

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

Not needed immediately for B2G 1.4 per bug 968567.

Comment 14

[Wan-Teh Chang]

Attachment: Patch (work in progress)

Camilo,

We talked about this bug yesterday afternoon. I just spent about
an hour on this. It is incomplete. If you are going to work on
this bug, please use this patch as a starting point. You can also
indicate which changes in this patch can be checked in first.

Comment 15

[Camilo Viecco (:cviecco)]

Comment on attachment 8401454 [details] [diff] [review]
Patch (work in progress)

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

Seems like A good start. I will read more about it today.

Comment 16

[Sam Penrose]

We're hoping this will land in the next couple weeks to unblock bug 968567 . Does anyone have a status update?

Comment 17

[Wan-Teh Chang]

I assume Camilo will work on this bug. Camilo, could you give a status update?

Comment 18

[Camilo Viecco (:cviecco)]

It is low on my priorty list. You can take over if you desire.

Comment 19

[Frederik Braun [:freddy]]

Camilo left Mozilla, afaiu, and is not going to work this.
Can you help re-assigning this?

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Taking.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Patch at: https://codereview.appspot.com/270310043

Comment 22

[Martin Thomson [:mt:]]

Feedback provided.  Awaiting the addition of new tests.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

So I took a look at writing tests and I'm not sure how to proceed. We have cmd/bltest but that's probably not the right place to test these APIs. It seems like in the past we simply added a new binary to test new features/algorithms, although I think going forward we want to write gtests? What would the best way be to test this?

Comment 24

[Martin Thomson [:mt:]]

Adding gtests is pretty straightforward.  Rather than adding to ssl_gtests, I would recommend creating a new directory and binary.

You can probably copy of a lot of what we did for ssl_gtests.  I would suggest using the no data init so that you don't need to do all the setup that you will see in tests/ssl_gtests/ssl_gtests.sh  Copy the ssl_gtest.cc, Makefile and manifest.mn files from external_test/ssl_gtest into a new external_test/pk11_test (pick a better name, of course) and work from there.

This might take a little bit to get acclimatized, but writing the tests should be easy once you get going.

Comment 25

[Eric Rescorla (:ekr)]

You can also move the PRF tests into the new directory.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Patch updated: https://codereview.appspot.com/270310043

It's based on the one in bug 1215295.

(In reply to Eric Rescorla (:ekr) from comment #25)
> You can also move the PRF tests into the new directory.

Would love to do that in a follow-up.

Comment 27

[Martin Thomson [:mt:]]

Seems like having the follow-up bug open and assigned would be a good idea.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Martin Thomson [:mt:] from comment #27)
> Seems like having the follow-up bug open and assigned would be a good idea.

Filed bug 1223073 and assigned myself to it.

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Bug 1215295 adds pk11_gtest and thus blocks this one.

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

Wan-Teh approved patch set 4 on Rietveld.

Comment 31

[Martin Thomson [:mt:]]

Ping me again when this gets unblocked.

Comment 32

[Wan-Teh Chang]

(In reply to Tim Taubert [:ttaubert] from comment #29)
> Bug 1215295 adds pk11_gtest and thus blocks this one.

Tim: this is unfortunate. If the test part of your patch has
been approved, I suggest you check in the non-test part of
your patch first.

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

Yeah, I agree. I thought about doing that too, can prepare a patch probably tomorrow.

Comment 34

[Martin Thomson [:mt:]]

Bob is OK with the changes in bug 1215295, so I'm going to go ahead and land this, once I've checked that it doesn't break anything obvious, that is.

Comment 35

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/541dd3958e59