Last Comment Bug 498897 - (CVE-2009-2665) Web code gets "permission denied" error if a content policy is present
(CVE-2009-2665)
: Web code gets "permission denied" error if a content policy is present
Status: RESOLVED FIXED
[sg:critical?][1.9.1 fix in bug 49991...
: regression
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 Windows XP
: -- major (vote)
: mozilla1.9.2a1
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
http://market.yandex.ru/guru.xml?CMD=...
Depends on:
Blocks: abp 457022 499910
  Show dependency treegraph
 
Reported: 2009-06-17 07:21 PDT by Wladimir Palant
Modified: 2009-09-18 14:18 PDT (History)
21 users (show)
mbeltzner: blocking1.9.1.1-
mbeltzner: wanted1.9.1.x+
dveditz: wanted1.9.0.x-
peterv: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2+
.2-fixed


Attachments
Minimal content policy for testing (1.74 KB, text/plain)
2009-06-17 07:21 PDT, Wladimir Palant
no flags Details
Testcase (231 bytes, text/html)
2009-06-17 22:45 PDT, Wladimir Palant
no flags Details
Backtraces showing the problem (6.44 KB, text/plain)
2009-06-18 05:09 PDT, Peter Van der Beken [:peterv]
no flags Details
v1 (665 bytes, patch)
2009-06-18 05:38 PDT, Peter Van der Beken [:peterv]
peterv: review-
Details | Diff | Review
Mochitest (5.08 KB, patch)
2009-06-18 08:32 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Review
v2 (2.06 KB, patch)
2009-06-18 14:35 PDT, Peter Van der Beken [:peterv]
mrbkap: review+
mrbkap: superreview+
Details | Diff | Review

Description Wladimir Palant 2009-06-17 07:21:02 PDT
Created attachment 383707 [details]
Minimal content policy for testing

I reproduced this issue with 20090616031659 Minefield nightly and with 20090617032001 Shiretoko nightly, everything works fine in Firefox 3.0.11 however. Functionality of market.yandex.ru website is broken if any JavaScript-based content policy is present. In particular, the panel on the right (the one with orange border) doesn't load. I see two errors in the console:

Error: Permission denied for <http://market.yandex.ru> to call method HTMLDocument.createComment on <>.
Source File: http://js.static.yandex.net/jquery/1.3.2/_jquery.js
Line: 19

Error: Permission denied for <http://market.yandex.ru> to get property HTMLDocument.compatMode from <>.
Source File: http://lego.static.yandex.net/2.0/common/js/_common.js
Line: 1

There are more errors but I guess that those are follow-up errors. The website uses http://suggest.market.yandex.ru/jquery.crossframeajax.html frame for cross-site communication, I guess that's where the errors come from. I will try to analyze further but the way this site works is pretty complex (packed jQuery doesn't make it easier either).

Steps to reproduce:
1. Download nsTestPolicy.js from the attachment and put it into "c:\Program files\Mozilla Firefox\components" directory (adjust the path if the Firefox installation directory is different for you).
2. Put an .autoreg file into your Firefox profile.
3. Start Firefox.
4. Go to http://market.yandex.ru/guru.xml?CMD=-RR=0,0,0,0-VIS=160-CAT_ID=754893-EXC=1-PG=50&view=&num=&sort=&hid=91020&filter=&grhow=

Observed results:
Panel on the right shows a progress indicator that never goes away.

Expected results:
The panel on the right should load eventually (usually within a few seconds).

Requesting blocking1.9.1 because this doesn't seem to be limited to this website or even to jQuery.
Comment 1 Wladimir Palant 2009-06-17 07:35:31 PDT
I stand corrected, typing "javascript:alert(document.createComment())" into the location bar on this webpage produces the same exception (document.createElement() can be used however). The website cannot access its own document for some reason.
Comment 2 Wladimir Palant 2009-06-17 07:57:32 PDT
I traced it back to the http://bs.yandex.ru/resource/watch.js script - having only this script on the page already causes the issue. So not related to jQuery after all.
Comment 3 Mike Beltzner [:beltzner, not reading bugmail] 2009-06-17 08:14:27 PDT
Wladimir: in comment 0 you say that this isn't limited to the website or jQuery, and then in comment 2 you say that it's limited to a single script. Which is it?
Comment 4 Mike Beltzner [:beltzner, not reading bugmail] 2009-06-17 08:22:02 PDT
(Basically, I'm trying to figure out what the effect of this bug is in terms of how many sites/users it will hit. So far it sounds very very niche)
Comment 5 Wladimir Palant 2009-06-17 08:24:40 PDT
I managed to reduce the testcase to the following:

<script type="text/javascript">
    new function () {
        function foo() {}
    };
</script>

It is hard to test this because there is some weird kind of persistence - once the bug manifests itself it continues manifesting itself on that host and somehow survives even browser restarts (???).

Moving to JavaScript Engine, seems to belong there.

(In reply to comment #3)
> Wladimir: in comment 0 you say that this isn't limited to the website or
> jQuery, and then in comment 2 you say that it's limited to a single script.
> Which is it?

Apparently the former. The minimal testcase is very generic.
Comment 6 Mike Beltzner [:beltzner, not reading bugmail] 2009-06-17 10:18:06 PDT
And what's the effect on users?
Comment 7 Wladimir Palant 2009-06-17 12:52:52 PDT
The effect on users is broken functionality on any site that uses a construct like this, that would typically be JavaScript-heavy sites. I am not sure why only some DOM methods are affected however - e.g. document.createComment() won't work while document.createElement() is fine. Assuming that an extension using content policies is installed (that includes Adblock Plus, NoScript, GreaseMonkey, Google Toolbar and a bunch of other extensions).
Comment 8 Boris Zbarsky [:bz] 2009-06-17 14:38:30 PDT
createComment is not quickstubbed, while createElement is.  So createElement doesn't do a call-time security check, while createComment does.

Wladimir, I'm not going to be able to look into this until tomorrow; not sure about Blake.  Do you think you can hunt down a regression range in the meantime?  This sounds like something goes wrong with our wrapperization to me.
Comment 9 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-06-17 14:52:11 PDT
Yeah, doubt this is a JS engine problem proper.
Comment 10 Mike Beltzner [:beltzner, not reading bugmail] 2009-06-17 19:31:31 PDT
I'm still having trouble understanding what the practical impact of this is. Can someone tell me about an add-on / website pair that has broken functionality, and how that brokenness manifests itself?
Comment 11 Wladimir Palant 2009-06-17 22:45:06 PDT
Created attachment 383874 [details]
Testcase

Attaching testcase. New steps to reproduce:

1. Download nsTestPolicy.js from attachment 383707 [details] and put it into "c:\Program
files\Mozilla Firefox\components" directory (adjust the path if the Firefox
installation directory is different for you).
2. Put an .autoreg file into your Firefox profile.
3. Start Firefox.
4. Open the testcase.

Observe an error message - document.createComment() couldn't be called. Will show "Success" in Firefox 3.0.11, that's the correct behavior.

I'm not sure any more that the code before the try..catch block is even needed to reproduce the problem. It seems that document.createComment() is broken most of the time but might work eventually - I simply don't understand under which conditions it is working. I originally thought about some kind of persistence but starting Firefox with an empty profile didn't change anything.
Comment 12 Wladimir Palant 2009-06-17 23:04:32 PDT
The testcase gives me "Success" from Bugzilla but "Error" from localhost. I can only guess that the bug manifests on plain HTTP.
Comment 13 Wladimir Palant 2009-06-17 23:22:58 PDT
Regression range is 925fe560ecce to dc0e07e3d380: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=925fe560ecce&tochange=dc0e07e3d380

Looks like this was caused by bug 457022.
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2009-06-17 23:30:06 PDT
So, to be clear, this is something that's been broken since before we branched, and we're only noticing now? To me that's a pretty strong indicator that it needn't block, though should likely be fixed in a stability increase.

bz/jonas/jst: agree?
Comment 15 Jonas Sicking (:sicking) 2009-06-18 01:22:53 PDT
One thing that may have made it harder for people to run into this is that you need a scripted contentpolicy installed. So you need to have something like adblockplus or noscript installed. Have those been working for a long time on the 1.9.1 branch?
Comment 16 Jonas Sicking (:sicking) 2009-06-18 01:23:16 PDT
I'm very concerned that this is a security problem. If we're handing back some sort of wrapper here, then it's quite likely that this can be exploited by the page.
Comment 17 Wladimir Palant 2009-06-18 01:45:01 PDT
Adblock Plus was always compatible with Firefox nightlies, same with NoScript from what I can tell. So it is very surprising that I got the first report about this issue only yesterday (less so that this went unnoticed on NoScript's side - breaking websites is the expected side-effect there).
Comment 18 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-06-18 01:48:09 PDT
I, too, am very concerned about the possibility for exploits here. In particular, we're handing back an XPCWrappedNative for the document that is parented to a chrome scope. We don't know of any current exploits for this, but it's scary...
Comment 19 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-06-18 01:49:48 PDT
(In reply to comment #15)
> One thing that may have made it harder for people to run into this is that you
> need a scripted contentpolicy installed. So you need to have something like
> adblockplus or noscript installed. Have those been working for a long time on
> the 1.9.1 branch?

This also requires a "link" HTTP header that links to something requiring a call to the content policy.
Comment 20 Jonas Sicking (:sicking) 2009-06-18 02:04:57 PDT
(In reply to comment #19)
> This also requires a "link" HTTP header that links to something requiring a
> call to the content policy.

While I agree that that should be the case, the attached testcase doesn't seem need this. But I can't reproduce on the attached testcase, possibly due to it being https (according to comment 12), though not sure why that would be.

Wladimir, you can for sure reproduce with the testcase as attached when loaded through http, right?
Comment 21 Wladimir Palant 2009-06-18 02:20:03 PDT
(In reply to comment #19)
> This also requires a "link" HTTP header that links to something requiring a
> call to the content policy.

Ouch, things suddenly start to make sense. Yes, I have a Link HTTP header on localhost, left-over from testing something else. This explains why the problem isn't reproducible in Bugzilla.

Not blocking then. However, if web content gets a cached wrapper meant for chrome code (content policy), this might have security implications.
Comment 22 Peter Van der Beken [:peterv] 2009-06-18 05:09:09 PDT
Created attachment 383901 [details]
Backtraces showing the problem

What's happening is that we create a wrapper for the document at a point where it doesn't have a mScopeObject yet, so nsNodeSH::PreCreate just returns the global object that it gets passed in as the parent. In this case that's the content policy component's global object. The first backtrace is where we create the wrapper for the document, the second is where it gets its mScopeObject set.
Comment 23 Peter Van der Beken [:peterv] 2009-06-18 05:38:05 PDT
Created attachment 383906 [details] [diff] [review]
v1

We could do this, it seems to fix the problem for me. Blake, what do you think? The old wrapper will stick around in the hash for the wrong scope, but that was already the case before. If we want to fix that we'll need to somehow get the right parent from the start in the PreCreate hook.
Comment 24 Wladimir Palant 2009-06-18 05:54:34 PDT
Yandex "fixed" the problem on their side by removing the "Link" header, their site won't work as a testcase any more.
Comment 25 Mike Beltzner [:beltzner, not reading bugmail] 2009-06-18 07:49:25 PDT
Leaving on the blocking list for a 1.9.1.x release.
Comment 26 Peter Van der Beken [:peterv] 2009-06-18 08:31:02 PDT
Comment on attachment 383906 [details] [diff] [review]
v1

This sometimes triggers the "There's a wrapper in the hashtable but it wasn't cached?" assertion.
Comment 27 Peter Van der Beken [:peterv] 2009-06-18 08:32:06 PDT
Created attachment 383939 [details] [diff] [review]
Mochitest
Comment 28 Jonas Sicking (:sicking) 2009-06-18 09:16:37 PDT
Marking security sensitive since it probably is.
Comment 29 Peter Van der Beken [:peterv] 2009-06-18 12:58:50 PDT
(In reply to comment #23)
> The old wrapper will stick around in the hash for the wrong scope, but that was
> already the case before.

We might need to just remove the "There's a wrapper in the hashtable but it wasn't cached?" assertion. :-/

> If we want to fix that we'll need to somehow get the
> right parent from the start in the PreCreate hook.

This won't work since we need the inner window, which probably doesn't exist yet when we call the content policy.
Comment 30 Boris Zbarsky [:bz] 2009-06-18 13:06:23 PDT
Another option, as I suggested to Blake, is to do those Link loads later...
Comment 31 Peter Van der Beken [:peterv] 2009-06-18 14:35:38 PDT
Created attachment 383995 [details] [diff] [review]
v2

Thanks to blake for the idea.
I also think it might be good to move the loading of those links to a later point, maybe we should file a bug for that?
Comment 32 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-06-18 15:04:02 PDT
Comment on attachment 383995 [details] [diff] [review]
v2

Can you use nsContentUtils::GetContextAndScopes? r+sr=mrbkap either way.
Comment 33 Peter Van der Beken [:peterv] 2009-06-19 05:11:25 PDT
(In reply to comment #32)
> Can you use nsContentUtils::GetContextAndScopes? r+sr=mrbkap either way.

Not really, there's only one document (which might not have an "old" scope object either).
Comment 34 Peter Van der Beken [:peterv] 2009-06-21 10:55:04 PDT
http://hg.mozilla.org/mozilla-central/rev/dca035c2bb32

Still working on the testcase, it works when running only this test but not when running all mochitests.
Comment 35 Peter Van der Beken [:peterv] 2009-06-23 06:54:09 PDT
Landed a corrected mochitest: http://hg.mozilla.org/mozilla-central/rev/628ac996f472
Comment 36 Daniel Veditz [:dveditz] 2009-06-23 09:31:58 PDT
(In reply to comment #28)
> Marking security sensitive since it probably is.

proven true in bug 499910
Comment 37 Mike Beltzner [:beltzner, not reading bugmail] 2009-07-14 14:10:01 PDT
mrbkap says that we can wait until 1.9.1.2 if that's coming in the next few weeks, which it is!
Comment 38 Samuel Sidler (old account; do not CC) 2009-07-21 17:35:17 PDT
Blake: Is this patch ready for 1.9.1? Please request approval on it.
Comment 39 Samuel Sidler (old account; do not CC) 2009-07-21 17:35:52 PDT
And when I say "Blake", I mean "Peter". :)
Comment 40 Peter Van der Beken [:peterv] 2009-07-23 00:39:12 PDT
We won't be taking this patch for 1.9.1, we're going with the patch in bug 499910 which also fixes this bug.
Comment 41 Daniel Veditz [:dveditz] 2009-07-27 23:18:05 PDT
Which is now checked in -- fixed for 1.9.1.2
Comment 42 juan becerra [:juanb] 2009-07-30 19:27:38 PDT
I tried reproducing the problem using the test case in comment #11 on 3.5, loading the test case from bugzilla and localhost. In both I got "Success." The same thing happens on 3.5.2.

Wladimir, or anyone, could you help us verify this?
Comment 43 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-07-31 13:41:07 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2

Original test case works for me. Don't want to step on Juan's toes here...verified1.9.1?
Comment 44 Nochum Sossonko [:Natch] 2009-08-05 13:11:20 PDT
See comment 11 and comment 24, Yandex fixed the issue on their side, it seems you need to find a site with a "Link" header that triggers the content policy.

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