If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.
Bug 1050340 (CVE-2014-8632)

Structured clone should not unwrap security wrappers during traversal

RESOLVED FIXED in Firefox 34

Status

()

Core
XPConnect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({sec-moderate})

unspecified
mozilla34
sec-moderate
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox-esr31 wontfix)

Details

(Whiteboard: [adv-main34+])

Attachments

(11 attachments)

1.57 KB, patch
luke
: review+
Details | Diff | Splinter Review
2.23 KB, patch
luke
: review+
Details | Diff | Splinter Review
8.80 KB, patch
luke
: review+
Details | Diff | Splinter Review
15.17 KB, patch
luke
: review+
Details | Diff | Splinter Review
6.48 KB, patch
luke
: review+
Details | Diff | Splinter Review
4.02 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.99 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
1.32 KB, patch
adw
: review+
Details | Diff | Splinter Review
9.00 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
3.57 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.31 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
I realized this as a potential issue when reviewing bug 1038716.

With the work I've done this summer in bug 914970 and bug 856067, we now have a much stronger security membrane to protect privileged code from getting confused by unprivileged non-DOM objects. In order for these wrappers to do their job though, we need to be careful about the places where we strip them off.

In general, CheckedUnwrap allows wrappers to be removed when the wrapper subsumes the wrappee. This is generally what we want, since it allows chrome to do document.getElementById.call(contentDocument, 'foo') and whatnot.

We also use CheckedUnwrap in the Structured Clone algorithm to handle wrappers encountered when traversing the object graph, which is what I'm concerned about here. When chrome manipulates a content Object or Array, the XrayWrappers filter out tricky stuff like getter properties, and properties that shadow the standard prototype.

So when Mike does his input validation in bug 1038716, the object may _seem_ correct. But when he then passes that object to IndexedDB, the serialization will strip off the security wrapper, and invoke getters for properties that didn't appear to exist from the Xray side. This stuff then ends up in the object store, and gets pulled out later. Yikes.

I think the simplest thing to do here is to perform a reverse-subsumes check in the structured clone code. We can give consumers the option to explicitly clone an unprivileged object with Cu.cloneInto (which is what Mike is going to need to do) by making xpc::CloneInto do its own CheckedUnwrap (sans subsumes check) on the starting object.

Doing this is important for defense-in-depth.
OS: Mac OS X → All
Hardware: x86 → All
Bobby, should this be sec-high, or is it not an actual vulnerability now and so it should be more like sec-other or something?
Flags: needinfo?(bobbyholley)
AFAICT the only serious security issue here would be bug 1038716, which hasn't landed yet. Let's block that on this, and call this sec-other.

I had an idea to do this in a much simpler way which wouldn't involve bending over backwards in JS and would be more permissive. Trying that out now.
Blocks: 1038716
Flags: needinfo?(bobbyholley)
Keywords: sec-other
I'm actually going to call this sec-moderate. I'll have it fixed soon anyway.
Keywords: sec-other → sec-moderate
https://tbpl.mozilla.org/?tree=Try&rev=09b10719b43d
Summary: Structured clone should refuse to unwrap non-same-origin wrappers → Structured clone should not unwrap security wrappers during traversal
https://tbpl.mozilla.org/?tree=Try&rev=1498d50ad2d0
https://tbpl.mozilla.org/?tree=Try&rev=65f2ffa260cd
Created attachment 8473940 [details] [diff] [review]
Part 1 - Access regexp guts generically. v1
Attachment #8473940 - Flags: review?(luke)
Created attachment 8473941 [details] [diff] [review]
Part 2 - Handle dates generically. v1
Attachment #8473941 - Flags: review?(luke)
Created attachment 8473942 [details] [diff] [review]
Part 3 - Handle ArrayBuffers and TypedArrays pseudo-generically. v1
Attachment #8473942 - Flags: review?(luke)
Created attachment 8473943 [details] [diff] [review]
Part 4 - Handle boxed values with a new proxy trap. v1
Attachment #8473943 - Flags: review?(luke)
Created attachment 8473944 [details] [diff] [review]
Part 5 - Handle maps and sets. v1
Attachment #8473944 - Flags: review?(luke)
Created attachment 8473945 [details] [diff] [review]
Part 6 - Handle object and array. v1
Attachment #8473945 - Flags: review?(luke)
Created attachment 8473946 [details] [diff] [review]
Part 7 - Do a CheckedUnwrap in xpc::IsReflector. v1

This matches the behavior of UnwrapReflectorToISupports, and makes all of the
structured clone callbacks scattered throughout Gecko compatible with wrappers
that have not already had CheckedUnwrap invoked on them.
Attachment #8473946 - Flags: review?(gkrizsanits)
Created attachment 8473947 [details] [diff] [review]
Part 8 - Fix up content search test framework. v1
Attachment #8473947 - Flags: review?(adw)
Created attachment 8473948 [details] [diff] [review]
Part 9 - Fix up cloning test. v1

