Last Comment Bug 390813 - Overlay scripts compiled using principal of first document sourcing overlay
: Overlay scripts compiled using principal of first document sourcing overlay
: testcase, verified1.8.1.13
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9beta4
Assigned To: Olli Pettay [:smaug]
: Neil Deakin
Depends on:
  Show dependency treegraph
Reported: 2007-08-03 08:57 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2008-07-31 03:24 PDT (History)
21 users (show)
bzbarsky: blocking1.9+
dveditz: blocking1.8.1.13+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

possible patch (2.65 KB, patch)
2008-02-13 13:07 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
simple test for chrome overlays (572 bytes, application/vnd.mozilla.xul+xml)
2008-02-13 15:24 PST, Olli Pettay [:smaug]
no flags Details
Only chrome may load chrome overlays (1.84 KB, patch)
2008-02-18 14:22 PST, Olli Pettay [:smaug]
neil: review+
jst: superreview+
Details | Diff | Splinter Review
for 1.8 (2.99 KB, patch)
2008-02-29 05:28 PST, Olli Pettay [:smaug]
dveditz: approval1.8.1.13+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2007-08-03 08:57:18 PDT
This is spun off of bug 378377 comment 11, since that bug is really about something else and this is getting lost in the information about the problems Alex is seeing.

In brief, we have a non-chrome (not loaded from a chrome:// URI, doesn't have
system principal) XUL document loading an overlay from a chrome:// URI.  This
is explicitly allowed by

The overlay has a <xul:script> in it.  We compile this script with the
principal of the main document:

Now the script has an nsPrincipal as the principal.  But the URI of the overlay
is a chrome:// URI, so when we get into nsXULDocument::EndLoad we go ahead and
write the overlay to the fastload file:
 mIsWritingFastLoad is true any time the XUL cache is enabled; dunno why this
code is checking both that and useXULCache.

So we try to serialize the script (and its principal) into the fastload file. 
Then the next time around we just fastload the overlay, including the
nsPrincipal, and hit the breakage in nsPrincipal::Read.  Note also that even if
we didn't, just creating two principals from the same data doesn't mean they're
equal, so fastloading the principal of the script here is wrong anyway....

But the part that worries me is the following: Say a chrome document loads a
chrome overlay.  Then a non-chrome XUL document loads the same chrome overlay. 
The overlay will be fastloaded, including the script.  The script object will
be given the system principal.  Then we will execute this script against the
non-chrome document's global object using JS_ExecuteScript().

What I want to know is how the principal the script was compiled with is used
in this case.  Does the script end up executing with that principal against the
untrusted global?  If so, I think we should fix things so that doesn't happen,
if nothing else, because the script will define scripted system-principal
functions on said global....

Replicating the blocking flags that bug got based on this information.

Benjamin says in bug 378377 comment 13:

> We should disable fastload for non-chrome main documents altogether. Is that
> easy?

And I reply in bug 378377 comment 14:

> Actually, we also need to disable the prototype cache for non-chrome main
> documents (how do we define "chrome" here, btw?  URI, or principal, or
> both?), since it's the prototype instantiation that's a problem here.  That
> seems a little more complicated, at least with my low knowledge of the code.
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-08-03 11:57:54 PDT
I'm pretty sure that mixing principals will end up being exploitable one way or another. I bet moz_bug would find a way to abuse it in minutes.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2007-08-03 12:18:10 PDT
Perhaps we need to only put things in the proto cache if they have the system principal and only get them from proto cache if the getter has the system principal?  Not sure how easy that would be...  At least now we give overlay documents sane principals, so maybe...
Comment 3 moz_bug_r_a4 2007-08-04 06:17:00 PDT
The script in question is executed with the system principal in content, and
that can be exploited.  Note: functions defined by that script in content does
*not* have the system principal. (Maybe those are cloned functions.)
Comment 5 :Gavin Sharp [email:] 2007-08-04 19:33:59 PDT
That testcase shows the problem for me when using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007080305 Minefield/3.0a7pre

However, with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20070804 BonEcho/
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20070508 Firefox/

I get "Error: uncaught exception: Permission denied to get property UnnamedClass.classes", and the testcase does not work.

(I realize this might just mean that the testcase needs tweaking for the branches, but I figured I'd note it here anyways.)
Comment 6 moz_bug_r_a4 2007-08-06 00:02:29 PDT
I can reproduce on the latest nightly builds of Firefox trunk and branches, and
SeaMonkey trunk and branches.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080505
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20070805
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20070805

Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9a8pre) Gecko/2007080503
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20070805
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20070805

But, I can see the "Permission denied ..." error on Firefox in certain cases.
(not on SeaMonkey.)

If you get "Permission denied ..." error, please try this steps:
  1. Restart Firefox.
  2. View source of any page to open chrome://global/content/viewSource.xul.
     (viewSource.xul loads charsetOverlay.xul.)
  3. Load the testcase.

I'll attach another testcase that uses baseMenuOverlay.xul, which is loaded by
browser.xul, instead of charsetOverlay.xul.  Please try it if you cannot
reproduce with the first testcase.
Comment 8 :Gavin Sharp [email:] 2007-08-06 07:28:53 PDT
OK, I can reproduce with both testcases on the 1.8 branch when I use new profiles. I guess my existing profiles caused the testcase to not work for some reason.
Comment 9 Daniel Veditz [:dveditz] 2007-09-26 13:58:00 PDT
Why do non-chrome documents get to/need to load chrome overlays? What breaks if we disallow that? What purpose does it serve?

