Overlay scripts compiled using principal of first document sourcing overlay

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
XUL
P1
critical
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: bz, Assigned: smaug)

Tracking

({testcase, verified1.8.1.13})

Trunk
mozilla1.9beta4
testcase, verified1.8.1.13
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.13 +
wanted1.8.1.x +
blocking1.8.0.next +
wanted1.8.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical][dbaron-1.9:RsCe])

Attachments

(4 attachments)

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.
Flags: blocking1.9+
Flags: blocking1.8.1.7+
Flags: blocking1.8.0.13+
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.
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

10 years ago
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.)
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

10 years ago
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.
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.
Severity: normal → critical
Keywords: testcase
OS: Linux → All
Hardware: PC → All
Whiteboard: [sg:critical]
Flags: blocking1.8.0.13+ → blocking1.8.0.14+
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.
Assignee: nobody → bzbarsky
Any suggestions on who might be a good fit?  :)   Sorry to ask a possibly obvious question.  
Maybe one of the Neils?  Someone more familiar with the overlay stuff, at least.  There's no one who's obviously a good fit. :(
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
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.9+
Flags: blocking1.8.1.8+
Whiteboard: [sg:critical] → [sg:critical][dbaron-1.9:RsCe]
Priority: -- → P3
Flags: blocking1.8.0.14+ → blocking1.8.0.15?

Comment 13

9 years ago
got try to get this for fx3. up'ing pri
Priority: P3 → P2
Boris says this is hard and isn't likely to safely make 1.8.1.12
Flags: blocking1.8.1.12+ → blocking1.8.1.13+

Comment 15

9 years ago
sounds like a beta3 or beta4 would be a good place to shake out problems with a patch..  p1
Priority: P2 → P1
Pinged Neil Deakin on this.  Let's see what he says.  

Comment 17

9 years ago
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.
 
I can't see an obvious use case for allowing anyone to load overlays from anywhere they wouldn't be able to link to.
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

9 years ago
Adjusting to Beta4
Target Milestone: --- → mozilla1.9beta4
Any progress on this?
Smaug: Any chance you'd be able to help out here?
(Assignee)

Comment 23

9 years ago
Taking, though I'm not too familiar with this code.
Assignee: bzbarsky → Olli.Pettay
(Assignee)

Comment 24

9 years ago
Created attachment 303104 [details] [diff] [review]
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.)
(Assignee)

Comment 25

9 years ago
Created attachment 303141 [details]
simple test for chrome overlays

testcase and testcase 2 test scripts. You'll get some js
errors when running them.
(Assignee)

Comment 26

9 years ago
And the testcase shows that the patch doesn't quite work :(
(Assignee)

Comment 27

9 years ago
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.
(Assignee)

Comment 28

9 years ago
Created attachment 304096 [details] [diff] [review]
Only chrome may load chrome overlays
(Assignee)

Comment 29

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

Neil, what do you think about this?
Attachment #304096 - Flags: review?(neil)
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 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.
Attachment #304096 - Flags: review?(neil) → review+
(Assignee)

Comment 32

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

Jst, is this ok to you?
Attachment #304096 - Flags: superreview?(jst)

Updated

9 years ago
Flags: blocking1.8.0.15? → blocking1.8.0.15+

Updated

9 years ago
Attachment #304096 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 33

9 years ago
Checked in
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
(Assignee)

Comment 34

9 years ago
Created attachment 306506 [details] [diff] [review]
for 1.8

Because bug 320211 was trunk only, we need changes in 2 places.
Attachment #306506 - Flags: approval1.8.1.13?
Comment on attachment 306506 [details] [diff] [review]
for 1.8

approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #306506 - Flags: approval1.8.1.13? → approval1.8.1.13+
(Assignee)

Updated

9 years ago
Keywords: fixed1.8.1.13
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.
Keywords: fixed1.8.1.13 → verified1.8.1.13

Updated

9 years ago
Attachment #306506 - Flags: approval1.8.0.15?
Group: security

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.