Closed Bug 419794 Opened 15 years ago Closed 15 years ago

work out key API for nsICryptoHMAC


(Core :: Security: PSM, defect, P2)






(Reporter: dcamp, Assigned: mayhemer)




(1 file, 2 obsolete files)

From 415799 comment #11:

The PK11_ImportSymKey can and will fail on most FIPS devices (including our own
FIPS module). The problem is FIPS modules do not allow import or export of
plain-text keys. This means any crypto interface that uses plain-text keys will
not support only of these modes/devices. Exporting such an API can get your own
customers in trouble (just like my allowing it is potentially getting you in

Ideally you need to design an API where keys are exported as real objects.
There may actually be such an object in the mozilla crypto world for symmetric

Anyway you should consider modifying this api to take a key, then you can set
the key to some plaintext key (still won't work in fips mode), but you have an
API that could in the future move to something that can handle real tokens.
Flags: blocking1.9?
I had to fix nsKeyObjectFactory because the key produced was assigned wrong mechanism and operation. Creation of HMAC crypto context failed when key was created with CKA_ENCRYPT op and RC4 slot.

Disputable is name of new enum 'HMAC' in nsIKeyObject. Probably better way is to have separate constants for HMAC_SHA1, HMAC_MD5 anyway, enhanced test proofs that using CKA_SHA_1_HMAC when creating the key works for all algorithms. API is also simpler to use. I also added test against test vectors.
Attachment #306014 - Flags: review?(rrelyea)
Attachment #306014 - Flags: review?(dveditz)
Opps, sorry, missed to add the new test.
Attachment #306014 - Attachment is obsolete: true
Attachment #306017 - Flags: review?(rrelyea)
Attachment #306017 - Flags: review?(dveditz)
Attachment #306014 - Flags: review?(rrelyea)
Attachment #306014 - Flags: review?(dveditz)
Comment on attachment 306017 [details] [diff] [review]
API moved to use nsIKeyObject + fix in nsKeyObjectFactory

r+ Thanks for the quick work!

RE different HMAC key types.

Currently PKCS #11 accepts CKK_GENERIC_SECRET for all HMACS, but the pkcs #11 group is just now finalizing defining specific key types for each HMAC.

Upshot. Either way will work (one HMAC key type or one per HMAC), Each have advantages and disadvantages. (If you specify the HMAC specifically today, NSS will map all the keys to CKK_GENERIC_SECRET).

Attachment #306017 - Flags: review?(rrelyea) → review+
Flags: tracking1.9? → blocking1.9?
1. I added usage of CKM_GENERIC_SECRET_KEY_GEN for HMAC keys in nsKeyObjectFactory as Bob suggested in previous comment.

2. I changed ISUPPORTS implementation of nsKeyObjectFactory to be thread safe, because test failed on assertion about not thread-safety of that object. nsKeyObjectFactory uses PK11 functions that are (AFAIK) thread safe. This step is disputable because it might be solved a different way - nsUrlClassifierDBServiceWorker should get the service on main thread and also release it on it. But this might be very complicated and the service is still used on a different thread. David, your opinion?
Attachment #306017 - Attachment is obsolete: true
Attachment #306284 - Flags: review?(dveditz)
Attachment #306017 - Flags: review?(dveditz)
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Version: unspecified → Trunk
Attachment #306284 - Flags: review?(dveditz) → superreview?(dveditz)
Comment on attachment 306284 [details] [diff] [review]
API use nsIKeyObject + fix nsKeyModule + nsICryptoHMAC usage fix

Attachment #306284 - Flags: superreview?(dveditz) → superreview+
Keywords: checkin-needed
Does nsIKeyModule.idl need its IID revved due to the addition of |const short HMAC|?

Checking in security/manager/ssl/public/nsIKeyModule.idl;
/cvsroot/mozilla/security/manager/ssl/public/nsIKeyModule.idl,v  <--  nsIKeyModule.idl
new revision: 1.3; previous revision: 1.2
Checking in security/manager/ssl/src/nsKeyModule.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsKeyModule.cpp,v  <--  nsKeyModule.cpp
new revision: 1.4; previous revision: 1.3
Checking in security/manager/ssl/src/nsNSSComponent.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.cpp,v  <--  nsNSSComponent.cpp
new revision: 1.161; previous revision: 1.160
Checking in security/manager/ssl/tests/test_hmac.js;
/cvsroot/mozilla/security/manager/ssl/tests/test_hmac.js,v  <--  test_hmac.js
new revision: 1.2; previous revision: 1.1
Checking in netwerk/base/public/nsICryptoHMAC.idl;
/cvsroot/mozilla/netwerk/base/public/nsICryptoHMAC.idl,v  <--  nsICryptoHMAC.idl
new revision: 1.2; previous revision: 1.1
Checking in toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp,v  <--  nsUrlClassifierDBService.cpp
new revision: 1.70; previous revision: 1.69
Checking in toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.cpp,v  <--  nsUrlClassifierHashCompleter.cpp
new revision: 1.6; previous revision: 1.5
Checking in toolkit/components/url-classifier/tests/unit/test_streamupdater.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_streamupdater.js,v  <--  test_streamupdater.js
new revision: 1.7; previous revision: 1.6
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.