Last Comment Bug 477979 - (CVE-2009-1840) XUL scripts skip some security checks
(CVE-2009-1840)
: XUL scripts skip some security checks
Status: RESOLVED FIXED
[sg:moderate]
: fixed1.9.1, verified1.9.0.11
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Wladimir Palant
:
Mentors:
Depends on:
Blocks: abp
  Show dependency treegraph
 
Reported: 2009-02-11 02:13 PST by Wladimir Palant
Modified: 2013-08-25 17:59 PDT (History)
11 users (show)
mbeltzner: blocking1.9.1+
dveditz: blocking1.9.0.11+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (151 bytes, application/vnd.mozilla.xul+xml)
2009-02-11 02:13 PST, Wladimir Palant
no flags Details
Proposed patch (5.16 KB, patch)
2009-02-11 06:58 PST, Wladimir Palant
jst: review+
Details | Diff | Splinter Review
Patch to be checked in (5.19 KB, patch)
2009-02-12 23:54 PST, Wladimir Palant
trev.moz: review+
Details | Diff | Splinter Review
1.9.0 patch (2.58 KB, patch)
2009-04-06 16:12 PDT, Wladimir Palant
jonas: review+
jonas: superreview+
dveditz: approval1.9.0.11+
Details | Diff | Splinter Review
1.8 patch (1.78 KB, patch)
2009-06-15 07:46 PDT, Martin Stránský
no flags Details | Diff | Splinter Review

Description Wladimir Palant 2009-02-11 02:13:03 PST
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.
Comment 1 Wladimir Palant 2009-02-11 06:58:55 PST
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.
Comment 2 Daniel Veditz [:dveditz] 2009-02-11 14:11:06 PST
I don't think this needs two reviewers, added Jonas to let him and jst fight over who wants to look at it.
Comment 3 Daniel Veditz [:dveditz] 2009-02-11 14:16:44 PST
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).
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2009-02-12 14:44:52 PST
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
Comment 5 Boris Zbarsky [:bz] 2009-02-12 15:07:16 PST
At least one of those casts isn't actually needed, is it?
Comment 6 Wladimir Palant 2009-02-12 23:42:04 PST
(In reply to comment #5)
> At least one of those casts isn't actually needed, is it?

Yes, the first cast can be implicit.
Comment 7 Wladimir Palant 2009-02-12 23:54:01 PST
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).
Comment 8 Johnathan Nightingale [:johnath] 2009-03-23 07:43:40 PDT
Is this something that should land on 1.9.1 as well?
Comment 9 Wladimir Palant 2009-03-23 07:49:59 PDT
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.
Comment 10 Boris Zbarsky [:bz] 2009-03-23 07:52:18 PDT
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.
Comment 11 Boris Zbarsky [:bz] 2009-03-23 11:55:13 PDT
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.
Comment 12 Boris Zbarsky [:bz] 2009-03-26 14:26:36 PDT
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/80cd94e79e3f.
Comment 13 Boris Zbarsky [:bz] 2009-03-26 14:27:27 PDT
The patch doesn't apply cleanly to 1.9.0; Wladimir, would you be willing to backport it there?
Comment 14 Wladimir Palant 2009-03-26 23:56:23 PDT
(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.
Comment 15 Boris Zbarsky [:bz] 2009-03-27 06:29:59 PDT
Hmm.  That check is there on trunk too, so we only really needed a content policy check in nsXULDocument, no?
Comment 16 Wladimir Palant 2009-03-27 06:45:42 PDT
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()).
Comment 17 Boris Zbarsky [:bz] 2009-03-27 06:53:14 PDT
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.
Comment 18 Wladimir Palant 2009-03-27 06:56:50 PDT
I don't really know much about fastloading but isn't it only used for chrome documents where we skip the checks anyway?
Comment 19 Boris Zbarsky [:bz] 2009-03-27 08:18:51 PDT
It's used for chrome scripts, which is not the same thing.
Comment 20 Boris Zbarsky [:bz] 2009-03-27 16:55:15 PDT
So it seems to me we should only do the content policy check in the XULDocument code.
Comment 21 Boris Zbarsky [:bz] 2009-04-03 12:40:49 PDT
Wladimir, do you want to do that, or should I?
Comment 22 Wladimir Palant 2009-04-04 08:12:33 PDT
I might be able to do this on Monday evening.
Comment 23 Wladimir Palant 2009-04-06 16:12:26 PDT
Created attachment 371330 [details] [diff] [review]
1.9.0 patch
Comment 24 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-04-22 16:34:05 PDT
Don't you need to call CheckLoadURIWithPrincipal as well?
Comment 25 Boris Zbarsky [:bz] 2009-04-22 18:27:45 PDT
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 26 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-04-23 01:13:55 PDT
Comment on attachment 371330 [details] [diff] [review]
1.9.0 patch

Makes sense
Comment 27 Daniel Veditz [:dveditz] 2009-05-01 10:47:07 PDT
Comment on attachment 371330 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.11, a=dveditz for release-drivers
Comment 28 Daniel Veditz [:dveditz] 2009-05-07 01:05:56 PDT
fix checked into the 1.9.0 branch
Comment 29 Boris Zbarsky [:bz] 2009-05-07 13:15:44 PDT
So should there be a followup on the fact that we now CheckLoadURI twice for XUL?
Comment 30 Wladimir Palant 2009-05-08 01:03:32 PDT
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?
Comment 31 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-05-08 18:33:13 PDT
Please.
Comment 32 Al Billings [:abillings] 2009-05-14 17:12:36 PDT
(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
----------------------------------------------------------
Comment 33 Boris Zbarsky [:bz] 2009-05-14 17:17:39 PDT
> 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).
Comment 34 Al Billings [:abillings] 2009-05-14 17:19:20 PDT
Doh. Apparently, I know nothing about content policy. I disabled through preferences.
Comment 35 Al Billings [:abillings] 2009-05-14 17:33:16 PDT
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
Comment 36 Karl Tomlinson (:karlt) 2009-05-18 01:11:34 PDT
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?
Comment 37 Daniel Veditz [:dveditz] 2009-05-29 10:55:18 PDT
Bug exists on the 1.8 branch.
Comment 38 Brandon Sterne (:bsterne) 2009-06-10 16:03:21 PDT
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.
Comment 39 Giorgio Maone [:mao] 2009-06-12 05:13:31 PDT
(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 Martin Stránský 2009-06-15 07:46:40 PDT
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...
Comment 41 Boris Zbarsky [:bz] 2011-03-18 19:51:34 PDT
I filed bug 643078 on comment 30.

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