Closed Bug 1082450 Opened 5 years ago Closed 5 years ago

Disallow exposing privileged functions via COWs

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: addon-compat)

Attachments

(9 files, 1 obsolete file)

4.62 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
2.79 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
2.54 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.45 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
7.82 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
6.88 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
2.39 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
5.69 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
1.37 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
When I went to land bug 1065186, I started getting nervous about the fact that the privileged Array.prototype will no longer be remapped to the content scope. This shouldn't be a problem in general, but it made me think that we could really tighten down a bit in that area.

I don't think we'll ever be able to drop support for invoking chrome callables from content, so I think we should focus on mechanisms of obtaining those callables in the first place. I've got some patches that I'll be pushing to try.
Attachment #8504957 - Flags: review?(gijskruitbosch+bugs)
Attachment #8504959 - Flags: review?(jmaher)
Attachment #8504959 - Flags: review?(gkrizsanits)
In the next patch, we deny access to any accessor property, so this is now
obsolete for COWs. We also do something like this for new-style XOWs, but that's
exhaustively covered in test_crossOriginObjects.html.
Attachment #8504960 - Flags: review?(gkrizsanits)
Attachment #8504965 - Flags: review?(gkrizsanits)
Comment on attachment 8504957 [details] [diff] [review]
Part 1 - Rip exposedProps out of Social API code. v1

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

This generally looks OK. Wants a try push to be sure, but you did that already.

::: toolkit/components/social/FrameWorker.jsm
@@ -153,5 @@
> -    onmessage: "rw",
> -    postMessage: "r",
> -    close: "r",
> -    toString: "r"
> -  },

Are you sure the socialapi use is the only use of the port here? I thought there were other consumers, but perhaps I'm wrong?

::: toolkit/components/social/FrameWorkerContent.js
@@ +124,5 @@
>        userAgent: workerWindow.navigator.userAgent,
> +    }, sandbox);
> +    Object.defineProperty(Cu.waiveXrays(navigator), 'onLine',
> +                          {configurable: true, enumerable: true,
> +                           get: Cu.exportFunction(() => workerWindow.navigator.onLine, sandbox)});

Nit: maybe use a (..., {
  foo: bar
  baz: quux
});

notation here in order to make it wrap a little nicer? :-)

::: toolkit/components/social/MozSocialAPI.jsm
@@ +122,5 @@
> +        }, targetWindow, {cloneFunctions: true});
> +
> +        // Jump through hoops to define the accessor property.
> +        let abstractPortPrototype = Object.getPrototypeOf(Object.getPrototypeOf(port));
> +        let desc = Object.getOwnPropertyDescriptor(port.__proto__.__proto__, 'onmessage');

Ew.
Attachment #8504957 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8504958 [details] [diff] [review]
Part 2 - Use automation machinery instead of exposedProps in Mock{Color,File}Picker.jsm. v1

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

Could you also remove the __exposedProps__ part from MockColorPicker?
Attachment #8504958 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8504959 [details] [diff] [review]
Part 3 - Remove exposedProps from reftest harness. v1

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

I assume the read-only part of these properties are not very important anyway.
Attachment #8504959 - Flags: review?(gkrizsanits) → review+
Attachment #8504960 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8504961 [details] [diff] [review]
Part 5 - Correctly propagate exceptions from ExposedPropertiesOnly::check. v1

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

::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
@@ +286,5 @@
>          return true;
>      // COWs fail silently for GETs, and that also happens to be the only case
>      // where we might want to redirect the lookup to the home prototype chain.
> +    *bp = (act == Wrapper::GET || act == Wrapper::ENUMERATE ||
> +           act == Wrapper::GET_PROPERTY_DESCRIPTOR) && !JS_IsExceptionPending(cx);

So how can we get here exactly with exception already pending?
Comment on attachment 8504962 [details] [diff] [review]
Part 6 - Deny access to accessor properties on COWs. v1

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

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +343,5 @@
>          return false;
>      }
>  
> +    // Inspect the property on the underlying object to check for red flags.
> +    if (!JS_GetPropertyDescriptorById(cx, wrappedObject, id, &desc))

This probably not the most interesting case, but shouldn't we check also if the underlying object is a js proxy here that could interrupt this call and fool us?
Attachment #8504962 - Flags: review?(gkrizsanits) → review+
Attachment #8504964 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8504959 [details] [diff] [review]
Part 3 - Remove exposedProps from reftest harness. v1

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

::: layout/tools/reftest/reftest.js
@@ +600,5 @@
>      var sandbox = new Components.utils.Sandbox(aURL.spec);
>      var xr = CC[NS_XREAPPINFO_CONTRACTID].getService(CI.nsIXULRuntime);
>      var appInfo = CC[NS_XREAPPINFO_CONTRACTID].getService(CI.nsIXULAppInfo);
>      sandbox.isDebugBuild = gDebug.isDebugBuild;
> +    sandbox.xulRuntime = CU.cloneInto({widgetToolkit: xr.widgetToolkit, OS: xr.OS}, sandbox);

