Closed
Bug 291858
Opened 19 years ago
Closed 19 years ago
Some NSS mechanism numbers don't match the PKCS11 spec
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.10.2
People
(Reporter: nelson, Assigned: wtc)
Details
Attachments
(1 file)
1.94 KB,
patch
|
nelson
:
review+
rrelyea
:
superreview+
|
Details | Diff | Splinter Review |
"robd" <rmdugal@hotmail.com> reported in n.p.m.crypto that NSS's numbers for certain PKCS11 mechanisms do not match the numbers in the PKCS11 spec. He cited the following examples: Mechanism Name RSA # (hex) NSS # ----------------------- CKM_SHA256_HMAC 251 252 CKM_SHA256_HMAC_GENERAL 252 251 CKM_SHA384_HMAC 261 262 CKM_SHA384_HMAC_GENERAL 262 261 CKM_SHA512_HMAC 271 272 CKM_SHA512_HMAC_GENERAL 272 271 CKM_ECDH1_DERIVE 1050 1043 CKM_ECDH1_COFACTOR_DERIVE 1051 1044 CKM_ECMQV_DERIVE 1052 1045 Fixing the EC mechanisms is easy, because we've not yet released any bit with the EC code enabled, either above or below the PKCS11 API "line". But I see no way to fix the SHA mechanimsm withour breaking binary compatibility. Suggestions? Andreas, does your code use any of these symbols? If so, which ones? and whose header files do you use to define them? (NSS? RSA? other?)
Comment 1•19 years ago
|
||
(In reply to comment #0) > > Andreas, does your code use any of these symbols? If so, which ones? > and whose header files do you use to define them? (NSS? RSA? other?) Of these mechanisms, we support CKM_SHA256/384/512_HMAC. We get the ids from the RSA header files, so this bug breaks us, too. We have never encountered it because our PKCS#11/MAC tests are incomplete.
Assignee | ||
Comment 2•19 years ago
|
||
CKM_SHA256_HMAC has no parameter, but CKM_SHA256_HMAC_GENERAL takes a CK_MAC_GENERAL_PARAMS parameter. I'm wondering if we can use this difference to determine which mechanism is intended when we receive 0x251.
Assignee | ||
Comment 3•19 years ago
|
||
I looked into this and believe that simply fixing the macro definitions is sufficient. Since I don't know NSS that well, my findings need to be reviewed by either Nelson or Bob. But I believe that we are not affected by the incorrect values of the CKM_SHAxxx_HMAC and CKM_SHAxxx_HMAC_GENERAL macros in our pkcs11t.h header. The evidences I found that support this conclusion are as follows. 1. In lib/pk11wrap and lib/softoken, CKM_SHAxxx_HMAC and CKM_SHAxxx_HMAC_GENERAL are used in a way that their values can be swapped. For example, in lib/pk11wrap/pk11mech.c, we have: CK_MECHANISM_TYPE PK11_GetKeyType(CK_MECHANISM_TYPE type,unsigned long len) { switch (type) { ... case CKM_SHA_1_HMAC: case CKM_SHA_1_HMAC_GENERAL: case CKM_SHA256_HMAC: case CKM_SHA256_HMAC_GENERAL: case CKM_SHA384_HMAC: case CKM_SHA384_HMAC_GENERAL: case CKM_SHA512_HMAC: case CKM_SHA512_HMAC_GENERAL: case CKM_MD2_HMAC: case CKM_MD2_HMAC_GENERAL: case CKM_MD5_HMAC: case CKM_MD5_HMAC_GENERAL: case CKM_TLS_PRF_GENERAL: return CKK_GENERIC_SECRET; default: return pk11_lookup(type)->keyType; } } 2. CKM_SHAxxx_HMAC_GENERAL is not used in any other NSS lib. 3. CKM_SHAxxx_HMAC is used in only one other NSS lib: lib/pkcs12, function sec_pkcs12_algtag_to_mech. That function returns CKM_SHAxxx_HMAC if the input argument is SEC_OID_SHAxxx. But NSS only calls that function with an input of SEC_OID_SHA1, SEC_OID_MD5, or SEC_OID_MD2. 4. In cmd, CKM_SHAxxx_HMAC and CKM_SHAxxx_HMAC_GENERAL are only used in pk11util. If I understand it correctly, pk11util will print the wrong mechanism names because of the wrong values in NSS's pkcs11t.h. (That is, CKM_SHAxxx_HMAC will be printed as CKM_SHAxxx_HMAC_GENERAL and vice versa.) Fortunately we haven't publicized the pk11util tool.
Attachment #181826 -
Flags: superreview?(rrelyea)
Attachment #181826 -
Flags: review?(nelson)
Assignee | ||
Comment 4•19 years ago
|
||
One more comment on pk11util: this tool was added during NSS 3.10 development. It is not in NSS 3.9.x or earlier. So if my findings are right, the severity of this bug can be lowered.
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•19 years ago
|
||
I missed one lib/softoken file that uses CKM_SHAxxx_HMAC and CKM_SHAxxx_HMAC_GENERAL: pkcs11c.c, functions NSC_SignInit and NSC_VerifyInit. #define INIT_HMAC_MECH(mmm) \ case CKM_ ## mmm ## _HMAC_GENERAL: \ crv = sftk_doHMACInit(context, HASH_Alg ## mmm ,key, \ *(CK_ULONG *)pMechanism->pParameter); \ break; \ case CKM_ ## mmm ## _HMAC: \ crv = sftk_doHMACInit(context, HASH_Alg ## mmm ,key, mmm ## _LENGTH); \ break; ... INIT_HMAC_MECH(SHA256) INIT_HMAC_MECH(SHA384) INIT_HMAC_MECH(SHA512) This code does depend on the values of the CKM_SHAxxx_HMAC and CKM_SHAxxx_HMAC_GENERAL macros. Now things make sense because the values of these macros should matter. But now we are also faced with the dilemma of breaking backward compatibility.
Reporter | ||
Comment 6•19 years ago
|
||
Marking P1 for 3.10.1. Since this is not yet in widespread use, I think the sooner we fix it, (even if it means breaking binary compatibility) the better. It seems that the only identified external (to NSS) users of the PKCS11 mechanisms are using the RSA headers, so fixing the numbers should only improve things for them, and should not break anything for users of NSS APIs.
Priority: -- → P1
Target Milestone: --- → 3.10.1
Reporter | ||
Comment 7•19 years ago
|
||
Comment on attachment 181826 [details] [diff] [review] Proposed patch r=nelson for checkin after 3.10 RTM
Attachment #181826 -
Flags: review?(nelson) → review+
Comment 8•19 years ago
|
||
Comment on attachment 181826 [details] [diff] [review] Proposed patch I agree r+ relyea
Attachment #181826 -
Flags: superreview?(rrelyea) → superreview+
Assignee | ||
Comment 9•19 years ago
|
||
The incorrect CKM_SHAxxx_HMAC and CKM_SHAxxx_HMAC_GENERAL macro definitions were introduced in NSS 3.8. The incorrect CKM_EC*_*DERIVE macro definitions were introduced in NSS 3.4.
Version: 3.9 → 3.8
Assignee | ||
Comment 10•19 years ago
|
||
I checked in the patch on the NSS trunk for NSS 3.10.1. Checking in pkcs11t.h; /cvsroot/mozilla/security/nss/lib/softoken/pkcs11t.h,v <-- pkcs11t.h new revision: 1.12; previous revision: 1.11 done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Target Milestone: 3.10.1 → 3.10.2
You need to log in
before you can comment on or make changes to this bug.
Description
•