681937 - Enhance PSM's key generation strategy with tokens/escrow

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert (:KaiE:)]

Firefox is used to enroll for certificates. A CA may request that the private key is escrowed. (We display a prompt that the user must confirm.)

This bug reports a problem with the following scenario.
- a CA requesting a key of type XYZ
- the CA requesting to escrow the new key
- our software token is unable to generate keys of type XYZ,
  because the respective algorithms are not implemented
- having a supplemental token installed that 
  is able to generate keys of type XYZ

As of today, PSM assumes that we are never able to extract keys from a supplemental token. It therefore attempts to generate a temporary key using our internal code (the software token), then escrow the key, then move the key to the supplemental token, then delete our temporary internal key.

In the above scenario, this fails, because our internal code is unable to produce the key of the desired type XYZ.

This bug proposes to enhance PSM's logic to try more alternatives for generating the key.

With some supplemental tokens, such as external software tokens, it's possible to extract the keys.

Initial improvement:
- change PSM to a use two step approach when generating the key
- attempt to generate the key on the intended token,
  but when doing so, set a flag that requires the key to be extractable.
  (CKA_EXTRACTABLE).
  If the token accepts this requirement, it will succeed,
  and we are done and are able to escrow the key.
- With this flag set, if extraction is not possible,
  the key generation attempt will fail.
- Only in this failure scenario, attempt to generate the key
  using our internal code.

It might be necessary to add new APIs to NSS.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: experimental patch v1 [untested]

Comment 2

[Kai Engert (:KaiE:)]

Attachment: experimental patch v2 [untested]

Comment 3

[Robert Relyea]

Comment on attachment 555730 [details] [diff] [review]
experimental patch v2 [untested]

r- from a feedback perspective.

First, the idea is great, but there are things that are wrong or doggy with this patch:

1) PK11_GenerateExtractableKeyPair is not necessary PK11_GenerateKeyPairWithFlags() will allow you to set the extractable flag without NSS changes. You can use PK11_GenerateKeyPairWithFlags for all our cases, and simply pass in different flags. That should simplify your if's.

2) Senstive and Extractable are different animals. In general, you want the key to be Senstive (cannot be removed without encrypting). By default tokens will try to make keys both sensitive and non-extractable. Your preferred state for a permanent key is Sensitive = TRUE, even if you are escrowing it.

3) It's not necessary, but I would still generate key key in softoken if we are escrowing and softoken can do the keygen function. That would create less risk of failures with existing RSA tokens.

4) NOTE: the target token may not actually do keygen. Clearly if softoken can't do keygen, you'll have to look for another token that can and still insert the key in the target token.

bob

Comment 4

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #3)
> 
> 3) It's not necessary, but I would still generate key key in softoken if we
> are escrowing and softoken can do the keygen function. That would create
> less risk of failures with existing RSA tokens.


I don't know how to query softoken whether it will be able to generate the key.

Am I right in assuming, that you propose:
- don't query softoken ability
- instead, simply try to generate the key in the softoken
- if it works, done
- if above failed, then proceed to create on the destination token 
  with "extractable" set
- if that fails too, bad luck, failure

Comment 5

[Kai Engert (:KaiE:)]

Attachment: Patch v4

Untested, but will proceed with testing now.

Feedback welcome.

Comment 6

[Robert Relyea]

> I don't know how to query softoken whether it will be able to generate the key.

PK11_DoesMechanism(slot) ... but really we should probably call PK11_GetBestSlot() to get the mechanism directly. It will return a slot which can do the given mechanism. If it doesn't return a slot, we are done and you can fail.

bob

Comment 7

[Robert Relyea]

Comment on attachment 588982 [details] [diff] [review]
Patch v4

r+ The runnable stuff makes it a little harder to follow, but it clearly is necessary (I presume to prevent locking up the UI in keygen).

I probably would have pushed all the logic to a single function that can be called either by the runnable object or by the parent, but this patch fits with the existing code better.

The notion of looking up a different EC device isn't in this patch, but it's a much lower priority issue, I think. The use of the internal slot for RSA is important because it is the only slot we can ask for the key to be extractable on with any confidence.

bob

Comment 8

[Kai Engert (:KaiE:)]

Bob, you gave me an r+, but then you made comments which sound like you are requesting changes. I'm confused.

Comment 9

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #7)
> 
> The notion of looking up a different EC device isn't in this patch, but it's
> a much lower priority issue, I think. 


I believe this means: "patch could be improved, not important now"


> The use of the internal slot for RSA
> is important because it is the only slot we can ask for the key to be
> extractable on with any confidence.


I believe this means: "it's good that the r+ patch deals with RSA correctly".

Comment 10

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3738aa6670

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2e3738aa6670