291858 - Some NSS mechanism numbers don't match the PKCS11 spec

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

"robd" <[e-mail]> 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

[Andreas Sterbenz]

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

Comment 2

[Wan-Teh Chang]

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.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch

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.

Comment 4

[Wan-Teh Chang]

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.

Comment 5

[Wan-Teh Chang]

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.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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. 

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 181826 [details] [diff] [review]
Proposed patch

r=nelson for checkin after 3.10 RTM

Comment 8

[Robert Relyea]

Comment on attachment 181826 [details] [diff] [review]
Proposed patch

I agree r+ relyea

Comment 9

[Wan-Teh Chang]

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.

Comment 10

[Wan-Teh Chang]

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