Last Comment Bug 609437 - (CVE-2010-3771) <isindex> doesn't CheckLoadURI
: <isindex> doesn't CheckLoadURI
[sg:critical (stepping stone)]
: verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: unspecified
: x86 Windows XP
: P1 normal (vote)
: mozilla2.0b8
Assigned To: Boris Zbarsky [:bz]
Depends on:
  Show dependency treegraph
Reported: 2010-11-03 16:37 PDT by echo
Modified: 2014-09-04 10:25 PDT (History)
8 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

redpil.html (1.24 KB, text/plain)
2010-11-03 17:10 PDT, Bob Clary [:bc:]
no flags Details
simpler testcase (586 bytes, text/html)
2010-11-03 19:19 PDT, Daniel Veditz [:dveditz]
no flags Details
Fix (2.34 KB, patch)
2010-11-03 21:01 PDT, Boris Zbarsky [:bz]
jonas: review+
dveditz: approval1.9.2.13+
dveditz: approval1.9.1.16+
Details | Diff | Review

Description echo 2010-11-03 16:37:36 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv: Gecko/20101012 Firefox/3.6.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv: Gecko/20101012 Firefox/3.6.11

potential privilege escalation via elements without window 

Reproducible: Always

Steps to Reproduce:
1. open the redpil.html
Actual Results:  
Redirection to chrome://browser/content/broser.xul?

Expected Results:  
incrase privileges 
(but I do not know when it happens)

Comment 1 Bob Clary [:bc:] 2010-11-03 17:10:39 PDT
Created attachment 488081 [details]
Comment 2 Daniel Veditz [:dveditz] 2010-11-03 19:19:12 PDT
Created attachment 488117 [details]
simpler testcase

The first opened/closed window was a red herring, all you need is an <isindex> element. You do seem to need to import it into the new document though. Are we forgetting to set the principal when we touch the new window? Testcase contains two variants that create a similar child window that works the way I'd expect (picks up the origin of the creating page).
Comment 3 Daniel Veditz [:dveditz] 2010-11-03 19:41:21 PDT
Boris or Blake may know where we're going wrong in setting the new window's origin here. It's interesting that a web page can end up with a reference to a window containing a chrome:// url

I didn't try synthesizing events to see if we can avoid the need for the user to submit the prompt, but even if user interaction is required we could probably play clickjacking games or other social engineering to elicit it.

I expected a frame to work identically to the popup window, but doing this into a frame appears to work correctly. Maybe if the isindex element came from another window/document?

So far there's not much privilege escalation I can see -- you can't pick arbitrary chrome:// urls, it's just the main browser window, and once you have a handle to it you can no longer access that window (unless you find an additional bug in our same-origin/wrapper code, in which case the main problem is that additional bug).

Minefield appears immune. In the "broken" case here you just get an about:blank popup with no content. There's no exception or message on the error console about why the element wasn't appended to the popup (shouldn't there be?).
Comment 4 Boris Zbarsky [:bz] 2010-11-03 20:13:53 PDT
Minefield is showing a bug (filed elsewhere) where appending stuff to the DOM created by CreateAboutBlankContentViewer isn't creating frames for the nodes.  They're there in the DOM, if you look in DOM inspector....

Looking into the rest now.
Comment 5 Boris Zbarsky [:bz] 2010-11-03 20:41:57 PDT
OK.  So the document involved has about:blank as URI, chrome://browser/content/browser.xul as the base URI (that would be a bug caused by the fix for bug 482659).  <isindex> submits to the base URI in this case because it has no action set.

<isindex> also has no CheckLoadURI check, which is a bug.  It should.  If it did, the load in this case would be denied and there would be no problem.  I did check that the principal of the <isindex> and its document is correct: it's the principal of the testcase page.

I'm going to post a patch to add the CheckLoadURI call.  We should file a separate bug on about:blank's base URI being wrong in this situation, I think.  And perhaps another bug on the fact that the code in nsDocument.cpp that does the baseURI channel property check doesn't then CheckLoadURI the result, unlike SetBaseURI?  Then again, we've been meaning to remove that last check, I think.
Comment 6 Boris Zbarsky [:bz] 2010-11-03 20:58:17 PDT
Blake, for the base URI issue, wherever we change the principal is probably the right spot to generate a new about:blank URI... the problem is getting the right base URI there.

Note that on trunk we don't have this problem, because the CreateAboutBlankContentViewer call in nsGlobalWindow that we added for compartments makes the base URI about:blank in this situation.  It's not clear to me that this is right, fwiw; worth checking the spec.
Comment 7 Boris Zbarsky [:bz] 2010-11-03 21:01:39 PDT
Created attachment 488126 [details] [diff] [review]
Comment 8 Boris Zbarsky [:bz] 2010-11-03 21:04:35 PDT
The patch was written + tested on 1.9.2, but it applies just fine to 1.9.1 and trunk, and is needed on both, I think.
Comment 9 Jonas Sicking (:sicking) 2010-11-04 02:21:33 PDT
Comment on attachment 488126 [details] [diff] [review]

Glad this code is going away on m-c
Comment 10 Boris Zbarsky [:bz] 2010-11-04 23:35:22 PDT
Comment 11 Daniel Veditz [:dveditz] 2010-11-05 10:18:17 PDT
Comment on attachment 488126 [details] [diff] [review]

Approved for and, a=dveditz for release-drivers
Comment 13 Boris Zbarsky [:bz] 2010-11-05 13:58:12 PDT
I filed bug 610001 on the base URI issue.
Comment 15 Al Billings [:abillings] 2010-11-17 16:49:16 PST
Verified fixed in 1.9.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv: Gecko/20101117 Namoroka/3.6.13pre and in 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv: Gecko/20101117 Shiretoko/3.5.16pre using Dan's testcase.

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