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

VERIFIED FIXED in Firefox 47

Status

()

defect
VERIFIED FIXED
3 years ago
4 months ago

People

(Reporter: clare.tor86, Assigned: ttaubert)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox45 wontfix, firefox46 wontfix, firefox47 fixed, firefox48 fixed)

Details

(Whiteboard: btpp-follow-up-2016-03-09)

Attachments

(6 attachments, 2 obsolete attachments)

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.
Actually the error returned is

> permission denied to access property 'then'
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.
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) })
> 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...
Product: Add-on SDK → Core
Component: General → DOM
Summary: WebCrypto API doesn't work in Add-on SDK content scripts → WebCrypto API doesn't work in Add-on SDK content scripts, or over Xrays in general
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(ttaubert)
Whiteboard: btpp-follow-up-2016-03-09
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Flags: needinfo?(ttaubert)
Will work on tests in a subsequent patch.
Attachment #8727527 - Attachment is obsolete: true
Attachment #8727527 - Flags: review?(bzbarsky)
Attachment #8727565 - Flags: review?(bzbarsky)
Comment on attachment 8727493 [details] [diff] [review]
0001-Bug-1250930-Use-SubtleCrypto-s-global-when-creating-.patch

r=me
Attachment #8727493 - Flags: review?(bzbarsky) → review+
Comment on attachment 8727494 [details] [diff] [review]
0002-Bug-1250930-Use-correct-global-when-creating-a-key-i.patch

r=me
Attachment #8727494 - Flags: review?(bzbarsky) → review+
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.
Attachment #8727495 - Flags: review?(bzbarsky) → review+
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...)
Attachment #8727565 - Flags: review?(bzbarsky) → review+
(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.
(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.
Attachment #8727493 - Flags: checkin+
Attachment #8727494 - Flags: checkin+
Attachment #8727495 - Flags: checkin+
Attachment #8727565 - Flags: checkin+
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
Attachment #8727792 - Flags: review+
Attachment #8727792 - Flags: approval-mozilla-aurora?
Posted patch Patch for Beta (obsolete) — Splinter Review
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
Attachment #8727802 - Flags: review+
wyohknott, could you please verify this issue is fixed as expected on a latest Nightly build (03/09)? Thanks!
Flags: needinfo?(clare.tor86)
Attachment #8727792 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
It does seem to work as intended on Nightly.

Thanks much to everyone involved!
Flags: needinfo?(clare.tor86)
(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.
Status: RESOLVED → VERIFIED
Attachment #8727792 - Flags: checkin+
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.
Flags: needinfo?(ttaubert)
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).
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.
Flags: needinfo?(ttaubert)
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).
Actually, if you're talking about the spread on the Uint8Array, then you're presumably hitting bug 1023984.
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..
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)
> 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...
Attachment #8727802 - Attachment is obsolete: true
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.