419794 - work out key API for nsICryptoHMAC

Status: RESOLVED FIXED

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

Description

[Dave Camp (:dcamp)]

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
trouble).

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
operations.

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.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: API moved to use nsIKeyObject + fix in nsKeyObjectFactory

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.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: API moved to use nsIKeyObject + fix in nsKeyObjectFactory

Opps, sorry, missed to add the new test.

Comment 3

[Robert Relyea]

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).

bob

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: API use nsIKeyObject + fix nsKeyModule + nsICryptoHMAC usage fix

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?

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 306284 [details] [diff] [review]
API use nsIKeyObject + fix nsKeyModule + nsICryptoHMAC usage fix

sr=dveditz

Comment 6

[Reed Loden [:reed] (use needinfo?)]

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
done
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
done
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
done
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
done
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
done
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
done
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
done
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
done