1034851 - Add wrapKey and unwrapKey methods to WebCrypto API

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: bug-1034851.patch

The edits in this patch have a few parts:
* Plumbing from WebIDL -> SubtleCrypto -> WebCryptoTask
* New classes WrapKeyTask and UnwrapKeyTask
* Tests for it all

WrapKeyTask and UnwrapKeyTask work by composing encrypt/decrypt tasks and import/export tasks (following the pattern for deriveKey in Bug 1021607).  That required some updates to allow the encryption and import tasks to take dummy data at constructor time, with the real data provided in a SetData() call later.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8451404 [details] [diff] [review]
bug-1034851.patch

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

It doesn't actually look like there are any PSM/NSS-related changes here, so I don't think my formal review is needed. I did make a few comments.

::: dom/base/SubtleCrypto.cpp
@@ +155,5 @@
> +                        bool extractable,
> +                        const Sequence<nsString>& keyUsages)
> +{
> +  SUBTLECRYPTO_METHOD_BODY(UnwrapKey, cx, format, wrappedKey, unwrappingKey,
> +                                    unwrapAlgorithm, unwrappedKeyAlgorithm,

nit: indentation

::: dom/crypto/WebCryptoTask.cpp
@@ +236,5 @@
>      : mSymKey(aKey.GetSymKey())
>      , mEncrypt(aEncrypt)
>    {
> +    if (aData.IsArrayBuffer() || aData.IsArrayBufferView()) {
> +      ATTEMPT_BUFFER_INIT(mData, aData);

Are we sure these changes don't allow mData to be used uninitialized later?

Comment 3

[Richard Barnes [:rbarnes]]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #2)
> Comment on attachment 8451404 [details] [diff] [review]
> bug-1034851.patch
> 
> Review of attachment 8451404 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It doesn't actually look like there are any PSM/NSS-related changes here, so
> I don't think my formal review is needed. I did make a few comments.
> 
> ::: dom/base/SubtleCrypto.cpp
> @@ +155,5 @@
> > +                        bool extractable,
> > +                        const Sequence<nsString>& keyUsages)
> > +{
> > +  SUBTLECRYPTO_METHOD_BODY(UnwrapKey, cx, format, wrappedKey, unwrappingKey,
> > +                                    unwrapAlgorithm, unwrappedKeyAlgorithm,
> 
> nit: indentation

Thanks, will fix with any resolutions from bz.


> ::: dom/crypto/WebCryptoTask.cpp
> @@ +236,5 @@
> >      : mSymKey(aKey.GetSymKey())
> >      , mEncrypt(aEncrypt)
> >    {
> > +    if (aData.IsArrayBuffer() || aData.IsArrayBufferView()) {
> > +      ATTEMPT_BUFFER_INIT(mData, aData);
> 
> Are we sure these changes don't allow mData to be used uninitialized later?

mData is automatically initialized to a zero-length buffer (since it's a FallibleTArray), so there's no safety issue here.  There might be a correctness issue if someone both provides dummy data to the ctor and forgets a SetData() call, but it seems like that should usually be discovered quickly in tests.

If this still a concern, we could do something like "private: bool mDataProvided = false; ... mDataProvided = true; ... if (!mDataProvided) { return NS_ERROR_DOM_OPERATION_ERR; }"

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8451404 [details] [diff] [review]
bug-1034851.patch

>+++ b/dom/crypto/WebCryptoTask.cpp
>+    if (aData.IsArrayBuffer() || aData.IsArrayBufferView()) {

Please document, in all places where this is done why it might not be (and in particular that in this case it's not initialized at all and the data will come via an out-of-band SetData call).  Oh, and assert that it's not initialized in the else branch.

I'm assuming the inheritance from things vs the owning of things as members makes sense here; I have not step-by-step compared it to the spec algorithm.  If no one else has either, please let me know and I'll try to find the time to do it, but I'd appreciate you finding someone else for it if possible.

>+WebCryptoTask::UnwrapKeyTask(JSContext* aCx,
>+  // Deal with the Nullable aspect of aUnwrappedKeyAlgorithm

Why is it nullable?  It's not nullable in the spec...

>+    return new mozilla::dom::UnwrapKeyTask<AesTask>(aCx, aWrappedKey,

Why do you need the "mozilla::dom" bit here (or anywhere else in this patch for that matter)?

Getting rid of that should also allow sane indentation...

>+    return new mozilla::dom::UnwrapKeyTask<RsaesPkcs1Task>(aCx, aWrappedKey,
>+    return new mozilla::dom::UnwrapKeyTask<RsaOaepTask>(aCx, aWrappedKey,

Likewise.

r+ modulo the above, but again note the bit about the parts I didn't review.

Comment 5

[Richard Barnes [:rbarnes]]

Attachment: bug-1034851.1.patch r=bz

(In reply to Boris Zbarsky [:bz] from comment #4)
> Comment on attachment 8451404 [details] [diff] [review]
> bug-1034851.patch
> 
> >+++ b/dom/crypto/WebCryptoTask.cpp
> >+    if (aData.IsArrayBuffer() || aData.IsArrayBufferView()) {
> 
> Please document, in all places where this is done why it might not be (and
> in particular that in this case it's not initialized at all and the data
> will come via an out-of-band SetData call).  Oh, and assert that it's not
> initialized in the else branch.

I've refactored to do this a little more elegantly, to avoid passing dummy data:
* Add a class DeferredData that provides SetData() and mDataIsSet
* Made AesTask, RsaesPkcs1Task, and RsaOaepTask inherit DeferredData
* Add a check of mDataIsSet before any operation that requires data

I've left the ImportFooTask classes alone, since the JWK patch refactors them similarly anyway.

> I'm assuming the inheritance from things vs the owning of things as members
> makes sense here; I have not step-by-step compared it to the spec algorithm.
> If no one else has either, please let me know and I'll try to find the time
> to do it, but I'd appreciate you finding someone else for it if possible.

Tim Taubert and I looked at this for the PBKDF2 patch in Bug 1021607, which composes deriveBits and importKey.  Our conclusion there was that composing things in this way was compatible with the WebCrypto event model.  From my reading of wrapKey and unwrapKey, things look similar (composing exportKey/encrypt and decrypt/importKey), so I think we're OK re-using the pattern from Bug 1021607.


> >+WebCryptoTask::UnwrapKeyTask(JSContext* aCx,
> >+  // Deal with the Nullable aspect of aUnwrappedKeyAlgorithm
> 
> Why is it nullable?  It's not nullable in the spec...

It was in an earlier draft, I believe to allow the algorithm in a JWK to override.
https://dvcs.w3.org/hg/webcrypto-api/file/a70efe4807ee/spec/Overview.html

I've made it non-nullable.


> >+    return new mozilla::dom::UnwrapKeyTask<AesTask>(aCx, aWrappedKey,
> 
> Why do you need the "mozilla::dom" bit here (or anywhere else in this patch
> for that matter)?

It conflicted with the method name WebCryptoTask::UnwrapKeyTask.  Same reason we have quirky class names like UnifiedExportKeyTask and SimpleDigestTask.  I've renamed the methods to CreateFooTask and the classes to FooTask.

Comment 6

[Richard Barnes [:rbarnes]]

Attachment: bug-1034851.2.patch r=bz

Successful try run for Bug 1034851, Bug 1025230, and Bug 1340852:
https://tbpl.mozilla.org/?tree=Try&rev=3518668ece4e

Comment 7

[Richard Barnes [:rbarnes]]

Attachment: bug-1034851.3.patch r=bz

Cleaning up merge issues.

Comment 8

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e75c02fedfc

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e75c02fedfc

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "WebCrypto: wrapKey and unwrapKey implemented" as wording.