Closed Bug 897524 Opened 11 years ago Closed 11 years ago

b2g iframe sandbox is an ineffective security measure

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:-, firefox-esr24 unaffected, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 wontfix)

Status:
RESOLVED FIXED
Project Flags:
blocking-b2g -
Tracking Flags:
Tracking Status
firefox-esr24 --- unaffected
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- wontfix

People

(Reporter: freddy, Unassigned)

References

Details

(Keywords: meta, sec-high) 

This bug is to make the link between the security review of the html sanitizer and bug 886164 secret.

Bug 886164 says that CSP is not enforced if the content is in an iframe sandbox.
Without this bug fixed the sandbox will surely allow blocking scripts but will also remove any CSP rules that block other resources like styles and images.

In essence:
CSP (no http images) -> IframeSandbox (all images).

I couldn't make a testcase to confirm this *in b2g*, but there are mochitests in bug 886164 and I have tested this again in a PHP script against Firefox Aurora (currently at 23).
This sounds bad, so I'm setting this to sec-high.  Feel free to adjust as appropriate.
Keywords: sec-high
It's worth noting that the e-mail app's HTML sanitizer does try to render all remote resource fetches inert by default, it's just quite possible that it can be outwitted.  We hate all remote resources but images, which we will conditionally turn on based on user intent.

(What we really want is nsIContentPolicy or https://github.com/slightlyoff/NavigationController/ super-powers.)
So the worst case scenario is somebody finding a sanitizer bypass and showing images regardless of user intent?
Let's assume our sanitizer is useless.  In that case it hits our iframe.  Our code is here: https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/iframe_shims.js#L197 but the short story is that we set "allow-same-origin" so we can monkey with the DOM tree to turn on remote images and allow ourselves to listen for click events.  We don't set "allow-scripts", "allow-forms", "allow-pointer-lock", "allow-popups", or "allow-top-navigation".

So in that case (assuming the iframe can't create a sub-iframe with "allow-scripts"), the evil email could result in standard information leakage and potentially style itself in a wildly gaudy fashion.  It could probably also effectively use a lot of network and or CPU/GPU by having crazy CSS transitions/animations.  But that should presumably be it?
er, although it's probably worth noting that even if the sanitizer screws up, the way we do our innerHTML, the rogue e-mail won't be able to generate things that only can live inside a <head> tag since innerHTML should discard them, so stuff like 'meta refresh' probably shouldn't work.
This is getting worse: You can execute scripts using object tags... (bug 886262).
Depends on: CVE-2013-5614
I'll be on PTO tomorrow (Friday UTC+2) and will probably leave this untouched until Monday. The current state is that the iframe sandbox nullifies the CSP policies and object tags may be used to execute JavaScript.

We're just one sanitizer bypass away from a working XSS in the email app!

My current testbed is available at <http://werjvdafgertwertf.pastebin.mozilla.org/2708432>, but has the following problems that I plan to address very soon:
- It currently only executes in nodejs. I'd prefer using gecko/spidermonkey as this is the engine we're using it in :)
- bleach.clean(htmlStr, BLEACH_SETTINGS) doesn't actually reflect the code that is used in the email app. I'm still working on it...
I don't have access to bug 886262, so I can't see what's going on there.  That does sound pretty bad, though.  I assume there's not going to be any debate about the fact that that's a serious platform bug that will need to be fixed though...
Adding :imelven here.
I tried really really long bypassing the sandbox, the HTML sanitizer used in the email app *and* CSP:
I found a way that executes JS in a different scope (though a bit complicated to repro on the phone and requires user interaction). We should get this fixed.. :)
(In reply to Frederik Braun [:freddyb] from comment #10)
> I found a way that executes JS in a different scope (though a bit
> complicated to repro on the phone and requires user interaction). We should
> get this fixed.. :)

Kudos / aaaaaaaaaaaargh!

By different scope do you mean:
- different origin, same process
- different origin, different process
- same origin, different global
Different origin. How would I find out if the process changed?
If you're using an actual device and pulling logs off via 'adb logcat', the pid is enclosed in parens at the start of the line.  Assuming dump() is in the global scope (it should be) and the pref is on (it should be?), you can do something like:
  dump("I'm in ur JS interpreter\n")

and should see something like:
I/GeckoDump  ( 4705): I'm in UR JS interpreter


The e-mail app already spews all kind of GeckoDump output, so it should be clear if the pid is the same or not.
Also, you probably are in the same process since I think the bug to let an iframe opened from an app already in a sub-process live in another subprocess is ongoing (bug 879475).  But if your trick involved effectively triggering window.open or an equivalent that would cause the system app to create another iframe or have the browser app do it, that would be a different process.
The real device testing is a bit....troublesome. My PoC involves a multitude of indirections using frames, iframes *and* meta refreshes within the sandboxed iframe. If that helps... :)
Depends on: 899070
Sorry for not updating this properly, but *at least* one of the bugs 886164 886262 899070 needs to be fixed for this to become less worrying:

The blocking bugs are sec-moderate but combining these into a chain of exploits that bypasses all security measures makes this bug a sec-high.

Since all except bug 899070 are gecko fixes, I assume changing the Gaia part is more likely to be finished in time.
blocking-b2g: --- → leo?
Pushing this to koi? since we'd be very unlikely to take more risk at this point with changing the approach. Given that chaining exploits is necessary here and the level is high, not critical, we still believe this is better suited for a more complete fix in v1.2 than cramming in one of three fixes in an unsupported, late-landing fashion before v1.1's release.
blocking-b2g: leo? → koi?
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #17)

