Implement HKDF in Softoken

RESOLVED FIXED in 3.12.9

Status

RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

unspecified
3.12.9

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

See http://tools.ietf.org/html/rfc5869 for the definition of HKDF.

Note that this is exactly he same KDF used in IKEv2 and some other IETF protocols. It is very similar to TLS's PRF. It doesn't require random authenticated nonces like the TLS PRF does (though it can take advantage of random nonces if the application has them.) Besides our use in Firefox Sync, I believe that HKDF will also be useful for things like bug 610095 and in many other applications.

Also, elsewhere in Firefox Sync HKDF will be used to derive keys from the Sync key.
Created attachment 492443 [details] [diff] [review]
HKDF implementation in Softoken

This is the first patch in my J-PAKE related series.

Clearly we should have tests to go along with this implementation but I am not quite sure about what kinds of tests for this we should have in the test suite. Suggestions are welcome.

This mechanism is designed for an application to use CKM_EXTRACT_KEY_FROM_KEY to extract the actual key bits out of the resultant key block.
Assignee: nobody → bsmith
Attachment #492443 - Flags: review?(rrelyea)
Attachment #492443 - Attachment is patch: true
Attachment #492443 - Attachment mime type: application/octet-stream → text/plain
Attachment #492443 - Attachment description: HKDF implementatoin in Softoken → HKDF implementation in Softoken

Comment 2

8 years ago
In general, this patch looks good.

I would be tempted to change the variable names in the hdf block, but I see they match those from the RFC, so simply putting a reference to the RFC should be sufficient.

I'll note that at some point the whole C_Derive function needs it's own file and need to be broken into smaller functions. That's not required for this patch, however.

I've looked at the pbe to see what is different between it and hdf. pbe's are key gen functions which take a user supplied password, not derive functions that take a key, so even if they are similiar superficially, it looks like brian's choice of implementing it as a full derive is justified.

The one thing I'd like to ask before I r+ this patch is the question of 1 mechanism or three. Does it make more sense to take the hash function as a parameter (either a kdf # as pkcs #5, or a hmac mechanism number) rather than have 3 different mechanism, one for each supported hash type?

bob
I am pretty indifferent to the strategy for parameterizing mechanisms with other mechanisms and/or hash functions specifically. The main advantage of the approach I took here is that an application can query which hashes are supported for this feature simply by querying the list of implemented mechanisms. If the hash function is a parameter, then presumably the list of supported hash functions would be implemented as an attribute or attributes of a mechanism object. But, that means specifying new attributes (and implementing whole new infrastructure for mechanism objects?) or leaving the application unable to query for support of these mechanisms before trying them out. It looks ugly either way so I chose the way that requires less code.

I think the gotos in this code are particularly ugly but I figure that will all get sorted out during the (perpetually postponed) refactoring that you mentioned.

Comment 4

8 years ago
Comment on attachment 492443 [details] [diff] [review]
HKDF implementation in Softoken

r+ rrelyea.

Your MECHANISM lookup argument sways me completely.

re: gotos: they are extremely common in softoken switch statements where multiple mechanism only vary by some simple input.

bob
Attachment #492443 - Flags: review?(rrelyea) → review+
Comment on attachment 492443 [details] [diff] [review]
HKDF implementation in Softoken

I am working on a better patch that adds HKDF_SHA1 and adds the missing support for CKM_GetMechanismInfo/CKM_GetMechanismList.
Attachment #492443 - Flags: review+ → review-
Created attachment 494769 [details] [diff] [review]
Same as previous patch + C_GetMechanism{Info,List} + SHA-1 support

This version adds support for C_GetMechanismInfo/C_GetMechanismList.

This version also adds a single line to pkcs11c.c to support SHA-1 in addition to the SHA-2 family. (Reviewing the interdiff would probably be a good shortcut.) This may be useful for implementing/emulating IKE.
Attachment #492443 - Attachment is obsolete: true
Attachment #494769 - Flags: review?(rrelyea)
Created attachment 494770 [details] [diff] [review]
Same as previous patch + C_GetMechanism{Info,List} + SHA-1 support (corrected) (checked in)
Attachment #494769 - Attachment is obsolete: true
Attachment #494770 - Flags: review?(rrelyea)
Attachment #494769 - Flags: review?(rrelyea)

Comment 8

8 years ago
Comment on attachment 494770 [details] [diff] [review]
Same as previous patch + C_GetMechanism{Info,List} + SHA-1 support (corrected) (checked in)

r+ rrelyea
Attachment #494770 - Flags: review?(rrelyea) → review+

Comment 9

8 years ago
Brian, I think this patch is independent of the other patches, is it ready to go in?

bob
Yes, My other patches are based on this one to avoid conflicts so it is good to check this one in first.

Comment 11

8 years ago
trunk/tip checkin:

Checking in lib/softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.173; previous revision: 1.172
done
Checking in lib/softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.117; previous revision: 1.116
done
Checking in lib/util/pkcs11n.h;
/cvsroot/mozilla/security/nss/lib/util/pkcs11n.h,v  <--  pkcs11n.h
new revision: 1.20; previous revision: 1.19
done

Comment 12

8 years ago
NSS 3.12 Branch...

Checking in lib/softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.167.6.3; previous revision: 1.167.6.2
done
Checking in lib/softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.111.22.1; previous revision: 1.111
done
Checking in lib/util/pkcs11n.h;
/cvsroot/mozilla/security/nss/lib/util/pkcs11n.h,v  <--  pkcs11n.h
new revision: 1.19.22.1; previous revision: 1.19
done
bobs-laptop(83)
This patch is broken because:

1. hashLen is uninitialized.
2. the derive sensitivity check is missing.

I am working on a patch to address both issues.
Created attachment 495201 [details] [diff] [review]
Patch (to apply on top of previous patch) to fix the two problems mentioned above.
Attachment #495201 - Flags: review?(rrelyea)

Comment 15

8 years ago
Comment on attachment 495201 [details] [diff] [review]
Patch (to apply on top of previous patch) to fix the two problems mentioned above.

r+ rrelyea
Attachment #495201 - Flags: review?(rrelyea) → review+

Updated

8 years ago
Attachment #494770 - Attachment description: Same as previous patch + C_GetMechanism{Info,List} + SHA-1 support (corrected) → Same as previous patch + C_GetMechanism{Info,List} + SHA-1 support (corrected) (checked in)

Comment 16

8 years ago
version 2 checked in

Tip/Trunk
Checking in lib/softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.119; previous revision: 1.118
done

Branch
Checking in lib/softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.111.22.3; previous revision: 1.111.22.2
done
Status: NEW → ASSIGNED

Comment 17

8 years ago
I'll leave brian to close this bug.
I am working on tests (in pk11mode) for this. Let's keep the bug open to track those patches.
No longer blocks: 601645
Target Milestone: --- → 3.12.9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.