1324140 - Various dom::Promise methods assume that it's holding an actual Promise object, when it may not

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Per discussion in bug 1323324, a dom::Promise created via Promise::resolve can well be holding a pointer to a cross-compartment wrapper for a JS Promise, right?

The following dom::Promise methods call JSAPI bits that assume the passed-in thing is an actual JS Promise:

1) Promise::MaybeResolve, calling JS::ResolvePromise.
2) Promise::MaybeReject, calling JS::RejectPromise.
3) Promise::AppendNativeHandler, calling JS::AddPromiseReactions.
4) Promise::State, calling JS::PromiseState.

Of these, #3 is the most worrying, since calling AppendNativeHandler on a page-provided Promise is a totally sensible thing to do.  #1 and #2 shouldn't get hit in practice.  I don't know about #3.

OK, so the question here is what the right behavior is.  Should those APIs CheckedUnwrap?  Should dom::Promise CheckedUnwrap and store the canonical Promise?  Something else?  

My gut feeling is that the JSAPI access points should CheckedUnwrap here.

Comment 1

[Andrew McCreight [:mccr8]]

Till or anybody else, any comments here?

Comment 2

[Andrew McCreight [:mccr8]]

I have emailed Till about this bug.

Comment 3

[Till Schneidereit [:till]]

Looking into this now, sorry for the long delay.

Comment 4

[Till Schneidereit [:till]]

Attachment: Unwrap given Promise in some JSAPI functions

(needinfo? because r? doesn't is blocked. Feel free to reject and I'll ask someone else.)

The reason JSAPI functions don't unwrap is because it doesn't make sense for all of them and seemed complicated for some, so I didn't do it for any.

See bug 911216 comment 282 for when this was changed and bug 911216 226 and a few before that for some background.

The attached patch changes a bunch of JSAPI functions to unwrap. I didn't change JS::PromiseState: it seemed less dangerous and we'd have to change it to take a JSContext. If you think that should be changed, too, I can do that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8851318 [details] [diff] [review]
Unwrap given Promise in some JSAPI functions

>+        if (!unwrappedPromiseObj)
>+            return false;

Is uncatchable exception what we want to throw here?  Wouldn't some security exception make more sense?

I wonder whether it makes sense to common up the duplicated code in RejectPromise/ResolvePromise, and also to common up the duplicated code in CallOriginalPromiseThen/AddPromiseReactions.

GetPromiseState is less dangerous only because it sticks to reads, not writes, right?  I think it would be fine to have it return "pending" for a non-unwrappable promise and document accordingly....

Comment 6

[Till Schneidereit [:till]]

Attachment: Unwrap given Promise in some JSAPI functions. v2

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5)
> Comment on attachment 8851318 [details] [diff] [review]
> Unwrap given Promise in some JSAPI functions
> 
> >+        if (!unwrappedPromiseObj)
> >+            return false;
> 
> Is uncatchable exception what we want to throw here?  Wouldn't some security
> exception make more sense?

You're right, this should report properly. Fixed.
> 
> I wonder whether it makes sense to common up the duplicated code in
> RejectPromise/ResolvePromise, and also to common up the duplicated code in
> CallOriginalPromiseThen/AddPromiseReactions.

Done. A bit annoying for the latter two, but the right thing to do.

> GetPromiseState is less dangerous only because it sticks to reads, not
> writes, right?  I think it would be fine to have it return "pending" for a
> non-unwrappable promise and document accordingly....

Makes sense, changed to do that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8857970 [details] [diff] [review]
Unwrap given Promise in some JSAPI functions. v2

>+GetWaitForAllPromise(JSContext* cx, const JS::AutoObjectVector& promises);

Preexisting, but this needs documentation.  At the very least, what things are allowed in the "promises" array?  What happens if other things are in there?

The duplicated code in RejectPromise/ResolvePromise is still there.  I can live with that, but the bug comments sounded like you thought it made sense to combine them?

The uncatchable exceptions bits are also still there...

Maybe you forgot to qrefresh or equivalent?

Comment 8

[Till Schneidereit [:till]]

Attachment: Unwrap given Promise in some JSAPI functions. v3

Indeed, I botched a rebase, sorry about that.

I copied the comment for GetWaitForAllPromise from jsapi.h after fixing it to match what the function actually does (i.e., have it say that the passed-in objects can be wrappers for Promise objects.)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8858050 [details] [diff] [review]
Unwrap given Promise in some JSAPI functions. v3

>+ResolveOrRejectPromise(JSContext* cx, JS::HandleObject promiseObj, JS::HandleValue resultOrReason_,
>+        if (!unwrappedPromiseObj)
>+            return false;

Again, that should report an exception, right?

Same thing in CallOriginalPromiseThenImpl.

>+CallOriginalPromiseThenImpl(JSContext* cx, JS::HandleObject promiseObj,
>+        bool isWrapper = false;

This seems to be unused?

> JS::CallOriginalPromiseThen(JSContext* cx, JS::HandleObject promiseObj,
>-                            JS::HandleObject onResolveObj, JS::HandleObject onRejectObj)
>+                            JS::HandleObject onResolvedObj_, JS::HandleObject onRejectedObj_)

Why is this renaming needed?


> JS::AddPromiseReactions(JSContext* cx, JS::HandleObject promiseObj,
>-                        JS::HandleObject onResolvedObj, JS::HandleObject onRejectedObj)
>+                        JS::HandleObject onResolvedObj_, JS::HandleObject onRejectedObj_)

Likewise.

>+    RootedObject resultPromise(cx);

Should this be asserted null after the CallOriginalPromiseThenImpl call returns (this is still in AddPromiseReactions)?

r=me with the above fixed

Comment 10

[Till Schneidereit [:till]]

Attachment: Unwrap given Promise in some JSAPI functions. v4

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Unclear, no known way to exploit this.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
All supported branches, this was introduced when landing the SpiderMonkey Promise implementation in bug 911216.

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I don't, but I think the backports should be easy to do, assuming the patch doesn't just apply.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely. It makes some Promise-related JSAPI functions handle more inputs than they used to. Shouldn't need testing beyond a normal try run.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ for checkin on May 3, which is two weeks into the next cycle. We're releasing 53 in the next few days.

We'll want branch patches nominated as well so we can ship this on both ESR branches and 54 and 55.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c098fc762da975c0e47ea664100d234174bcd7f9

We're have no more planned ESR45 releases, so calling this wontfix for that branch.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/c098fc762da9

Comment 14

[Till Schneidereit [:till]]

Attachment: Patch for esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Has sec-approval and has landed on inbound.

Depends on the esr patch from bug 1346012.

Comment 15

[Gerry Chang [:gchang]]

Hi :till,
Should we consider to uplift this to Beta54?

Comment 16

[Till Schneidereit [:till]]

Comment on attachment 8858061 [details] [diff] [review]
Unwrap given Promise in some JSAPI functions. v4

Approval Request Comment

See esr-approval request.

Comment 17

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

Comment on attachment 8858061 [details] [diff] [review]
Unwrap given Promise in some JSAPI functions. v4

Sec-high, Beta54+

Comment 18

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

Comment on attachment 8864324 [details] [diff] [review]
Patch for esr52

Sec-high, ESR52.2+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9368d544cbc4
https://hg.mozilla.org/releases/mozilla-esr52/rev/8886f9cd5dd3