356897 - PK11_Derive should try harder to find viable slot

Status: ASSIGNED

(NSS :: Libraries, enhancement, P5)

Description

[Julien Pierre]

I configured secmod.db with the Solaris Crypto Framework . This module supports CKM_TLS_MASTER_KEY_DERIVE .
I used an RSA private key in a token in that module, and ran selfserv.
I then ran a TLS connection with the cipher suite SSL3 RSA EXPORT WITH RC2 CBC 40 MD5 .

This caused libssl to call PK11_Derive and try to derive an RC2 key using the 
CKM_TLS_MASTER_KEY_DERIVE mechanism. But the module didn't know anything about RC2, and its C_DeriveKey returned CKR_TEMPLATE_INCONSISTENT .

At that point, NSS didn't try to move  the base key to another token . It just failed the derivation operation.

There is precedent for code that moves the base key if the token that contains it doesn't do the derivation mechanism, but it doesn't have provision for specific failures of the derivation mechanism . If the module supports the derivation mechanism, but not with all possible types of target keys, we fail, where we wouldn't necessarily have to.

I'm aware that moving the base key is expensive, not always possible, and should be avoided at all costs if possible. But I'm wondering if there is perhaps something we should do in this case and detect the error earlier, especially since the module was not configured as a default mechanism for RC2, and doesn't report any knowledge for RC2 .

Comment 1

[Julien Pierre]

Note the stack of the failure for the record.

current thread: t@3
=>[1] pk11_DeriveWithTemplate(baseKey = 0x8198240, derive = 886U, param = 0xfe61ae1c, target = 258U, operation = 260U, keySize = 16, userAttr = (nil), numAttrs = 0, isPerm = 0), line 1456 in "pk11skey.c"
  [2] PK11_Derive(baseKey = 0x8198240, derive = 886U, param = 0xfe61ae1c, target = 258U, operation = 260U, keySize = 16), line 1316 in "pk11skey.c"
  [3] ssl3_DeriveConnectionKeysPKCS11(ss = 0x818dee0), line 2764 in "ssl3con.c"
  [4] ssl3_InitPendingCipherSpec(ss = 0x818dee0, pms = 0x8172a58), line 1541 in "ssl3con.c"
  [5] ssl3_HandleRSAClientKeyExchange(ss = 0x818dee0, b = 0x819847c "", length = 66U, serverKey = 0x818ce50), line 6527 in "ssl3con.c"
  [6] ssl3_HandleClientKeyExchange(ss = 0x818dee0, b = 0x819847c "", length = 66U), line 6620 in "ssl3con.c"
  [7] ssl3_HandleHandshakeMessage(ss = 0x818dee0, b = 0x819847c "", length = 66U), line 7722 in "ssl3con.c"
  [8] ssl3_HandleHandshake(ss = 0x818dee0, origBuf = 0x818e13c), line 7793 in "ssl3con.c"
  [9] ssl3_HandleRecord(ss = 0x818dee0, cText = 0xfe61b12c, databuf = 0x818e13c), line 8056 in "ssl3con.c"
  [10] ssl3_GatherCompleteHandshake(ss = 0x818dee0, flags = 0), line 206 in "ssl3gthr.c"
  [11] ssl_GatherRecord1stHandshake(ss = 0x818dee0), line 1258 in "sslcon.c"
  [12] ssl_Do1stHandshake(ss = 0x818dee0), line 149 in "sslsecur.c"
  [13] ssl_SecureRecv(ss = 0x818dee0, buf = 0xfe61b51c "", len = 10239, flags = 0), line 1032 in "sslsecur.c"
  [14] ssl_SecureRead(ss = 0x818dee0, buf = 0xfe61b51c "", len = 10239), line 1051 in "sslsecur.c"
  [15] ssl_Read(fd = 0x8179170, buf = 0xfe61b51c, len = 10239), line 1434 in "sslsock.c"
  [16] PR_Read(fd = 0x8179170, buf = 0xfe61b51c, amount = 10239), line 141 in "priometh.c"
  [17] handle_connection(tcp_sock = 0x8179170, model_sock = 0x8083740, requestCert = 0), line 960 in "selfserv.c"
  [18] jobLoop(a = (nil), b = (nil), c = 0), line 518 in "selfserv.c"
  [19] thread_wrapper(arg = 0x817d954), line 486 in "selfserv.c"
  [20] _pt_root(arg = 0x817dba0), line 220 in "ptthread.c"
  [21] _thr_setup(0xfe790000), at 0xfebff9ae
  [22] _lwp_start(), at 0xfebffc90

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Interesting.  libSSL has a function, ssl3_config_match_init, that attempts
to determine, for each and every known cipher suite, if we have access to 
PKCS#11 tokens that implement all the mechanisms necessary for that suite.
Its purpose is to prevent us from negotiating cipher suites whose component
algorithms are not actually available to us.

