1106087 - WebCrypto exportKey fails to export newly generated ECDH private key.

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[[e-mail]]

Attachment: ecdh-exportKey.js

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_1) AppleWebKit/600.2.2 (KHTML, like Gecko) Version/8.0.1 Safari/600.2.2

Steps to reproduce:

After generating a new ECDH key pair, it is possible to export the public key but not the private key in jwk format. 


Actual results:

An attempt to export the private key fails with the "OperationError: The operation failed for an operation-specific reason".


Expected results:

exportKey should have produced the jwk output.

Comment 1

[:Gijs (he/him)]

Tim, can you help triage this? :-)

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1106087-Add-test-to-ensure-we-can-export-newly-g.patch

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1106087-Add-test-to-ensure-we-can-export-newly-g.patch, v2

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1106087-Recreate-newly-generated-ECDH-private-ke.patch

Would have liked to only add a new CKA_EC_POINT attribute to the existing ScopedSECKEYPrivateKey but all helper methods seem hidden in the sftk_ API and I didn't want to fiddle around with linked lists. Recreating the private key with a new template seemed the easiest and works fine.

Comment 5

[Richard Barnes [:rbarnes]]

Comment on attachment 8597613 [details] [diff] [review]
0001-Bug-1106087-Add-test-to-ensure-we-can-export-newly-g.patch, v2

Review of attachment 8597613 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with this issue fixed.

::: dom/crypto/test/test_WebCrypto_ECDH.html
@@ +310,5 @@
> +    }
> +
> +    crypto.subtle.generateKey(alg, true, ["deriveKey", "deriveBits"])
> +      .then(doExportToJWK)
> +      .then(complete(that), error(that));

You need to check that the right fields are present.  The thing returned by exportKey needs to be an object, and it needs to have:
* x.kty == "EC"
* x.crv == "P-256"
* x.d is a string and matches the base64url character set [a-zA-Z0-9_-]
* x.x is a string and has length 45 (32 octets, base64 encoded)
* x.y is a string and has length 45 (32 octets, base64 encoded)

Comment 6

[Richard Barnes [:rbarnes]]

Comment on attachment 8597615 [details] [diff] [review]
0002-Bug-1106087-Recreate-newly-generated-ECDH-private-ke.patch

Review of attachment 8597615 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed

::: dom/crypto/CryptoKey.cpp
@@ +88,5 @@
> +  if (!privKey) {
> +    return nullptr;
> +  }
> +
> +  return SECKEY_CopyPrivateKey(privKey);

This is just moved up from below, right?  Any particular reason for the move?

Do you really need the SECKEY_CopyPrivateKey at the end?  Or can you just return privKey?

@@ +252,5 @@
>    }
>  }
>  
> +nsresult
> +CryptoKey::RecreateWithECPoint(SECKEYPublicKey* aPublicKey)

It might be helpful to add a comment here to explain why this is necessary, and to put the asserts below into prose.

This name also seems a little opaque to me, though I don't have a tremendously better suggestion.  "AddPublicKeyData"?  The recreation isn't really important at the API level, just the fact that the private key is getting augmented with data from the public key that wasn't there before.

::: dom/crypto/CryptoKey.h
@@ +115,5 @@
>    KeyType GetKeyType() const;
>    nsresult SetType(const nsString& aType);
>    void SetType(KeyType aType);
>    void SetExtractable(bool aExtractable);
> +  nsresult RecreateWithECPoint(SECKEYPublicKey* point);

Ditto comments about name.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Richard Barnes [:rbarnes] from comment #5)
> > +    crypto.subtle.generateKey(alg, true, ["deriveKey", "deriveBits"])
> > +      .then(doExportToJWK)
> > +      .then(complete(that), error(that));
> 
> You need to check that the right fields are present.  The thing returned by
> exportKey needs to be an object, and it needs to have:
> * x.kty == "EC"
> * x.crv == "P-256"
> * x.d is a string and matches the base64url character set [a-zA-Z0-9_-]
> * x.x is a string and has length 45 (32 octets, base64 encoded)
> * x.y is a string and has length 45 (32 octets, base64 encoded)

Will do, thanks. The length is 43 btw, The first 10 3-byte pairs = 40 chars. The remaining 2 bytes will yield 3 base64 characters without padding, i.e. 43 in total.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Richard Barnes [:rbarnes] from comment #6)
> > +  return SECKEY_CopyPrivateKey(privKey);
> 
> This is just moved up from below, right?  Any particular reason for the move?

Just moved it up so I can use it in CryptoKey::AddPublicKeyData() and its in one place with the other utility functions.

> Do you really need the SECKEY_CopyPrivateKey at the end?  Or can you just
> return privKey?

We can just return it probably.

> > +CryptoKey::RecreateWithECPoint(SECKEYPublicKey* aPublicKey)
> 
> It might be helpful to add a comment here to explain why this is necessary,
> and to put the asserts below into prose.

Will do.

> This name also seems a little opaque to me, though I don't have a
> tremendously better suggestion.  "AddPublicKeyData"?  The recreation isn't
> really important at the API level, just the fact that the private key is
> getting augmented with data from the public key that wasn't there before.

Yeah, that's a good suggestion.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7627c939c0bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/82f24d061990

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7627c939c0bd
https://hg.mozilla.org/mozilla-central/rev/82f24d061990