477979 - (CVE-2009-1840) XUL scripts skip some security checks

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Wladimir Palant]

Attachment: 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]

Attachment: 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]]

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]]

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)]

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 [:bzbarsky]]

At least one of those casts isn't actually needed, is it?

Comment 6

[Wladimir Palant]

(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]

Attachment: 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]]

Is this something that should land on 1.9.1 as well?

Comment 9

[Wladimir Palant]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/80cd94e79e3f.

Comment 13

[Boris Zbarsky [:bzbarsky]]

The patch doesn't apply cleanly to 1.9.0; Wladimir, would you be willing to backport it there?

Comment 14

[Wladimir Palant]

(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 [:bzbarsky]]

Hmm.  That check is there on trunk too, so we only really needed a content policy check in nsXULDocument, no?

Comment 16

[Wladimir Palant]

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 [:bzbarsky]]

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]

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 [:bzbarsky]]

It's used for chrome scripts, which is not the same thing.

Comment 20

[Boris Zbarsky [:bzbarsky]]

So it seems to me we should only do the content policy check in the XULDocument code.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Wladimir, do you want to do that, or should I?

Comment 22

[Wladimir Palant]

I might be able to do this on Monday evening.

Comment 23

[Wladimir Palant]

Attachment: 1.9.0 patch

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Don't you need to call CheckLoadURIWithPrincipal as well?

Comment 25

[Boris Zbarsky [:bzbarsky]]

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]

Comment on attachment 371330 [details] [diff] [review]
1.9.0 patch

Makes sense

Comment 27

[Daniel Veditz [:dveditz]]

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]]

fix checked into the 1.9.0 branch

Comment 29

[Boris Zbarsky [:bzbarsky]]

So should there be a followup on the fact that we now CheckLoadURI twice for XUL?

Comment 30

[Wladimir Palant]

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: [e-mail]]]

Please.

Comment 32

[Al Billings [:abillings - ex-MoCo]]

(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 [:bzbarsky]]

> 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 - ex-MoCo]]

Doh. Apparently, I know nothing about content policy. I disabled through preferences.

Comment 35

[Al Billings [:abillings - ex-MoCo]]

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)]

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]]

Bug exists on the 1.8 branch.

Comment 38

[Brandon Sterne (:bsterne)]

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 [:ma1]]

(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ý [:stransky] (ni? me)]

Attachment: 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 [:bzbarsky]]

I filed bug 643078 on comment 30.