If you do: proxy = Proxy(new Map(), {}), then for proxy ObjectClassIs(obj, ESClass_Map) is going to return true, but CheckedUnwrap(proxy) is not going to unwrap anything. Crash in JSStructuredCloneWriter::traverseMap -> MapObject::getKeysAndValuesInterleaved.

ObjectClassIs is just a major footgun, if random Proxy objects can return true for it... we have all sorts of code assuming ObjectClassIs sees only through cross-compartment wrappers.

The spec doesn't allow the kind of generic behavior that we have. (Special case: IsArray). There is also "className" and "fun_toString", but those seem a lot less dangerous.

(In reply to Vacation Dec 15-31 from comment #1) > ObjectClassIs is just a major footgun, if random Proxy objects can return > true for it... we have all sorts of code assuming ObjectClassIs sees only > through cross-compartment wrappers. Yes, this was always sketchy, and now straight-up untrue in a world with CPOWs. Somebody needs to audit all uses of ObjectClassIs, and make sure that the ensuing code operates on the target generically, rather than using CheckedUnwrap. CPOWs do this successfully, FWIW.

(Also note that ObjectClassIs is a great tool, and the bedrock of our CPOW story - we just need to use it correctly)

It's seem like at least this code for traverseMap/Set was introduced in bug 1050340, another security bug! However it's apparent that this is probably not the only flaw caused by this. So how shall we proceed? I tried to answer this criteria, but I am not really sure it's right. How easily can the security issue be deduced from the patch? I think it' not necessarily totally obvious, but when you actually look at ObjectClassIs uses, it's probably not hard to figure out that we don't handle scripted proxies correctly in all places. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? I haven't included anything like that yet. Which older supported branches are affected by this flaw? All? I think this exists for a longish time. At least this POC doesn't work on ESR 31, but I think even that version has objectClassIs. If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? How likely is this patch to cause regressions; how much testing does it need? Proxies are not yet commonly used.

(In reply to Tom Schuster [:evilpie] from comment #5) > How easily can the security issue be deduced from the patch? > I think it' not necessarily totally obvious, but when you actually look at > ObjectClassIs uses, it's probably not hard to figure out that we don't > handle scripted proxies correctly in all places. This sort of analysis is always a bit fuzzy, but that's about how I think I'd say it. I think the connection to code that would crash is fairly straightforward here, if not quite turnkey. > How likely is this patch to cause regressions; how much testing does it > need? > Proxies are not yet commonly used. That's part of the analysis, but not all. How commonly used a feature is, affects how scared to be about whether a mistake inadvertently breaks code. But it doesn't have any relevance to regressions that would themselves be security issues. A fifteen-step process that no code will ever stumble upon, that can nonetheless produce arbitrary code execution, is a problem regardless whether the relevant feature is "commonly used". Here, I'd say the fix is simple enough that regressions are unlikely. And if they happen, fuzzers will probably find them, more in-the-field testing is unlikely to find them, and the benefit of getting rid of possibly-exploitable code in a less-used feature well outweighs the chance of regressions.

Comment on attachment 8536762 [details] [diff] [review] generic-directproxy Review of attachment 8536762 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/proxy/ScriptedDirectProxyHandler.cpp @@ +1115,5 @@ > + return false; > +} > + > +bool > +ScriptedDirectProxyHandler::objectClassIs(HandleObject obj, ESClassValue classValue, maybe this function will have to be rewritten to accomodate the IsArray stuff, but this should do for now.

On Nightly we also need to patch these tests.

Comment on attachment 8536762 [details] [diff] [review] generic-directproxy [Security approval request comment] See comment 5 and comment 6.

Sadly this patch seems to cause some failures in the devtools tests. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a681e7f5eeed. ObservableObject seems to be using a Proxy to wrap an object, and then the tests try to compare JSON.stringify(proxy) == JSON.stringify(raw). This fails, because we don't treat the proxy like an array in JSON.stringify anymore. We would probably need bug 1111785 here.

Comment on attachment 8536762 [details] [diff] [review] generic-directproxy sec-approval+ for checkin on 1/26 or later.

Even with IsArray implemented we still get some test failures, probably because we try to structure clone a proxy wrapping an array. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c154ebe88de2

So most of these test failures was a problem with storing a proxy-wrapped array into indexdb. The last two failures are probably related to a change with in behavior with .concat, we don't treat the proxy as an array anymore. If we implemented IsConcatSpreadable from the spec this would actually be the case again. So I would argue to just use IsArray there as well in the meantime.

Not sure how to get this reviewed. But the method below "add", called "update" has this comment/code. // Clone object to make it storable by IndexedDB. // Projects are proxified objects (for the template // mechanismn in the first version of the App Manager). // This will change in the future. project = JSON.parse(JSON.stringify(project));

Comment on attachment 8552480 [details] [diff] [review] Implement IsArray Review of attachment 8552480 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. This expansion is necessary because of the failures, as discussed. ::: js/src/jsobjinlines.h @@ +710,5 @@ > > switch (classValue) { > case ESClass_Object: return obj->is<PlainObject>(); > + case ESClass_Array: > + case ESClass_IsArray: Can we add a comment here about how it allows proxies to differentiate, but makes no sense for native objects? ::: js/src/proxy/ScriptedDirectProxyHandler.cpp @@ +1121,5 @@ > { > + if (classValue != ESClass_IsArray) > + return false; > + > + // In ES6 IsArray pokes at the target, instead we do this here. We should have a comment here about why this is not in DirectProxyHandler, I think. We want to leave all of Wrapper (XPConnect) out of it, right? Xrays don't play this game, for example?

Nightly patch applies to Aurora as well.

Comment on attachment 8554312 [details] [diff] [review] Patch for Nightly Approval Request Comment See comment 5 and comment 6.

Comment on attachment 8554314 [details] [diff] [review] Patch for Beta Approval Request Comment See comment 5 and comment 6.

What shall we do about ESR31? This POC doesn't work, because we didn't use ObjectClassIs inside structured clone, but every other use of this function is potentially dangerous as well.

https://hg.mozilla.org/releases/mozilla-aurora/rev/c4a9f04aa23f https://hg.mozilla.org/releases/mozilla-beta/rev/bf8644a5c52a

(In reply to Tom Schuster [:evilpie] from comment #24) > What shall we do about ESR31? This POC doesn't work, because we didn't use > ObjectClassIs inside structured clone, but every other use of this function > is potentially dangerous as well. There wasn't anything wrong with landing the original patch, right? We just folded in the IsArray stuff because it was crashing without? If that's the case, can we just land the 4 proxy handler functions that will mitigate crashing uses?

Not really, but we might break some stuff without the IsArray patch, like the WebIDE?

Looks like this has some non-trivial conflicts on b2g34 as well.

I made this patch for ESR31, because I still had that branch locally. ESR34 is going to take a few more minutes.

Comment on attachment 8556020 [details] [diff] [review] Patch for ESR31 Looks fine to me.

Comment on attachment 8556020 [details] [diff] [review] Patch for ESR31 [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: Fix Landed on Version: Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch: See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/022205dd7713 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c8d31cc0da1c https://hg.mozilla.org/releases/mozilla-esr31/rev/67231f96efac https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/3a02f3a644d5

Reproduced the original issue using the following build: * http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-01-24-03-02-02-mozilla-central/ ** https://crash-stats.mozilla.com/report/index/db454df4-5d7a-4366-b899-63dfa2150219 Went through verification using the following builds: fx38 -> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-02-19-07-07-49-mozilla-central/ fx37 -> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-02-19-00-41-26-mozilla-aurora/ fx36 -> http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/36.0b10-candidates/build1/ OS's Used: - Win 8.1 x64 - PASSED - Ubuntu 14.04.1 x64 - PASSED Test Cases Used: * Opened the POC several times in normal windows/tabs * Opened the POC several times in private windows/tabs * Opened the POC several times in e10s windows/tabs (only applies to m-c) As per comment # 24, I couldn't test this on ESR31 because ObjectClassIs wasn't being used inside a structured clone. If there's a way or POC to test this on ESR, please needinfo and let me know!

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/022205dd7713 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/c8d31cc0da1c