802429 - NSS does not respect cipherOrder when evaluating modules to use to perform operations

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Ryan Sleevi]

https://developer.mozilla.org/fr/docs/PKCS11_Module_Specs documents cipherOrder (incompletely), which suggests it may be used to prioritize the manner in which modules are used to perform cryptographic operations.

However, while cipherOrder is parsed, and is stored on the SECMODModule, it is not actually used to influence the order in which modules or slots are evaluated.

As a result, in the presence of two slots that support a given algorithm, one "fast" (such as the internal slot, with a cipherOrder of "use first" - presumably, the higher the better?), and one slow (such as a hardware token with a default cipher order), NSS will prefer the first slot it finds, which, due the fact that recently added slots are moved to the head of the slot linked list ( http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11slot.c&rev=1.113&mark=172#172 ), will typically be the slow slot.

Comment 1

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

In general, I think Firefox and Thunderbird would prefer to have all operations happen in the internal slot whenever possible--i.e. whenever we're not doing an operation on a key in a smartcard, so I think that it makes sense for the internal slot to be first in the search order. I'm not sure what problems this might cause though.

Comment 2

[Robert Relyea]

So here's the current semantic

1) NSS prefers the token where the key for the given operation lives.
2) If there is no existing key, NSS prefers the default token for the given operation. By default this is the softoken.
3) If softoken can't do the operation, the selected token is effectively random.

Cipher order was meant to deal with case 3, but we only got as far as storing the number in our database. It turns out that you seldom end up falling through to case 3. Usually softoken can handle the cipher, and if you want to override softoken to use some hardware accellerator, you can make the hardware accellerator be the default device. The only cases that are squirrely is if you have more than one default device or if softoken doesn't do the requested cipher and you have more than one token that does (and neither are default).

bob

Comment 3

[Ryan Sleevi]

Unfortunately Bob, while I agree that sounds correct for key-based operations and is what I've observed with the "force slot" logic, it does not seem correct for keyless operations (such as hashing, which is specifically documented in cipherOrder)

Of course, I could be completely reading the stacktraces wrong, but I've definitely got backtraces of ChromeOS where it's triggering our "Oh God, this operation is taking forever, kill me now" logic, and the "this operation is taking forever" is where it's trying to use the ChromeOS TPM to perform SHA-256 hashing, rather than softoken.

The TPM is *not* set as default for any operation. It is merely given the publicCerts flag - no other per-cipher flags. So one would presume that softoken remains the default - unless the "other" bug is that all tokens are considered default, and some are more default than other.

