Crash with structured-cloning and proxy wrapped Map/Sets

RESOLVED FIXED in Firefox 36, Firefox OS v1.4

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

(Blocks: 1 bug, {sec-high})

Trunk
mozilla38
sec-high
Points:
---

Firefox Tracking Flags

(firefox35 wontfix, firefox36+ verified, firefox37+ verified, firefox38+ verified, firefox-esr31 fixed, firefox-esr3836+, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main36+][adv-esr31.5+])

Attachments

(6 attachments, 4 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8536057 [details]
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.
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.
(Assignee)

Comment 2

3 years ago
Created attachment 8536762 [details] [diff] [review]
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.
Keywords: sec-high
(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)
(Assignee)

Comment 5

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8536762 - Flags: review?(efaustbmo)
(Assignee)

Updated

3 years ago
Assignee: nobody → evilpies
(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

3 years ago
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.
Attachment #8536762 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 8

3 years ago
Created attachment 8545918 [details] [diff] [review]
bug-1111243-tests

On Nightly we also need to patch these tests.
(Assignee)

Comment 9

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

[Security approval request comment]
See comment 5 and comment 6.
Attachment #8536762 - Flags: sec-approval?
(Assignee)

Comment 10

3 years ago
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.
status-firefox35: --- → wontfix
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox-esr31: --- → ?
tracking-firefox36: --- → +
tracking-firefox37: --- → +
Whiteboard: [checkin on 1/26]
Comment on attachment 8536762 [details] [diff] [review]
generic-directproxy

sec-approval+ for checkin on 1/26 or later.
Attachment #8536762 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 12

3 years ago
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
(Assignee)

Comment 13

3 years ago
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.
(Assignee)

Comment 14

3 years ago
Created attachment 8552479 [details] [diff] [review]
Fix now passing tests.
Attachment #8545918 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
Created attachment 8552480 [details] [diff] [review]
Implement IsArray
Attachment #8552480 - Flags: review?(efaustbmo)
(Assignee)

Comment 16

3 years ago
Created attachment 8552481 [details] [diff] [review]
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));
status-firefox38: --- → affected
tracking-firefox38: --- → +
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?
Attachment #8552480 - Flags: review?(efaustbmo) → review+
(Assignee)

Updated

3 years ago
Attachment #8552479 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
Created attachment 8554312 [details] [diff] [review]
Patch for Nightly
Attachment #8552480 - Attachment is obsolete: true
Attachment #8552481 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
Created attachment 8554314 [details] [diff] [review]
Patch for Beta
(Assignee)

Comment 21

3 years ago
Nightly patch applies to Aurora as well.
(Assignee)

Comment 22

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

Approval Request Comment
See comment 5 and comment 6.
Attachment #8554312 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 23

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

Approval Request Comment
See comment 5 and comment 6.
Attachment #8554314 - Flags: approval-mozilla-beta?
(Assignee)

Comment 24

3 years ago
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.
Flags: needinfo?(efaustbmo)
Attachment #8554312 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8554314 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/c1fb4bf7b043
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
https://hg.mozilla.org/releases/mozilla-aurora/rev/c4a9f04aa23f
https://hg.mozilla.org/releases/mozilla-beta/rev/bf8644a5c52a
status-b2g-v1.4: --- → ?
status-b2g-v2.0: --- → ?
status-b2g-v2.0M: --- → ?
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
status-firefox36: affected → fixed
status-firefox37: affected → fixed
Whiteboard: [checkin on 1/26]
(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?
Flags: needinfo?(efaustbmo) → needinfo?(evilpies)
(Assignee)

Comment 28

3 years ago
Not really, but we might break some stuff without the IsArray patch, like the WebIDE?
Flags: needinfo?(evilpies)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1115361
Looks like this has some non-trivial conflicts on b2g34 as well.
Flags: needinfo?(evilpies)
Keywords: branch-patch-needed
(Assignee)

Comment 32

3 years ago
Created attachment 8556020 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Attachment #8556020 - Flags: feedback?(efaustbmo)
(Assignee)

Comment 33

3 years ago
Created attachment 8556037 [details] [diff] [review]
Patch for b2g34
Flags: needinfo?(evilpies)
Comment on attachment 8556020 [details] [diff] [review]
Patch for ESR31

Looks fine to me.
Attachment #8556020 - Flags: feedback?(efaustbmo) → feedback+
(Assignee)

Comment 35

3 years ago
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.
Attachment #8556020 - Flags: approval-mozilla-esr31?
(Assignee)

Updated

3 years ago
Attachment #8556037 - Flags: approval-mozilla-b2g34?
Attachment #8556037 - Flags: approval-mozilla-b2g34?
status-firefox-esr31: ? → affected
tracking-firefox-esr38: --- → 36+
Attachment #8556020 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
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
status-b2g-v1.4: ? → fixed
status-b2g-v2.0: ? → fixed
status-b2g-v2.0M: ? → affected
status-b2g-v2.1: affected → fixed
status-firefox-esr31: affected → fixed
Keywords: branch-patch-needed
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1133249
(Assignee)

Updated

3 years ago
Blocks: 694100
Whiteboard: [adv-main36+][adv-esr31.5+]
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!
status-firefox36: fixed → verified
status-firefox37: fixed → verified
status-firefox38: fixed → verified

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.