Perhaps that test is wrong.  Perhaps that test should have disabled the 
RC2 cipher suite in this case, rather than negotiating it and then failing.

It checks merely to see if we have ANY token that implements each of the 
component mechanisms.  It doesn't ask if we have a single token that 
supports them all, or that supports (say) all the symmetric ciphers and 
hashes.  

That function should agree with PK11wrap.  If that function says we support
the cipher suite, then pk11wrap should succeed.  Altnatively, we can say 
that if pk11wrap fails, then that function should also indicate that the
cipher suite is not supported.  

So this is either a PK11wrap bug or an SSL bug, IMO.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

The results found by ssl3_config_match_init are interrogated by calls to 
ssl3_config_match.  ssl3_config_match and PK11_Derive should agree about
whether we can do the necessary operations, or not, IMO.

Comment 4

[Robert Relyea]

If this is a server side problem, the token the operation is done is is determined by where the RSA private key lives.

Without any deep research, I would guess Julien's initial look is correct, Derive should try to move the key. (Unwrap already does something similiar where it moves the key if the target can't do the requested operation).

It's also possible that Unwrap had moved the key looking for a CKM_TLS token, but not one that can also do RC2.

bob

Comment 5

[Julien Pierre]

Bob,

The SCF token can do key derivation for both SSL and TLS, so the master secret key didn't get moved. The problem is that the derivation can only derive certain key types, not including RC2 keys (or AES-256 keys). The code in PK11_Derive only looks for a slot with the derivation mechanism, and ignores the target key type or the key size.

For the problem of the key type, it should be simple to optimize the slot location algorithm and predict that the derivation will fail if the slot for the derivation doesn't support the mechanism for the target key type. C_GetMechanismList returns the mechanism list for the slot, and we record that mechanism list early on at module load time, so we can check the list at any time without calling C_GetMechanismList each time. I'm going to write some code to interrogate that list. I think it makes sense to move the key there, since PK11_Derive already  tries to move the base key in other sitations.

For the problem of the key size, it is trickier. C_GetMechanismInfo returns the information about minimum and maximum key size. Unfortunately, we don't call that function at module load time. So, the information about minimum and maximum key size is not readily available before derivation. It would be be expensive to query that info each time. I think we need to cache this info in the slot structure, in addition to the mechanism list.

Comment 6

[Julien Pierre]

Attachment: patch for PK11_Derive . No caching of mechanism info

This patch takes the following approach to fix PK11_Derive :
1) Checks the original slot for supported derive mech (already there)
2) Checks the original slot for supported target key mech and size . This is done with a new function, PK11_DoesMechanismWithLen . It is the same as PK11_DoesMechanism, except for an extra call to C_GetMechanismInfo .
3) If either of those tests fail, PK11_Derive tries to move the base key to a new slot. The algorithm to find that slot is new .
It calls PK11_GetBestSlotMultipleWithLen . This is a new variant of PK11_GetBestSlotMultiple that also checks for key length, by taking an extra array of required lengths as input, and calling PK11_DoesMechanismWithLen instead of PK11_DoesMechanism .
I did not duplicate any code - PK11_GetBestSlotMultiple now calls PK11_GetBestSlotMultipleWithLen, but passes a NULL length array .

