Closed Bug 1303714 Opened 3 years ago Closed 3 years ago

"Promise rejection value is a non-unwrappable cross-compartment wrapper" while using WebCrypto in content-script

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 + fixed
firefox52 + fixed

People

(Reporter: clare.tor86, Assigned: evilpie)

References

Details

(Keywords: regression)

Attachments

(3 files)

See example extension for the code. It's a content-script that runs on mozilla.org for testing.

The same code runs fine in a background script, but fails in a content script with error: "Promise rejection value is a non-unwrappable cross-compartment wrapper"

It seems that crypto.subtle.sign can't pass its results to the next part of the Promise.

I have no idea if it's a problem specific to WebCrypto or yet another Compartment/XRays problem (Boris Zbarsky previously filled bug 1256376 and bug 1256342 that were affecting this part of my code).

Thanks
¡Hola Boris!

I'm guessing this is a legit bug, right? If so, could you please mark it NEW and pick the right component for it.

¡Gracias!
Alex
Sounds like a legit bug, yes.

This might well be the right component. The other option is Security, if webcrypto is messing up where it's creating its objects...

Till, do you have time to take a look?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(till)
That said, if I actually run this over Xrays by running it in a browser scratchpad and prefixing a few things like "crypto" with "content." it works fine.
So... We land in RejectMaybeWrappedPromise, ultimately called off a PromiseJobRunnable::Run.  

On entry to the method, "reason_" is an Error object.  That Error object's global is a Sandbox.  The Promise being rejected comes from a Window (the mozilla.org one, I assume).  That would make sense if it's created by webcrypto, and is correct.

So where is the sandbox-created Error coming from?  Digging into it, it's coming from line 39 of the script, and has the message "invalid arguments".  We (correctly-ish) don't want to expose this Error to the untrusted promise.

OK, so three things:

1)  We should really log the original exception somewhere.  That would have made it clear which part of this script is failing.
2)  We should really try to make rejecting with a non-unwrappable thing work.  What prevents that, again?
3)  Why is the Uint8Array constructor throwing?

For #3, it's happening because the thing passed to that constructor is an opaque Xray.  And that's happening because the value passed in (the thing sign() generated) is in the _content_ compartment (see bug 1250930 for the history there) but the typed array being constructed is in the sandbox compartment.  And we don't seem to have useful Xrays for ArrayBuffer.  I don't see a bug for that...  Anyway, no Xray means in TypedArrayObjectTemplate<unsigned char>::fromBufferWithProto we get js::Other for GetBuiltinClass and then we can't create the typed array.

If this were our chrome code I would claim the typed array should be getting created in the content compartment, but for extensions that's not really viable....

So the good news for Till is that I think #3 is a problem with DOM promises too.  So this is not a regression from his patch there.  clare.tor86@gmail.com, am I right that you see this on Fx48 too, but possibly with a more informative error message?

