Bug 897678 (CVE-2013-5602)

ASAN SEGV on unknown address in Worker::SetEventListener

RESOLVED FIXED in Firefox 25

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: nils, Assigned: Waldo)

Tracking

({sec-critical})

Trunk
mozilla26
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 wontfix, firefox24 wontfix, firefox25+ verified, firefox26+ verified, firefox-esr1725+ verified, firefox-esr2425+ verified, b2g1825+ fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [dupe 831090?][adv-main25+][adv-esr1710+][adv-esr24-1+])

Attachments

(8 attachments, 11 obsolete attachments)

10.40 KB, text/plain
Details
410 bytes, text/html
Details
48.61 KB, patch
mrbkap
: review+
dveditz
: sec-approval+
Details | Diff | Splinter Review
6.26 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
48.79 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
47.66 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
49.37 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
47.50 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
Posted file testcase (crashes firefox) (obsolete) —
The attached testcase crashes the ASAN build of Firefox with the attached testcase.

It involves event handlers and exchange of __proto__ attributes, similar to Bug 831090.
Reporter

Comment 1

6 years ago
Posted file ASAN stack
This looks like exactly the issue in bug 831090, but with a testcase that reproduces in a current build.

Jeff, now you should be able to reproduce....
Assignee: general → jwalden+bmo
Blocks: 831090
Flags: needinfo?(jwalden+bmo)

Comment 3

6 years ago
Attachment #780586 - Attachment is obsolete: true

Comment 4

6 years ago
Assertion failure: JSID_IS_INT(aIdval), at dom/workers/Worker.cpp:206

Comment 5

6 years ago
I see why I missed this.  I filed bug 899046.

Nils, how did your fuzzer get past bug 899046?  Did you teach it specifically about workers?
Reporter

Comment 6

6 years ago
Jesse, yes it knows about workers as part of one of it's modules.
Keywords: sec-critical
Whiteboard: [dupe 831090?]
Assignee

Comment 7

6 years ago
(gdb) bt
#0  0x00007ffff1382879 in (anonymous namespace)::Worker::SetEventListener (
    aCx=0x7fffd98dfe70, aObj=(JSObject * const) 0x7fffe48f9800 [object Text], 
    aIdval=$jsid("onmessage"), aStrict=0, aVp=JSVAL_NULL)
    at /home/jwalden/moz/slots/dom/workers/Worker.cpp:207
#1  0x00007ffff35ee7c0 in js::CallJSPropertyOpSetter (cx=0x7fffd98dfe70, 
    op=0x7ffff1382805 <(anonymous namespace)::Worker::SetEventListener(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JSBool, JS::MutableHandle<JS::Value>)>, 
    obj=(JSObject * const) 0x7fffe48f9800 [object Text], id=$jsid("onmessage"), strict=0, 
    vp=JSVAL_NULL) at /home/jwalden/moz/slots/js/src/jscntxtinlines.h:309
#2  0x00007ffff35ee983 in js::CallSetter (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9800 [object Text], id=$jsid("onmessage"), 
    op=0x7ffff1382805 <(anonymous namespace)::Worker::SetEventListener(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JSBool, JS::MutableHandle<JS::Value>)>, attrs=65, shortid=0, 
    strict=0, vp=JSVAL_NULL) at /home/jwalden/moz/slots/js/src/jscntxtinlines.h:335
#3  0x00007ffff36027bf in js::baseops::SetPropertyHelper (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9800 [object Text], 
    receiver=(JSObject * const) 0x7fffe48f9800 [object Text], id=$jsid("onmessage"), 
    defineHow=0, vp=JSVAL_NULL, strict=0) at /home/jwalden/moz/slots/js/src/jsobj.cpp:4558
#4  0x00007ffff33693be in JSObject::setGeneric (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9800 [object Text], 
    receiver=(JSObject * const) 0x7fffe48f9800 [object Text], id=$jsid("onmessage"), 
    vp=JSVAL_NULL, strict=0) at /home/jwalden/moz/slots/js/src/jsobj.h:868
