Closed Bug 1025230 Opened 10 years ago Closed 10 years ago

Allow import/export of JWK-formatted keys in WebCrypto

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
relnote-firefox --- 33+

People

(Reporter: rbarnes, Assigned: rbarnes)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 5 obsolete files)

      No description provided.
Blocks: web-crypto
Assignee: nobody → rlb
Attached patch bug-1025230.0.patch (obsolete) — Splinter Review
* There are two major pieces to this patch
  * CryptoKey.cpp: NSS interactions to insert/extract keys
  * WebCryptoTask.cpp: Integration with WebIDL and import/export processes
* Tests
  * Import and use of a symmetric key
  * Import and use of an RSA private key
  * Import and use of an RSA public key
  * Import failures due to controls on algorithm, usages, extractability
  * Export of symmetric, private, and public keys
* More mundane aspects
  * WebIDL updates & plumbing through
  * New constants for JWK
  * Encoding / decoding of JWK's flavor of Base64
  * Translation from KeyAlgorithms to JWK 'alg' values
  * Minor refactoring of import/export processes

Summary per-file:

# WebIDL updates
dom/webidl/SubtleCrypto.webidl - Add JsonWebKey and update importKey
dom/base/SubtleCrypto.cpp - Plumbing through of WebIDL change to importKey
dom/base/SubtleCrypto.h - Plumbing through of WebIDL change to importKey
dom/crypto/WebCryptoTask.h - Plumbing through of WebIDL change to importKey

# Tests
dom/crypto/test/test-vectors.js - Add JWK forms of some test keys
dom/crypto/test/tests.js - Add tests for JWK importKey/exportKey

# JWK-awareness for KeyAlgorithm sub-classes
dom/crypto/KeyAlgorithm.h - Add method to translate to JWK 'alg' values
dom/crypto/KeyAlgorithm.cpp - Default ToJwkAlg returns empty string
dom/crypto/AesKeyAlgorithm.h - Override KeyAlgorithm::ToJwkAlg
dom/crypto/AesKeyAlgorithm.cpp - Translate to "alg" values "A*GCM", "A*CTR", "A*CBC"
dom/crypto/HmacKeyAlgorithm.h - Override KeyAlgorithm::ToJwkAlg
dom/crypto/HmacKeyAlgorithm.cpp - Translate to "alg" values "HS*"
dom/crypto/RsaHashedKeyAlgorithm.h - Override KeyAlgorithm::ToJwkAlg
dom/crypto/RsaHashedKeyAlgorithm.cpp - Translate to "alg" values "RS*"

# Other minor changes
dom/crypto/CryptoBuffer.cpp - Methods to support JWK Base64 encode/decode
dom/crypto/CryptoBuffer.h - Methods to support JWK Base64 encode/decode
dom/crypto/CryptoKey.cpp - ToJwk/FromJwk methods and better usage handling
dom/crypto/CryptoKey.h - ToJwk/FromJwk methods and better usage handling
dom/crypto/WebCryptoCommon.h - Additional JWK constant values; CloneData method
dom/crypto/WebCryptoTask.cpp - Integration into importKey and exportKey task
security/manager/ssl/src/ScopedNSSTypes.h - Add ScopedPK11GenericObject
Attachment #8454937 - Flags: review?(dkeeler)
Attachment #8454937 - Flags: review?(bzbarsky)
Comment on attachment 8454937 [details] [diff] [review]
bug-1025230.0.patch

Clearing reviews because I want to rebase this on top of Bug 1026398 and Bug 1034851, to get RSA-OAEP wrap/unwrap support and make sure it works properly with JWK.
Attachment #8454937 - Flags: review?(dkeeler)
Attachment #8454937 - Flags: review?(bzbarsky)
Attached patch bug-1025230.1.patch (obsolete) — Splinter Review
This version incorporates support for JWK wrapKey and unwrapKey, but has to do some very ugly stuff to stringify a dictionary for wrapKey.  It works, but suggestions for more elegant solutions very welcome.
Attachment #8454937 - Attachment is obsolete: true
Attachment #8455085 - Flags: feedback?(bzbarsky)
Attachment #8455085 - Flags: feedback?(bugs)
Comment on attachment 8455085 [details] [diff] [review]
bug-1025230.1.patch