The bad news is that fixing this requires either changing how webcrypto works in ways that might break other stuff or implementing Xrays to ArrayBuffer.  Is there a reason we didn't implement those?
Flags: needinfo?(bobbyholley)
(In reply to Boris Zbarsky [:bz] (TPAC) from comment #4)
> The bad news is that fixing this requires either changing how webcrypto
> works in ways that might break other stuff or implementing Xrays to
> ArrayBuffer.  Is there a reason we didn't implement those?

Nope, just wasn't a pressing need. See bug 1248865.
Flags: needinfo?(bobbyholley)
>am I right that you see this on Fx48 too, but possibly with a more informative error message?

:bz

Actually I ran all my tests on Nightly so far, and now running it on the Firefox 48.0.1 I have, it is working correctly, showing me the signature result. So it appears that it's a regression.
[Tracking Requested - why for this release]: Breaks extensions trying to do webcrypto

Oh, interesting.

Looks like this regressed in this checkin range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cf06fbc831754e54c6abb71d3136597488a530e0&tochange=f97a056ae6235de7855fd8aaa04fb1c8d183bd06

The obviously related change in there is bug 1114580.  Before that change, the code in fromBufferWithProto landed in Wrapper::getBuiltinClass in the Xray case and claimed to be whatever class the underlying thing is.  Then it went on to do a CheckedUnwrap which _succeeded_, because this is an Xray.  So everything ended up working.

After that change, fromBufferWithProto gets js::Other (because Xrays to ArrayBuffer use OpaqueXrayTraits) and so throws.

We should probably fix this regression, then spin off the other stuff into other bugs.  I'm not sure whether it's best to do that by implementing proper Xrays for ArrayBuffer or whether the typed array code should be changed or what.

In general, the fact that GetBuiltinClass and CheckedUnwrap disagree with each other seems like a bug to me...
Blocks: 1114580
Flags: needinfo?(evilpies)
I think we basically worked around the same issue in bug 1288903. In generally I believe that ::getBuiltinClass for OpaqueXrayTraits should be Other. Changing that would be quite some work from what I remember, and in my opinion this also pierces through what is supposed to be opaque to the user. Example: you might treat something as a Boolean object after testing with Object#toString, but you actually can't touch it.

The best solution seems to be implementing Xrays for ArrayBuffer. However seeing as how TypedArrayObjectTemplate::fromBufferWithProto already calls CheckedUnwrap anyway, we might just do that before calling GetBuiltinClass? (In think in practice just removing the GetBuiltinClass check should make this function work)
Flags: needinfo?(evilpies)
I implemented the fix suggested in my previous comment: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c731881a9d86 and it fixes the problem reported in comment 1. So the only potential issue here might be that we call CheckedUnwrap on something that isn't ArrayBuffer, I guess that is probably fine?
That seems pretty reasonable to me, but I'd want bholley to review to make sure...
Tracking 52+ for the reason noted in Comment 7.
Flags: needinfo?(till)
Assignee: nobody → evilpies
I still would be interested in understanding when unwrapping Opaque Xrays makes sense, if our current toString behavior should maybe be changed and how much work would go into creating JSXrays for ArrayBuffer.
Attachment #8794369 - Flags: review?(bobbyholley)
> when unwrapping Opaque Xrays makes sense

Sounds like a question for Bobby...
Flags: needinfo?(bobbyholley)
Track for 51+ as webcrypto regression.
Comment on attachment 8794369 [details] [diff] [review]
Unwrap ArrayBuffer in TypedArray constructor

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

Sorry for the delay.
Attachment #8794369 - Flags: review?(bobbyholley) → review+
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> In general, the fact that GetBuiltinClass and CheckedUnwrap disagree with
> each other seems like a bug to me...

This isn't an inherent contradiction. Xrays are designed to protect the caller from confusion. CheckedUnwrap waives that protection if doing so is allowed (kinda like the C++ version of .wrappedJSObject).

(In reply to AWAY Tom Schuster [:evilpie] from comment #12)
> I still would be interested in understanding when unwrapping Opaque Xrays
> makes sense

I think that the new behavior of GetBuiltinClass for Xrays makes sense. I think that unwrapping them as the patch does is fine for a stopgap, but we should really just implement xrays for ArrayBuffer. The original JSXrays were a best-effort project, and only implemented what was needed to turn our tree green. Now that we're exposing Xrays to WebExtensions, I think we should fill in those gaps.

>, if our current toString behavior should maybe be changed

I don't really know what this is about.

> and how much work would go into creating JSXrays for ArrayBuffer.

Should be pretty straightforward I'd think if you model it after the TypedArray Xrays, unless any unforeseen complications come up.

Thanks for working on this stuff! Sorry I'm busy with other things. :-(
Flags: needinfo?(bobbyholley)
I filed bug 1306200 on comment 4 item 1.

I guess comment 4 item 2 is covered by this code comment in RejectMaybeWrappedPromise:

        // The rejection reason might've been created in a compartment with higher
        // privileges than the Promise's. In that case, object-type rejection
        // values might be wrapped into a wrapper that throws whenever the
        // Promise's reaction handler wants to do anything useful with it. To
        // avoid that situation, we synthesize a generic error that doesn't
        // expose any privileged information but can safely be used in the
        // rejection handler.

it's a bit unfortunate that we can't just make it work for the cases when the rejection handler _is_ privileged enough.  And what's special about rejection handlers here that doesn't apply to resolution handlers?  Or put another way, why doesn't FulfillMaybeWrappedPromise have similar code?

How did this use to work with DOM promises?
Flags: needinfo?(till)
Thanks. I pushed this to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c26e5c6cd934. I think it makes sense to request uplift to 51 for this after landing on m-c.

My comment about .toString was about the behavior with Object.prototype.toString, where with bug 1114580 an Opaque wrapper wrapping a String object isn't going to start returning "[object String]", because we censor getBuiltinClass.

I will talk about this with Till in person this week, maybe he has ideas for the JSXrays as well.
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e438f32ce3
Always Unwrap ArrayBuffer in TypedArray constructor. r=bholley
https://hg.mozilla.org/mozilla-central/rev/19e438f32ce3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8794369 [details] [diff] [review]
Unwrap ArrayBuffer in TypedArray constructor

Approval Request Comment
[Feature/regressing bug #]: bug 1114580
[User impact if declined]: Breaks add-ons using WebCrypto and other uses of ArrayBuffer/typed arrays
[Describe test coverage new/current, TreeHerder]: Has a simple test that covers wrapped arraybuffers
[Risks and why]: Low risk, small modification to existing code
[String/UUID change made/needed]: none
Attachment #8794369 - Flags: approval-mozilla-aurora?
Finally my extension works! The last bug has been vanquished! 
Thanks to everyone involved for your hard work!
Comment on attachment 8794369 [details] [diff] [review]
Unwrap ArrayBuffer in TypedArray constructor

Fix a regression related to WebCrypto. Take it in 51 aurora.
Attachment #8794369 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This needs rebasing for Aurora.
Flags: needinfo?(evilpies)
Attached patch Aurora rebaseSplinter Review
One function name changed, but that code was removed anyway.
Flags: needinfo?(evilpies) → needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Flags: needinfo?(till)
You need to log in before you can comment on or make changes to this bug.