614076 - Implement HKDF in Softoken

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

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

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.

Comment 1

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

Attachment: 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.

Comment 2

[Robert Relyea]

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

Comment 3

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

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

[Robert Relyea]

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

Comment 5

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

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.

Comment 6

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

Attachment: 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.

Comment 7

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

Attachment: Same as previous patch + C_GetMechanism{Info,List} + SHA-1 support (corrected) (checked in)

Comment 8

[Robert Relyea]

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

r+ rrelyea

Comment 9

[Robert Relyea]

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

bob

Comment 10

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

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

Comment 11

[Robert Relyea]

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

[Robert Relyea]

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)

Comment 13

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

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.

Comment 14

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

Attachment: Patch (to apply on top of previous patch) to fix the two problems mentioned above.

Comment 15

[Robert Relyea]

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

r+ rrelyea

Comment 16

[Robert Relyea]

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

Comment 17

[Robert Relyea]

I'll leave brian to close this bug.

Comment 18

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

I am working on tests (in pk11mode) for this. Let's keep the bug open to track those patches.