1264771 - ECDSA crypto.subtle.sign gives InvalidAccessError sporadically in testing

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Ben Schwartz]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.7 Safari/537.36

Steps to reproduce:

Ran my application's test suite:

On x86 linux:
unzip webcrypto_test.zip
cd freedom-pgp-e2e
node_modules/karma/bin/karma start


Actual results:

All tests pass on Chrome.  Many fail on Firefox.  The failures are probabilistic.  I have not been able to reproduce them in a reduced testcase.

Comment 1

[Ben Schwartz]

webcrypto_test.zip is available here: http://bemasc.net/~bemasc/webcrypto_test.zip

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

David, can you take a look? Potentially something we have to put in the backlog. Thanks!

Comment 3

[Dana Keeler (she/her) [:keeler]]

I'm getting a 403 Forbidden on that link - looks like the permissions aren't set up quite right.

Comment 4

[Ben Schwartz]

Oops.  Fixed.

Comment 5

[Dana Keeler (she/her) [:keeler]]

Attachment: MozReview Request: bug 1264771 - randomize key IDs in WebCrypto r?ttaubert

To import private keys, WebCrypto creates a generic PKCS#11 object with a chosen
key ID with PK11_CreateGenericObject and then looks up that object as a
SECKEYPrivateKey using PK11_FindKeyByKeyID. It turns out that this is only safe
to do as long as the ID is unique. If another SECKEYPrivateKey exists that has
the same key ID (realistically this will only happen if an identical key is
imported again), PK11_FindKeyByKeyID may return the other key. Since
SECKEYPrivateKey objects are unique and not meant to be shared, this causes
problems in that when one key is destroyed, the resources backing the other key
are no longer valid, and any cryptographic operations using that key will fail.
The solution is to use random IDs and check for preexisting keys. NSS doesn't
expose an elegant API for this, but this patch implements a workaround.

Review commit: https://reviewboard.mozilla.org/r/50615/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50615/

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8748927 [details]
MozReview Request: bug 1264771 - randomize key IDs in WebCrypto r?ttaubert

https://reviewboard.mozilla.org/r/50615/#review50638

Sorry you had to wait so long for a review. This is great work, thank you!

::: dom/crypto/CryptoKey.cpp:71
(Diff revision 1)
> +DestroyPrivateKeyWithoutDestroyingPKCS11Object(SECKEYPrivateKey* key)
> +{
> +  PK11_FreeSlot(key->pkcs11Slot);
> +  PORT_FreeArena(key->arena, PR_TRUE);
> +}

I wish there was something else besides PK11_FindKeyByKeyID() that wouldn't require us to have that and just told us if there's something using the given object ID :/

::: dom/crypto/CryptoKey.cpp:101
(Diff revision 1)
> +    DestroyPrivateKeyWithoutDestroyingPKCS11Object(preexistingKey);
> +    // Try again with a new ID (but only once - collisions are very unlikely).
> +    rv = PK11_GenerateRandomOnSlot(slot, objID->data, objID->len);
> +    if (rv != SECSuccess) {
> +      return nullptr;
> +    }

Duplicating the code isn't great but I can't come up with anything better either :)

::: dom/crypto/CryptoKey.cpp:118
(Diff revision 1)
> +      if (aTemplate[i].pValue != nullptr || aTemplate[i].ulValueLen != 0) {
> +        return nullptr;
> +      }

So we're aborting private key creation here if someone gives us a template with an object ID filled in? Just wondering whether we should allow specifying an object ID, OTOH we wouldn't use it anywhere right now.

::: dom/crypto/test/test_WebCrypto_Import_Multiple_Identical_Keys.html:29
(Diff revision 1)
> +    })
> +    .then(function(exportedKey) {
> +      let keyImportPromises = [];
> +      for (let i = 0; i < 20; i++) {
> +        keyImportPromises.push(
> +          crypto.subtle.importKey("jwk", exportedKey, alg, true, ["sign"]));

nit: set export=false, we're not going to export those again

::: dom/crypto/test/test_WebCrypto_Import_Multiple_Identical_Keys.html:35
(Diff revision 1)
> +      }
> +      return Promise.all(keyImportPromises);
> +    })
> +    .then(function(importedKeys) {
> +      let signPromises = [];
> +      let data = new Uint8Array(32);

Maybe use crypto.getRandomValues() to either get random bytes once of for every signature.

::: dom/crypto/test/test_WebCrypto_Import_Multiple_Identical_Keys.html:68
(Diff revision 1)
> +    })
> +    .then(function(exportedKey) {
> +      let keyImportPromises = [];
> +      for (let i = 0; i < 20; i++) {
> +        keyImportPromises.push(
> +          crypto.subtle.importKey("jwk", exportedKey, alg, true, ["sign"]));

nit: export=false

Comment 8

[Dana Keeler (she/her) [:keeler]]

https://reviewboard.mozilla.org/r/50615/#review50638

Thanks!

> I wish there was something else besides PK11_FindKeyByKeyID() that wouldn't require us to have that and just told us if there's something using the given object ID :/

Well, we could add a function to NSS and ship it with the next version.

> Duplicating the code isn't great but I can't come up with anything better either :)

Yeah, I was thinking of doing it as a loop, but I figured it wouldn't be worth it if it made the code less clear.

> So we're aborting private key creation here if someone gives us a template with an object ID filled in? Just wondering whether we should allow specifying an object ID, OTOH we wouldn't use it anywhere right now.

Right - this was an attempt to enforce that PrivateKeyFromPrivateKeyTemplate pick the IDs, so that the caller isn't surprised when the ID they set doesn't actually get used. I guess it would be best to document this clearly.

> nit: set export=false, we're not going to export those again

Sounds good.

> Maybe use crypto.getRandomValues() to either get random bytes once of for every signature.

Ok.

Comment 9

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8748927 [details]
MozReview Request: bug 1264771 - randomize key IDs in WebCrypto r?ttaubert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50615/diff/1-2/

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> > I wish there was something else besides PK11_FindKeyByKeyID() that wouldn't require us to have that and just told us if there's something using the given object ID :/
> 
> Well, we could add a function to NSS and ship it with the next version.

The problem is that we can't just add a new PK11_ function without having to propose it to the OASIS WG :( See for example bug 1245777 and bug 1245244.

> > So we're aborting private key creation here if someone gives us a template with an object ID filled in? Just wondering whether we should allow specifying an object ID, OTOH we wouldn't use it anywhere right now.
> 
> Right - this was an attempt to enforce that PrivateKeyFromPrivateKeyTemplate
> pick the IDs, so that the caller isn't surprised when the ID they set
> doesn't actually get used. I guess it would be best to document this clearly.

Sounds good.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://reviewboard.mozilla.org/r/50615/#review50890

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1525aa98cd1

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e1525aa98cd1