Closed Bug 1050340 (CVE-2014-8632) Opened 6 years ago Closed 6 years ago

Structured clone should not unwrap security wrappers during traversal

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox-esr31 --- wontfix

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main34+])

Attachments

(11 files)

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
gkrizsanits
: review+
Details | Diff | Splinter Review
1.32 KB, patch
adw
: review+
Details | Diff | Splinter Review
9.00 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
3.57 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.31 KB, patch
gkrizsanits
: 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-othersec-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
Attachment #8473941 - Flags: review?(luke)
Attachment #8473944 - Flags: review?(luke)
Attachment #8473945 - Flags: review?(luke)
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)
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).
Attachment #8473950 - Flags: review?(gkrizsanits)
Attachment #8473948 - Flags: review?(gkrizsanits)
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+
Attachment #8473940 - Flags: review?(luke) → review+
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+
Attachment #8473942 - Flags: review?(luke) → review+
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?
Attachment #8473944 - Flags: review?(luke) → review+
Attachment #8473945 - Flags: review?(luke) → review+
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 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+
Flags: qe-verify-
Flags: in-testsuite+
Whiteboard: [adv-main34+]
Alias: CVE-2014-8632
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.