1111243 - Crash with structured-cloning and proxy wrapped Map/Sets

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

Attachment: POC

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Tom S [:evilpie]]

Attachment: generic-directproxy

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.

Comment 3

[Bobby Holley (:bholley)]

(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.

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Tom S [:evilpie]]

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.

Comment 6

[Jeff Walden [:Waldo]]

(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 7

[Eric Faust [:efaust]]

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.

Comment 8

[Tom S [:evilpie]]

Attachment: bug-1111243-tests

On Nightly we also need to patch these tests.

Comment 9

[Tom S [:evilpie]]

Comment on attachment 8536762 [details] [diff] [review]
generic-directproxy

[Security approval request comment]
See comment 5 and comment 6.

Comment 10

[Tom S [:evilpie]]

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 11

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

Comment on attachment 8536762 [details] [diff] [review]
generic-directproxy

sec-approval+ for checkin on 1/26 or later.

Comment 12

[Tom S [:evilpie]]

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

Comment 13

[Tom S [:evilpie]]

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.

Comment 14

[Tom S [:evilpie]]

Attachment: Fix now passing tests.

Comment 15

[Tom S [:evilpie]]

Attachment: Implement IsArray

Comment 16

[Tom S [:evilpie]]

Attachment: fix-app-projects

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 17

[Eric Faust [:efaust]]

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?

Comment 18

[Tom S [:evilpie]]

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

Comment 19

[Tom S [:evilpie]]

Attachment: Patch for Nightly

Comment 20

[Tom S [:evilpie]]

Attachment: Patch for Beta

Comment 21

[Tom S [:evilpie]]

Nightly patch applies to Aurora as well.

Comment 22

[Tom S [:evilpie]]

Comment on attachment 8554312 [details] [diff] [review]
Patch for Nightly

Approval Request Comment
See comment 5 and comment 6.

Comment 23

[Tom S [:evilpie]]

Comment on attachment 8554314 [details] [diff] [review]
Patch for Beta

Approval Request Comment
See comment 5 and comment 6.

Comment 24

[Tom S [:evilpie]]

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.

Comment 25

[Carsten Book [:Tomcat]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Eric Faust [:efaust]]

(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?

Comment 28

[Tom S [:evilpie]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c4a9f04aa23f

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Tom S [:evilpie]]

Attachment: Patch for ESR31

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

Comment 33

[Tom S [:evilpie]]

Attachment: Patch for b2g34

Comment 34

[Eric Faust [:efaust]]

Comment on attachment 8556020 [details] [diff] [review]
Patch for ESR31

Looks fine to me.

Comment 35

[Tom S [:evilpie]]

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.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[Kamil Jozwiak [:kjozwiak]]

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!

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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