WebCrypto exportKey fails to export newly generated ECDH private key.

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: roustem, Assigned: ttaubert)

Tracking

(Blocks 1 bug)

36 Branch
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Posted file ecdh-exportKey.js (obsolete) —
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

5 years ago
Tim, can you help triage this? :-)
Component: Untriaged → Untriaged
Flags: needinfo?(ttaubert)
Product: Firefox → Core
(Assignee)

Updated

4 years ago
Component: Untriaged → Security
Flags: needinfo?(ttaubert)
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

4 years ago
Flags: needinfo?(ttaubert)
Attachment #8530309 - Attachment is obsolete: true
Flags: needinfo?(ttaubert)
(Assignee)

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → ttaubert
Attachment #8597263 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8597613 - Flags: review?(rlb)
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.
Attachment #8597615 - Flags: review?(rlb)
(Assignee)

Updated

4 years ago
Blocks: web-crypto
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)
Attachment #8597613 - Flags: review?(rlb) → review+
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.
Attachment #8597615 - Flags: review?(rlb) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/7627c939c0bd
https://hg.mozilla.org/mozilla-central/rev/82f24d061990
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: Security → DOM: Security
You need to log in before you can comment on or make changes to this bug.