#5  0x00007ffff3630e8b in js::DirectProxyHandler::set (
    this=0x7ffff66766e0 <js::CrossCompartmentWrapper::singleton>, cx=0x7fffd98dfe70, 
    proxy=(JSObject * const) 0x7fffe48f9840 [object Proxy], 
    receiver=(JSObject * const) 0x7fffe48f9800 [object Text], id=$jsid("onmessage"), 
    strict=false, vp=JSVAL_NULL) at /home/jwalden/moz/slots/js/src/jsproxy.cpp:576
#6  0x00007ffff3698109 in js::CrossCompartmentWrapper::set (
    this=0x7ffff66766e0 <js::CrossCompartmentWrapper::singleton>, cx=0x7fffd98dfe70, 
    wrapper=(JSObject * const) 0x7fffe48f9840 [object Proxy], 
    receiver=(JSObject * const) 0x7fffe48f9840 [object Proxy], id=$jsid("onmessage"), 
    strict=false, vp=JSVAL_NULL) at /home/jwalden/moz/slots/js/src/jswrapper.cpp:314
#7  0x00007ffff363cd77 in js::Proxy::set (cx=0x7fffd98dfe70, 
    proxy=(JSObject * const) 0x7fffe48f9840 [object Proxy], 
    receiver=(JSObject * const) 0x7fffe48f9840 [object Proxy], id=$jsid("onmessage"), 
    strict=false, vp=JSVAL_NULL) at /home/jwalden/moz/slots/js/src/jsproxy.cpp:2553
#8  0x00007ffff363e479 in proxy_SetGeneric (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9840 [object Proxy], id=$jsid("onmessage"), 
    vp=JSVAL_NULL, strict=0) at /home/jwalden/moz/slots/js/src/jsproxy.cpp:2871
#9  0x00007ffff35f76b6 in JSObject::nonNativeSetProperty (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9840 [object Proxy], id=$jsid("onmessage"), 
    vp=JSVAL_NULL, strict=0) at /home/jwalden/moz/slots/js/src/jsobj.cpp:1742
#10 0x00007ffff3369391 in JSObject::setGeneric (cx=0x7fffd98dfe70, 
    obj=(JSObject * const) 0x7fffe48f9840 [object Proxy], 
    receiver=(JSObject * const) 0x7fffe48f9840 [object Proxy], id=$jsid("onmessage"), 
    vp=JSVAL_NULL, strict=0) at /home/jwalden/moz/slots/js/src/jsobj.h:867

