Closed Bug 554827 Opened 10 years ago Closed 4 years ago

Support additional pseudorandom functions for PKCS #5 PBKDF2.

Categories

(NSS :: Libraries, enhancement, P1)

enhancement

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
tracking-b2g backlog

People

(Reporter: wtc, Assigned: ttaubert)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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!
Assignee: wtc → brian
Blocks: 968567
Whiteboard: [ETA: end of February 2014]
Target Milestone: --- → 3.16
No longer blocks: 968567
Blocks: 968567
Version: 3.12.6 → trunk
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?
Flags: needinfo?(wtc)
Yes. I should also submit a proposal to the PKCS #11 Technical Committee
to add CKP_PKCS5_PBKD2_HMAC_SHA256, etc. officially.
Flags: needinfo?(wtc)
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.
(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.
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.
(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.
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 */
I just submitted this proposal to the PKCS 11 Technical Committee.
I fixed a typo in the value of CKP_PKCS5_PBKD2_HMAC_SHA512.
The value should be 0x00000006UL rather than 0x000000026UL.
Attachment #8371878 - Attachment is obsolete: true
Requesting this as a blocker to b2g 1.4 since Firefox Accounts depends on it.
blocking-b2g: --- → 1.4?
Also added HMAC SHA-512-224 and HMAC SHA-512-256 PRFs for completeness.
Attachment #8372386 - Attachment is obsolete: true
Not needed immediately for B2G 1.4 per bug 968567.
Whiteboard: [ETA: end of February 2014]
Target Milestone: 3.16 → ---
blocking-b2g: 1.4? → backlog
Assignee: brian → nobody
Attached patch Patch (work in progress) (obsolete) — Splinter Review
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.
Attachment #8401454 - Flags: review?(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.
Attachment #8401454 - Flags: review?(cviecco) → feedback+
We're hoping this will land in the next couple weeks to unblock bug 968567 . Does anyone have a status update?
I assume Camilo will work on this bug. Camilo, could you give a status update?
Assignee: nobody → cviecco
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 3.16.2
Flags: needinfo?(cviecco)
It is low on my priorty list. You can take over if you desire.
Flags: needinfo?(cviecco)
Blocks: 1021607
Camilo left Mozilla, afaiu, and is not going to work this.
Can you help re-assigning this?
Flags: needinfo?(rlb)
Assignee: cviecco → jjones
Flags: needinfo?(rlb)
blocking-b2g: backlog → ---
Blocks: 1216109
Taking.
Assignee: jjones → ttaubert
Target Milestone: 3.16.2 → ---
Attachment #8401454 - Attachment is obsolete: true
Patch at: https://codereview.appspot.com/270310043
Flags: needinfo?(wtc)
Flags: needinfo?(martin.thomson)
Feedback provided.  Awaiting the addition of new tests.
Flags: needinfo?(martin.thomson)
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?
Flags: needinfo?(wtc) → needinfo?(martin.thomson)
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.
Flags: needinfo?(martin.thomson)
You can also move the PRF tests into the new directory.
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.
Flags: needinfo?(wtc)
Flags: needinfo?(martin.thomson)
Seems like having the follow-up bug open and assigned would be a good idea.
Blocks: 1223073
(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.
Bug 1215295 adds pk11_gtest and thus blocks this one.
Depends on: 1215295
Blocks: 1223730
Wan-Teh approved patch set 4 on Rietveld.
Flags: needinfo?(wtc)
Ping me again when this gets unblocked.
Flags: needinfo?(martin.thomson)
(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.
Yeah, I agree. I thought about doing that too, can prepare a patch probably tomorrow.
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.
https://hg.mozilla.org/projects/nss/rev/541dd3958e59
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.22
Blocks: 1238277
You need to log in before you can comment on or make changes to this bug.