The default bug view has changed. See this FAQ.
Bug 477979 (CVE-2009-1840)

XUL scripts skip some security checks

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: Wladimir Palant, Assigned: Wladimir Palant)

Tracking

(Blocks: 1 bug, {fixed1.9.1, verified1.9.0.11})

Trunk
fixed1.9.1, verified1.9.0.11
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.11 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Created attachment 361729 [details]
Testcase

I experimented with different types of scripts and apparently XUL scripts don't trigger content policies. The result is for example that the scripts are loaded even though JavaScript is disabled (nsWebBrowserContentPolicy doesn't have a say about this). I attached a XUL document loading https://bugzilla.mozilla.org/js/util.js - LiveHTTPHeaders extension confirms that this script is loaded regardless of preferences.

Looking at nsXULDocument::LoadScript(), the download is apparently started without any checks at all. At least further down in nsXULDocument::OnStreamComplete() there is a call to nsScriptLoader::ShouldExecuteScript(), this must be the one preventing me from running a script from file:///. However, if I compare to nsScriptLoader - there are additionally calls to CheckLoadURIWithPrincipal() and CheckContentPolicy() in StartLoad(), both are missing for XUL scripts.
(Assignee)

Updated

8 years ago
Blocks: 467520
(Assignee)

Comment 1

8 years ago
Created attachment 361765 [details] [diff] [review]
Proposed patch

I refactored the security checks into a method nsScriptLoader::ShouldLoadScript() and made nsXULDocument call it. Unfortunately, it seems that the connection between the script and the XUL element isn't kept in a XUL document which is why the document has to be passed as context node.

Not sure who would be a good reviewer for this.
Assignee: nobody → trev.moz
Status: NEW → ASSIGNED
Attachment #361765 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #361765 - Attachment description: Proposed testcase → Proposed patch
(Assignee)

Updated

8 years ago
Attachment #361765 - Attachment is patch: true
Attachment #361765 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

8 years ago
Attachment #361765 - Flags: review? → review?(jst)
Attachment #361765 - Flags: review?(jonas)
I don't think this needs two reviewers, added Jonas to let him and jst fight over who wants to look at it.
This seems at least sg:high, but of course for some users this could be critical if it allows bypassing content policies they think they're relying on (like NoScript, or future Content-Security-Policy stuff).
Whiteboard: [sg:high]

Updated

8 years ago
Attachment #361765 - Flags: review?(jst) → review+
Comment on attachment 361765 [details] [diff] [review]
Proposed patch

+    rv = nsScriptLoader::ShouldLoadScript(static_cast<nsIDocument*>(this), static_cast<nsIDocument*>(this), aScriptProto->mSrcURI, type);

Maybe try to wrap this line to fit in 80 columns...

r=jst
At least one of those casts isn't actually needed, is it?
(Assignee)

Comment 6