This test currently assume that cloning a COW will fail. Once we always go over
security wrappers, this isn't true anymore, because COWs silently deny
disallowed property gets. So it makes sense to rewrite the test so that we can
see what actually makes it through the structured clones. Promises galore.

We also take out the buggy Blob/FileList stuff and replace it with ImageData,
which does a bonafide clone in NS_DOMWriteStructuredClone instead of the weird
pointer swapping we do in nsGlobalWindow.cpp (baku is in the process of cleaning
that up, but it's not done yet).
Created attachment 8473949 [details] [diff] [review]
Part 10 - Stop doing a CheckedUnwrap while crawling the object graph in Structured Clone. v1

\o/
Attachment #8473949 - Flags: review?(luke)
Created attachment 8473950 [details] [diff] [review]
Part 11 - Tests. v1
Attachment #8473950 - Flags: review?(gkrizsanits)
Attachment #8473948 - Flags: review?(gkrizsanits)

Comment 18

3 years ago
Comment on attachment 8473947 [details] [diff] [review]
Part 8 - Fix up content search test framework. v1

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

It would be cool if the sendAsyncMessage here and content.dispatchEvent in content.js could handle this transparently so their callers don't need to worry about it.
Attachment #8473947 - Flags: review?(adw) → review+
(In reply to Drew Willcoxon :adw from comment #18)
> It would be cool if the sendAsyncMessage here and content.dispatchEvent in
> content.js could handle this transparently so their callers don't need to
> worry about it.

The problem is that content.dispatchEvent needs to decide which compartment to create the object in. It creates it in the content global (which is generally the right thing), but from that point on the object is untrusted as far as chrome is concerned, so sendAsyncMessage should definitely not be waiving Xrays outside of test code.
Attachment #8473946 - Flags: review?(gkrizsanits) → review+
Attachment #8473948 - Flags: review?(gkrizsanits) → review+
Attachment #8473950 - Flags: review?(gkrizsanits) → review+

Updated

3 years ago
Attachment #8473940 - Flags: review?(luke) → review+

Comment 20

3 years ago
Comment on attachment 8473941 [details] [diff] [review]
Part 2 - Handle dates generically. v1

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

Is the reason for using CheckedUnwrap() in js_DateGetMsecSinceEpoch and not ObjectClassIs(obj, ESClass_Date, cx) just the lack of cx?
Attachment #8473941 - Flags: review?(luke) → review+

Updated

3 years ago
Attachment #8473942 - Flags: review?(luke) → review+

Comment 21

3 years ago
Comment on attachment 8473943 [details] [diff] [review]
Part 4 - Handle boxed values with a new proxy trap. v1

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

I'm reticent to add a virtual function to proxy that only makes sense for a subset of the derived classes.  Since we're talking about a finite set of cases here (string, number, boolean), can't we just explicitly CheckedUnwrap(obj)->unbox() them?

Updated

3 years ago
Attachment #8473944 - Flags: review?(luke) → review+

Updated

3 years ago
Attachment #8473945 - Flags: review?(luke) → review+

Updated

3 years ago
Attachment #8473949 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #20)
> Comment on attachment 8473941 [details] [diff] [review]
> Part 2 - Handle dates generically. v1
> 
> Review of attachment 8473941 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is the reason for using CheckedUnwrap() in js_DateGetMsecSinceEpoch and not
> ObjectClassIs(obj, ESClass_Date, cx) just the lack of cx?

We still need the CheckedUnwrap for the as<DateObject>() call below.

Comment 23

3 years ago
Comment on attachment 8473943 [details] [diff] [review]
Part 4 - Handle boxed values with a new proxy trap. v1

Ah, I see there is already some precedent for this approach (even set forth by me).  r+ assuming some sort of naming prefix analogous to regexp_toShared/fun_toString.  Perhaps boxedValue_unbox?
Attachment #8473943 - Flags: review?(luke) → review+
https://tbpl.mozilla.org/?tree=Try&rev=54e448c8c4fa
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=64db0d9db944
https://hg.mozilla.org/mozilla-central/rev/a3761879135a
https://hg.mozilla.org/mozilla-central/rev/a4014b7b418a
https://hg.mozilla.org/mozilla-central/rev/f752615f4e19
https://hg.mozilla.org/mozilla-central/rev/7d1e2bb43a8c
https://hg.mozilla.org/mozilla-central/rev/2491548a1dcc
https://hg.mozilla.org/mozilla-central/rev/12fd4b703210
https://hg.mozilla.org/mozilla-central/rev/33e142669649
https://hg.mozilla.org/mozilla-central/rev/8f7cecb87572
https://hg.mozilla.org/mozilla-central/rev/7774f27d0d31
https://hg.mozilla.org/mozilla-central/rev/320e1620d36d
https://hg.mozilla.org/mozilla-central/rev/64db0d9db944
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox34: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
Flags: in-testsuite+
Blocks: 1066701
status-firefox33: --- → affected
status-firefox-esr31: --- → wontfix
Whiteboard: [adv-main34+]
Alias: CVE-2014-8632
status-firefox33: affected → wontfix

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.