676114 - pk11_PubDeriveECKeyWithKDF does not work with some smart cards

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[David Cooper]

Attachment: Patch to allow pk11_PubDeriveECKeyWithKDF to work with smart cards that can only implement the ECDH primitive

pk11_PubDeriveECKeyWithKDF assumes that the slot that performs the ECDH primitive can also perform the specified KDF and perform any symmetric key operations that are required with the resulting derived key.  Some smart cards, such as the PIV Card, however, can only perform the ECDH primitive.

This patch adds support for such smart cards by breaking up the implementation of the key derivation function.  If calls to C_DeriveKey fail when specifing a KDF other than CKD_NULL, then the code in this patch will call C_DeriveKey with the CKD_NULL KDF specified, use pk11_ForceSlot to move the resulting shared secret from PKCS #11 module that performed the ECDH primitive to the software PKCS #11 module, and then perform the appropriate KDF in software. Note that in order for this patch to work, the patch that I submitted for bug #591640 must be accepted first since that patch implements the CKM_NSS_ANSI_X9_63_KDF mechanism.

Comment 1

[Ludovic Hirlimann [:Usul]]

Comment on attachment 550227 [details] [diff] [review]
Patch to allow pk11_PubDeriveECKeyWithKDF to work with smart cards that can only implement the ECDH primitive

Setting the review flag to someone else than kaie as he's quite busy. Honza sorry if you're not a peer for this code.

Comment 2

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

It seems reasonable that if no module implements both ECDH and the KDF, that we should try to find a combination of modules that do.

In the configuration you are testing, is Softoken built without full NSS_ENABLE_ECC=1? Softoken should be chosen for both operations unless a higher-priority module also implements both, unless all the ECC support has been disabled.

Comment 3

[Honza Bambas (:mayhemer)]

Not exactly my area, but I can take a look.  But it will take time, I have other stuff on the list now, is that OK?  Anyone else, feel free to steal the review.

Comment 4

[David Cooper]

