390813 - Overlay scripts compiled using principal of first document sourcing overlay

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/document/src/nsXULDocument.cpp&rev=1.759&mark=2695,2700#2693>.

The overlay has a <xul:script> in it.  We compile this script with the
principal of the main document:
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/content/src/nsXULElement.cpp&rev=1.694&mark=2999-3002#2996>.

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:
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/document/src/nsXULDocument.cpp&rev=1.759&mark=550,559-562#538>.
 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]

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 [:bzbarsky]]

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]

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: [e-mail]]]

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:1.8.1.7pre) Gecko/20070804 BonEcho/2.0.0.7pre
and
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.12) Gecko/20070508 Firefox/1.5.0.12

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]

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
Minefield/3.0a8pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070805
BonEcho/2.0.0.7pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.13pre) Gecko/20070805
Firefox/1.5.0.13pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9a8pre) Gecko/2007080503
SeaMonkey/2.0a1pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070805
SeaMonkey/1.1.5pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.13pre) Gecko/20070805
SeaMonkey/1.0.9

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: [e-mail]]]

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]]

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 1.8.1.8 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)]

Any suggestions on who might be a good fit?  :)   Sorry to ask a possibly obvious question.  

Comment 11

[Boris Zbarsky [:bzbarsky]]

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]]

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]

got try to get this for fx3. up'ing pri

Comment 14

[Daniel Veditz [:dveditz]]

Boris says this is hard and isn't likely to safely make 1.8.1.12

Comment 15

[chris hofmann]

sounds like a beta3 or beta4 would be a good place to shake out problems with a patch..  p1

Comment 16

[Damon Sicore (:damons)]

Pinged Neil Deakin on this.  Let's see what he says.  

Comment 17

[Neil Deakin]

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

[[e-mail]]

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 [:bzbarsky]]

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]

Adjusting to Beta4

Comment 21

[Damon Sicore (:damons)]

Any progress on this?

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Smaug: Any chance you'd be able to help out here?

Comment 23

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

Taking, though I'm not too familiar with this code.

Comment 24

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

Attachment: possible patch

Could we do as simple as this? Only chrome should load overlays from
cache.
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][[e-mail]]]

Attachment: 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][[e-mail]]]

And the testcase shows that the patch doesn't quite work :(

Comment 27

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

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][[e-mail]]]

Attachment: Only chrome may load chrome overlays

Comment 29

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

Comment on attachment 304096 [details] [diff] [review]
Only chrome may load chrome overlays

Neil, what do you think about this?

Comment 30

[Boris Zbarsky [:bzbarsky]]

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

[[e-mail]]

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][[e-mail]]]

Comment on attachment 304096 [details] [diff] [review]
Only chrome may load chrome overlays

Jst, is this ok to you?

Comment 33

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

Checked in

Comment 34

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

Attachment: for 1.8

Because bug 320211 was trunk only, we need changes in 2 places.

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 306506 [details] [diff] [review]
for 1.8

approved for 1.8.1.13, a=dveditz for release-drivers

Comment 36

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

Verified for 1.8 branch with testcase 2 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13) Gecko/2008031114 Firefox/2.0.0.13. Verified bug in 2.0.0.12 first.