856067 - Unwaived wrappers to unprivileged JS should not be transparent

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

The basic idea here is to extend the approach in bug 843829 to all scopes, rather than just XBL scopes. If an object isn't Xrayable, we currently default transparent, but that can end up being a security problem in bugs like bug 856042.

Bug 843829 will be a good testing ground to see how feasible this is.

Ideally, I think the best approach here would be to unify the semantics of Xray and non-Xray objects by implementing some kind of BlankXrayTraits that just makes pure JS objects appear as vanilla objects. This would allow callers to place expandos on them, among other things.

Comment 1

[Andrew McCreight [:mccr8]]

Marking sec-other because it looks like defense-in-depth and not a known security problem per se.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Marking sec-other because it looks like defense-in-depth and not a known
> security problem per se.

Well, it's the root cause of bug 856042, which we can only really wallpaper for now. Shrug.

Comment 3

[Andrew McCreight [:mccr8]]

Ah, okay.  Well, rate this and that other bug whatever you think is appropriate. :)

Comment 4

[Bobby Holley (:bholley)]

Well, I guess sec-other still probably makes the most sense here, with bug 856042 tracking the issue we know about currently.

Comment 5

[Bobby Holley (:bholley)]

Here's a patch that experiments with making unwaived content objects opaque modulo some special cases. This fails some XPConnect tests, and I'm not super optimistic that it's going to be tractable compat-wise. But we might as well see how try looks before pursuing more complicated solutions.

https://tbpl.mozilla.org/?tree=Try&rev=7ce373e4f000

Comment 6

[Bobby Holley (:bholley)]

(Note to self: 'more complicated solutions' include de-clawing getters and keeping a map of transitively-whitelisted wrappers)

Comment 7

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=592012c192fe

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Was the try-push successful?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Was the try-push successful?

The solutions in comment 6 are necessary. I did a fair amount of engineering on this last month, but haven't had time to work on it since then. I'll get back to it in the coming weeks.

Comment 10

[Andrew McCreight [:mccr8]]

Probably nobody else is going to work on this before you have a chance to get to it, so I'll just assign it to you for the time being to reflect the situation on the ground. :)

Comment 11

[Bobby Holley (:bholley)]

Getting back to this.

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

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

[Bobby Holley (:bholley)]

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

Comment 14

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=742d7a30feef

Comment 15

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=1294dabf2cfa

Comment 16

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #15)
> https://tbpl.mozilla.org/?tree=Try&rev=1294dabf2cfa

This is green, which is an incredible milestone for slaughterhouse. Still need to sort out some of the dependencies, but we're closing in on it.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #16)
> This is green, which is an incredible milestone for slaughterhouse. Still
> need to sort out some of the dependencies, but we're closing in on it.

Correction - there was one devtools orange, which I've now fixed. Let's try an all-platform push:

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

Comment 18

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #17)
> Correction - there was one devtools orange, which I've now fixed. Let's try
> an all-platform push:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=de358a189684

_This_ one was all green. :-)

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix Object Actor Previews. v1

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 2 - Fix up test_xrayToJS.xul. v1

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 3 - Waive Xrays when SpecialPowers encounters an Opaque XrayWrapper. v1

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 4 - Make unwaived non-Xrayable objects opaque from privileged code. v1

Comment 23

[Bobby Holley (:bholley)]

Attachment: Part 5 - Remove special case for content XBL scopes. v1

This is now the default behavior. \o/

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 6 - Add some helpful logging to the console when we deny access to a non-Xrayable object. v1

Comment 25

[Bobby Holley (:bholley)]

Final try push with all of the proper deps in place:

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

Comment 26

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8453353 [details] [diff] [review]
Part 5 - Remove special case for content XBL scopes. v1

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

So my only concern about these patches are traditional add-ons. SDK based add-ons should be fine, but I have the feeling that important add-ons _might_ be broken in an unexpected and hard to trace way causing a lot of frustration (while the fix is a simple waive call at the right spot if you know what are you looking for). Could you make sure to communicate this at the right channels before/while landing? I'm thinking about something very short at https://blog.mozilla.org/addons for example...

Comment 27

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8453354 [details] [diff] [review]
Part 6 - Add some helpful logging to the console when we deny access to a non-Xrayable object. v1

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

Awesome idea. This should make it really easy to trace any breakage with a debug build at hand :) Only mentioning this part and the waiving would be a great help to some add-on developers. I wish I could check it easily how big of an issue it is really, I would expect only a handful add-ons (top most) to be affected.

Comment 28

[Bobby Holley (:bholley)]

NeedInfoing jorge based on comment 26 and comment 27.

