Should be quite trivial.

Comment on attachment 8470430 [details] [diff] [review] 0001-Bug-1050175-Add-raw-import-export-for-ECDH-public-ke.patch Review of attachment 8470430 [details] [diff] [review]: ----------------------------------------------------------------- Maybe I'm missing it, but this code needs to check at some point that the buffer passed actually represents an EC curve point. Right now you're just checking length. Unfortunately, I'm not sure how to do that with NSS; keeler might. You could start by adding a test with an invalid curve point (change some bits in the Y coordinate) and seeing what happens if you import it and do an operation with it. ::: dom/crypto/WebCryptoTask.cpp @@ +1751,2 @@ > pubKey = CryptoKey::PublicKeyFromJwk(mJwk, locker); > + } else { It seems like checking if mFormat == "raw" here would help with readability and extensibility. @@ +1852,5 @@ > { > nsNSSShutDownPreventionLock locker; > > if (mFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_RAW)) { > + if (mPublicKey && mPublicKey->keyType == ecKey) { It would be nicer if you could make the symmetric-key case parallel to this, so that you could have one "return NS_OK" at the end of the block. ::: dom/crypto/test/tests.js @@ +2313,5 @@ > + > + doTryImport(tvs.raw_short)() > + .then(error(that), doTryImport(tvs.raw_long)) > + .then(error(that), doTryImport(tvs.raw_compressed)) > + .then(error(that), complete(that)); FWIW, this will have correct results with mochitest (though the same test will show as having run 3 times), but it might not display correctly in the graphical version. If the last test passes (complete(that)), then the whole test will show as passed, even if the others fail.

(In reply to Richard Barnes [:rbarnes] from comment #4) > Maybe I'm missing it, but this code needs to check at some point that the > buffer passed actually represents an EC curve point. Right now you're just > checking length. Unfortunately, I'm not sure how to do that with NSS; > keeler might. You could start by adding a test with an invalid curve point > (change some bits in the Y coordinate) and seeing what happens if you import > it and do an operation with it. Yeah, I thought about this as well when implementing JWK import but afaict there is only EC_ValidatePublicKey(), which we can't use unfortunately. Validating a point on the curve could then only be done by generating a key pair for the curve and trying to derive the secret. If it fails then the given public point was probably invalid. This sounds quite expensive for a sanity check, no?

Also, the spec doesn't say anything about validating the given point when importing a public key.

David, do you have any opinion on whether and how we should validate a given point on the curve when importing public EC keys?

Why can't we use EC_ValidatePublicKey? Because it's not exported from NSS? We could either export it or add a function to NSS that calls it and export that.

We had been sticking to the "official" PK11 interface, which is obviously pretty deficient for EC/DH. If calling directly to freebl is legal, then we might want to do some real refactoring on WebCrypto, since I think there are several things that would be much simpler.

Looks like freebl is private to NSS. We could either start exporting things from it directly, or essentially add wrappers to the pk11 functions (e.g. PK11_ValidateECKey or something). Either way would require (at least) modifications to NSS and possibly some discussion/buy-in from the NSS folks.

Before we go off messing with NSS, I want to make sure we actually need to do validation here. It's not clear in the current spec. I sent mail to the WebCrypto list to request clarification.

Any news?

Nope, nothing yet. Re-sent the ping.

Didn't address any review comments yet, same patch as before just rebased.

Unassigning until the spec is clear about validation.

Here's a potentially bad idea based on the hack that I've used to get raw from spki: prepend the DER preceding the key from SPKI and use the SPKI import function. Whatever validation already happens there will be inherited. I just checked seckey.c and it appears as though there is none.

Addressed review comments and using CryptoKey::PublicKeyValid() to check whether the given point is on the curve.

It seems like this could be trivially extended to cover ECDSA public keys. Is there any reason you haven't done that here as well?

Oh, and I'm going to volunteer to verify this independently.

No special reason, no. Just like to do things step-by-step and smaller patches are easier to review :)

Seems like the WebCrypto API doesn't specify raw public key import for ECDSA.

That seems like a bug. I can use spki import for ECDSA and the shim for raw is therefore trivial: https://github.com/martinthomson/secure-chat/blob/master/entity.js#L13-L28

(In reply to Martin Thomson [:mt] from comment #22) > That seems like a bug. Yup. https://www.w3.org/Bugs/Public/show_bug.cgi?id=27447 Guess we could either wait, as Chromium does or just go ahead and add it.

Comment on attachment 8598555 [details] [diff] [review] 0001-Bug-1050175-Add-raw-import-export-for-ECDH-public-ke.patch, v2 Review of attachment 8598555 [details] [diff] [review]: ----------------------------------------------------------------- I can confirm that this works for both ECDSA and ECDH. You might want to change the commit message to reflect that (remove "DH").

Oh, if this works for ECDSA, as it seems to, then I think that you should be adding ECDSA tests. That was a problem for bug 1158296.

Comment on attachment 8598555 [details] [diff] [review] 0001-Bug-1050175-Add-raw-import-export-for-ECDH-public-ke.patch, v2 Review of attachment 8598555 [details] [diff] [review]: ----------------------------------------------------------------- I agree with :mt that we should have a test that covers EDCSA. Should just be able to test the positive case. Will r+ when that's added. ::: dom/crypto/CryptoKey.cpp @@ +1091,5 @@ > + } else { > + return nullptr; > + } > + > + // Check length of uncompressed point coordinates. Might help to explain the math: 2 field elements plus a leading point form octet (which must be EC_POINT_FORM_UNCOMPRESSED) @@ +1109,5 @@ > +CryptoKey::PublicECKeyToRaw(SECKEYPublicKey* aPubKey, > + CryptoBuffer& aRetVal, > + const nsNSSShutDownPreventionLock& /*proofOfLock*/) > +{ > + aRetVal.Assign(&aPubKey->u.ec.publicValue); Check return value in case of OOM. ::: dom/crypto/WebCryptoTask.cpp @@ +1717,3 @@ > pubKey = CryptoKey::PublicKeyFromJwk(mJwk, locker); > + } else { > + MOZ_ASSERT(false); This is a runtime error, unless you're checking mFormat above. It seems like you should return NS_ERROR_DOM_SYNTAX_ERR.

(In reply to Richard Barnes [:rbarnes] from comment #26) > > + // Check length of uncompressed point coordinates. > > Might help to explain the math: 2 field elements plus a leading point form > octet (which must be EC_POINT_FORM_UNCOMPRESSED) Done. > > + aRetVal.Assign(&aPubKey->u.ec.publicValue); > > Check return value in case of OOM. Good catch. I'll file a follow-up, there's a few more places where we don't do that. > ::: dom/crypto/WebCryptoTask.cpp > @@ +1717,3 @@ > > pubKey = CryptoKey::PublicKeyFromJwk(mJwk, locker); > > + } else { > > + MOZ_ASSERT(false); > > This is a runtime error, unless you're checking mFormat above. It seems > like you should return NS_ERROR_DOM_SYNTAX_ERR. mFormat is checked above. Unknown import formats are handled in the else-branch of the parent block. The parent block is only entered if we see JWK/raw/SPKI.

Add another test for importing EC keys with an unknown format.

Will importing point compressed format be added at all? The algorithm is very simple, and the (definitely invalid anyway) patent expired last year.

NSS doesn't support the compressed format, although it seem to do in some parts of the code, not the parts we're using here though. It's probably not worth adding support to NSS only to have the WebCrypto API support it.

Comment on attachment 8611072 [details] [diff] [review] 0001-Bug-1050175-Add-raw-import-export-for-EC-public-keys.patch, v3 Olli, can you please take a look a the WebIDL changes? https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#EcKeyImportParams-dictionary

Updated https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/exportKey https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/importKey and https://developer.mozilla.org/en-US/Firefox/Releases/41#Interfaces.2FAPIs.2FDOM