Is there a reason not to use ToJSValue followed by JS_Stringify?
Attachment #8455085 - Flags: feedback?(bzbarsky)
No, I just didn't know about ToJSValue.  I'll give that a shot.
Comment on attachment 8455085 [details] [diff] [review]
bug-1025230.1.patch

bz answered
Attachment #8455085 - Flags: feedback?(bugs)
Attached patch bug-1025230.2.patch (obsolete) — Splinter Review
Now using ToJSValue and JS_Stringify.
Attachment #8455085 - Attachment is obsolete: true
Attachment #8455724 - Flags: review?(dkeeler)
Attachment #8455724 - Flags: review?(bzbarsky)
Comment on attachment 8455724 [details] [diff] [review]
bug-1025230.2.patch

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

I reviewed the NSS-related code. The only issues I saw were that everything from NSS should be null-checked before use and that stack-allocated SECItems passed to NSS functions as pointers can have memory allocated to them that needs to be freed (which is quite a bummer, but there you go). r=me with those fixed. I also noted some style nits and a suggestion for an additional test.

::: dom/crypto/CryptoBuffer.cpp
@@ +86,5 @@
> +  nsCString temp = NS_ConvertUTF16toUTF8(aBase64);
> +  temp.StripWhitespace();
> +
> +  // Re-add padding
> +  if      (temp.Length() % 4 == 1) {

just one space after "if"

@@ +88,5 @@
> +
> +  // Re-add padding
> +  if      (temp.Length() % 4 == 1) {
> +    return NS_ERROR_FAILURE; // bad Base64
> +  } else if (temp.Length() % 4 == 3) {

no else after return

@@ +94,5 @@
> +  } else if (temp.Length() % 4 == 2) {
> +    temp.AppendLiteral("==");
> +  }
> +
> +  // Translate from URL-safe character set to normal

Use temp.ReplaceChar (and then verify that it actually replaces all occurrences of the target character - the documentation is unclear)

@@ +134,5 @@
> +  // Strip whitespace and padding
> +  base64.StripWhitespace();
> +  base64.Trim("=");
> +
> +  // Translate to the URL-safe charset

base64.ReplaceChar

::: dom/crypto/CryptoKey.cpp
@@ +388,5 @@
> +
> +  // Compute the ID for this key
> +  // This is generated with a SHA-1 hash, so unlikely to collide
> +  ScopedSECItem nItem(n.ToSECItem());
> +  ScopedSECItem objID(PK11_MakeIDFromPubKey(nItem.get()));

These should be null-checked before they're used

@@ +396,5 @@
> +  CK_KEY_TYPE rsaValue = CKK_RSA;
> +  CK_BBOOL falseValue = CK_FALSE;
> +  const size_t keyTemplateSize = 14;
> +  CK_ATTRIBUTE keyTemplate[14] = {
> +    {CKA_CLASS,             &privateKeyValue,        sizeof(privateKeyValue)},

nit: space after/before {/}

@@ +414,5 @@
> +  };
> +
> +
> +  // Create a generic object with the contents of the key
> +  ScopedPK11SlotInfo slot(PK11_GetInternalSlot());

We should probably null-check slot too, just in case.

@@ +417,5 @@
> +  // Create a generic object with the contents of the key
> +  ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
> +  ScopedPK11GenericObject obj(PK11_CreateGenericObject(slot.get(),
> +                                                       keyTemplate,
> +                                                       keyTemplateSize,

Let's use PR_ARRAY_SIZE or similar here

@@ +435,5 @@
> +}
> +
> +bool ReadAndEncodeAttribute(SECKEYPrivateKey* aKey,
> +                                CK_ATTRIBUTE_TYPE aAttribute,
> +                                Optional<nsString>& aDst)

nit: indentation

@@ +438,5 @@
> +                                CK_ATTRIBUTE_TYPE aAttribute,
> +                                Optional<nsString>& aDst)
> +{
> +  SECItem item;
> +  SECStatus ss = PK11_ReadRawAttribute(PK11_TypePrivKey, aKey, aAttribute, &item);

"srv" or "status" is more common than "ss", I think (in any case, you probably don't need a separate variable for this)
Also, PK11_ReadRawAttribute can allocate memory that gets owned by item, so this needs to be cleaned up.

@@ +458,5 @@
> +
> +nsresult
> +CryptoKey::PrivateKeyToJwk(SECKEYPrivateKey* aPrivKey,
> +                JsonWebKey& aRetVal,
> +                const nsNSSShutDownPreventionLock& /*proofOfLock*/)

nit: indentation

@@ +512,5 @@
> +    SECItem n;
> +    SECItem e;
> +  };
> +  const RSAPublicKeyData input = {
> +    {siUnsignedInteger, n.Elements(), (unsigned int) n.Length()},

nit: spaces

@@ +534,5 @@
> +
> +nsresult
> +CryptoKey::PublicKeyToJwk(SECKEYPublicKey* aPubKey,
> +               JsonWebKey& aRetVal,
> +               const nsNSSShutDownPreventionLock& /*proofOfLock*/)

nit: indentation

::: dom/crypto/WebCryptoTask.cpp
@@ +23,5 @@
>  
>  #include "mozilla/Telemetry.h"
>  
> +#include "mozilla/dom/ToJSValue.h"
> +#include "jsapi.h"

My understanding is these #includes should all be together, sorted.

::: dom/crypto/test/tests.js
@@ +1542,5 @@
> +);
> +
> +// -----------------------------------------------------------------------------
> +TestArray.addTest(
> +  "JWK import failure on extractable mismatch",

How about testing for an import failure due to missing fields?

@@ +1695,5 @@
> +      );
> +  }
> +);
> +
> +