(In reply to comment #2)
> It seems reasonable that if no module implements both ECDH and the KDF, that
> we should try to find a combination of modules that do.
> 
> In the configuration you are testing, is Softoken built without full
> NSS_ENABLE_ECC=1? Softoken should be chosen for both operations unless a
> higher-priority module also implements both, unless all the ECC support has
> been disabled.

I agree with Softoken should be chosen whenever possible, but that is not always an option since the ECDH primitive has to be performed wherever the private key happens to be.  In the code that I developed to implement ECDH in CMS (see bug #676118), PK11_PubDeriveWithKDF (and thus pk11_PubDeriveECKeyWithKDF) is called in two places, in the function to create an encrypted message and in the function to decrypt a message.

When encrypting a message, an ephemeral ECC key is created by calling SECKEY_CreateECPrivateKey and the slot in which this key is created is the slot in which pk11_PubDeriveECKeyWithKDF will perform the ECDH primitive.  I expect that this ephemeral key pair will be created in Softoken, in which case the first call to C_DeriveKey (line 1871) will be successful and the code in this patch will not be run.

Similarly, when decrypting a message, if the private key is not stored on a hardware token, I would expect the ECDH primitive to be performed in Softoken, and so again the first call to C_DeriveKey (on line 1871) will return CKR_OK and the code introduced in this patch will not be run.

I only expect the code in this patch to be run in the case that one is decrypting a message in which the key needed to decrypt the message is stored on a hardware token.  In this case, NSS cannot choose which module performs the ECDH primitive.  So, it wouldn't matter that Softoken is capable of performing both the ECDH primitive and the KDF since Softoken wouldn't have access to the private key.  In such a case, after asking the hardware token to perform the C_DeriveKey function with the KDF (and getting an error response), this patch will try to have the token perform the C_DeriveKey function with the CKD_NULL KDF and then move the resulting shared secret to another module (presumably Softoken) to do the rest.

I know that this patch is necessary to support PIV Cards, since PIV Cards can only perform the ECDH primitive and cannot perform either the KDF or any operations with the resulting symmetric key, and I would guess that other hardware tokens are similarly limited.  I consider this patch to perform a function that is analogous to the code in pk11_AnyUnwrapKey, which will either call C_UnwrapKey or pk11_HandUnwrap depending on whether the slot that holds the private key is capable of performing the target operation.

Comment 5

[David Cooper]

Attachment: Patch to allow pk11_PubDeriveECKeyWithKDF to work with smart cards that can only implement the ECDH primitive

This new version of the patch just contains changes needed to work with the revised patch that I submitted for bug #591640.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 551569 [details] [diff] [review]
Patch to allow pk11_PubDeriveECKeyWithKDF to work with smart cards that can only implement the ECDH primitive

Nelson, can you take a look at this please?  This is really not my area, but if urgent and you are out of time I can be one of those you want to redirect further/back.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

This is an issue with NSS, not SMIME, even if it affects SMIME.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 551569 [details] [diff] [review]
Patch to allow pk11_PubDeriveECKeyWithKDF to work with smart cards that can only implement the ECDH primitive

r-,

>+	    if (crv == CKR_OK) {
>+		    CK_MECHANISM_TYPE kdfMech;
>+		    SECItem ansi_x9_63_kdf_mechParams;
>+
>+		    if (kdf == CKD_SHA1_KDF) {
>+			kdfMech = CKM_NSS_ANSI_X9_63_SHA1_KDF;
>+		    } else if (kdf == CKD_SHA224_KDF) {
>+			kdfMech = CKM_NSS_ANSI_X9_63_SHA224_KDF;
>+		    } else if (kdf == CKD_SHA256_KDF) {
>+			kdfMech = CKM_NSS_ANSI_X9_63_SHA256_KDF;
>+		    } else if (kdf == CKD_SHA384_KDF) {
>+			kdfMech = CKM_NSS_ANSI_X9_63_SHA384_KDF;
>+		    } else if (kdf == CKD_SHA512_KDF) {
>+			kdfMech = CKM_NSS_ANSI_X9_63_SHA512_KDF;
>+		    }

If kdf was none of the above 5 values, then kdfMech is uninitialized here

>+		    SharedSecret2 = pk11_ForceSlot(SharedSecret, kdfMech,
>+								CKF_DERIVE);

Please direct any further review requests for this code to Bob Relyea

Comment 9

[Robert Relyea]

Comment on attachment 551569 [details] [diff] [review]
Patch to allow pk11_PubDeriveECKeyWithKDF to work with smart cards that can only implement the ECDH primitive

r-, though I agree with the general idea.

The r- is for a couple of basic things:

1) As nelson pointed out, kdfMech may be uninitialized.
In addition,  I would prefer a short function that takes a CKD_XXX_KDF and returns the correspond mechanism. Also a reverse function should also exist. It's OK to use a switch in the function. Be sure to have an error proper error condition if there is no match. CKM_INVALID_MECHANSISM would be appropriate for the first function.
2) We should report where we are with the Cryptoki group on the proposed mechanism. Pinging them would be a good idea. They seem a little slow.

(At this point only a report is necessary for an r+).

bob

Comment 10

[David Cooper]

(In reply to Robert Relyea from comment #9)
> Comment on attachment 551569 [details] [diff] [review]
> Patch to allow pk11_PubDeriveECKeyWithKDF to work with smart cards that can
> only implement the ECDH primitive
> 
> r-, though I agree with the general idea.
> 
> The r- is for a couple of basic things:
> 
> 1) As nelson pointed out, kdfMech may be uninitialized.
> In addition,  I would prefer a short function that takes a CKD_XXX_KDF and
> returns the correspond mechanism. Also a reverse function should also exist.
> It's OK to use a switch in the function. Be sure to have an error proper
> error condition if there is no match. CKM_INVALID_MECHANSISM would be
> appropriate for the first function.
> 2) We should report where we are with the Cryptoki group on the proposed
> mechanism. Pinging them would be a good idea. They seem a little slow.
> 
> (At this point only a report is necessary for an r+).
> 
> bob