8 years ago
(In reply to comment #5)
> At least one of those casts isn't actually needed, is it?

Yes, the first cast can be implicit.
(Assignee)

Updated

8 years ago
Attachment #361765 - Flags: review?(jonas)
(Assignee)

Comment 7

8 years ago
Created attachment 362188 [details] [diff] [review]
Patch to be checked in

Only changed ShouldLoadScript call - removed first cast, wrapped the line to make sure it doesn't get too long and replaced nsAutoString by NS_LITERAL_STRING (yes, I am still learning all those macros).
Attachment #361765 - Attachment is obsolete: true
Attachment #362188 - Flags: review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Is this something that should land on 1.9.1 as well?
(Assignee)

Comment 9

8 years ago
Yes, certainly (maybe even 3.0.x). I was just waiting for it to land on trunk first to see whether there will be issues.
Er, crap.  I bet this is not showing up for people who do checkin-needed landings because it's security sensitive... :(

I'll try to get it landed today.
Pushed http://hg.mozilla.org/mozilla-central/rev/b307ac6d68c9

This should be blocking the branches, imo...  I didn't push the test, in the usual security bug way.  :(

Sorry again for the lag to checkin.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite?
Flags: blocking1.9.1?
Flags: blocking1.9.0.9?
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: blocking1.9.1? → blocking1.9.1+
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/80cd94e79e3f.
Keywords: fixed1.9.1
The patch doesn't apply cleanly to 1.9.0; Wladimir, would you be willing to backport it there?
(Assignee)

Comment 14

8 years ago
(In reply to comment #13)
> The patch doesn't apply cleanly to 1.9.0; Wladimir, would you be willing to
> backport it there?

Will be a problem - I don't have a development environment for 1.9.0 any more. So this will take a while, better if somebody else can do that.

Anyway, 1.9.0 patch should probably be simpler - copy content policy check from nsScriptLoader::CheckContentPolicy() into nsXULDocument::LoadScript() rather than make all scripts use the same code. Copying CheckLoadURIWithPrincipal call is not necessary, there is one in XULContentSinkImpl::OpenScript() already which should be sufficient.
Hmm.  That check is there on trunk too, so we only really needed a content policy check in nsXULDocument, no?
(Assignee)

Comment 16

8 years ago
True. I only found the check in XULContentSinkImpl::OpenScript() a few days ago, when trying to figure out why including scripts from file:/// doesn't work. However, for trunk I would rather vote for removing the check in XULContentSinkImpl::OpenScript() - has the advantage that both XUL and HTML scripts use the same code for security checks and it probably also makes "if we ever allow scripts added via the DOM to run, we need to add a CheckLoadURI call for that as well" comment unnecessary (if adding scripts via DOM is added, it will certainly go through nsXULDocument::LoadScript()).
We can't remove the check in OpenScript, as far as I can tell.  If we did, it would allow fastloading of scripts that you can't normally link to.
(Assignee)

Comment 18

8 years ago
I don't really know much about fastloading but isn't it only used for chrome documents where we skip the checks anyway?
It's used for chrome scripts, which is not the same thing.
So it seems to me we should only do the content policy check in the XULDocument code.
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.10?
Flags: blocking1.9.0.10+
Wladimir, do you want to do that, or should I?
(Assignee)

Comment 22

8 years ago
I might be able to do this on Monday evening.
(Assignee)

Comment 23

8 years ago
Created attachment 371330 [details] [diff] [review]
1.9.0 patch
Attachment #371330 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #371330 - Flags: review? → review?(jst)
Whiteboard: [sg:high] → [sg:high] needs r=jst for 1.9.0
Don't you need to call CheckLoadURIWithPrincipal as well?
Jonas, see comment 14 through comment 20.

Note that we should change the trunk/1.9.1 code as well to not do the extra CheckLoadURI check...
Comment on attachment 371330 [details] [diff] [review]
1.9.0 patch

Makes sense
Attachment #371330 - Flags: superreview+
Attachment #371330 - Flags: review?(jst)
Attachment #371330 - Flags: review+
Attachment #371330 - Flags: approval1.9.0.10?
Whiteboard: [sg:high] needs r=jst for 1.9.0 → [sg:high]
Comment on attachment 371330 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.11, a=dveditz for release-drivers
Attachment #371330 - Flags: approval1.9.0.11? → approval1.9.0.11+
Keywords: checkin-needed
Whiteboard: [sg:high] → [sg:high][needs checkin on 1.9.0]
Keywords: fixed1.9.0.11
fix checked into the 1.9.0 branch
Whiteboard: [sg:high][needs checkin on 1.9.0] → [sg:high]
So should there be a followup on the fact that we now CheckLoadURI twice for XUL?
(Assignee)

Comment 30

8 years ago
I guess we should back out current trunk patch and check in 1.9.0 patch instead (if it applies). Should this be done in a separate bug?
Please.
(In reply to comment #0)
> I experimented with different types of scripts and apparently XUL scripts don't
> trigger content policies. The result is for example that the scripts are loaded
> even though JavaScript is disabled (nsWebBrowserContentPolicy doesn't have a
> say about this). I attached a XUL document loading
> https://bugzilla.mozilla.org/js/util.js - LiveHTTPHeaders extension confirms
> that this script is loaded regardless of preferences.

I tried this with both 1.9.0.10 and a 1.9.0.11pre build. Running the 1.9.0.11pre build with Javascript disabled, Live HTTP Headers still shows that https://bugzilla.mozilla.org/js/util.js is loaded even though I have Javascript disabled. Shouldn't the patch be blocking this?

Headers:

https://bugzilla.mozilla.org/attachment.cgi?id=361729

GET /attachment.cgi?id=361729 HTTP/1.1
Host: bugzilla.mozilla.org
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11pre) Gecko/2009051404 GranParadiso/3.0.11pre
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive
Cookie: __utma=150903082.1539278081.1242344153.1242345910.1242345988.4; __utmb=150903082; __utmz=150903082.1242344153.1.1.utmccn=(direct)|utmcsr=(direct)|utmcmd=(none); Bugzilla_login=280903; Bugzilla_logincookie=rd6p05tCnQ; dloadday=69.181.187.135.1242344852944218

HTTP/1.x 302 Moved
Date: Fri, 15 May 2009 00:08:55 GMT
Server: Apache/2.2.3 (Red Hat)
Location: https://bug477979.bugzilla.mozilla.org/attachment.cgi?id=361729&t=Unii2Eg1Ae
Content-Length: 350
Keep-Alive: timeout=300, max=1000
Connection: Keep-Alive
Content-Type: text/html; charset=iso-8859-1
----------------------------------------------------------
https://bug477979.bugzilla.mozilla.org/attachment.cgi?id=361729&t=Unii2Eg1Ae

GET /attachment.cgi?id=361729&t=Unii2Eg1Ae HTTP/1.1
Host: bug477979.bugzilla.mozilla.org
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11pre) Gecko/2009051404 GranParadiso/3.0.11pre
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive
Cookie: __utma=150903082.1539278081.1242344153.1242345910.1242345988.4; __utmb=150903082; __utmz=150903082.1242344153.1.1.utmccn=(direct)|utmcsr=(direct)|utmcmd=(none); dloadday=69.181.187.135.1242344852944218

HTTP/1.x 200 OK
Date: Fri, 15 May 2009 00:08:55 GMT
Server: Apache/2.2.3 (Red Hat)
Content-Disposition: inline; filename="script_policies.xul"
Content-Length: 151
X-Backend-Server: mrapp51
Keep-Alive: timeout=300, max=1000
Connection: Keep-Alive
Content-Type: application/vnd.mozilla.xul+xml; name="script_policies.xul"
----------------------------------------------------------
https://bugzilla.mozilla.org/js/util.js

GET /js/util.js HTTP/1.1
Host: bugzilla.mozilla.org
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11pre) Gecko/2009051404 GranParadiso/3.0.11pre
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive
Cookie: __utma=150903082.1539278081.1242344153.1242345910.1242345988.4; __utmb=150903082; __utmz=150903082.1242344153.1.1.utmccn=(direct)|utmcsr=(direct)|utmcmd=(none); Bugzilla_login=280903; Bugzilla_logincookie=rd6p05tCnQ; dloadday=69.181.187.135.1242344852944218

