2.65 KB, patch
|Details | Diff | Splinter Review|
572 bytes, application/vnd.mozilla.xul+xml
1.84 KB, patch
|Details | Diff | Splinter Review|
2.99 KB, patch
|Details | Diff | Splinter Review|
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.
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...
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:18.104.22.168pre) Gecko/20070804 BonEcho/22.214.171.124pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:126.96.36.199) Gecko/20070508 Firefox/188.8.131.52 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.)
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:184.108.40.206pre) Gecko/20070805 BonEcho/220.127.116.11pre Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:18.104.22.168pre) Gecko/20070805 Firefox/22.214.171.124pre 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:126.96.36.199pre) Gecko/20070805 SeaMonkey/1.1.5pre Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:188.8.131.52pre) 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
OS: Linux → All
Hardware: PC → All
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 184.108.40.206 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
11 years ago
Whiteboard: [sg:critical] → [sg:critical][dbaron-1.9:RsCe]
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 220.127.116.11
Flags: blocking18.104.22.168+ → blocking22.214.171.124+
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.
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.
Adjusting to Beta4
Target Milestone: --- → mozilla1.9beta4
Any progress on this?
Smaug: Any chance you'd be able to help out here?
Taking, though I'm not too familiar with this code.
Assignee: bzbarsky → Olli.Pettay
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.)
Created attachment 303141 [details] simple test for chrome overlays testcase and testcase 2 test scripts. You'll get some js errors when running them.
And the testcase shows that the patch doesn't quite work :(
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 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+
Comment on attachment 304096 [details] [diff] [review] Only chrome may load chrome overlays Jst, is this ok to you?
Attachment #304096 - Flags: superreview?(jst)
Attachment #304096 - Flags: superreview?(jst) → superreview+
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Created attachment 306506 [details] [diff] [review] for 1.8 Because bug 320211 was trunk only, we need changes in 2 places.
Attachment #306506 - Flags: approval126.96.36.199?
Comment on attachment 306506 [details] [diff] [review] for 1.8 approved for 188.8.131.52, a=dveditz for release-drivers
Attachment #306506 - Flags: approval184.108.40.206? → approval220.127.116.11+
Verified for 1.8 branch with testcase 2 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:18.104.22.168) Gecko/2008031114 Firefox/22.214.171.124. Verified bug in 126.96.36.199 first.
Keywords: fixed188.8.131.52 → verified184.108.40.206
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.