If it is decided in bug #591640 to specify different mechanisms for each hash function, I can certainly modify the code so that a separate function is used to convert from CKD_SHA*_KDF to the corresponding CKM_NSS_ANSI_X9_63_SHA*_KDF.  I will note, however, that there is no risk that kdkMech may be uninitialized since there is code at the beginning of pk11_PubDeriveECKeyWithKDF that returns an error if kdf has an unacceptable value.  So, the code listed in Comment 8 will never be executed unless kdf has one of the listed values.

At this point, I don't plan to make any changes to the patch submitted for this bug until a patch for bug #591640 has been approved, since the patch for this bug needs to be aligned with whatever is done there.

Comment 11

[David Cooper]

Attachment: Implementation of the ANSI X.9.63 KDF using PKCS #11 mechanisms to support smart cards that only implement the ECDH primitive

Here is an implementation of the ANSI X9.63 KDF that does not require the creation of any new mechanisms, as was discussed in bug #591640.  This implementation does, however, require the implementation of the SHA-2-based key derivation mechanisms from bug #590515.

Comment 12

[Robert Relyea]

Comment on attachment 635910 [details] [diff] [review]
Implementation of the ANSI X.9.63 KDF using PKCS #11 mechanisms to support smart cards that only implement the ECDH primitive

r-

In general, this is exactly what we need. There are some problems with the specific implementation:

1) NIT - it's not using the same naming comventions as the pk11wrap directory.
   a. Public functions have names like PK11_MyNewFunction and private functions have names of the form pk11_MyPrivateFunction rather than pk11_my_private_function.
   b. private and stack variables have names like myStackVariable rather than MyStackVariable or my_stack_variable. I'm less strict about the _ in stack variables (you can see a slight inconsistency throughout the file, but in general it's pretty rare and stands out when you look at the file.

In NSS we prefer to try to stay with the style of the given component we are modifying.

2) You created a pk11_derive_key_from_key() which looks like it does exactly what the public PK11_Derive does. (it has almost exactly the same signature).

3) The new pk11_concatenate_base_and_data appears unused.

4) in pk11_ASNI_X9_63Derive you have several places where you return NULL without setting an NSS error code (PORT_SetError()). This is OK for the case where you called another function that failed, since it should have returned an error code itself. It looks like most of these should be SEC_ERROR_INVALID_ARGS.

5) The most serious issue is the handling of keySize. Inside pk11wrap, keySize may validly be 0, and in those case we should probably pass on 'zero' to the next level. Also, the size parameter of the key is not always set, If you need to know an existing key's size, you need to call PK11_GetKeyLength(), which will work really hard to give you the real key size. In those cases where it can't, it will return 0 and you need to be able to deal with that.

Here are my recommendations:

1) use PK11_Derive() it already knows how to properly deal with keysize==0. (Also know PK11_Derive knows how to get the wincx from the symkey, so you can lose the wincx parameter from all our helper functions).

2) pk11_concatenate_base_and_data - as noted above, it doesn't appear to be necessary and should be removed. If it is necessary, it should take a key size from above, and not try to calculate one. Passing a zero key size into PK11_Derive will result in the CKA_VALUE_LEN parameter being unset. In this case the PKCS #11 module will create a key length of exactly the same size that the current function is trying to create.

3) pk11_concatenate_base_and_key - It is use, so you will need to pass in key size from above. Like pk11_concatenate_base_and_data, passing in a key size of 0 will get you exactly the same semantics as the current function.

4) pk11_ASNI_X9_63Derive - In here, I'd manage 3 key sizes: one that tells how many times to call the hash function, one that I pass to the next level derive function (like the 2 keyTypes you are managing), and one the preserves what has been passed in by the application. If you get a keysize of 0, you can call pk11_GetPredefinedKeyLength() with the type to get the real keysize. You can use that to determine how many calls to pk11_hash_key_derive() to make. you can set keySize2 to 0. When you set keyType2 to keyType, also set keySize2 to keySize (the one passed in). This will cause the underlying PKCS #11 module to do the right thing.

