1250930 - WebCrypto API doesn't work in Add-on SDK content scripts, or over Xrays in general

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[wyohknott]

WebCrypto API always returns an error with an empty object when used in a content script.

For example:

>//main.js
>const {pageMod} = require('sdk/page-mod');
>pageMod.PageMod({
>  include: "*",
>  contentScript: 'crypto.subtle.importKey("raw",crypto.getRandomValues(new Uint8Array(10)),{name: "HMAC",hash: {name: "SHA-1"},},false,["sign", "verify"]).then(function (key) {console.log("key", key);}).catch(function(err){console.log("error", err);});'
>});

always returns error, instead of a valid key.

Comment 1

[wyohknott]

Actually the error returned is

> permission denied to access property 'then'

Comment 2

[Martin Thomson [:mt:]]

This is likely to be a principal-related error.  Working with promises is always tricky when you cross compartments.  See for example: https://dxr.mozilla.org/mozilla-central/source/dom/media/PeerConnectionIdp.jsm#308

I don't know much about the addon SDKs, but this looks a lot like that problem.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Is the page-mode script running over Xrays?

Because it's pretty simple to reproduce the "permission denied to access property 'then'" thing by simply doing:

  content.crypto.subtle.importKey("raw",crypto.getRandomValues(new Uint8Array(10)),{name: "HMAC",hash: {name: "SHA-1"},},false,["sign", "verify"]).catch(function(e) { alert(e) })

in the browser console when running in "Browser" mode against a non-e10s window....

I also get the same thing even if I'm a bit more careful:

  content.crypto.subtle.importKey("raw",content.crypto.getRandomValues(new content.Uint8Array(10)),{name: "HMAC",hash: {name: "SHA-1"},},false,["sign", "verify"]).catch(function(e) { alert(e) })

Comment 4

[Boris Zbarsky [:bzbarsky]]

> in the browser console when running in "Browser" mode

Er, I meant Scratchpad in "Browser" mode, of course.

Anyway, that exception comes from mozilla::dom::ImportKeyTask::Resolve calling:

  mResultPromise->MaybeResolve(mKey);

Here mResultPromise was created in the page scope.  But mKey was created in the chrome scope (which I think is the real bug here), so the content-side promise can't touch it.

The basic problem is that ImportKeyTask::Init does this:

  nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));

and then uses that global for creating the CryptoKey.  But the creation of its result promise is done like so, by SUBTLECRYPTO_METHOD_BODY:

  RefPtr<Promise> p = Promise::Create(mParent, aRv);

where mParent is the mParent of the SubtleCrypto object.

Anyway, this seems like a pretty clear bug in the SubtleCrypto implementation.  I think it should be consistently creating all its objects in its own global, not the caller global...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: File showing this is a problem with pure web content too

I believe this testcase should log false, true, false, true.  It doesn't do that.  Another option would be true, false, true, false (what Chrome does); that requires creating the Promise in the caller compartment too.  But having the Promise and the key come from different places is just a recipe for badness.

The spec doesn't actually define behavior here, but needs to.  I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=29514 on that.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1250930-Use-SubtleCrypto-s-global-when-creating-.patch

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1250930-Use-correct-global-when-creating-a-key-i.patch

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1250930-Use-correct-global-when-creating-a-key-i.patch

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Will work on tests in a subsequent patch.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1250930-Add-test-to-ensure-we-re-using-the-right.patch

Tests.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1250930-Add-test-to-ensure-we-re-using-the-right.patch, v2

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8727493 [details] [diff] [review]
0001-Bug-1250930-Use-SubtleCrypto-s-global-when-creating-.patch

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8727494 [details] [diff] [review]
0002-Bug-1250930-Use-correct-global-when-creating-a-key-i.patch

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8727495 [details] [diff] [review]
0003-Bug-1250930-Use-correct-global-when-creating-a-key-i.patch

r=me.

By the way, I hope that commit message is actually one line and this diff viewer is just lying to me...  Same for the other patches.

Once this is done, are there still users of SUBTLECRYPTO_METHOD_BODY that don't pass mParent to the callee?  If not, we could push that bit down into SUBTLECRYPTO_METHOD_BODY, but I expect there are.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8727565 [details] [diff] [review]
0004-Bug-1250930-Add-test-to-ensure-we-re-using-the-right.patch, v2

