Closed Bug 1243311 Opened 8 years ago Closed 8 years ago

Add structured cloning tests for CryptoKeys

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

Attachments

(1 file, 1 obsolete file)

We should have tests to ensure structured cloning of CryptoKey objects works as intended.
DH, ECDH, and ECDSA tests included but commented out until we have PKCS8/SPKI format import/export for those.
Attachment #8712585 - Flags: review?(rlb)
Comment on attachment 8712585 [details] [diff] [review]
0001-Bug-1243311-Add-structured-cloning-tests-for-CryptoK.patch

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

This is mostly OK, but it's not great to infer equality of keys from equality of results of a crypto operation.  It seems like it would be cleaner and more correct to test an import/export round trip instead.)

crypto.subtle.importKey(key)
  .then(clone)
  .then(exportKey)
  .then(util.ab2hex)
  .then(equals)

You might even be able to wrap that all in one method, since the only thing changing is inputs to the WebCrypto methods.
Attachment #8712585 - Flags: review?(rlb) → review-
(In reply to Richard Barnes [:rbarnes] from comment #2)
> This is mostly OK, but it's not great to infer equality of keys from
> equality of results of a crypto operation.  It seems like it would be
> cleaner and more correct to test an import/export round trip instead.)

That definitely ensures that the key bits are the same, but I think it is also valuable to check whether we clone attributes correctly, and thus algorithm, usages, etc. allow an operation to succeed.

I added util.cloneExportCompareKeys() to do what you suggested. It's in util so that all structured cloning tests can use it and that won't break the patch that makes tests run in workers.

HKDF was left untouched because the spec doesn't define any export operation and we don't support any.
Attachment #8712585 - Attachment is obsolete: true
Attachment #8728427 - Flags: review?(rlb)
Comment on attachment 8728427 [details] [diff] [review]
0001-Bug-1243311-Add-structured-cloning-tests-for-CryptoK.patch, v2

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

::: dom/crypto/test/test_WebCrypto_Structured_Cloning.html
@@ +185,5 @@
> +    crypto.subtle.generateKey(alg, true, ["sign", "verify"])
> +      .then(util.cloneExportCompareKeys)
> +      .then(x => {
> +        return crypto.subtle.sign(alg, x.privateKey, data)
> +          .then(sig => crypto.subtle.verify(alg, x.publicKey, sig, data));

Why is this not just part of the top-level .then() chain?  Likewise elsewhere.
Attachment #8728427 - Flags: review?(rlb) → review+
(In reply to Richard Barnes [:rbarnes] from comment #4)
> > +    crypto.subtle.generateKey(alg, true, ["sign", "verify"])
> > +      .then(util.cloneExportCompareKeys)
> > +      .then(x => {
> > +        return crypto.subtle.sign(alg, x.privateKey, data)
> > +          .then(sig => crypto.subtle.verify(alg, x.publicKey, sig, data));
> 
> Why is this not just part of the top-level .then() chain?  Likewise
> elsewhere.

Only because we don't have a single key but a keypair. If we'd move those to the top-level chain then I'd have to introduce a helper function that sets function-global variables to keep the public and private keys.
https://hg.mozilla.org/mozilla-central/rev/55a7478c3343
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: