1050340 - (CVE-2014-8632) Structured clone should not unwrap security wrappers during traversal

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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?

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Bobby Holley (:bholley)]

I'm actually going to call this sec-moderate. I'll have it fixed soon anyway.

Comment 4

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=09b10719b43d

Comment 5

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=1498d50ad2d0

Comment 6

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=65f2ffa260cd

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 1 - Access regexp guts generically. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 2 - Handle dates generically. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 3 - Handle ArrayBuffers and TypedArrays pseudo-generically. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 4 - Handle boxed values with a new proxy trap. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 5 - Handle maps and sets. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 6 - Handle object and array. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 8 - Fix up content search test framework. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: 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).

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 10 - Stop doing a CheckedUnwrap while crawling the object graph in Structured Clone. v1

\o/

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 11 - Tests. v1

Comment 18

[Drew Willcoxon :adw]

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.

Comment 19

[Bobby Holley (:bholley)]

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

Comment 20

[Luke Wagner [:luke]]

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?

Comment 21

[Luke Wagner [:luke]]

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?

Comment 22

[Bobby Holley (:bholley)]

(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

[Luke Wagner [:luke]]

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?

Comment 24

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=54e448c8c4fa

Comment 25

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=64db0d9db944

Comment 26

[Ed Morley [:emorley]]

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