r=me assuming this code runs onload.  If it runs before then, we could run into problems with the global in the iframe changing after we kick off the operations but before they complete (though it really shouldn't change, so maybe it's ok no matter what...)

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #15)
> r=me assuming this code runs onload.  If it runs before then, we could run
> into problems with the global in the iframe changing after we kick off the
> operations but before they complete (though it really shouldn't change, so
> maybe it's ok no matter what...)

I verified that our test runner starts running test when onload fires.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #14)
> By the way, I hope that commit message is actually one line and this diff
> viewer is just lying to me...  Same for the other patches.

Git breaks those lines for whatever reason. I always just fix them before pushing.

> Once this is done, are there still users of SUBTLECRYPTO_METHOD_BODY that
> don't pass mParent to the callee?  If not, we could push that bit down into
> SUBTLECRYPTO_METHOD_BODY, but I expect there are.

There are eight other uses that don't require the global to be passed. I started out modifying SUBTLECRYPTO_METHOD_BODY but then reverted to change only what's needed, to keep the patch small.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e27bd6b2188
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a0c2680427b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f197594181f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/f141fd7658f8

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Patch for Aurora

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Add-on and browser developers working with the WebCrypto API might hit this error.
[Describe test coverage new/current, TreeHerder]: Patch includes tests.
[Risks and why]: Risk is low.
[String/UUID change made/needed]: None.

r=bz

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Patch for Beta

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Add-on and browser developers working with the WebCrypto API might hit this error.
[Describe test coverage new/current, TreeHerder]: Patch includes tests.
[Risks and why]: Risk is low.
[String/UUID change made/needed]: None.

r=bz

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e27bd6b2188
https://hg.mozilla.org/mozilla-central/rev/5a0c2680427b
https://hg.mozilla.org/mozilla-central/rev/f197594181f9
https://hg.mozilla.org/mozilla-central/rev/f141fd7658f8

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

wyohknott, could you please verify this issue is fixed as expected on a latest Nightly build (03/09)? Thanks!

Comment 23

[wyohknott]

It does seem to work as intended on Nightly.

Thanks much to everyone involved!

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/648da47fced6

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to wyohknott from comment #23)
> It does seem to work as intended on Nightly.
> 
> Thanks much to everyone involved!

Great! Thank you for a prompt follow up.

Comment 26

[wyohknott]

I may have spoken too quickly. I checked it with the quick test I had made the first time, but now using my real code, it still fails with NS_ERROR_UNEXPECTED.

This is the gist of the code I'm trying to run: https://gist.github.com/WyohKnott/fd69492ca92bab93b8d4


It works when loaded as a background script in an extension but not while inside a content script.

Comment 27

[Boris Zbarsky [:bzbarsky]]

It's probably best to file a separate bug on this, since this one fixed a real problem... sorry for hijacking.  :(

What the new bug could really use is an actual way to reproduce (e.g. an actual SDK extension attached, and the exact steps to perform after installing it).

Comment 28

[wyohknott]

Actually you're right, Web Crypto is not the culprit at all but the use of the spread operator. I look for the right component to file a bug against.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Please cc me?  Especially if there is no decent error message in the browser console (in addition to the NS_ERROR_UNEXPECTED exception, which could just be coming from sanitization of a more-privileged exception which was reported to the browser console).

Comment 30

[Boris Zbarsky [:bzbarsky]]

Actually, if you're talking about the spread on the Uint8Array, then you're presumably hitting bug 1023984.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Actually, I was wrong.  This is a typed array specific issue; I filed bug 1256342.

I'm still not quite sure why you're getting an NS_ERROR_FAILURE sanitized exception, but I do expect that the right exception was reported to the browser console..

Comment 32

[wyohknott]

With further testing, I've got a "Error: Permission denied to access object" if I try to iterate over Uint8Array. I don't think this is related to the new bug you filed because I do get [object Array Iterator] if I follow the steps you've mentioned.

This is my test: https://gist.github.com/WyohKnott/151edcc3e6c53679aacf (run in a content script)

Comment 33

[Boris Zbarsky [:bzbarsky]]

> I don't think this is related to the new bug you filed

It's the same, though it has a similar underlying cause.  Filed bug 1256376.

If you can send me a complete extension that's having these problems, such that I can reproduce them locally, I can try to figure out what a sane way to get a typed array in your own global is here...