Can we make non-chrome parent docs never load things from the fastload file?

I'm looking for some simple fix we can put into and the FF3 beta (M9)

assigning to bz because this shouldn't be assigned to "nobody", but I think dsicore needs to find a better owner given bz's current load.
Comment 10 Damon Sicore (:damons) 2007-09-26 14:03:03 PDT
Any suggestions on who might be a good fit?  :)   Sorry to ask a possibly obvious question.  
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2007-09-26 15:31:22 PDT
Maybe one of the Neils?  Someone more familiar with the overlay stuff, at least.  There's no one who's obviously a good fit. :(
Comment 12 Daniel Veditz [:dveditz] 2007-10-01 15:12:58 PDT
We still need this on the branch, but no time left for testing a fix on the trunk given the lack of progress. Moving to next security update
Comment 13 chris hofmann 2008-01-10 15:10:04 PST
got try to get this for fx3. up'ing pri
Comment 14 Daniel Veditz [:dveditz] 2008-01-11 10:27:45 PST
Boris says this is hard and isn't likely to safely make
Comment 15 chris hofmann 2008-01-15 14:53:13 PST
sounds like a beta3 or beta4 would be a good place to shake out problems with a patch..  p1
Comment 16 Damon Sicore (:damons) 2008-01-16 11:28:19 PST
Pinged Neil Deakin on this.  Let's see what he says.  
Comment 17 Neil Deakin 2008-01-16 11:37:43 PST
What's the question?

If non-chrome should be able to load chrome overlays? I don't see why that would be needed, perhaps someone trying to use globalOverlay.xul/utilityOverlay.xul

The other Neil may be able to provide better historical info.
Comment 18 2008-01-16 12:13:29 PST
I can't see an obvious use case for allowing anyone to load overlays from anywhere they wouldn't be able to link to.
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2008-01-16 13:01:56 PST
Sure.  The question is what all the places that need to be patched to ensure this are.  We do security checks in some cases for overlays but not in others, etc, etc.
Comment 20 Mike Schroepfer 2008-01-29 22:08:30 PST
Adjusting to Beta4
Comment 21 Damon Sicore (:damons) 2008-02-12 14:47:49 PST
Any progress on this?
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-02-12 17:17:12 PST
Smaug: Any chance you'd be able to help out here?
Comment 23 Olli Pettay [:smaug] 2008-02-13 02:11:58 PST
Taking, though I'm not too familiar with this code.
Comment 24 Olli Pettay [:smaug] 2008-02-13 13:07:25 PST
Created attachment 303104 [details] [diff] [review]
possible patch

Could we do as simple as this? Only chrome should load overlays from
Note, with the patch mIsWritingFastLoad is false when handling
non-chrome loaded chrome-overlays.
Passes mochitest, but have to still write some tests.
Enn, do you happen to have any testcases for this?

(And only chrome main documents use xul cache.)
Comment 25 Olli Pettay [:smaug] 2008-02-13 15:24:30 PST
Created attachment 303141 [details]
simple test for chrome overlays

testcase and testcase 2 test scripts. You'll get some js
errors when running them.
Comment 26 Olli Pettay [:smaug] 2008-02-13 15:28:53 PST
And the testcase shows that the patch doesn't quite work :(
Comment 27 Olli Pettay [:smaug] 2008-02-18 13:55:15 PST
Ok, so to make this work nsChromeProtocolHandler shouldn't create 
nsCachedChromeChannel for chrome overlay, if overlay is being loaded from 
content. Can't see any reasonable ways to do that, without somewhat big changes
to chrome loading.
One option is that we disable chrome overlays for content.
Does anyone know if chrome overlays are used in some remote XUL application?

I'll make a patch to prevent chrome overlays for content.
Comment 28 Olli Pettay [:smaug] 2008-02-18 14:22:16 PST
Created attachment 304096 [details] [diff] [review]
Only chrome may load chrome overlays
Comment 29 Olli Pettay [:smaug] 2008-02-18 15:16:10 PST
Comment on attachment 304096 [details] [diff] [review]
Only chrome may load chrome overlays

Neil, what do you think about this?
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2008-02-18 15:24:18 PST
I wonder whether we could make nsContentUtils::CheckSecurityBeforeLoad usable here.  I hate having these ad-hoc URI checks...

That said, that can be a followup bug.  That patch looks pretty reasonable to me!
Comment 31 2008-02-19 04:22:09 PST
Comment on attachment 304096 [details] [diff] [review]
Only chrome may load chrome overlays

Looks good to me, I can't think why content should need chrome overlays.
Comment 32 Olli Pettay [:smaug] 2008-02-19 04:32:09 PST
Comment on attachment 304096 [details] [diff] [review]
Only chrome may load chrome overlays

Jst, is this ok to you?
Comment 33 Olli Pettay [:smaug] 2008-02-19 14:21:23 PST
Checked in
Comment 34 Olli Pettay [:smaug] 2008-02-29 05:28:37 PST
Created attachment 306506 [details] [diff] [review]
for 1.8

Because bug 320211 was trunk only, we need changes in 2 places.
Comment 35 Daniel Veditz [:dveditz] 2008-02-29 16:06:01 PST
Comment on attachment 306506 [details] [diff] [review]
for 1.8

approved for, a=dveditz for release-drivers
Comment 36 Al Billings [:abillings] 2008-03-14 17:25:02 PDT
Verified for 1.8 branch with testcase 2 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2008031114 Firefox/ Verified bug in first.

Note You need to log in before you can comment on or make changes to this bug.