bob

Comment 13

[David Cooper]

Attachment: Revised implementation of the ANSI X9.63 KDF using PKCS #11 mechanisms

Bob,

I tried to address all of the comments that you made on the previous patch.  Here are a few notes:

1) I renamed all of the functions and variables to comply with the convention that you described, replaced calls to pk11_derive_key_from_key with calls to PK11_Derive, and tried to ensure that an NSS error code is always set when NULL is returned by a function.

2)  The function pk11_concatenate_base_and_data (now pk11_ConcatenateBaseAndData) is used within pk11_ANSIX963Derive to concatenate the SharedData with the SharedSecret to create the value to be hashed.

3) I tried to address the handling of keySize.  The only outstanding issue that I believe I have with this is that if the keySize value provided to pk11_ANSIX963Derive is 0 and the size of the key to be derived is less than or equal to the length of the hash output then the returned key will not have size=0.  However, I don't know any way around that since a specific key size has to be provided to the hash-based key derivation function whenever the desired output length is shorter than the hash length.

Comment 14

[Robert Relyea]

> 3) I tried to address the handling of keySize.  The only outstanding issue that I believe I have > with this is that if the keySize value provided to pk11_ANSIX963Derive is 0 and the size of the
> key to be derived is less than or equal to the length of the hash output then the returned key 
> will not have size=0.  However, I don't know any way around that since a specific key size has 
> to be provided to the hash-based key derivation function whenever the desired output length is
> shorter than the hash length.

So this whole keysize thing is PKCS #11 semantic shining through. If keysize == 0, then the keyType needs to completely specify the key size. PK11_GetPredefinedKeySize knows these types and and the key sizes.

Your code already handles the case where the key is less than a block, you Set keyType to keyType2. At this point you also set keySize to keySize2. The PKCS #11 module will automatically truncate the key based on keyType.

The issue is the underlying PKCS #11 module may fail if you set CKA_KEY_TYPE to a key of known size and also set CKA_VALUE_LEN.

bob

Comment 15

[Robert Relyea]

Here's what I meant for the case we are talking about:


The following will result in the correct results, change:


+	/* Hash value */
+	if (counter < maxCounter) {
+	    hashOutputSize = HashLen;
+	    hashTarget = CKM_CONCATENATE_BASE_AND_KEY;
+	} else {
+	    hashOutputSize = derivedKeySize - ((maxCounter - 1) * HashLen);
+	    if (intermediateResult == NULL) {
+		hashTarget = target;
+	    } else {
+		hashTarget = CKM_CONCATENATE_BASE_AND_KEY;
+	    }
+	}

To 
+	/* Hash value */
+	hashOutputSize = 0; /* Hash derive will return keys of hashSize if no
+	                     * Key size is specified */
+	hashTarget = CKM_CONCATENATE_BASE_AND_KEY;
+	if ((counter < maxCounter) && (intermediateResult == NULL)) {
+	    /* handle the case where the key is less than hash size and we
+	     * aren't doing a final concatenate. NOTE: in the case where
+ 	     * this is the final block and we are doing a final concatenate,
+ 	     * that final concatenate will properly truncate the key, so we
+ 	     * don't need to get a partial result here. */
+	    hashTarget = target;
+	    hashOutputSize = keySize;
+	}

bob

Comment 16

[David Cooper]

Attachment: Second revised implementation of the ANSI X9.63 KDF using PKCS #11 mechanisms

I revised the call to pk11_HashKeyDerivation based on your suggestion so that the key size specified in the call is 0 if the output will be concatenated with other data and is keySize if the output is the derived key that will be returned by the function.  Hopefully this addresses all of the issues with the code.

Comment 17

[Robert Relyea]

Comment on attachment 643577 [details] [diff] [review]
Second revised implementation of the ANSI X9.63 KDF using PKCS #11 mechanisms

r+ rrelyea

Thanks for you patience and work on this. This will be a real improvement to NSS.

bob

Comment 18

[Kai Engert (:KaiE:)]

Checking in pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.126; previous revision: 1.125
done