For most cases where no key movement is needed, this patch causes one additional call to C_GetMechanismInfo . This could be fixed in the future by caching.

For cases where key movement is needed, many of them didn't work before and will now work. There may be several C_GetMechanismInfo calls for those cases. I verified that both the SSL RC2 and AES-256 cipher suites work even if the server's RSA private key is located within the SCF token that doesn't do these mechanisms or key sizes. I also checked in the debugger that for other mechs, such as AES-128, the master secret key is not moved and the derivation happens correctly in the SCF token.

Comment 7

[Julien Pierre]

Attachment: small fix

There was a bug where targetKeyMech could be used uninitialized . I just moved the initialization early, since the value is always needed.
Also fixed the headers and some compiler warnings.

Comment 8

[Julien Pierre]

I would like this bug to be about the PK11_Derive function in pk11wrap only, so I'm changing the summary. Even though libssl was affected here, this function is a public one, so this bug may affect more than just libssl.

We can fix issues specific to the SSL layer and the configuration as part of a different bug. I suggest we use bug 356623 which is very closely related - it talks about error conditions in SSL in some cases of PK11_Derive failures that I'm fixing here . The reason I filed 2 separate bugs was because I think there were 2 separate layers with issues - one layer with the root cause, pk11wrap, and one layer that I see as the victim, libssl. I am only attempting to fix the pk11wrap issue here.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

As I understand it, the immediate cause of failure here was a failure of one
of these mechanisms: CKM_TLS_KEY_AND_MAC_DERIVE, CKM_SSL3_KEY_AND_MAC_DERIVE
in the particular module, due to the fact that the module did not support 
the specific combination of crpto algorithm and key size for that algorithm.

In selecting a module to do either of those mechanisms (from among several
that claim to do them), we much somehow choose a module that supports both
of the specific algorithms (cipher, MAC) and their respective key sizes.  
Today, the code does not consider all those factors when selecting a module
and slot to do those operations. 

IINM, today we look for a slot that implements the two above-named mechanisms
and effectively assume that those KEY_AND_MAC_DERIVE mechanisms will work for
all relevant underlying crypto algorithms and key sizes.  Apparently that's
an inadquate selection criteria.  

Comment 10

[Robert Relyea]

1. So I'm a little worried about using the PKCS length parameter in PKCS #11.
Several early tokens didn't agree on the meaning of length (was it bits or bytes) that is now sorted out based on the mechanism, but that introduces 2 new issues:
   Issue 1: does the module use the correct 'flavor' of bit/bytes, or is the module 'old' and use inconsistant flavors.
   Issue 2: Are we using the appropriate value for length in the comparison. The current patch contains no check to see if length is supposed to be bits or bytes for the mechanism we are checking against. RC2, RC4 are in bits, RC5, AES, CAST, CAST3, CAST125 are in bytes, DES, DES3, IDEA, CDMF, the length fields are unused.
Finally some tokens have not 'limits' and may use '0' to indicate that.

These 2 issues are the primary reason I haven't done anything with length yet. I think in this context there may be hope to come up with something that works reasonably well, but it will need more care than the current code.

2. I'm really worried about calling C_GetMechanismInfo() in PK11_GetBestSlotMultipleWinLen() without caching. If we hit a slow token, it will affect any operation that uses PK11_GetBestSlotMultipleWithLen. (I'm not so worried about the PK11_DoesMechanismWithLen() because that call is only affected by the single token you are calling it on). [Caching could be a way around the issues of keySize, we could regularize our internal representation of keysize when we build the cache].

Question, is it a key length problem you are running into? If not, we should dispense with this.

Comment 11

[Julien Pierre]

Bob,

Re: comment 10,

1.