The basic problem is in this code in js::baseops::SetPropertyHelper:

    RootedObject pobj(cx);
    RootedShape shape(cx);
    if (!LookupPropertyWithFlags(cx, obj, id, cx->resolveFlags, &pobj, &shape))
        return false;
    if (shape) {
        if (!pobj->isNative()) {
            if (pobj->is<ProxyObject>()) {
                AutoPropertyDescriptorRooter pd(cx);
                if (!Proxy::getPropertyDescriptor(cx, pobj, id, &pd, JSRESOLVE_ASSIGNING))
                    return false;

                if ((pd.attrs & (JSPROP_SHARED | JSPROP_SHADOWABLE)) == JSPROP_SHARED) {
                    return !pd.setter ||
                           CallSetter(cx, receiver, id, pd.setter, pd.attrs, pd.shortid, strict,
                                      vp);
                }

We look up the property on |frameText| from the testcase.  That's a proxy, so we go through proxy handling and look it up on the underlying Text object.

Looking up on that, we recur into |w.__proto__|, which is from the original page (not the iframe page) and so is a proxy/wrapper.  Lookups on proxies return |pobj == the proxy object| and |shape| is set to a bogus, non-null value by |MarkNonNativePropertyFound|.

So we have |shape|, |!pobj->isNative()|, and |pobj->is<ProxyObject>()|.  We descend into the block containing the getPropertyDescriptor and such.  The returned descriptor includes the actual JSStrictPropertyOp setter to be called.

JSStrictPropertyOp has a couple requirements.  You have to pass in |shape->userid()| as id, not whatever was used for the lookup, so that the op gets what it expects (the shortid, not the id used for the lookup).  That's the assert being hit here.  In theory, if |pd.attrs & JSPROP_SHORTID|, |pd.shortid| will be used by |CallSetter| and all will be well.  But 1) |pd.attrs == 0x41| and |JSPROP_SHORTID == 0x100| so no match, and 2) |pd.shortid == 0| -- and the underlying shape's propid_ is "onmessage", despite having setter be Worker::SetEventListener.  I'm not sure why that is, exactly -- conceivably that's the bug, maybe?  That said, if memory serves, propertyop getters/setters can't assume they're passed an object of the kind they expect -- so GetEventListener/SetEventListener may need fixing in that case even if this (tiny|short)id nonsense needs fixing.

More investigation needed, not clearing needinfo yet.
No matter what we do on the JS engine side here, we should just stop using propertyops (with shortid at that!) on the worker side, imo...
That means converting everything to WebIDL and/or ditching the event implementation?  We plan to do all of that.

If it means something more complex than that then ...
Well, converting to WebIDL will certainly do that.  The question is whether there's a simpler switch to JSNative getters/setters we can do before then, and whether it's worthwhile.
Assignee

Comment 11

6 years ago
Agreed about not using propertyops -- that holds everywhere, not just here.  I also think we shouldn't be using shortids/tinyids, which would fall out of not using propertyops here, but which could be done independent of that (although I'm not sure it'd fix this bug, exactly).  Exactly what set of changes will fix this, as minimally as possible, remains unclear.  Ergo, more investigation.
Assignee

Comment 12

6 years ago
Posted patch Possible patch (obsolete) — Splinter Review
Building this now to test.  Written against a slightly-oldish tree, so probably already needs rebasing due to include/JSBool/etc. refactoring work.  Good times.
Flags: needinfo?(jwalden+bmo)
Assignee

Comment 13

6 years ago
Okay, this really seems to work.  (Without crashing on shutdown because I called |worker->SetEventListener("onmessage", listener, rv)| rather than |worker->SetEventListener("message", listener, rv)|.  Ugh.  That should *really* be fixed so you're not specifying a string you could get wrong, via typos or whatever, at some point.)

The original code shouldn't have used propertyops and tinyids, but I'm not actually sure the JS engine's doing nothing wrong with this.  I'll probably try to investigate more before declaring this patch a complete success/solution (to all possible instances of the problem, not just this specific one, if the ultimate issue is not specific to this worker code).  But it seems worth getting the review-clock going while that happens.  (Even if this can't land for a good long while because of uplift-cycle timing.  Sigh.)
Attachment #787798 - Attachment is obsolete: true
Attachment #787864 - Flags: review?(bent.mozilla)
Assignee

Comment 14

6 years ago
I considered investigating further to figure out whether the JS engine's at fault here or not.  But it seems like it might be easier to search for all instances of PropertyOp/StrictPropertyOp properties and audit them:

http://mxr.mozilla.org/mozilla-central/search?string=JSOP_WRAPPER

What makes this relatively simple -- assuming I understand the uses sufficiently, and I need people to double-check me on this -- is that not all instances are actually exposed to untrusted code.  Looking down the list of files in question:

js/src/ctypes/CTypes.cpp: trusted, not exposed to web -- no changes needed

js/src/perf/jsperf.cpp: trusted, not exposed to web -- no changes needed

js/src/builtin/RegExp.cpp: none of the instances here touch |obj| at all -- they go straight to RegExpStatics -- so no changes needed
js/src/shell/js.cpp: trusted, not exposed to web -- no changes needed

dom/workers/WorkerScope.cpp:
dom/workers/Exceptions.cpp:
dom/workers/File.cpp:
dom/workers/Events.cpp:
These are untrusted.  But they *only* implement fields visible within workers.  Workers can never observe a main-thread global object.  Is there any way for them to observe some other global object, to trigger this cross-compartment issue?  If there isn't, I don't think these need any changes.

There's one other possibility for the dom/workers/ hits -- that they can be triggered on an object using something like |new Proxy(relevantObjectInsideWorker, {})| and an access of one of the properties at issue.  That needs investigation.

So I think there's only the direct-proxy issue to investigate, to determine that the fix posted here, is adequate to address every instance of the problem in Gecko -- regardless whether SpiderMonkey is or is not doing the right thing here.  I'll want a double-check on this logic from some other JS engine person, of course, before declaring any sort of victory on this front, tho.  :-)
Status: NEW → ASSIGNED
Comment on attachment 787864 [details] [diff] [review]
Convert onerror/onmessage to accessors from propertyops

Blake has kindly offered to review this for me.
Attachment #787864 - Flags: review?(bent.mozilla) → review?(mrbkap)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> if memory serves, propertyop getters/setters can't assume they're passed an
> object of the kind they expect -- so GetEventListener/SetEventListener may
> need fixing in that case even if this (tiny|short)id nonsense needs fixing.

Both the getter and setter here check that the class of the passed-in object is the expected class, so they should be fine.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> I considered investigating further to figure out whether the JS engine's at
> fault here or not. 

The JS engine is definitely at fault here. More in a bit...

> js/src/shell/js.cpp: trusted, not exposed to web -- no changes needed

For what it's worth, this is a bit of a touchy subject. At least one distro offers the JS shell as a package for running JS. I don't know if that's still the case, though.

> There's one other possibility for the dom/workers/ hits -- that they can be
> triggered on an object using something like |new
> Proxy(relevantObjectInsideWorker, {})| and an access of one of the
> properties at issue.  That needs investigation.

This definitely needs to be tested, but from my understanding of the problem here, these cases almost certainly need to be fixed. Also, the code at http://mxr.mozilla.org/mozilla-central/source/gfx/skia/src/xml/SkJSDisplayable.cpp#183 might be affected by this.

> So I think there's only the direct-proxy issue to investigate, to determine
> that the fix posted here, is adequate to address every instance of the
> problem in Gecko -- regardless whether SpiderMonkey is or is not doing the
> right thing here.  I'll want a double-check on this logic from some other JS

I'm wary of this approach, in part because it makes the SpiderMonkey API even harder to reason about than it already is. I'm all for simplifying the worker code to not use tinyids anymore, but doing only Worker.cpp seems incomplete and prone to error.

There are a couple of problems that lead to the assertion here: firstly, whether or not a js::Shape has a tinyid is not stored in its attrs but rather in its flags (!). This is why the attrs in comment 7 didn't include JSPROP_SHORTID. Furthermore, js::DirectProxyHandler::getPropertyDescriptor uses jsapi.cpp:GetPropertyDescriptorById, which doesn't copy the tinyid from the shape *and* doesn't copy the information about the tinyid out of the shape's attrs anyway. Fixing that might be simple (and possibly enough) except that reading through JS_DefineProperties, I think every property defined ends up with Shape::HAS_SHORTID, so we'd probably want to fix that as well (unless I'm misreading the code). 

I also find it kind of weird that we have two entirely different ways of calling setters: js::Shape::set and js::CallSetter. It was only through reading both of those methods that I noticed the attrs/flags distinction.

So I'd rather either ween workers off of tinyids entirely or just fix the immediate problems with tinyids here and be done with it.
Comment on attachment 787864 [details] [diff] [review]
Convert onerror/onmessage to accessors from propertyops

Clearing request based on comment 17.
Attachment #787864 - Flags: review?(mrbkap)
Assignee

Comment 19

6 years ago
I'll wean them off tinyids entirely, then -- should be fairly quick, I think.
Please at least file a followup on fixing tinyids or nuking them entirely. I don't fully understand the reluctance to fix the tinyid bug here, though.
Assignee

Comment 21

6 years ago
The GetterOnlyJSNative bit (a native mimicking the behavior of js_GetterOnlyPropertyStub) is partly paranoia, partly motivated by existing tests that think that setting some of these properties *should* throw errors even outside strict mode code.  We can remove it on trunk, but it'll require fixing a bunch of tests.  Not something to do now, for something we'd want to backport.

I forgot that Property<SLOT_loaded>::Get will work even with old gcc, where GetProperty<SLOT_loaded> won't.  So that at least lets me get rid of N code-different function definitions to placate the JS_PSG macro-calls in gcc 4.4.

I originally did all the functions without JS::CallNonGenericMethod.  But doing so happened to trigger some fairly obscure "foo called on incompatible Proxy" errors in our own test suite.  Given that, it seemed fairly risky to only spot-fix the ones needed to make mochitests pass, when it might break sites during a backport.  (The old PropertyOp code didn't need this non-generic thing, because property ops semi-automatically get the unwrapped-y thing passed into them, not the proxy object.)

Double-checks of the IsFoo static methods are probably the most important thing -- if those over-accept stuff that's the easiest way for anything here to go off the rails.

The JS_GetConstructor usage is hackitudinous, but given the precise scoping of it to immediately after the JS_InitClass, it shouldn't be able to hit anything dodgy.

The WorkerScope.cpp getters technically should accept primitive |this| values, and work out the global object themselves in that case.  But we're "saved" here because SpiderMonkey wrongly boxes primitives up into objects for implicit calls to accessor functions -- bug 603201, bug 869098 -- so these don't need to do extra work now to work correctly.  When those bugs are fixed, we'll have to circle back and fix this code accordingly.

This is a large-ish patch, but it's mostly pretty simple.  I have more faith in its being correct, than I do in my understanding the existing interactions of tinyids in the JS engine, and "fixing" how tinyids work wrt proxies and proxied gets and stuff.

This passes try:

https://tbpl.mozilla.org/?tree=Try&rev=c107d0fd95ec
https://tbpl.mozilla.org/?tree=Try&rev=bef3335c33c5
Attachment #787864 - Attachment is obsolete: true
Attachment #796399 - Flags: review?(mrbkap)
Comment on attachment 796399 [details] [diff] [review]
Fix all the workers code using propertyops

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

Please file followups on getting rid of propertyops in the other places.
Attachment #796399 - Flags: review?(mrbkap) → review+
Assignee

Comment 23

6 years ago
I'm working in branch backports now.  Enough has changed at the periphery that blindly importing the patch here triggers a bunch of rejects, so I'm trying to get the branch-targeted work done before moving ahead on trunk.
Assignee

Comment 24

6 years ago
At a glance, the vast majority of the differences are the result of s/JSBool/bool/g on trunk.  Converting the few JSBool to bool in context, then reapplying the patch so the unchanged parts are picked up, appears as if it'll get the job done (of course, with another post-step afterward to revert bool->JSBool again).
Assignee

Comment 27

6 years ago
Posted patch Real aurora backport (obsolete) — Splinter Review
The various Is* methods needed to accept const Value&, not a handle.  Also maybe a typo or two beyond that.
Assignee

Comment 28

6 years ago
Posted patch Real beta backport (obsolete) — Splinter Review
Same extra set of fixes, plus some tweaks so that two uses of DOMException will work.  (BindingUtils.h gets added to Exceptions.cpp, but that pulls in domstubs.h, which forward-declares a DOMException class.  Then the two DOMException::* calls near the end of that file are ambiguous between the anonymous-namespaced DOMException in the file, and the global forward-declaration of DOMException.  I added anonymous-namespaced forwarding methods to resolve the ambiguity.  Bleh.  No idea why this issue doesn't exist on alpha/trunk, no real point finding out, either.)

The aurora backport patch built locally.  The beta patch has built past dom/workers now.  So these are probably good to go, but I'm going to finish this build and try them before declaring success.
Assignee

Comment 30

6 years ago
...and even still those aren't quite entirely right -- beta needs MOZ_STATIC_ASSERT still -- but it's close enough to declare success.  Requesting approvals momentarily...
Assignee

Comment 31

6 years ago
The trunk patch depends on bug 908898, so that has to be backported as well.  It's a much simpler backport than the main patch here did.
Assignee

Comment 32

6 years ago
Posted patch Backported patch for aurora (obsolete) — Splinter Review
Attachment #798586 - Attachment is obsolete: true
Attachment #798587 - Attachment is obsolete: true
Attachment #798658 - Attachment is obsolete: true
Attachment #798660 - Attachment is obsolete: true
Assignee

Comment 34

6 years ago
Posted patch Backported patch for beta (obsolete) — Splinter Review
Assignee

Comment 35

6 years ago
Comment on attachment 796399 [details] [diff] [review]
Fix all the workers code using propertyops

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not super-easily.  There's trickery to getting pointers lined up "just so" to avoid crashing, then to avoid it in usefully-exploitable ways.  But this might be something heap-spraying would eat for breakfast, at cursory thought: I've never done the exercise of writing an exploit with invalid heap-memory use as vector, so I don't know for sure.

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

Which older supported branches are affected by this flaw?
All of them.

If not all supported branches, which bug introduced the flaw?
Probably dates back to the use of tinyids in this stuff in the first place.  I suspect tinyids have been busted in the through-a-proxy case ever since proxies were introduced.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Aurora/beta backports are finished.  I don't have a b2g18 backport or an esr17 backport yet.  None of this code has meaningfully changed in some time, but the JSAPI has changed substantially at the surface level -- JSBool->bool, adding handles to things, and so on -- such that creating backport patches involves a fair bit of tedium to apply the trunk patch's changes to older branches.  Risk of such changes is pretty low, tho -- if they pass mochitests, they should be good.

How likely is this patch to cause regressions; how much testing does it need?
Just for sheer quantity of changes, probably the branch-targeted backports want more than the two weeks of testing they'd get if landed now.  :-\  I think, given there's no hint of the issue in the patch, we might land this September 17 (just after uplift) on trunk, then on branches a week or so later, for settling time.  But up to you if you think it's worth pushing on this sooner.
Attachment #796399 - Flags: sec-approval?
Al is PTO. In his absence, perhaps Dan can approve.
Flags: needinfo?(dveditz)
I do appreciate the work to create the branch patches proactively, but that's a much bigger chunk of change than we're comfortable taking on beta this close to release. And thankfully the patch is opaque so we're not forced to take it by landing on M-C.

We will want esr-17 fixes for 25 (don't land until after the merge). Not sure about b2g18--this won't make the 1.1 release and I think 1.2 is based on a further branch point, but we'll almost certainly have at least one 1.1 update worth of fixes (1.1.1?) before 1.2
tracking-b2g18: --- → ?
Flags: needinfo?(dveditz)
Comment on attachment 796399 [details] [diff] [review]
Fix all the workers code using propertyops

sec-approval+
Attachment #796399 - Flags: sec-approval? → sec-approval+
Assignee

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Hey Waldo, any reason to wait till later in Beta 25 to uplift?
Flags: needinfo?(jwalden+bmo)
Assignee

Comment 42

6 years ago
I think we can land this early without issues.
Flags: needinfo?(jwalden+bmo)
Duplicate of this bug: 831090
Confirmed crash on FF26, 2013-08-25.
Verified fix on FF26, 2013-09-22.
Assignee

Comment 45

6 years ago
[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: unclear, hard to say how far the issue here can be levered; sec-critical at worst
Testing completed (on m-c, etc.): landed on m-c, uplifted to aurora
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #798921 - Attachment is obsolete: true
Attachment #798923 - Attachment is obsolete: true
Attachment #798924 - Attachment is obsolete: true
Attachment #798926 - Attachment is obsolete: true
Attachment #814401 - Flags: review+
Attachment #814401 - Flags: approval-mozilla-beta?
Assignee

Comment 46

6 years ago
[Approval Request Comment]
Bug caused by (feature/regressing bug #): probably dates to initial DOM workers code landing or so
User impact if declined: unclear, hard to say how far the issue here can be levered; sec-critical at worst
Testing completed (on m-c, etc.): landed on m-c, uplifted to aurora
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

A note: tryservering these patches doesn't work -- I think because try-infrastructure isn't geared to build as-old-as-beta code.  :-\

https://tbpl.mozilla.org/?tree=Try&rev=20cbf831f091

I'm doing a local build now as the second-best double-check.
Attachment #814404 - Flags: review+
Attachment #814404 - Flags: approval-mozilla-beta?
Comment on attachment 814401 [details] [diff] [review]
Bug 908898 patch, for current beta

Low risk sec-crit fix. Approving for Beta 25.
Attachment #814401 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #814404 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Yes, we need this in ESR24 and ESR17, as well as B2G.
Jeff is working on the backporting patches now.
Assignee

Comment 51

6 years ago
esr17-targeted: https://tbpl.mozilla.org/?tree=Try&rev=cfa8f5c81988
b2g18-targeted: https://tbpl.mozilla.org/?tree=Try&rev=c8669b9ad988
esr24-targeted: https://tbpl.mozilla.org/?tree=Try&rev=4fa0796171be

Odds on those actually working on modern try infrastructure, anyone?  ;-)  (Last I suspect works, the other two seem doubtful to me.)
Yeah, it looks like only esr24 even sort of worked.  The unfortunate reality of landing on old branches is that you have to use the branch itself as try server...
Whiteboard: [dupe 831090?] → [dupe 831090?][adv-main25+]
Assignee

Comment 53

6 years ago
This builds locally and passes dom/workers mochitests.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: sec-critical
Fix Landed on Version: in everything bug esr{17,24} and b2g18 now, incl. aurora/beta
Risk to taking this patch (and alternatives if risky): patch size not ideal, but safer than trying to change JSPropertyOp calling code, I think
String or UUID changes made by this patch: none
Attachment #818902 - Flags: review+
Attachment #818902 - Flags: approval-mozilla-esr17?
Assignee

Comment 54

6 years ago
This patch passed try, so one better than esr17.  :-)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: sec-critical
Fix Landed on Version: everywhere except esr{17,24} and b2g18 now
Risk to taking this patch (and alternatives if risky): see above
String or UUID changes made by this patch: none
Attachment #818904 - Flags: review+
Attachment #818904 - Flags: approval-mozilla-esr24?
Attachment #818902 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
Keywords: verifyme
QA Contact: mwobensmith
Assignee

Comment 55

6 years ago
Still building/running mochitests, but worth getting out there now for approval pending those results.  I'm building the b2g18 tree, but honestly at this point I don't know if I'm supposed to be working with that tree or some v1.blah.blah tree -- clarification of where this should land there would be helpful.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): there forever
User impact if declined: sec-critical
Testing completed: will run dom/workers mochitests locally before landing
Risk to taking this patch (and alternatives if risky): big but relatively straightforward, better than the alternative of changing fundamental property-access code in SpiderMonkey
String or UUID changes made by this patch: none

Oh, to be clear: I'm only posting one patch each for approvals, but I'll be landing backports of bug 908898 (which are relatively trivial to construct) as well as these patches (which require those backports).
Attachment #818908 - Flags: review+
Attachment #818908 - Flags: approval-mozilla-b2g18?
Assignee

Comment 57

6 years ago
The b2g18 patches are golden on dom/workers mochitests.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #55)
> Still building/running mochitests, but worth getting out there now for
> approval pending those results.  I'm building the b2g18 tree, but honestly
> at this point I don't know if I'm supposed to be working with that tree or
> some v1.blah.blah tree -- clarification of where this should land there
> would be helpful.

b2g18 is the correct (and only) tree for this to be landing on. Thanks for asking.
Assignee

Comment 59

6 years ago
b2g18, as RyanVM tells me there's a blanket approval to land there given the tracking flags being set to 25+:

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8ab36ae7129b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e4cb5a852e3d

Will keep an eye on it to make sure it greenifies correctly.
Whiteboard: [dupe 831090?][adv-main25+] → [dupe 831090?][adv-main25+][adv-esr1710+]
Attachment #818904 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Comment on attachment 818908 [details] [diff] [review]
b2g18-targeted patch

already uplifted ! had spoken offline with RyanVM to uplift any b2g18 affected.
Attachment #818908 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Whiteboard: [dupe 831090?][adv-main25+][adv-esr1710+] → [dupe 831090?][adv-main25+][adv-esr1710+][adv-esr24-1+]
Verified fixed in 24esr, 25, ASan builds from 2013-10-21.
Alias: CVE-2013-5602
Group: core-security
You need to log in before you can comment on or make changes to this bug.