The call stacks are:
HASH_HashBuf ( http://mxr.mozilla.org/security/source/security/nss/lib/cryptohi/sechash.c#280 )
HASH_Create ( http://mxr.mozilla.org/security/source/security/nss/lib/cryptohi/sechash.c#305 )
sha512_NewContext 
sha512_NewContext ( http://mxr.mozilla.org/security/source/security/nss/lib/cryptohi/sechash.c#78 )
PK11_CreateDigestContext ( http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11cxt.c#371 )
PK11_GetBestSlot ( http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11slot.c#2107 )
PK11_GetBestSlotMultipleWithAttributes ( http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11slot.c#2013 )
PK11_GetSlotList ( http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11slot.c#801 )

which returns &pk11_sha256SlotList ( http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11slot.c#91 )

pk11_sha256SlotList starts as an empty slot, and then as modules are added that support SHA-256, expands according to PK11_AddSlotToList ( http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11slot.c#91 )

The first module added to support SHA-256 is softoken.
The second module added to support SHA-256 is the TPM.

Because PK11_AddSlotToList performs head-insertion, rather than tail insertion, the softoken slot is preferred last.

I've got two different but "equivalent" patches that I'm testing.

One is to simply make PK11_AddSlotToList perform tail-insertion rather than head-insertion. This is the common case where softoken is the first slot in the first module to be added.

The other is to perform insertion sort in the slot list, according to cipherOrder. This ensures that all PK11SlotLists are ordered according to cipherOrder, WITH THE EXCEPTION of PK11_GetAllTokens, which orders on two dimensions

Slots that do not require login
  - ordered by cipherOrder
Slots that require login, but are friendly
  - Ordered by cipherOrder
Slots that require login, but are not friendly

Comment 4

[Ryan Sleevi]

Attachment: Add new slots to the tail of the slot list

Comment 5

[Robert Relyea]

Hmmm, I suspect that's because there are is no default table for sha-2 hashes, which we should fix.

I also don't have a problem actually implementing cipherOrder. It should be implemented by using it to order the slotlists.

bob

Comment 6

[Ryan Sleevi]

Attachment: Make the NSS internal slot the default slot for SHA256 and SHA512 operations

Bob, you're right that we're not making the internal slot the default slot (as returned by PK11_GetSlotList) for SHA256 and SHA512.

The disconnect between the two is found at http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11slot.c#32 compared with http://mxr.mozilla.org/security/source/security/nss/lib/util/utilpars.c#528

I'm a little concerned with the disconnect between "SHA-1" and "SHA1", but I don't see the former being used.

Comment 7

[Robert Relyea]

Comment on attachment 673369 [details] [diff] [review]
Make the NSS internal slot the default slot for SHA256 and SHA512 operations

r+

Also let's add the 384 and 224 entries to the table. (they are currently commented out).

The strings in the table are meant for output.

I'm also OK with a patch that orders the slot list based on the cipher order value.

bob

Comment 8

[Robert Relyea]

Actually never mind the 384 and 224 entries, they are already handled in GetSlotList.

Comment 9

[Robert Relyea]

Comment on attachment 672625 [details] [diff] [review]
Add new slots to the tail of the slot list

r-

Unfortunately we can't do this, or we'll break the way default works.
We could look at cipher order and add the slot to the list based on sipher order.

bob

Comment 10

[Ryan Sleevi]

Attachment: Sort on cipherOrder when adding

Bob, I'm not sure if this addresses your concern for defaults any better - in both cases, the ordering changes with defaults. In the current tree, it was whatever was the most-recently added module that said it was default. In the proposed tail patch, it became whatever the first-added module that said it was default was. In this patch, it goes on cipherOrder, but the common behaviour will be to put the internal module first (eg: same as tailOrder), because the internal module puts it cipherOrder at 100, while external modules default to a cipherOrder of 0 (assuming I'm reading correctly - I can easily be mistaken)

Either way, this does a simple insertion sort on the module's cipherOrder when adding to lists, EXCEPT in the case of returning all lists (in which cases, it uses the logged in/friendly logic first, then sorts by cipherOrder in there)

Note I renamed a PK11 function to pk11, if only to provide a clearer signal that this is an "internal only" function (though it's not exported today, I think it might be tempting if it was called PK11 as the other public functions are).

If you are not comfortable with the change of behaviour to PK11_AddSlotToList, I could separate it out into another function, but it seemed to me that we should always be respecting cipherOrder as the documentation suggests.

Comment 11

[Robert Relyea]

All else being equal, I think your patch is right.

The problem we need to address, however, is how not to regress anything. I think the best way to do this would be to include your patch, but also bump up the cipherOrder from any token with a defaults value which is non-zero by 101.

This means: If you use defaults, your token will continue to have it's current precedence. If you don't use defaults, cipher order now allows you to control the order more carefully.

In the 90% case, if softoken does the function, softoken will be the default provider. You can still use defaults to override softoken like you can today. If softoken doesn't do the function, then everything is strictly by cipher order (which will usually mean softoken unless you did something special when you installed your module.).

It will call for a bit of explanation, some things won't be obvious (like if you install a token that does everything softoken does, and set it's cipher order to 150, softoken will still be use for anything with a default value).

bob

Comment 12

[Ryan Sleevi]

I'm not sure I parsed that last paragraph for the edge case you were intending.

I had been (perhaps incorrectly) assuming that you meant that every module **but softoken** has the cipherOrder set to +101 if it has any non-zero defaults value.

Under such a scheme, it would be have as follows:
If you added a token, with a cipherOrder of 150, and no defaults (effective cipherOrder 150), softoken continues to be used for all defaults (effective cipherOrder 100)
If you added a token, with a cipherOrder of 150, and defaults set (effective cipherOrder 251), the token will be used for all defaults that its set for
If you added a token, with a cipherOrder of 0, and defaults set (effective cipherOrder 101), the token will be used for all the defaults that its set for

Is there concern with the above? Is there some layering concern that I'm missing that doesn't make it possible to exclude softoken from the +101 boost?

Comment 13

[Ryan Sleevi]

Er, I suppose the downside of the above is

If you added a token, with a cipherOrder of 0, that supports algorithms X, Y, and Z, with a defaults for X, then the token will also be preferred for algorithms Y and Z (where Y and Z are any algorithm other than http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11slot.c#32 )


Or I can just do some form of head-order insert for explicit defaults in PK11_LoadSlotList/PK11_UpdateSlotAttribute, but ordered insert for all other slot lists...

Still, I don't like the "new" edge case that it implies, which is that "defaults" always trumps "cipherOrder". That feels a little weird (even though that IS the current effective behaviour)

Comment 14

[Robert Relyea]

Now that I think about it, I don't like the bumping up of cipher order.

What we want is the following semantic:

Default case: if there is a default cipher, softoken is first by default unless some other token explicitly sets itself as the default. In that later case the other token should be selected. Upshot: Softoken is always on the defaults array, and softoken should always be last on that array.

Non-default case: strictly by cipher order (and by default softoken should be higher than other tokens).

default softoken value: 100
default installed token order: 0


The complication is that PK11_AddSlotList is called in both the 'default ordering' cases as the 'pick the token from the air' cases.

Here are the cases where we use PK11_AddSlotList (which is private BTW):

PK11_GetAllSlotsForCert():   list is returned in a random order, so reordering should not be a problem. (In this case the caller wants all the slots and will operate on all the slots, or choose the desired slot by his own criteria).
PK11_FindSlotsByName():  again the list is unordered like the above, so reordering isn't a problem.
PK11_LoadSlotList(): This is adding to the defaults array, so it should have the defaults semantic.
PK11_UpdateSlotAttribute(): This is a defaults array, so it should have the defaults semantic.
PK11_GetAllTokens(): This list should be sorted by cipher order (within the 'isloggedin/need login/isfriendly' semantic).

So we have a couple of options:

1) Create a new sort for just PK11_GetAllTokens().
2) Always sort by cipher order, but add code that pushes softoken to the bottom (cipher order = -1) if we are adding to the defaults array (PK11_LoadSlotList/PK11_UpdateSlotAttribute).

Option 1 is less disruptive. Option 2 handles the case when you have more than one new default token.

bob

Comment 15

[Ryan Sleevi]

Attachment: Updated patch to optionally sort when using PK11_AddSlotToList

Updated patch. For PK11_GetSlotList, keeps head inserts, while for PK11_GetAllSlots, keeps cipherOrder-sorted inserts.

Note: I opted for a PRBool here, to indicate sorting, rather than requiring the caller to supply the cipherOrder themselves (slot->module->cipherOrder), as a way of keeping call sites clean. The alternative approach would have been to have every caller supply the desired cipherOrder, and to pass ULONG_MAX for every insert. However, it feels wrong that the insert order would be so fundamentally different than comparisons on future inserts (which would fall back to slot->module->cipherOrder), hence why I opted for the bool.

Comment 16

[Robert Relyea]

Comment on attachment 674026 [details] [diff] [review]
Updated patch to optionally sort when using PK11_AddSlotToList

So you've implemented case 1. I think that's certainly the easiest.

r+, but incomplete.
There is also a call in  pk11cert.c.

Also the calls in PK11_GetAllSlotsForCert (pk11cert.c- one call) and PK11_FindSlotsByName (pk11slot.c - two calls) are not looking for slots for use in various ciphers, but for other criteria. They are currently unordered, and only applications that intend to search the whole list will use them (there are already calls that return a single slot given a name, or a single slot for a cert). I think PR_FALSE, then would have the higher compatibility, but PR_TRUE isn't wrong (just a little less efficient and a little more likely to change things out from under the application).

bob

Comment 17

[Ryan Sleevi]

Attachment: Updated with compile fix

Forgot to touch coreconf/coreconf.dep when compiling, which is why I missed the extra PK11_AddSlotToList call.

While I agree that going with PR_TRUE or PR_FALSE for the referenced functions shouldn't matter either way, my inclination is to always prefer they be ordered by cipherOrder (even when dealing with keys on tokens), unless otherwise specified (eg: DEFAULTs)

This is the existing r+'d patch with the one extra call fix.

Comment 18

[Robert Relyea]

> While I agree that going with PR_TRUE or PR_FALSE for the referenced functions shouldn't matter 
> either way, my inclination is to always prefer they be ordered by cipherOrder (even when dealing > with keys on tokens), unless otherwise specified (eg: DEFAULTs)

That's fine, though my inclination is to reduce the chances of breaking clients unless there's a clear reason to. I'm pretty sure that PR_TRUE won't break clients, but I know PR_FALSE won't.

Comment 19

[Robert Relyea]

Comment on attachment 674049 [details] [diff] [review]
Updated with compile fix

r+ rrelyea

Comment 20

[Ryan Sleevi]

Committed both patches for 3.14.1

Checking in lib/pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.183; previous revision: 1.182
done
Checking in lib/pk11wrap/pk11priv.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v  <--  pk11priv.h
new revision: 1.19; previous revision: 1.18
done
Checking in lib/pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.114; previous revision: 1.113
done
Checking in lib/util/utilpars.c;
/cvsroot/mozilla/security/nss/lib/util/utilpars.c,v  <--  utilpars.c
new revision: 1.4; previous revision: 1.3
done

Comment 21

[Ryan Sleevi]

Committed both patches for 3.14.1

Checking in lib/pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.183; previous revision: 1.182
done
Checking in lib/pk11wrap/pk11priv.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v  <--  pk11priv.h
new revision: 1.19; previous revision: 1.18
done
Checking in lib/pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.114; previous revision: 1.113
done
Checking in lib/util/utilpars.c;
/cvsroot/mozilla/security/nss/lib/util/utilpars.c,v  <--  utilpars.c
new revision: 1.4; previous revision: 1.3
done