If the module isn't using the right len in C_MechanismInfo, wouldn't the derive operation also fail currently, since we pass the key length to C_DeriveKey already ? It seems to me that modules should be consistent about their handling for key size for both C_DeriveKey and C_MechanismInfo .

Regarding the fact that some key sizes are in bits and others in bytes, does PK11_Derive need to be aware of that, or is it only the caller of PK11_Derive that needs to know, since it passes the keySize value ? Again, consider the fact that C_DeriveKey works with the current values.

How common do you think the inconsistency is ?

2.

I'm running into 2 problems :
SCF doesn't support RC2.
SCF supports AES, but only up to 128 bits, not 256, due to export restrictions.
This is why the key length part of the fix is needed.

I think the caching would be a good improvement to make, I just didn't have the time to do it yesterday. It is not that hard to do and should probably be done in PK11_InitToken I believe . I didn't run any performance test to measure the impact of this extra call in PK11_Derive (which is due to PK11_DoesMechanismWithLen). 

Currently, only PK11_Derive calls PK11_GetBestSlotMultipleWithLen, so it is the only function that will suffer from the extra C_GetMechanismInfo calls.
Other callers call PK11_GetBestSlotMultiple (without length), which calls PK11_GetBestSlotMultipleWithLen with a NULL length array. When that happens, PK11_GetBestSlotMultipleWithLen calls PK11_DoesMechanismWithLen with a zero length argument, which means that it won't check the length, and won't call C_MechanismInfo ; ie. it will be the same as PK11_DoesMechanism .

Thus, in its current state my patch doesn't affect the behavior of any existing function except PK11_Derive .

Nelson,

Re: comment 9,

Yes, you accurately described the current behavior - PK11_Derive only checks for the presence of the derivation mechanism today. My patch adds the criteria of the mechanism associated with the key type, and the key size as well. However, in the case where multiple keys of different types are generated, such as for SSL, I'm not sure where to get the type of the MAC keys from, so that's currently not considered in my patch.

Comment 12

[Robert Relyea]

> If the module isn't using the right len in C_MechanismInfo, wouldn't the derive
> operation also fail currently, since we pass the key length to C_DeriveKey
> already ? It seems to me that modules should be consistent about their handling
> for key size for both C_DeriveKey and C_MechanismInfo .

The info from C_MechanismInfo is usually just some static informational data, you really can't presume that any given module uses that data itself in it's internal state (I don't think softoken does, for instance).

> Regarding the fact that some key sizes are in bits and others in bytes, does
> PK11_Derive need to be aware of that, or is it only the caller of PK11_Derive
> that needs to know, since it passes the keySize value ? Again, consider the
> fact that C_DeriveKey works with the current values.

No, keySize and CKA_VALUE_LEN are always in bytes.

> How common do you think the inconsistency is?

Betwen algorithms, I know the inconsistency is large (Just look at the list I gave you). Within modules, I have always treated the sizes as dubious. In the context of symetric key derive.. I think we are better. The type of tokens that implement the TLS_XXXX and SSL_XXX key derive functions tend to be more consistant than your average smart card token.

> Thus, in its current state my patch doesn't affect the behavior of any 
> existing function except PK11_Derive .

PK11_Derive happens between 2 and 3 times every SSL connection (even restarts). One slow token could have a large impact if we have to call PK11_GetBestSlotMultipleWithLen()... Though I think the bit/byte/zero length thing itself is enough to cache the results. There's a function called at slot init time that fetches the mechanism. I can also fetch the mechanism info as well.

Anyway, the upshot is: 1) the patch will not work as coded (the units for keySize do not always match the units for the mechanism info), 2) The key sizes specifed in the mechanism info are fragile, so we must use them with caution, and 3) SymKey Derive is a reasonable place to start looking at using lengths.

bob

Comment 13

[Julien Pierre]

Bob,

Re: comment 12,

1) I take exception that the patch doesn't work :)

all.sh is all green with attachment 242597 [details] [diff] [review], which checks they keySize fails within the range of the values returned by C_GetMechanismInfo . If C_GetMechanismInfo is supposed to return limits in different units than are acceptable for CKA_VALUE_LEN in C_DeriveKey, then I would say that it is probably not doing so at the moment. That may be a bug in our C_GetMechanismInfo return values. I will investigate.

2) I don't know how to fix this bug without looking at the key size limits from C_GetMechanismInfo . There is no other place for us to get the valid range.

If we need to make an exception to use the C_GetMechanismInfo info only for  CKM_TLS_MASTER_KEY_DERIVE and CKM_SSL3_KEY_AND_MAC_DERIVE, then I could write an alternate patch for PK11_Derive to do that .

3) Starting to use the C_GetMechanismInfo limits in symkey derive might be a good thing to do, but it would do nothing to fix this bug unfortunately.

Comment 14

[Julien Pierre]

Re: comment 13,

Our softoken claims to support 1-256 for RC4, and 1-128 for RC2 . These would certainly appear to be in bits, though I'm not sure that 1 bit key lengths ever makes sense ;)

Solaris Crypto Framework claims to support 8-128 bits for RC4, and doesn't do RC2.

As it turns out, we always specify a key size of 16 bytes, ie. 128 bits, when doing a derive for RC2 and RC4. The 16 falls into the range of valid value in bits for both RC2 and RC4 in our softoken. So this is why all.sh passes.

Are RC2 and RC4 the only 2 mechanisms for which C_GetMechanismInfo returns sizes in bits ? It seems more logical to use bits than bytes ...

I also tested 3DES. Solaris Crypto Framework returns a supported size of 24 (in bytes). 24 also happens to be the the value that we pass as CKA_VALUE_LEN to C_DeriveKey . So, it all works. But the length doesn't appear to be unused for 3DES.

My code already handles the case where the value is unused - if 0 is passed in as length to PK11_DoesMechanismWithLen, then it doesn't call C_GetMechanismInfo.
I could also change it to assume that if C_GetMechanismInfo returns 0 for both minimum and maximum key lengths, then there are no limits. That should take care of the "unused" cases.

Comment 15

[Robert Relyea]

Julien, please read my comments carefully. It makes suggestions on how to proceed. The current patch will leave a broken PK11_Derive with certain tokens (which themselves aren't necessarily broken). It is very possible that we don't trip over the brokenness in the current code.... until you plug in a different token.

Basically if you need to know the key lengths, then the only way to proceed is to build a mechanism to determine the supported key lengths. This mechanism can use Mechanism Info, but in needs to massage the output, it can't use it blindly. You will probably need a table much like the switch statements that determine how to build the parameters for a given Mechanism, or how to find the keyType for a given mechanism.

One last thing you should be aware of: some tokens only support specific enumerated lists of key sizes. The current PKCS #11 spec has no way to tell you that the list of keys not contiguous (some tokens may support 768, 1024 and 2048 bit RSA keys, but not 1000 bit Keys, for instance). If we really need to know *this* kind of information, we have a lot more work to do.

bob

Comment 16

[Julien Pierre]

Bob,

Do we have a function that tells us which mechs are in bits or bytes for C_GetMechanismInfo by any chance ? This would greatly help with the next patch, vs reading the entire PKCS#11 spec. Unfortunately the bits/bytes info isn't in the table of mechs ...

For the non-contiguous cases, the only to find out if it fails would be to try the derive in a slot where the key size is in the range, and see if it fail, and if it does, try to move the base key to another slot with matching properties. However, one would need a PK11_GetBestSlotsMultipleWithLen (note the plural of slots) in order to implement that, so the code would be different.

Also, this could turn out to be very expensive - how many times do we want to switch slots before we give up if it doesn't work the first time around ? I hope we don't have to go there.

Comment 17

[Christophe Ravel]

The target milestone is already released. Resetting target milestone.

Comment 18

[Julien Pierre]

Let's take care of this in 3.12 .

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.