where do we get XPCOMABI from then?
Attachment #8504959 - Flags: review?(jmaher) → review-
Attachment #8504965 - Flags: review?(gkrizsanits) → review+
(In reply to Joel Maher (:jmaher) from comment #17)
> where do we get XPCOMABI from then?

xulRuntime property will be an object from the sandbox itself because of the cloneInto, hence there will be no restriction from the sandbox to access its properties. So when few lines later XPCOMABI is set on it, that should be accessible from the sandbox without any further __exposedProps__ magic.
Comment on attachment 8504961 [details] [diff] [review]
Part 5 - Correctly propagate exceptions from ExposedPropertiesOnly::check. v1

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

r=me since it's correct, just want to understand the case where we actually need this...
Attachment #8504961 - Flags: review?(gkrizsanits) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> Comment on attachment 8504961 [details] [diff] [review]
> Part 5 - Correctly propagate exceptions from ExposedPropertiesOnly::check. v1
> 
> Review of attachment 8504961 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
> @@ +286,5 @@
> >          return true;
> >      // COWs fail silently for GETs, and that also happens to be the only case
> >      // where we might want to redirect the lookup to the home prototype chain.
> > +    *bp = (act == Wrapper::GET || act == Wrapper::ENUMERATE ||
> > +           act == Wrapper::GET_PROPERTY_DESCRIPTOR) && !JS_IsExceptionPending(cx);
> 
> So how can we get here exactly with exception already pending?

The exceptions thrown in the Part 6 and Part 7 hit this, because of the way that ChromeObjectWrapper calls enter() multiple times in funny ways. This will all be obsolete very soon (hopefully next week) when we rm ChromeObjectWrapper.cpp in bug 1081985

.(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16)
> This probably not the most interesting case, but shouldn't we check also if
> the underlying object is a js proxy here that could interrupt this call and
> fool us?

We never generate COWs for proxies (at least since bug 1064437 - see ForceCOWBehavior).
(In reply to :Gijs Kruitbosch from comment #12)
> Are you sure the socialapi use is the only use of the port here? I thought
> there were other consumers, but perhaps I'm wrong?

I don't know this code really. CI is green, which is all I can say.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #18)
> (In reply to Joel Maher (:jmaher) from comment #17)
> > where do we get XPCOMABI from then?
> 
> xulRuntime property will be an object from the sandbox itself because of the
> cloneInto, hence there will be no restriction from the sandbox to access its
> properties. So when few lines later XPCOMABI is set on it, that should be
> accessible from the sandbox without any further __exposedProps__ magic.

What gabor said. I'll upload an additional patch that's a bit clearer though.
Attachment #8504959 - Attachment is obsolete: true
Attachment #8505426 - Flags: review?(jmaher)
Comment on attachment 8505426 [details] [diff] [review]
Part 3 - Remove exposedProps from reftest harness. v2 r=gabor

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

thanks
Attachment #8505426 - Flags: review?(jmaher) → review+
Need this to get valgrind builds green.
Attachment #8505432 - Flags: review?(gkrizsanits)
Attachment #8505432 - Flags: review?(gkrizsanits) → review+
This patch appears to break the add-on https://addons.mozilla.org/en-US/firefox/addon/self-destructing-cookies/versions/0.4.7-pre2.

GOOD
----
20141015060116
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9d6c298b5a 

BAD
---
20141015060615
https://hg.mozilla.org/integration/mozilla-inbound/rev/204a676d3c8e  


Mozilla/5.0 (Windows NT 6.3; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Depends on: 1083571
Can this be backed out since this is breaking lots of jetpack and gaia integration tests?
Flags: needinfo?(bobbyholley)
Requesting for backout.
Keywords: checkin-needed
> Can this be backed out since this is breaking lots of jetpack and gaia integration tests?

It would be _really_ helpful to actually point to the relevant tests.  I could have helped hunt this down earlier today if you'd done that.

Backing stuff out for breaking secret tests is ... I don't know.  What is the developer supposed to do after that, exactly?
(In reply to Boris Zbarsky [:bz] from comment #32)
> > Can this be backed out since this is breaking lots of jetpack and gaia integration tests?
> 
> It would be _really_ helpful to actually point to the relevant tests.  I
> could have helped hunt this down earlier today if you'd done that.
> 
> Backing stuff out for breaking secret tests is ... I don't know.  What is
> the developer supposed to do after that, exactly?

Sorry, this is being tracked in bug 1083571 and has more info. I agree that it sucks that this suite is hidden, and we're working hard on getting this unhidden.
Given that the gaia stuff is a test-only issue and no actual device functionality was broken, I don't think this warrants a backout. I'm helping folks fix the tests today.
Keywords: checkin-needed
(In reply to Kevin Grandon :kgrandon from comment #30)
> Can this be backed out since this is breaking lots of jetpack and gaia
> integration tests?

I am in touch with timdream and the gaia folks, and am helping them fix the tests. I will look at the jetpack things next.
Flags: needinfo?(bobbyholley)
This also broke the tests for j2me.js, per https://github.com/andreasgal/j2me.js/issues/464.
Depends on: 1084245
Keywords: addon-compat
Depends on: 1097928
You need to log in before you can comment on or make changes to this bug.