Jorge - the basic situation here is that we now have enough objects covered under Xray Vision [1] that we stop using transparent delegation for the long tail of other sorts of objects. This provides huge security benefits for the browser (which we obviously shouldn't discuss too much in any devrel blog post). Let me know if you need more insight into what's going on.

[1] https://developer.mozilla.org/en-US/docs/Xray_vision

Comment 29

[Bobby Holley (:bholley)]

\o/

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c20ded724d44
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f3c87bf51185
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ce6dab62cbc9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ad3e496af06c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/01c8a2687cd9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d4e3267f18
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/66a22c818713

Comment 30

[Bobby Holley (:bholley)]

Forgot to |git add|: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2b0fb5cfff

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c20ded724d44
https://hg.mozilla.org/mozilla-central/rev/f3c87bf51185
https://hg.mozilla.org/mozilla-central/rev/ce6dab62cbc9
https://hg.mozilla.org/mozilla-central/rev/ad3e496af06c
https://hg.mozilla.org/mozilla-central/rev/01c8a2687cd9
https://hg.mozilla.org/mozilla-central/rev/d8d4e3267f18
https://hg.mozilla.org/mozilla-central/rev/66a22c818713
https://hg.mozilla.org/mozilla-central/rev/ab2b0fb5cfff

Comment 32

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Bobby Holley (:bholley) from comment #28)
> NeedInfoing jorge based on comment 26 and comment 27.

Thank you for the heads up. I would expect some add-ons to be affected by it. From comment #27 it looks like we have a clear console message, so that's something I can easily point developers to.

Is there a plan to uplift this?

Comment 33

[Bobby Holley (:bholley)]

(In reply to Jorge Villalobos [:jorgev] from comment #32)
> (In reply to Bobby Holley (:bholley) from comment #28)
> > NeedInfoing jorge based on comment 26 and comment 27.
> 
> Thank you for the heads up. I would expect some add-ons to be affected by
> it. From comment #27 it looks like we have a clear console message, so
> that's something I can easily point developers to.

Note that this isn't in the web console, but in the native console spew, and only for debug builds.

> Is there a plan to uplift this?

Nope. It will ride the trains.

Comment 34

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Bobby Holley (:bholley) from comment #33)
> (In reply to Jorge Villalobos [:jorgev] from comment #32)
> > Thank you for the heads up. I would expect some add-ons to be affected by
> > it. From comment #27 it looks like we have a clear console message, so
> > that's something I can easily point developers to.
> 
> Note that this isn't in the web console, but in the native console spew, and
> only for debug builds.

Is there any reason we can't add a browser console warning? That's where we can tell developers to look and where they are likely to be looking anyway.

Comment 35

[Bobby Holley (:bholley)]

(In reply to Jorge Villalobos [:jorgev] from comment #34)
> (In reply to Bobby Holley (:bholley) from comment #33)
> > (In reply to Jorge Villalobos [:jorgev] from comment #32)
> > > Thank you for the heads up. I would expect some add-ons to be affected by
> > > it. From comment #27 it looks like we have a clear console message, so
> > > that's something I can easily point developers to.
> > 
> > Note that this isn't in the web console, but in the native console spew, and
> > only for debug builds.
> 
> Is there any reason we can't add a browser console warning? That's where we
> can tell developers to look and where they are likely to be looking anyway.

I think that's a good idea. It might be too spammy if not done properly, but I think it's worth doing. I've filed bug 1042436.

Comment 36

[Al Billings [:abillings - ex-MoCo]]

Is there any need to take this on ESR31 since it didn't go there?

Comment 37

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #36)
> Is there any need to take this on ESR31 since it didn't go there?

This part of a large body of slaughterhouse work that would probably have to all be uplifted together (on the order of 100 patches). We can consider that at some point, but I want to get everything smoothed out and shipping on release first.

Comment 38

[Matt Wobensmith [:mwobensmith][:matt:]]

I see a xul test has been updated for this work, but otherwise, no other tests here to run, in order to verify. If anyone feels this bug needs further verification, let me know what I can do. Marking [qe-verify-] in the interim.

Comment 39

[Benjamin Kerensa [:bkerensa]]

(In reply to Bobby Holley (:bholley) from comment #37)
> (In reply to Al Billings [:abillings] from comment #36)
> > Is there any need to take this on ESR31 since it didn't go there?
> 
> This part of a large body of slaughterhouse work that would probably have to
> all be uplifted together (on the order of 100 patches). We can consider that
> at some point, but I want to get everything smoothed out and shipping on
> release first.

Any plans to still consider this for ESR31?

Comment 40

[Bobby Holley (:bholley)]

(In reply to Benjamin Kerensa [:bkerensa] from comment #39)
> (In reply to Bobby Holley (:bholley) from comment #37)
> > (In reply to Al Billings [:abillings] from comment #36)
> > > Is there any need to take this on ESR31 since it didn't go there?
> > 
> > This part of a large body of slaughterhouse work that would probably have to
> > all be uplifted together (on the order of 100 patches). We can consider that
> > at some point, but I want to get everything smoothed out and shipping on
> > release first.
> 
> Any plans to still consider this for ESR31?

I don't currently have plans to do this.