Lucas any update on what the target milestone can be?
(In reply to David Bolter [:davidb] from comment #18)
> (In reply to lsblakk@mozilla.com [:lsblakk] from comment #17)
> 
> Lucas any update on what the target milestone can be?

I think it's in hand.  The only outstanding bug, bug 886164 actually had a fix landed, but it broke a lot of stuff in B2G so got backed out.  Presumably that will be looked into and fixed and landed soon?

Ideally that bug should just be marked koi+ and this bug can be mark fixed when that bug gets fixed.
Andrew,

Any update with this bug?
Flags: needinfo?(bugmail)
(In reply to Preeti Raghunath(:Preeti) from comment #20)
> Any update with this bug?

The Gaia e-mail app bug, bug 899070, was fixed.

The only outstanding bug related to this is bug 886164 for CSP propagation for sandboxed iframes.  It sounded like Deian was looking for platform or security assistance but was having trouble finding it.  Since that's the only outstanding bug and I'm not sure what purpose this meta-ish bug serves, I'd suggest moving any koi prioritization to that bug, but that's just me.
Flags: needinfo?(bugmail)
Andrew,

bug 886164 - https://bugzilla.mozilla.org/show_bug.cgi?id=886164 is not a blocker though. So, not sure if this needs to be a koi?

Moving to minus but please re-nom if needed.
blocking-b2g: koi? → -
(In reply to Andrew Sutherland (:asuth) from comment #21)
> 
> The only outstanding bug related to this is bug 886164 for CSP propagation
> for sandboxed iframes.  It sounded like Deian was looking for platform or
> security assistance but was having trouble finding it.  Since that's the
> only outstanding bug and I'm not sure what purpose this meta-ish bug serves,
> I'd suggest moving any koi prioritization to that bug, but that's just me.

This issue has been fixed. What else is left in *this* bug to do? Can we close this?
(In reply to Al Billings [:abillings] from comment #23)
> This issue has been fixed. What else is left in *this* bug to do? Can we
> close this?

I agree there doesn't seem anything else left to do.  I think we can close this.  resolving fixed.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
I am marking the bugs like this, since if we create a 1.1.1 we should probably do something to fix the bug in the email app which allowed exploitation on this. Freddy can you add more information on this fix?
status-b2g18: --- → affected
status-b2g-v1.2: --- → wontfix
Flags: needinfo?(fbraun)
This here is just a meta bug explaining how a trivial combination of the linked bugs can cause some trouble.
The actual fix for the email app is in bug 899070.
Flags: needinfo?(fbraun)
(In reply to Paul Theriault [:pauljt] from comment #25)
> I am marking the bugs like this, since if we create a 1.1.1 we should
> probably do something to fix the bug in the email app which allowed
> exploitation on this. Freddy can you add more information on this fix?

(In reply to Frederik Braun [:freddyb] from comment #26)
> This here is just a meta bug explaining how a trivial combination of the
> linked bugs can cause some trouble.
> The actual fix for the email app is in bug 899070.

Somewhat confusing that this bug is marked sec-high, but none of the dependencies are higher than sec-moderate. Paul, do you want me to work on backports of the 3 blocking bugs?
Well sorry for the confusion, but "sec-moderate" means something like "This could be very harmful but there's something missing".
These through "could be"s can be turned into one "is actually harmful", if tied together. That's what this bug is for.
I will add the 'meta' keyword to stress this.

It would be great if we could backport all of the three, but somebody was worried we couldn't. In this very worst case bug 889070 might be the most desirable.
Keywords: meta
Paul, let me know what you want to do here.
Flags: needinfo?(ptheriault)
(In reply to Frederik Braun [:freddyb] from comment #28)
> It would be great if we could backport all of the three, but somebody was
> worried we couldn't. In this very worst case bug 889070 might be the most
> desirable.

I assume this is a typo of bug 899070.   That bug would be very easy to uplift if we want to / someone grants approval.
(In reply to Andrew Sutherland (:asuth) from comment #30)
> (In reply to Frederik Braun [:freddyb] from comment #28)
> > It would be great if we could backport all of the three, but somebody was
> > worried we couldn't. In this very worst case bug 889070 might be the most
> > desirable.
> 
> I assume this is a typo of bug 899070.   That bug would be very easy to
> uplift if we want to / someone grants approval.

Yes, sorry. bug 899070!
Ryan, yeh as above, bug 899070 is what I had hoped we could uplift, thanks.
Flags: needinfo?(ptheriault)
(In reply to Andrew Sutherland (:asuth) from comment #30)
> (In reply to Frederik Braun [:freddyb] from comment #28)
> > It would be great if we could backport all of the three, but somebody was
> > worried we couldn't. In this very worst case bug 889070 might be the most
> > desirable.
> 
> I assume this is a typo of bug 899070.   That bug would be very easy to
> uplift if we want to / someone grants approval.

Andrew, if you can do the nomination request, I can help get it uplifted once approved. Looks like we don't have a Gaia v1 approval flag, so I would just request b2g18 approval instead.

Paul, do you want to wontfix the other two deps for b2g18 then or did you still want them evaluated for possible uplift?
Flags: needinfo?(ptheriault)
Flags: needinfo?(bugmail)
I have performed the requested nomination.
Flags: needinfo?(bugmail)
Uplifted bug 899070 to v1-train and v1.1.0hd. Calling this fixed for those branches.
status-b2g18: affected → fixed
status-b2g-v1.1hd: --- → fixed
Flags: needinfo?(ptheriault)
Group: b2g-core-security → core-security
Group: core-security
You need to log in before you can comment on or make changes to this bug.