nit: unnecessary trailing blank lines
Attachment #8455724 - Flags: review?(dkeeler) → review+
Comment on attachment 8455724 [details] [diff] [review]
bug-1025230.2.patch

>Add a method DictionaryBase::Stringify to complement DictionaryBase::ParseJSON

This patch isn't doing that.

>+++ b/dom/crypto/CryptoBuffer.cpp
>+// Helpers to encode/decode JWK's special flavor of Base64

Ugh.  :(  We really have nothing so far that deals with this stuff?  :(

>+  nsCString temp = NS_ConvertUTF16toUTF8(aBase64);

  NS_ConvertUTF16toUTF8 temp(aBase64);

>+  temp.StripWhitespace();

Are you sure?  I believe StripWhitespace() will remove things in the set "\b\t\r\n ".  I really doubt you'll have a '\b' in there.  On the other hand, it will leave '\f', which is a bit odd.  The set of characters HTML treats as "whitespace", for example, is "\t\n\f\r ".

Can you point me to the spec that defines whitespace processing here, please?

>+  if (!Assign((const uint8_t*) binaryData.BeginReading(),

The fact that we end up copying this twice kinda sucks.  Can you please file a followup to add a nsTArray<uint8_t> version of Base64Decode?

>+CryptoBuffer::ToJwkBase64(nsString& aBase64)
>+    aBase64 = NS_LITERAL_STRING("");

  aBase64.Truncate();

>+  nsCString base64, binaryData((const char*) Elements(), Length());

This is making a pointless copy of our data.  binaryData should be of type nsDependentCSubstring; then it will just point to our data buffer.

>+  // Strip whitespace and padding
>+  base64.StripWhitespace();

Why would there be whitespace in there?  I don't think this call is needed.

>+  aBase64 = NS_ConvertASCIItoUTF16(base64);

  CopyASCIItoUTF16(base64, aBase64);

>+++ b/dom/crypto/CryptoBuffer.h
>+++ b/dom/crypto/CryptoKey.cpp
>+template<class Alloc>

Why not just make this a template on the array type itself?

>+CopyUsages(uint32_t aAttributes, nsTArray_Impl<nsString, Alloc>& aRetVal)

Maybe better named as UsagesArrayFromAttributes or AttributesToUsagesArray or some such?

>+CryptoKey::PrivateKeyFromJwk(const JsonWebKey& aJwk,

I didn't verify that the logic in this method (e.g. which dictionary entries we require) follows the spec.  If dkeeler is not planning to do that, please let me know so I can come back to it.  Please make sure you don't check this in until you confirm that one of us has looked at this code.

>+CryptoKey::PrivateKeyToJwk(SECKEYPrivateKey* aPrivKey,

Similar here.

>+bool ReadAndEncodeAttribute(SECKEYPrivateKey* aKey,
>+                                CK_ATTRIBUTE_TYPE aAttribute,
>+                                Optional<nsString>& aDst)

Fix indent?

>+CryptoKey::PrivateKeyToJwk(SECKEYPrivateKey* aPrivKey,
>+                JsonWebKey& aRetVal,
>+                const nsNSSShutDownPreventionLock& /*proofOfLock*/)

And here.

>+CryptoKey::PublicKeyToJwk(SECKEYPublicKey* aPubKey,
>+               JsonWebKey& aRetVal,
>+               const nsNSSShutDownPreventionLock& /*proofOfLock*/)

And here.

>+++ b/dom/crypto/WebCryptoCommon.h
>+CloneData(CryptoBuffer& aDst, JS::Handle<JSObject*> aSrc)
>+  ArrayBuffer ab;

This needs to be rooted.  See RootedArrayBuffer.  Otherwise the static analysis boxes will turn pretty colors on this push..

But see also my post to public-web-crypto just now about how importKey should maybe have a more restrictive argument type...  That should probably be a followup, though.

>+++ b/dom/crypto/WebCryptoTask.cpp
>+  static bool JwkCompatible(const JsonWebKey& aJwk, const CryptoKey* aKey)

Likewise, please let me know if dkeeler has not reviewed this bit.

>+  void SetKeyData(JSContext* aCx, JS::Handle<JSObject*> aKeyData) {
>+      // First try to treat as ArrayBuffer/ABV

This comment should probably come before the if, not in the first branch.

>+  void SetJwkFromKeyData()
>+    nsString json = NS_ConvertUTF8toUTF16(
>+                      (const char*) mKeyData.Elements(),
>+                      mKeyData.Length());

Are you sure you want NS_ConvertUTF8toUTF16?  Are we guaranteed that mKeyData is UTF-8 somehow?  If so, how?  Worth documenting.

>+    if (mJwk.Init(json)) {

If not, don't we need to throw or something?  It's not clear to me what the error-handling strategy here is.  It's also probably worth documenting.

>+  void SetKeyData(const JsonWebKey& aJwk)

Where is this used?

>+  ImportSymmetricKeyTask(JSContext* aCx,
>+    : ImportSymmetricKeyTask(aCx, aFormat, aAlgorithm, aExtractable, aKeyUsages)

This won't compile on at least b2g and Windows.  See https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code

>   virtual nsresult BeforeCrypto() MOZ_OVERRIDE
>+      if (!mJwk.mK.WasPassed() /* or other required stuff */) {

What about other required stuff?

>+      if (mDataIsJwk && mJwk.mUse.WasPassed()) {
>+        // There is not a 'use' value consistent with PBKDF

I assume this and the other changes in this bit are in fact rooted in spec language somewhere, right?

>+  nsresult AfterCrypto() MOZ_OVERRIDE
>+    if (!JwkCompatible(mJwk, mKey)) {

We want to do that even in the !mDataIsJwk case?x

>+  ImportRsaKeyTask(JSContext* aCx,
>+    : ImportRsaKeyTask(aCx, aFormat, aAlgorithm, aExtractable, aKeyUsages)

Again, this won't compile.

>@@ -1257,59 +1371,74 @@ private:
>+      nsRefPtr<RsaHashedKeyAlgorithm> temp = new RsaHashedKeyAlgorithm(global,
>+                                               mAlgName, mModulusLength,
>+                                               mPublicExponent, mHashName);

It's not clear to me why this indent is better than the old one.  I don't think it is.

>+class UnifiedExportKeyTask : public WebCryptoTask
>+  Sequence<nsString> mKeyUsages;

Why not just nsTArray?  Then it's not clear that you'd need the new overload of GetUsages...

>@@ -1336,21 +1465,71 @@ private:
>+      if (mAlg.Length() > 0) {

  if (!mAlg.IsEmpth())

?

>+      if (mKeyUsages.Length() > 0) {

And similar here.

>+  static bool StringifyJWK(const JsonWebKey& aJwk, nsAString& aRetVal)
>+    AutoSafeJSContext cx;

Please document that the exact compartment doesn't matter here, since we plan to produce an XPCOM string.

>   virtual void Resolve() MOZ_OVERRIDE {
>+      nsCString utf8 = NS_ConvertUTF16toUTF8(json);

  NS_ConvertUTF16toUTF8 utf(json);

r=me with the caveats above.
Attachment #8455724 - Flags: review?(bzbarsky) → review+
Oh, one more thing.  If ToJSValue fails, you need to JS_ClearPendingException on the cx, or report it.  Don't just leave it there.
+  static bool StringifyJWK(const JsonWebKey& aJwk, nsAString& aRetVal)
+  {
+    // XXX: This should move into DictionaryBase and Codegen.py,
+    //      in the same way as ParseJSON is split out. (Bug 1038399)
+    MOZ_ASSERT(NS_IsMainThread());
+    AutoSafeJSContext cx;
	
bholley: bz mentioned this AutoSafeJSContext yesterday and I confidently replied that it should be replaced by an AutoJSAPI.
Then he asked me about the compartment and I floundered, because we don't appear to have an obvious one to enter and being in the null compartment is not going to work.
I'm not sure if it is possible to enter some standard safe compartment, can we use GetSafeJSContextGlobal or something else?
What's the best way forward for situations like this?
Flags: needinfo?(bobbyholley)
I thought about the compartment some more, and while in this case it doesn't matter too much since the dictionary only contains primitives, for the generic method we'd want a compartment with a NullPrincipal, I think, so if a dictionary contains a Location things happen safely.
Who is triggering this code? Why can't we hold a ref to their global and enter that compartment?
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #13)
> Who is triggering this code? Why can't we hold a ref to their global and
> enter that compartment?

Good point.  In this case, the triggering code has a JSContext on hand, so I've modified things to just use that.  New patch shortly.
(In reply to Richard Barnes [:rbarnes] from comment #14)
> (In reply to Bobby Holley (:bholley) from comment #13)
> > Who is triggering this code? Why can't we hold a ref to their global and
> > enter that compartment?
> 
> Good point.  In this case, the triggering code has a JSContext on hand, so
> I've modified things to just use that.  New patch shortly.

Is this code async? If it is, holding onto the JSContext isn't going to do the right thing. You want to hold onto the nsIGlobalObject or nsPIDOMWindow (via GetOwner()).
> Who is triggering this code?

For purposes of this code we can do whatever.  I'm interested in the general "convert a dictionary to JSON" case, in which there may not be any JS "triggering" it at all (e.g. it could be some spec algorithm running in response to the network delivering data.
(In reply to Bobby Holley (:bholley) from comment #15)
> (In reply to Richard Barnes [:rbarnes] from comment #14)
> > (In reply to Bobby Holley (:bholley) from comment #13)
> > > Who is triggering this code? Why can't we hold a ref to their global and
> > > enter that compartment?
> > 
> > Good point.  In this case, the triggering code has a JSContext on hand, so
> > I've modified things to just use that.  New patch shortly.
> 
> Is this code async? If it is, holding onto the JSContext isn't going to do
> the right thing. You want to hold onto the nsIGlobalObject or nsPIDOMWindow
> (via GetOwner()).

This part is not async.
(In reply to Boris Zbarsky [:bz] from comment #16)
> > Who is triggering this code?
> 
> For purposes of this code we can do whatever.  I'm interested in the general
> "convert a dictionary to JSON" case, in which there may not be any JS
> "triggering" it at all (e.g. it could be some spec algorithm running in
> response to the network delivering data.

For that case, I'm happy to use the SafeJSContext global. But I think it's relatively rare, and in most cases people are doing JSAPI in preparation for delivering a GCThing to an API associated with a JS global. The XrayWrappers we'd get in that case are likely to cause trouble, so in general, I think we should avoid using catch-alls wherever possible.
Attached patch bug-1025230.3.patch r=bz,keeler (obsolete) — Splinter Review
Attachment #8455724 - Attachment is obsolete: true
Attachment #8457450 - Flags: review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #8)
> Comment on attachment 8455724 [details] [diff] [review]
> bug-1025230.2.patch
> 
> Review of attachment 8455724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I reviewed the NSS-related code. The only issues I saw were that everything
> from NSS should be null-checked before use and that stack-allocated SECItems
> passed to NSS functions as pointers can have memory allocated to them that
> needs to be freed (which is quite a bummer, but there you go). r=me with
> those fixed. I also noted some style nits and a suggestion for an additional
> test.

Thanks a lot for the review.  I fixed the instances noted in your detailed comments, and made my own scan through the new methods in CryptoKey.cpp, which didn't turn up anything further.
(In reply to Boris Zbarsky [:bz] from comment #9)
> Comment on attachment 8455724 [details] [diff] [review]
> bug-1025230.2.patch

> >+++ b/dom/crypto/CryptoBuffer.cpp
> >+// Helpers to encode/decode JWK's special flavor of Base64
> 
> Ugh.  :(  We really have nothing so far that deals with this stuff?  :(

<Shrug>  Blame the IETF.  I tried to fix it there and failed.  If we had anything that did URL-safe Base64, that would save the character transcoding, but the padding trimming is non-standard, so we wouldn't get out of that.


> >+  temp.StripWhitespace();
> 
> Are you sure?  I believe StripWhitespace() will remove things in the set
> "\b\t\r\n ".  I really doubt you'll have a '\b' in there.  On the other
> hand, it will leave '\f', which is a bit odd.  The set of characters HTML
> treats as "whitespace", for example, is "\t\n\f\r ".
> 
> Can you point me to the spec that defines whitespace processing here, please?

The relevant spec here is JWK, which gets its base64 from JWS, which says:
"""
   Base64url Encoding
      Base64 encoding using the URL- and filename-safe character set
      defined in Section 5 of RFC 4648 [RFC4648], with all trailing '='
      characters omitted (as permitted by Section 3.2) and without the
      inclusion of any line breaks, white space, or other additional
      characters.  (See Appendix C for notes on implementing base64url
      encoding without padding.)
"""

RFC 4648, of course, does not define "whitespace".  So I think we're free to choose what we like here, and I think this is close enough.


> >+  if (!Assign((const uint8_t*) binaryData.BeginReading(),
> 
> The fact that we end up copying this twice kinda sucks.  Can you please file
> a followup to add a nsTArray<uint8_t> version of Base64Decode?

https://bugzilla.mozilla.org/show_bug.cgi?id=1039152


> >+CopyUsages(uint32_t aAttributes, nsTArray_Impl<nsString, Alloc>& aRetVal)
> 
> Maybe better named as UsagesArrayFromAttributes or AttributesToUsagesArray
> or some such?

WebIDL binding code expects GetUsages.  The code here just modifies things to allow for FallibleTArray as well as nsTArray.


> >+CryptoKey::PrivateKeyFromJwk(const JsonWebKey& aJwk,
> 
> I didn't verify that the logic in this method (e.g. which dictionary entries
> we require) follows the spec.  If dkeeler is not planning to do that, please
> let me know so I can come back to it.  Please make sure you don't check this
> in until you confirm that one of us has looked at this code.

The WebCrypto spec defers to JWA and JWK:
http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-31#section-6.3

In brief, that section of JWA says that "n" and "e" are required for public keys, and for private keys, consumers may choose to require all of the parameters besides "oth" (which is what we do):
"""
   In addition to the members used to represent RSA public keys, the
   following members are used to represent RSA private keys.  The
   parameter "d" is REQUIRED for RSA private keys.  The others enable
   optimizations and SHOULD be included by producers of JWKs
   representing RSA private keys.  If the producer includes any of the
   other private key parameters, then all of the others MUST be present,
   with the exception of "oth", which MUST only be present when more
   than two prime factors were used.  The consumer of a JWK MAY choose
   to accept an RSA private key that does not contain a complete set of
   the private key parameters other than "d", including JWKs in which
   "d" is the only RSA private key parameter included.
"""

JWK requires that "kty" be present.  So we are within spec here.


> But see also my post to public-web-crypto just now about how importKey
> should maybe have a more restrictive argument type...  That should probably
> be a followup, though.

Yeah, I'll update the code once the spec updates :)


> >+++ b/dom/crypto/WebCryptoTask.cpp
> >+  static bool JwkCompatible(const JsonWebKey& aJwk, const CryptoKey* aKey)
> 
> Likewise, please let me know if dkeeler has not reviewed this bit.

I don't think he has reviewed this for spec compliance.  Unfortunately, the relevant bits of WebCrypto are sprinkled throughout the individual algorithm sections.


> >+  void SetJwkFromKeyData()
> >+    nsString json = NS_ConvertUTF8toUTF16(
> >+                      (const char*) mKeyData.Elements(),
> >+                      mKeyData.Length());
> 
> Are you sure you want NS_ConvertUTF8toUTF16?  Are we guaranteed that
> mKeyData is UTF-8 somehow?  If so, how?  Worth documenting.

Added a test for IsUTF8().


> >+    if (mJwk.Init(json)) {
> 
> If not, don't we need to throw or something?  It's not clear to me what the
> error-handling strategy here is.  It's also probably worth documenting.

Swapped this around to test "if (!mJwk.Init(JSON))" and set mEarlyRv if not (which will cause the promise to reject).


> >+  ImportSymmetricKeyTask(JSContext* aCx,
> >+    : ImportSymmetricKeyTask(aCx, aFormat, aAlgorithm, aExtractable, aKeyUsages)
> 
> This won't compile on at least b2g and Windows.  See
> https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code

Aha, that's why ttaubert used the Init() pattern in his PBKDF2 patch.  I have refactored to do that here.


> >+      if (mDataIsJwk && mJwk.mUse.WasPassed()) {
> >+        // There is not a 'use' value consistent with PBKDF
> 
> I assume this and the other changes in this bit are in fact rooted in spec
> language somewhere, right?

Yes, but trying to be a little more concise than the spec.  See the individual algorithm definitions.  E.g., from AES-GCM:

"If the "use" field of jwk is present, and is not "enc", then return an error named DataError."

One divergence here is that we are allowing JWK import for PBKDF2, which is more liberal than the spec:

"If format is not "raw", return an error named NotSupportedError"

However, a JWK of the form '{"k": "base64"}' is equivalent to a raw key, so there's no reason to forbid it.  I filed a spec bug.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26348


> >+  nsresult AfterCrypto() MOZ_OVERRIDE
> >+    if (!JwkCompatible(mJwk, mKey)) {
> 
> We want to do that even in the !mDataIsJwk case?

It turns out not to matter, since everything in JwkCompatible is premised on the existence of fields in the JWK, and !mDataIsJwk implies that the JWK is empty.  Nonetheless, added "mDataIsJwk &&" in front of these calls.


> >+class UnifiedExportKeyTask : public WebCryptoTask
> >+  Sequence<nsString> mKeyUsages;
> 
> Why not just nsTArray?  Then it's not clear that you'd need the new overload
> of GetUsages...

Beceause mJwk.mKey_ops.Construct() requires Sequence<nsString>.
> RFC 4648, of course, does not define "whitespace". 

Bloody useless IETF "specs".  :(  Can you please file a webcrypto issue to define this, since the IETF didn't bother to?  I seriously doubt that Chrome's definition of "whitespace" here matches the nsStringObsolete one (if only because the latter is pretty weird; '\b', really?).

> WebIDL binding code expects GetUsages. 

Yes, I know.  I'm talking about the naming of CopyUsages.

> The code here just modifies things to allow for FallibleTArray as well as nsTArray.

Why bother, if we're not checking for failures anyway?  Just use nsTArray at the new callsites.  I don't expect the usages array will ever be particularly long.

> JWK requires that "kty" be present.  So we are within spec here.

I believe you, but this sort of thing is a "trust but verify" kind of deal.  So I want to make sure that there is a second set of eyes on those changes.  Again, I can do that if dkeeler can't, though I'd somewhat prefer if he does it.

> I don't think he has reviewed this for spec compliance. 

OK.  I think I've convinced myself it's probably correct.  I agree the way the spec is structured here is a PITA.  :(

> Added a test for IsUTF8().

Can you add a comment about why the value is expected to be UTF-8 at all?

> Beceause mJwk.mKey_ops.Construct() requires Sequence<nsString>.

Ah.  You should be able to do:

  mJwk.mKey_ops.Construct();
  mJwk.mKey_ops.Value().AppendElements(aTArray);

and have it work: AppendElements is a template on the argument type that just wants it to quack like some sort of nsTArray-related thing.  That might be cleaner-simpler than doing the GetUsages template thing.
(In reply to Boris Zbarsky [:bz] from comment #22)
> > RFC 4648, of course, does not define "whitespace". 
> 
> Bloody useless IETF "specs".  :(  Can you please file a webcrypto issue to
> define this, since the IETF didn't bother to?  I seriously doubt that
> Chrome's definition of "whitespace" here matches the nsStringObsolete one
> (if only because the latter is pretty weird; '\b', really?).

The IETF specs are still in process, so we should define it there.  It seems like the most liberal thing to do would be to just strip anything that's not in the relevant base64 alphabet, yes?


> > JWK requires that "kty" be present.  So we are within spec here.
> 
> I believe you, but this sort of thing is a "trust but verify" kind of deal. 
> So I want to make sure that there is a second set of eyes on those changes. 
> Again, I can do that if dkeeler can't, though I'd somewhat prefer if he does
> it.

Keeler: Could you take a look at spec compliance for JwkCompatible?

 
> > Added a test for IsUTF8().
> 
> Can you add a comment about why the value is expected to be UTF-8 at all?

Because the spec says so?  ;)

"Let json be the Unicode string that results from interpreting data according to UTF-8."


> > Beceause mJwk.mKey_ops.Construct() requires Sequence<nsString>.
> 
> Ah.  You should be able to do:
> 
>   mJwk.mKey_ops.Construct();
>   mJwk.mKey_ops.Value().AppendElements(aTArray);
> 
> and have it work: AppendElements is a template on the argument type that
> just wants it to quack like some sort of nsTArray-related thing.  That might
> be cleaner-simpler than doing the GetUsages template thing.

Good point.  I've changed to this in my working copy.
Flags: needinfo?(dkeeler)
> It seems like the most liberal thing

Liberal, sure.  Desirable, I'm not convinced.  Seems pretty scary to me to ignore garbage in the middle of purported key data!

> Because the spec says so?  ;)

That's an excellent thing to put in a comment, yes.
Comment on attachment 8457450 [details] [diff] [review]
bug-1025230.3.patch r=bz,keeler

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

As far as I can tell, the appropriate properties of the JWK input do get checked (not all in one centralized place, but maybe that would be impractical). If we're concerned about proper validation, I recommend more tests with invalid inputs.

::: dom/crypto/CryptoBuffer.cpp
@@ +75,5 @@
>    SetLength(0);
>    return nullptr;
>  }
>  
> +// Helpers to encode/decode JWK's special flavor of Base64

As far as I can tell, the "special flavor" is just base64url encoding, right?

@@ +83,5 @@
> +nsresult
> +CryptoBuffer::FromJwkBase64(const nsString& aBase64)
> +{
> +  NS_ConvertUTF16toUTF8 temp(aBase64);
> +  temp.StripWhitespace();

I was going to say I think Trim(<whitespace characters>) would be safer, but then I considered the case where the data looks like this:

"MIIDQjCCAiqgAwIBAgIGATz/FuLiMA0GCSqGSIb3DQEBBQUAMGIxCzAJBDV
 gNVBAYTAlVTMQswCQYDVQQIEwJDTzEPMA0GA1UEBxMGRGVudmVyMRwwGgYD
 VQQKExNQaW5nIElkZW50aXR5IENvcnAuMRcwFQYDVQQDEw5Ccmlhbi..."

(i.e. it has line breaks in it, but would otherwise be valid)
So this is probably ok.

@@ +97,5 @@
> +
> +  // Translate from URL-safe character set to normal
> +  temp.ReplaceChar('-', '+');
> +  temp.ReplaceChar('_', '/');
> +  if (temp.FindCharInSet("-_", 0) != kNotFound) {

I guess I meant write a test that needs the implementation to handle multiple replacements. We probably don't need to enforce this here.

@@ +136,5 @@
> +
> +  // Translate to the URL-safe charset
> +  base64.ReplaceChar('+', '-');
> +  base64.ReplaceChar('/', '_');
> +  if (base64.FindCharInSet("+/", 0) != kNotFound) {

Same thing - just enforce this in a test.
Flags: needinfo?(dkeeler)
(In reply to David Keeler (:keeler) [use needinfo?] from comment #25)
> As far as I can tell, the "special flavor" is just base64url encoding, right?

With padding stripping.  If you know of a place in gecko where I can get base64url, I'll gladly use it, but it doesn't seem like the substitution here is a huge burden in the meantime.


> > +  if (temp.FindCharInSet("-_", 0) != kNotFound) {
> 
> I guess I meant write a test that needs the implementation to handle
> multiple replacements. We probably don't need to enforce this here.

This is actually already in several of the tests.  See, e.g., tv.aes_gcm_enc.key_jwk, which is used in the "JWK import and use of an AES-GCM key" test:

>    key_jwk: {
>      kty: "oct",
>      k: "_v_pkoZlcxxtao-UZzCDCP7_6ZKGZXMcbWqPlGcwgwg"
>    },
Attached patch bug-1025230.4.patch r=bz,keeler (obsolete) — Splinter Review
Successful try run for Bug 1034851, Bug 1025230, and Bug 1340852:
https://tbpl.mozilla.org/?tree=Try&rev=3518668ece4e
Attachment #8457450 - Attachment is obsolete: true
Attachment #8459170 - Flags: review+
Keywords: checkin-needed
Resolving some merge issues.
Attachment #8459170 - Attachment is obsolete: true
Attachment #8459176 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b0d3e4c8385a
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1042110
Added to the release notes with "WebCrypto: Import/export of JWK-formatted keys"
Keywords: relnotedev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: