Closed Bug 1134955 Opened 5 years ago Closed 5 years ago

Crash when accessing Symbol property on crossdomain window properties

Categories

(Core :: JavaScript Engine, defect)

37 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- wontfix
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: christian.buchmueller, Assigned: bzbarsky)

Details

(Keywords: crash, crashreportid)

Crash Data

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux i686; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150219004126

Steps to reproduce:

window.open('localhost').location[Symbol.iterator]


Actual results:

Crash


Expected results:

An Exception should be thrown
I can't reproduce this on OS X on either Nightly or devedition.

Can you reproduce this on Nightly on Linux?
Flags: needinfo?(christian.buchmueller)
Product: Firefox → Core
(In reply to :Gijs Kruitbosch from comment #1)
> I can't reproduce this on OS X on either Nightly or devedition.
> 
> Can you reproduce this on Nightly on Linux?

Yes i can reproduce it with 38.01a. All open e10s tabs crashed.
No plugins were active.
Steps to reproduce it with 38.01a
1. Create a new tab with https://www.mozilla.org
2. Open the js console
3. emit w = window.open('http://example.org'); w.location[Symbol.iterator]; // => crash
Flags: needinfo?(christian.buchmueller)
(In reply to christian.buchmueller from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > I can't reproduce this on OS X on either Nightly or devedition.
> > 
> > Can you reproduce this on Nightly on Linux?
> 
> Yes i can reproduce it with 38.01a. All open e10s tabs crashed.
> No plugins were active.
> Steps to reproduce it with 38.01a
> 1. Create a new tab with https://www.mozilla.org
> 2. Open the js console
> 3. emit w = window.open('http://example.org'); w.location[Symbol.iterator];
> // => crash

Is this on a clean profile? I just checked in my Linux VM, still can't reproduce (both when I do and when I do not allow popups). I wonder what's different about my system... and are you using the web console or the browser console?
Flags: needinfo?(christian.buchmueller)
It also happens with a new profile in Web console.
I have started the Nightly from a terminal.
It dumps:
getProperty threw an exception: Error: Permission denied to access property 'location[Symbol'Line: 251, column: 19
to the console right when i type "w.location[Symbol.iterator".

Btw. "w.location.href" produces the expected exception. Seems to be straight forward Symbol related.
Flags: needinfo?(christian.buchmueller)
https://crash-stats.mozilla.com/report/index/0eaa4795-aca2-44e0-aadc-c20872150220
https://crash-stats.mozilla.com/report/index/b487a5e0-8133-43ae-a5af-469412150220

So the trick is that, at least for me, evaluating the two statements on one line doesn't crash. Waiting for the second window to load and then evaluating the second statement crashes, at least on Linux.

AFAICT these are nullpointer crashes so 'safe', but I'll leave this sec-sensitive until someone more experienced with these comes along.

These are JS engine crashes, so --> js engine.
Status: UNCONFIRMED → NEW
Crash Signature: [@ js::AutoEnterPolicy::reportErrorIfExceptionIsNotPending(JSContext*, jsid) ]
Ever confirmed: true
Keywords: crash, crashreportid
Component: Untriaged → JavaScript Engine
Annd I can repro on devedition + mac using the same steps, with a similar stack:

https://crash-stats.mozilla.com/report/index/4e817310-a494-4a3a-977a-0fa892150220
OS: Linux → All
Hardware: x86 → All
So the issue is that AutoEnterPolicy::reportErrorIfExceptionIsNotPending does:

41         JSString *str = IdToString(cx, id);

and assumes that this returns non-null.
So it seems to me like IdToString is a footgun now that we have symbols and it can throw for non-OOM reasons.  We have the following consumers:

1)  The AutoEnterPolicy thing there.  It wants a string to put as a property name into an
    exception message.
2)  Walk() in json.cpp.  This has the id passed in; throwing on symbol is probably ok here?
3)  JO() in json.cpp.  This gets ids from GetPropertyKeys.  No idea what it should do with
    symbols.
4)  KeyStringifier in json.cpp.  Probably OK to throw on symbols.
5)  js::Throw, which is in the same situation as AutoEnterPolicy.
6)  js::SuppresDeletedProperty which checks for symbols and nops; I'm told this is fine.
7)  NativeIterator::allocateIterator.  No idea what this should do with symbols.
8)  ReportPropertyError in TypedObject.  Again, exception message bit.
Jason suggested using ValueToSource for the exception message cases, so will try that.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8567170 [details] [diff] [review]
Be more careful with how we stringify property ids for error message reporting

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

::: js/src/proxy/Proxy.cpp
@@ +41,5 @@
> +        RootedValue idVal(cx, IdToValue(id));
> +        JSString *str = ValueToSource(cx, idVal);
> +        if (!str) {
> +            // Now we have an OOM exception on cx, so might as well
> +            // bail out of here.

Drop the comment, please -- we don't comment every error-check.

If your goal is to make evildoers think this was just a missing OOM check rather than a crash, I think the comment is likely to have rather the opposite effect, by drawing attention to it... ;)
Attachment #8567170 - Flags: review?(jorendorff) → review+
Argh, I forgot -- please also remove the single-quotes from the error message for JSMSG_PROPERTY_ACCESS_DENIED, in js/src/js.msg.
I think the comment was more about why we're not reporting anything even though we were asked to report something, but I can drop it.
Unhiding, since this is a guaranteed null-deref.  

Christian, thank you for finding and reporting this!  It's very much appreciated!
Group: core-security
Comment on attachment 8567170 [details] [diff] [review]
Be more careful with how we stringify property ids for error message reporting

Approval Request Comment
[Feature/regressing bug #]: Bug 1066322
[User impact if declined]: Crashes in some situations involving symbols and cross-origin windows/locations.  Probably fairly rare for now.
[Describe test coverage new/current, TreeHerder]: Attached test passes.
[Risks and why]: Very low-risk.
[String/UUID change made/needed]: None.
Attachment #8567170 - Flags: approval-mozilla-beta?
Attachment #8567170 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/dfb2d0f9a97c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Glad it was helpfull, and you already got it fixed.
Comment on attachment 8567170 [details] [diff] [review]
Be more careful with how we stringify property ids for error message reporting

(In reply to Boris Zbarsky [:bz] from comment #15)
> [User impact if declined]: Crashes in some situations involving symbols and
> cross-origin windows/locations.  Probably fairly rare for now.

Given that this is rare and 36 is scheduled to ship tomorrow, I'm going to mark 36 as wontfix even though this is the release in which we introduced this crash. 37 is now Beta so dropping the Aurora approval request and approving for Beta. This fix has been on m-c for 2 days. Let's get this into Beta 1.

Beta+
Attachment #8567170 - Flags: approval-mozilla-beta?
Attachment #8567170 - Flags: approval-mozilla-beta+
Attachment #8567170 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.