747434 - way to blacklist / shadow Components from sandbox

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Currently we discourage use of `Components` in SDK at link time in favor of `require('chrome')` unfortunately this does not really prevents one to gain access to it by simple tricks like `let C = this['Compo' + 'nents']` and alike. Also there is no way to shadow it in the sandbox as it's defined as non-configurable and non-writable own property of a sandbox.

We need some changes on the platform to make it possible to make `Components` undefined for specific sandboxes.

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Gabor, you have landed several changes for sandboxes, do you have an idea how easy if possible would it be to shadow `Components` for sandboxes with system principal ?

Comment 2

[Gabor Krizsanits (INACTIVE)]

You want to permanently get rid of it in some sandboxes right? So the simplest solution probably if we add another flag for sandbox, like wantComponents or something similar, and if it's false we simply don't add the Components to the sandbox. Since there are quite some functions I would have to carry that flag internally, I might want to try remove it after it has been added, but this is just implementation detail. You will have to be careful though, since if it is a chrome sandbox, once it gets a hold of a Components object of another sandbox it will be capable of using it (unlike a none chrome sandbox). So the part I don't get is why is it a chrome sandbox if you don't want to let it to use Components on the first place?

Comment 3

[Alexandre Poirot [:ochameau]]

This bug looks like a duplicate of bug 636145.

I agree with gabor, it just fix one case of security leak.
The best would be to give non-system principal to such sandbox, 
but as we have seen in bug 636145, it isn't easy because of wrapper issues.

Having said that it would worth it to fix this so visible security leak,
if we keep in mind that it is still far from being completely secured.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I realize it won't be perfect and capabilities may be leaked form more privileged sandboxes. Also, as far as I understand, making non-system principals capable of interacting with ones that have system principals is not possible and is very hard to make possible.

So having a way to specify weather Components (via `wantComponents` as you suggested) are there would be an improvement as linker & reviewers will be able to track cases when access to `Components` was shared.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

>
> So the part I don't get is why is it a chrome
> sandbox if you don't want to let it to use Components on the first place?
>

To be more precise, when we used sandboxes with non-system principals, they were not capable of consuming anything from system sandboxes. Which meant that if one module was implementing a wrapper over XPCOMs, when exports of that module were touched by
modules with non-system principals exceptions were thrown.

Comment 6

[Gabor Krizsanits (INACTIVE)]

Sure, I remember the issues we had just wanted to be sure. Alright, I will look into this soon.

Comment 7

[Gabor Krizsanits (INACTIVE)]

Attachment: 1st try

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 620646 [details] [diff] [review]
1st try

Underneath this patch, please add another patch that removes DEBUG_CheckForComponentsInScope, otherwise we'll assert everywhere. The DEBUG_CheckForComponentsInScope stuff is no longer necessary now that we've killed ClearScope.

Looking at this stuff, it appears that InitClasses only does 3 things:
1 - Calling RemoveWrappedNativeProtos()
2 - Attaching the Components object
3 - Attaching the XPCNativeWrapper constructor

We don't care about #1 for sandboxes, and we want #2 to be optional. So I think we should actually remove the dependency of Sandbox on InitClasses altogether, and just manually do #2 (depending on the options object) and #3.

This is also good because we're going to rip out InitClasses as soon as JSD goes away. So it's a good opportunity to future-proof sandboxes, and should make for a smaller patch all in all.

Comment 9

[Gabor Krizsanits (INACTIVE)]

Attachment: part 1 : remove DEBUG_CheckForComponentsInScope

Comment 10

[Gabor Krizsanits (INACTIVE)]

Attachment: part 2: optional Components object in sandbox

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 623630 [details] [diff] [review]
part 2: optional Components object in sandbox


