Closed Bug 1324140 Opened 3 years ago Closed 2 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 54+ wontfix
firefox-esr52 54+ fixed
firefox53 --- wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: bzbarsky, Assigned: till)

References

Details

(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main54-][adv-esr52.2-])

Attachments

(2 files, 3 obsolete files)

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.
Flags: needinfo?(till)
Till or anybody else, any comments here?
I have emailed Till about this bug.
Looking into this now, sorry for the long delay.
Assignee: nobody → till
(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.
Flags: needinfo?(till) → needinfo?(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....
Flags: needinfo?(bzbarsky)
(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.
Attachment #8851318 - Attachment is obsolete: true
Attachment #8857970 - Flags: review?(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?
Flags: needinfo?(till)
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.)
Attachment #8857970 - Attachment is obsolete: true
Attachment #8857970 - Flags: review?(bzbarsky)
Flags: needinfo?(till)
Attachment #8858050 - Flags: review?(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
Attachment #8858050 - Flags: review?(bzbarsky) → review+
[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.
Attachment #8858061 - Flags: sec-approval?
Attachment #8858061 - Flags: review+
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.
Attachment #8858061 - Flags: sec-approval? → sec-approval+
Attachment #8858050 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/c098fc762da975c0e47ea664100d234174bcd7f9

We're have no more planned ESR45 releases, so calling this wontfix for that branch.
Whiteboard: [checkin on 5/3]
Depends on: 1346012
https://hg.mozilla.org/mozilla-central/rev/c098fc762da9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attached patch Patch for esr52Splinter Review
[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.
Attachment #8864324 - Flags: review+
Attachment #8864324 - Flags: approval-mozilla-esr52?
Hi :till,
Should we consider to uplift this to Beta54?
Flags: needinfo?(till)
Comment on attachment 8858061 [details] [diff] [review]
Unwrap given Promise in some JSAPI functions. v4

Approval Request Comment

See esr-approval request.
Flags: needinfo?(till)
Attachment #8858061 - Flags: approval-mozilla-beta?
Group: dom-core-security → core-security-release
Comment on attachment 8858061 [details] [diff] [review]
Unwrap given Promise in some JSAPI functions. v4

Sec-high, Beta54+
Attachment #8858061 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8864324 [details] [diff] [review]
Patch for esr52

Sec-high, ESR52.2+
Attachment #8864324 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54-][adv-esr52.2-]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.