HTTP/1.x 200 OK
Date: Fri, 15 May 2009 00:08:55 GMT
Server: Apache/2.2.3 (Red Hat)
Last-Modified: Fri, 29 Aug 2008 04:40:02 GMT
Etag: "187a-d8c12c80"
Accept-Ranges: bytes
Content-Length: 6266
X-Backend-Server: mrapp51
Keep-Alive: timeout=300, max=1000
Connection: Keep-Alive
Content-Type: application/x-javascript
----------------------------------------------------------
> even though I have Javascript disabled

Disabled how?  In preferences?  That doesn't use a content policy, and will not prevent loading of scripts from XUL (though it'll prevent their execution).
Doh. Apparently, I know nothing about content policy. I disabled through preferences.
All right. I used Adblock Plus and disabled https://bugzilla.mozilla.org/js/util.js within it. With Firefox 3.0.10, https://bugzilla.mozilla.org/js/util.js is still loaded. With the 1.9.0.11pre build, it is not loaded.

Verified1.9.0.11
Keywords: fixed1.9.0.11 → verified1.9.0.11
Removing checkin-needed, as that was added for the 1.9.0 checkin
(done comment 28).

(In reply to comment #29)
> So should there be a followup on the fact that we now CheckLoadURI twice for
> XUL?

Was a bug filed on this?
Keywords: checkin-needed
Bug exists on the 1.8 branch.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next?
Not actually an exploit in itself, would require some other exploit in the included script that it failed to block (or for Thunderbird it may be an sg:low privacy problem in its own right). Lowering severity to "sg:moderate" at best, although sg:low might even be accurate. Of course if you depend on things like NoScript you might not be happy about the bypass, but that doesn't make it an exploit per se.
Whiteboard: [sg:high] → [sg:moderate]
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Alias: CVE-2009-1840
(In reply to comment #38)
> NoScript you might not be happy about the bypass, but that doesn't make it an
> exploit per se.

NoScript was OK with the bypass, since it had a WebProgressListener safety net preventing the script from loading anyway.
Sorry for adding this clarification so late in the game, but I've just seen the release note with the NoScript reference (is it possible to correct them?)

Comment 40

8 years ago
Created attachment 383274 [details] [diff] [review]
1.8 patch

1.8 patch, but I'm unable to verify the fix. Live headers show me that the js script is not loaded with or without this patch applied. I even can't reproduce the issue with ff 3.0.10...
I filed bug 643078 on comment 30.
Group: core-security
You need to log in before you can comment on or make changes to this bug.