> 
>-        rv = xpc->InitClasses(cx, sandbox);
>-        if (NS_SUCCEEDED(rv) &&
>-            !JS_DefineFunctions(cx, sandbox, SandboxFunctions)) {
>-            rv = NS_ERROR_FAILURE;
>+        XPCCallContext ccx(NATIVE_CALLER, cx);
>+        if (!ccx.IsValid())
>+            return NS_ERROR_XPC_UNEXPECTED;

I don't see any reason why we need the ccx. I know it was in InitClases, but I don't think it should be necessary here. Let's remove it and see if anything breaks?

>+        
>+        {
>+          JSAutoEnterCompartment ac;
>+          if (!ac.enter(ccx, sandbox))
>+              return NS_ERROR_XPC_UNEXPECTED;
>+          XPCWrappedNativeScope* scope =
>+              XPCWrappedNativeScope::GetNewOrUsed(ccx, sandbox);
>+
>+          if (!scope)
>+              return NS_ERROR_XPC_UNEXPECTED;
>+
>+          if (wantComponents && !nsXPCComponents::AttachComponentsObject(ccx, scope, sandbox))
>+              return NS_ERROR_XPC_UNEXPECTED;
>+
>+          if (XPCPerThreadData::IsMainThread(ccx)) {
>+              if (!XPCNativeWrapper::AttachNewConstructorObject(ccx, sandbox))
>+                  return NS_ERROR_XPC_UNEXPECTED;
>+          }

XPConnect is single-threaded, so we're always on the main thread. As such, we can remove the conditional here.

> xpc_CreateSandboxObject(JSContext * cx, jsval * vp, nsISupports *prinOrSop,
>-                        JSObject *proto, bool preferXray, const nsACString &sandboxName);
>-
>+                        JSObject *proto, bool preferXray, 
>+                        const nsACString &sandboxName, 
>+                        bool wantComponents = true);

We only call this function in two places AFAICT, so I don't think the default arg is justified. I think the two boolean parameters should go next to each other, and sandboxName should come last. I know you're changing this over in bug 734891, but we might as well keep things tidy in the mean time.

r=bholley with those 3 changes.

Comment 12

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #11)

> I don't see any reason why we need the ccx. I know it was in InitClases, but
> I don't think it should be necessary here. Let's remove it and see if
> anything breaks?

Tried it already but AttachNewConstructorObject needs it, so I thought the best if I leave it as it is. Using the original cx for the other calls might work and would make it more clear why we need the ccx, I'll try it. 

> XPConnect is single-threaded, so we're always on the main thread. As such,
> we can remove the conditional here.

Ok, so I thought it might not be needed any longer but was not sure. So I can safely assume that when I see IsMainThread check in XPConnect I can just get rid of it?

> We only call this function in two places AFAICT, so I don't think the
> default arg is justified. I think the two boolean parameters should go next
> to each other, and sandboxName should come last. I know you're changing this
> over in bug 734891, but we might as well keep things tidy in the mean time.

Yeah I prefer to have the booleans next to each other too.

Comment 13

[Gabor Krizsanits (INACTIVE)]

Ah, I see, you mean AttachNewConstructorObject should not need it either.

Comment 14

[Gabor Krizsanits (INACTIVE)]

There is a bigger problem: XPCWrappedNativeScope::GetNewOrUsed needs it too, which is passing it to XPCWrappedNativeScope constructor. Shall I just leave it as it was then? Or I can change probably GetNewOrUsed too.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> There is a bigger problem: XPCWrappedNativeScope::GetNewOrUsed needs it too,
> which is passing it to XPCWrappedNativeScope constructor. Shall I just leave
> it as it was then? Or I can change probably GetNewOrUsed too.

Yeah, I don't think any of that stuff needs it. Can you make another patch that sits underneath this one that does ccx->cx for DefineConstructor, XPCWNScope::GetNewOrUsed, XPCWNScope::SetGlobal, and XPCWNScope::XPCWNScope? I _think_ that should be all of them. I'd also be happy to write the patch, but then we'd need to wait for someone to review it. ;-)

Comment 16

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)

> Ok, so I thought it might not be needed any longer but was not sure. So I
> can safely assume that when I see IsMainThread check in XPConnect I can just
> get rid of it?

Yep! I plan on ripping them out sometime soon. :-)

Comment 17

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #15)
> Yeah, I don't think any of that stuff needs it. Can you make another patch
> that sits underneath this one that does ccx->cx for DefineConstructor,
> XPCWNScope::GetNewOrUsed, XPCWNScope::SetGlobal, and XPCWNScope::XPCWNScope?
> I _think_ that should be all of them. I'd also be happy to write the patch,
> but then we'd need to wait for someone to review it. ;-)

