Unwaived wrappers to unprivileged JS should not be transparent

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({addon-compat, sec-vector})

unspecified
mozilla33
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox33 fixed, firefox-esr31 wontfix)

Details

(Whiteboard: [adv-main33-] can lead to sec-high/critical bugs. Embargo until esr31 EOL (slaughterhouse related))

Attachments

(6 attachments)

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.
Marking sec-other because it looks like defense-in-depth and not a known security problem per se.
Keywords: sec-other
(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.
Ah, okay.  Well, rate this and that other bug whatever you think is appropriate. :)
Keywords: sec-other
Well, I guess sec-other still probably makes the most sense here, with bug 856042 tracking the issue we know about currently.
Keywords: sec-other
Keywords: sec-othersec-moderate
Whiteboard: can lead to sec-high/critical bugs
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
(Note to self: 'more complicated solutions' include de-clawing getters and keeping a map of transitively-whitelisted wrappers)
Depends on: 881886
Was the try-push successful?
Flags: needinfo?(bobbyholley+bmo)
(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.
Flags: needinfo?(bobbyholley+bmo)
Assignee: nobody → bobbyholley+bmo
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. :)
Blocks: SH-wrappers
Blocks: 930091
Depends on: 1028987
Blocks: 987678
Blocks: 928415
Depends on: 1033927
Depends on: 1034239
(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.
(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
(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. :-)
Depends on: 987669
Attachment #8453347 - Flags: review?(past)
Attachment #8453349 - Flags: review?(gkrizsanits)
This is now the default behavior. \o/
Attachment #8453353 - Flags: review?(gkrizsanits)
Final try push with all of the proper deps in place:

https://tbpl.mozilla.org/?tree=Try&rev=b03835f88a94
Attachment #8453347 - Flags: review?(past) → review+
Attachment #8453349 - Flags: review?(gkrizsanits) → review+
Attachment #8453351 - Flags: review?(gkrizsanits) → review+
Attachment #8453352 - Flags: review?(gkrizsanits) → review+
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...
Attachment #8453353 - Flags: review?(gkrizsanits) → review+
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.
Attachment #8453354 - Flags: review?(gkrizsanits) → review+
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
Flags: needinfo?(jorge)
(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?
Flags: needinfo?(jorge)
Keywords: addon-compat
(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.
(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.
Depends on: 1042398
(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.
Depends on: 987111, 1020609, 987163, 976148
Blocks: 1042824
Is there any need to take this on ESR31 since it didn't go there?
(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.
Keywords: sec-moderatesec-vector
Whiteboard: can lead to sec-high/critical bugs → [adv-main33-] can lead to sec-high/critical bugs
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.
Flags: qe-verify-
(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?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
Whiteboard: [adv-main33-] can lead to sec-high/critical bugs → [adv-main33-] can lead to sec-high/critical bugs. Embargo until esr31 EOL (slaughterhouse related)
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.