+ AttachComponentsObject which also adds XPCWrappedNative::GetNewOrUsed and XPCNativeInterface::GetNewOrUsed to the list (didn't check any deeper) how about doing all these changes/cleanups in a follow up bug instead?

Comment 18

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #17)
> (In reply to Bobby Holley (:bholley) from comment #15)
> > Yeah, I don't think any of that stuff needs it. Can you make another patch
> > that sits underneath this one that does ccx->cx for DefineConstructor,
> > XPCWNScope::GetNewOrUsed, XPCWNScope::SetGlobal, and XPCWNScope::XPCWNScope?
> > I _think_ that should be all of them. I'd also be happy to write the patch,
> > but then we'd need to wait for someone to review it. ;-)
> 
> + AttachComponentsObject which also adds XPCWrappedNative::GetNewOrUsed and
> XPCNativeInterface::GetNewOrUsed to the list (didn't check any deeper) how
> about doing all these changes/cleanups in a follow up bug instead?

Sure.

Comment 19

[Gabor Krizsanits (INACTIVE)]

Attachment: part 1 : remove DEBUG_CheckForComponentsInScope

Same as the previous version just added mercurial queue header to the patch for easier commit.

Comment 20

[Gabor Krizsanits (INACTIVE)]

Attachment: part 2: optional Components object in sandbox

Removed the main thread check and swapped the arguments to get the booleans next to each other. Getting rid of the ccx will come in a follow up.

Comment 21

[Gabor Krizsanits (INACTIVE)]

Follow-up: Bug 755637 

try: https://tbpl.mozilla.org/?tree=Try&rev=539b111d2b6a

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 624312 [details] [diff] [review]
part 2: optional Components object in sandbox

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3272,3 @@
>          }
> +
> +        if (NS_FAILED(JS_DefineFunctions(cx, sandbox, SandboxFunctions)))

Eh.

@@ +3436,5 @@
> +            return NS_ERROR_INVALID_ARG;
> +
> +        if (found) {
> +            if (!JS_GetProperty(cx, optionsObject, "wantComponents", &option) ||
> +                !JSVAL_IS_BOOLEAN(option)) {

isBoolean()?

@@ +3437,5 @@
> +
> +        if (found) {
> +            if (!JS_GetProperty(cx, optionsObject, "wantComponents", &option) ||
> +                !JSVAL_IS_BOOLEAN(option)) {
> +                return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval);

I don't think you want to throw if JS_GetProperty fails... Can you test

{ get wantComponents() { throw "foo" } }

? I hope that trips an assert somewhere...

::: js/xpconnect/src/xpcprivate.h
@@ +4464,5 @@
>  // reachable through prinOrSop, a new null principal will be created
>  // and used.
>  nsresult
>  xpc_CreateSandboxObject(JSContext * cx, jsval * vp, nsISupports *prinOrSop,
> +                        JSObject *proto, bool preferXray, bool wantComponents,

Booleans :/

Comment 23

[Gabor Krizsanits (INACTIVE)]

(In reply to Ms2ger from comment #22)
> > +        if (NS_FAILED(JS_DefineFunctions(cx, sandbox, SandboxFunctions)))
> 
> Eh.

Thanks!

> I don't think you want to throw if JS_GetProperty fails... Can you test
> 
> { get wantComponents() { throw "foo" } }
> 
> ? I hope that trips an assert somewhere...

I will. We do that for every other property... note that there is a has property check prior to this check. But your example is tricky enough to pass that I guess. I'm not sure what should be the expected result here so I'll just test it tomorrow. And file a new version of the patch.

> 
> ::: js/xpconnect/src/xpcprivate.h
> @@ +4464,5 @@
> >  // reachable through prinOrSop, a new null principal will be created
> >  // and used.
> >  nsresult
> >  xpc_CreateSandboxObject(JSContext * cx, jsval * vp, nsISupports *prinOrSop,
> > +                        JSObject *proto, bool preferXray, bool wantComponents,
> 
> Booleans :/

Yeah, yeah... I hate them too there... but I'm refactoring this part in another patch I'm working on right now, so don't worry they won't add up this way in the future. (I'm pretty sure there will be more and more options for sandboxes)

Comment 24

[Gabor Krizsanits (INACTIVE)]

Attachment: part 2: optional Components object in sandbox

> I don't think you want to throw if JS_GetProperty fails... Can you test
So your example thorws foo, I don't quite see the problem with that... can you explain me your concern?

> isBoolean()?
Since I should refactor this whole function anyway to keep consistency (which I did already and have a patch for it) I will leave it for this patch (but changed my refactoring patch to use isSomething/toSomthing everywhere in this area and will r? you for that patch soon)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8be866038036
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d4d38cd4ad

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8be866038036
https://hg.mozilla.org/mozilla-central/rev/f5d4d38cd4ad

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part 3: Remove unused variable

Comment 28

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 626531 [details] [diff] [review]
Part 3: Remove unused variable

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

Thanks, nice catch!

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yay, build warnings!

https://hg.mozilla.org/mozilla-central/rev/561bfacbefee

Comment 30

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I'm trying to enable this on SDK side now but see some exceptions:

[Exception... "Unexpected error in XPConnect"  nsresult: "0x80570008 (NS_ERROR_XPC_UNEXPECTED)"  location: "JS frame :: resource://5f65773f-993c-4a5b-be79-f832932100f7-at-jetpack/api-utils/lib/loader.js -> resource://5f65773f-993c-4a5b-be79-f832932100f7-at-jetpack/api-utils/lib/self.js :: <TOP_LEVEL> :: line 13"  data: no]

I will try to investigate and narrow down a test case, it also could be some error on SDK side.

Comment 31

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

It looks like problem is in `Components.Constructor` function that we expose as
`{ CC: Constructor }`. While I was able to fix that via
`{ Cc: Function.bind.call(Components.Constructor, Components) }`
but there are some more issues. probably with other `Component` functions. I'll see if I can fix those too.

Comment 32

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Ok now it looks like `CC` works but only if you pass it two arguments if you pass third it seems to produce same error:

[Exception... "Unexpected error in XPConnect"  nsresult: "0x80570008 (NS_ERROR_XPC_UNEXPECTED)"  location: "JS frame :: resource://35f99f31-fa3c-4a4e-9d34-4839ad45cdd7-at-jetpack/api-utils/lib/loader.js -> resource://35f99f31-fa3c-4a4e-9d34-4839ad45cdd7-at-jetpack/api-utils/lib/passwords/utils.js :: <TOP_LEVEL> :: line 14"  data: no]

Comment 33

[Gabor Krizsanits (INACTIVE)]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #31)
> It looks like problem is in `Components.Constructor` function that we expose
> as
> `{ CC: Constructor }`. While I was able to fix that via
> `{ Cc: Function.bind.call(Components.Constructor, Components) }`
> but there are some more issues. probably with other `Component` functions.
> I'll see if I can fix those too.

I really need a better description of what is happening, and what are you exactly trying to achieve to be able to help. Are you trying to export a Components object from a chrome sandbox where Components is enabled to a chrome sandbox where Components are disabled? If so... why would you do that exactly? Or is this just about remove dependencies of Components object at some places in the SDK and you don't need me for it?

Comment 34

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #33)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #31)
> > It looks like problem is in `Components.Constructor` function that we expose
> > as
> > `{ CC: Constructor }`. While I was able to fix that via
> > `{ Cc: Function.bind.call(Components.Constructor, Components) }`
> > but there are some more issues. probably with other `Component` functions.
> > I'll see if I can fix those too.
> 
> I really need a better description of what is happening, and what are you
> exactly trying to achieve to be able to help. Are you trying to export a
> Components object from a chrome sandbox where Components is enabled to a
> chrome sandbox where Components are disabled? If so... why would you do that
> exactly? Or is this just about remove dependencies of Components object at
> some places in the SDK and you don't need me for it?

Sorry I forgot to update, that I've figured out what the issue was and included in
a patch that enables this feature for SDK loader. As of your question why do we expose `Components` to a sandboxes that have `wantComponents: false` is because all
of the SDK sandboxes will have `wantComponents: false` and only way to get access
to components will be through `require('chrome')` call that we are able to track and visualize appropriately to a reviewers... Also this will prevent untracked runtime access to `Components` that was possible without this feature.

Comment 35

[Gabor Krizsanits (INACTIVE)]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #34)
> Sorry I forgot to update, that I've figured out what the issue was and
> included in

In that case I just "re-resolve" this bug.

> a patch that enables this feature for SDK loader. As of your question why do
> we expose `Components` to a sandboxes that have `wantComponents: false` is
> because all
> of the SDK sandboxes will have `wantComponents: false` and only way to get
> access
> to components will be through `require('chrome')` call that we are able to
> track and visualize appropriately to a reviewers... Also this will prevent
> untracked runtime access to `Components` that was possible without this
> feature.

Thanks for the explanation, it's clear now.

Comment 36

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/b167ac2bf0b777bcf5c13ca5735e92d3d8bb666c
Start using platform changes from Bug 747434 to omit `Components` global from module scopes.

Comment 37

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> 
> In that case I just "re-resolve" this bug.
>

Yeah I just wanted to keep it around until we would land code that enable it in SDK. Which is happened in the commit above ;)

Comment 38

[Will Bamberg [:wbamberg]]

This was documented a while back: https://developer.mozilla.org/en-US/docs/Components.utils.Sandbox$compare?locale=en-US&to=344763&from=341317.