Last Comment Bug 441087 - NS_ERROR_XPC_SECURITY_MANAGER_VETO in Greasemonkey JavaScript eval containing object.property
: NS_ERROR_XPC_SECURITY_MANAGER_VETO in Greasemonkey JavaScript eval containing...
Status: VERIFIED FIXED
: regression, verified1.8.1.17, verified1.9.0.2, verified1.9.1
Product: Core
Classification: Components
Component: Security (show other bugs)
: 1.9.0 Branch
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
Depends on: 453310
Blocks: 422113
  Show dependency treegraph
 
Reported: 2008-06-21 20:09 PDT by Cacycle
Modified: 2009-02-05 22:33 PST (History)
15 users (show)
benjamin: blocking1.9.1+
mbeltzner: blocking1.9.0.2+
samuel.sidler+old: wanted1.9.0.x+
dveditz: blocking1.8.1.17+
samuel.sidler+old: wanted1.8.1.x+
asac: blocking1.8.0.next+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (1017 bytes, patch)
2008-07-14 09:55 PDT, Blake Kaplan (:mrbkap)
mrbkap: review-
Details | Diff | Splinter Review
Alternate fix (5.79 KB, patch)
2008-07-24 12:42 PDT, Blake Kaplan (:mrbkap)
jst: review+
bzbarsky: superreview+
samuel.sidler+old: approval1.9.0.2+
Details | Diff | Splinter Review
Possible 1.8 branch patch (16.57 KB, patch)
2008-08-15 16:50 PDT, Blake Kaplan (:mrbkap)
jst: review+
brendan: superreview+
samuel.sidler+old: approval1.8.1.17+
Details | Diff | Splinter Review
Last patch (17.74 KB, patch)
2008-08-18 14:49 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Lastest patch (1.40 KB, patch)
2008-08-27 11:06 PDT, Blake Kaplan (:mrbkap)
bzbarsky: review+
bzbarsky: superreview+
mbeltzner: approval1.8.1.17+
Details | Diff | Splinter Review
a backport to 1.8.0 line (6.19 KB, patch)
2008-09-15 03:29 PDT, Martin Stránský
igor: review+
Details | Diff | Splinter Review
1.8.0 (by stransky) (10.24 KB, patch)
2008-09-18 02:16 PDT, Alexander Sack
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description Cacycle 2008-06-21 20:09:14 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

In a Greasemonkey event handler routine under Firefox 3.0 with Greasemonkey 0.8.20080609.0 a NS_ERROR_XPC_SECURITY_MANAGER_VETO error is thrown if the eval statement contains an object.property in 'dot notation'.

var obj = event.currentTarget;
eval('javascript:var test = obj;'); // works

var objId = event.currentTarget.id;
eval('javascript:var test = objId;'); // works

eval('javascript:var test = obj.id;'); // throws NS_ERROR_XPC_SECURITY_MANAGER_VETO error

eval('javascript:var test = event;'); // works

eval('javascript:var test = event.target;'); // throws NS_ERROR_XPC_SECURITY_MANAGER_VETO error

This bug broke the button functionality of the MediaWiki editor wikEd <= 0.9.36b (http://en.wikipedia.org/wiki/User:Cacycle/wikEd) on http://lyricwiki.org/, but  but not on http://en.wikipedia.org/wiki/. 

The full error message is:

Error: [Exception... "Security Manager vetoed action"  nsresult: "0x80570027 (NS_ERROR_XPC_SECURITY_MANAGER_VETO)"  location: "JS frame :: http://lyricwiki.org/index.php?title=Blind_Faith:Had_To_Cry_Today&action=submit :: anonymous :: line 0"  data: no]
Source File: http://lyricwiki.org/index.php?title=Blind_Faith:Had_To_Cry_Today&action=submit
Line: 0



Reproducible: Always
Comment 1 Cacycle 2008-06-21 20:41:18 PDT
There is the same problem with object properties in "square bracket notation".
Comment 2 Cacycle 2008-06-21 22:27:14 PDT
It works fine outside the event handler. It seems to be broken on every non-Wikipedia wiki including wikia.com. For a testcase install the most recent Greasemonkey and the following Greasemonkey script: http://en.wikipedia.org/w/index.php?action=raw&ctype=text/javascript&title=User:Cacycle/wikEd.user.js&oldid=220722290. Then open a MediaWiki edit page such as http://lyricwiki.org/index.php?title=Blind_Faith&action=edit. The error is thrown when an edit button is pushed, e.g. the <b>B</b> (bold) button.
Comment 3 Daniel Veditz [:dveditz] 2008-07-08 12:02:10 PDT
Have you tried a more recent 2.0.0.x release? The same security fix that introduced this exception was later added to the 2.0.0.x releases.
Comment 4 Daniel Veditz [:dveditz] 2008-07-08 12:04:31 PDT
I think the fact that you're hitting this exception means you were unintentionally relying on a security hole in the past, although it might be an unintended regression we can fix. Need to figure out which.
Comment 5 Blake Kaplan (:mrbkap) 2008-07-14 09:41:13 PDT
I have a fix for this, which I'll attach in a minute. I think the fix is safe because GM provides an explicit XPCNativeWrapper to provide security and doesn't rely on implicit ones. Is that right?
Comment 6 Blake Kaplan (:mrbkap) 2008-07-14 09:55:03 PDT
Created attachment 329475 [details] [diff] [review]
Proposed fix

Passing null as the filename causes us to use the codebase principal's codebase as the filename.
Comment 7 moz_bug_r_a4 2008-07-17 21:51:11 PDT
(In reply to comment #5)
> I have a fix for this, which I'll attach in a minute. I think the fix is safe
> because GM provides an explicit XPCNativeWrapper to provide security and
> doesn't rely on implicit ones. Is that right?
> 

GM user scripts rely on implicit XPCNativeWrappers when accessing Event
objects.  Thus, with that patch, the following user script is unsafe.  (It
allows a web page to access a sandbox and abuse GM_* API functions.)

window.addEventListener("load", funcion(ev) {
  var t = ev.target;
}, false);
Comment 8 Blake Kaplan (:mrbkap) 2008-07-18 03:32:14 PDT
Comment on attachment 329475 [details] [diff] [review]
Proposed fix

Based on comment 7, we don't want this. I'll take a different tack next.
Comment 9 Blake Kaplan (:mrbkap) 2008-07-24 12:42:27 PDT
Created attachment 331171 [details] [diff] [review]
Alternate fix

jst, what do you think? This won't hand out implicit XPCNativeWrappers to content code, avoiding these "spurious" errors. The downside that I see here is expandos might seem to randomly disappear if a script doesn't keep a reference to a given wrapper.
Comment 10 Boris Zbarsky [:bz] 2008-08-04 14:01:38 PDT
In what situation will the expando behavior change, exactly?
Comment 11 Blake Kaplan (:mrbkap) 2008-08-04 15:15:50 PDT
In investigating the answer to that question, I found that this isn't as bad as I originally though. My first guess was that we'd break the testcase in GreaseMonkey:

  d = document.getElementById("d")
  d.flarpy = 5;

  d = document.getElementById("d");
  alert(d.flarpy == 5);

but that already fails because we find 'document' on the explicit XPCNativeWrapper GreaseMonkey provides its scrips. Because of this I think that the change in behavior (from GreaseMonkey's view) is limited to the event parameter to event listeners, where I don't think expandos are common.

However, if that code appeared in, e.g. browser.js and a content page did <script src=chrome://.../browser.js></script>, it'd alert "false."
Comment 12 Boris Zbarsky [:bz] 2008-08-04 23:35:56 PDT
Comment on attachment 331171 [details] [diff] [review]
Alternate fix

I think I can live with that, though we need to document this very clearly on devmo.
Comment 13 georgi - hopefully not receiving bugspam 2008-08-05 00:03:00 PDT
> content page did <script src=chrome://.../browser.js></script>, it'd alert "false."

should content page be able to do this at all?
Comment 14 Boris Zbarsky [:bz] 2008-08-05 00:14:28 PDT
Given the silly structure of the Firefox chrome, yes.  Said structure needs to be fixed.
Comment 15 georgi - hopefully not receiving bugspam 2008-08-05 00:35:45 PDT
i can't imagine *any* legitimate use of content accessing chrome scripts except maybe for xbl
Comment 16 Boris Zbarsky [:bz] 2008-08-05 09:17:32 PDT
I didn't say it's legitimate, just that that file is in a "accessible to content" chrome package.  It shouldn't be.
Comment 17 georgi - hopefully not receiving bugspam 2008-08-06 00:04:53 PDT
i don't understand the the full details.

since in this case there are no legitimate uses, "script src=chrome:" can be globally killed without caring about packing chrome?

for the remaining cases (when somebody believes chrome should be accessed from content) wouldn't it better to disallow all access to chrome, then specifically whitelisting potential legitimate access to chrome (i suspect xbl must be the only exception and it may be killed by another bug)?
Comment 18 Boris Zbarsky [:bz] 2008-08-06 00:12:05 PDT
> since in this case there are no legitimate uses, "script src=chrome:" 

Your "no legitimate uses" assumption happens to be wrong.  And this is all covered in the relevant bug.  Which I know you're cced on.  So why are we even polluting this bug with it?
Comment 19 Blake Kaplan (:mrbkap) 2008-08-11 16:04:06 PDT
Pushed as http://hg.mozilla.org/index.cgi/mozilla-central/rev/4593610d9715
Comment 20 Blake Kaplan (:mrbkap) 2008-08-14 17:33:39 PDT
Comment on attachment 331171 [details] [diff] [review]
Alternate fix

This patch applies as-is the the 1.9 branch.
Comment 21 Samuel Sidler (old account; do not CC) 2008-08-14 17:51:57 PDT
Comment on attachment 331171 [details] [diff] [review]
Alternate fix

Two down, one to go!

Approved for 1.9.0.2. Please land in CVS. a=ss
Comment 22 Blake Kaplan (:mrbkap) 2008-08-15 15:53:39 PDT
Fix checked into the 1.9 branch.
Comment 23 Blake Kaplan (:mrbkap) 2008-08-15 16:50:26 PDT
Created attachment 334050 [details] [diff] [review]
Possible 1.8 branch patch

Here's a potential backport to the 1.8 branch. Unfortunately, I'm having trouble testing this testcase in particular (or any testcase involving GreaseMonkey), but I've verified this works by a couple of other testcases.
Comment 24 Brendan Eich [:brendan] 2008-08-15 17:07:25 PDT
Comment on attachment 334050 [details] [diff] [review]
Possible 1.8 branch patch

rs=me based on mrbkap's grep of callee assignments piped to wc -l matching trunk.

/be
Comment 25 Johnny Stenback (:jst, jst@mozilla.com) 2008-08-15 23:19:34 PDT
Comment on attachment 334050 [details] [diff] [review]
Possible 1.8 branch patch

r=jst, sorry for not getting to this sooner.
Comment 26 Samuel Sidler (old account; do not CC) 2008-08-16 14:45:56 PDT
Comment on attachment 334050 [details] [diff] [review]
Possible 1.8 branch patch

Approved for 1.8.1.17. a=ss
Comment 27 Blake Kaplan (:mrbkap) 2008-08-18 14:49:28 PDT
Created attachment 334345 [details] [diff] [review]
Last patch

I'm going to assume {{,s}r,a}+. The only difference is this patch remembers to mark the callee object in the frame.
Comment 28 Blake Kaplan (:mrbkap) 2008-08-18 14:50:18 PDT
Fix checked into the 1.8 branch.
Comment 29 moz_bug_r_a4 2008-08-27 04:17:27 PDT
This bug is not fixed on fx-2.0.0.17pre-2008-08-26-03.

From Last patch:

>-XPCNativeWrapper::GetNewOrUsed(JSContext *cx, XPCWrappedNative *wrapper)
>+XPCNativeWrapper::GetNewOrUsed(JSContext *cx, XPCWrappedNative *wrapper,
>+                               JSObject *callee)
> {
>+  if (callee && PR_FALSE) {

This if block never be executed.

>+    if (NS_SUCCEEDED(rv) && prin) {
>+      nsCOMPtr<nsIPrincipal> sysprin;
>+      rv = ssm->GetSystemPrincipal(getter_AddRefs(sysprin));
>+      if (NS_SUCCEEDED(rv) && sysprin == prin) {

Is |sysprin == prin| OK?  The trunk code is |!isSystem|.
Comment 30 georgi - hopefully not receiving bugspam 2008-08-27 04:44:13 PDT
>>+  if (callee && PR_FALSE) {

>This if block never be executed.

some versions of gcc will optimize this away at compile time and won't even include it at all.
Comment 31 Boris Zbarsky [:bz] 2008-08-27 09:23:57 PDT
Unsetting branch fixed flag, per comment 29.
Comment 32 Blake Kaplan (:mrbkap) 2008-08-27 09:44:28 PDT
Ugh, crap. Sorry. I was debugging a problem I saw trying to verify this bug and forgot to remove that :-/
Comment 33 Samuel Sidler (old account; do not CC) 2008-08-27 09:45:26 PDT
Blake, let's get a fix asap.
Comment 34 Blake Kaplan (:mrbkap) 2008-08-27 09:47:15 PDT
(In reply to comment #29)
> Is |sysprin == prin| OK?  The trunk code is |!isSystem|.

Yeah. On the 1.9 branch, we've added nsIScriptSecurityManager::IsSystemPrincipal which performs this check without the need to actually addref the system principal singleton.
Comment 35 Boris Zbarsky [:bz] 2008-08-27 10:04:07 PDT
I think moz_bug_r's point was that we want != there instead of == to match what the 1.9 code does.
Comment 36 Blake Kaplan (:mrbkap) 2008-08-27 11:06:06 PDT
Created attachment 335735 [details] [diff] [review]
Lastest patch

Sigh, yeah.
Comment 37 Blake Kaplan (:mrbkap) 2008-08-27 11:07:02 PDT
Comment on attachment 335735 [details] [diff] [review]
Lastest patch

Clearly, I hate the 1.8 branch.
Comment 38 Boris Zbarsky [:bz] 2008-08-27 11:10:25 PDT
Comment on attachment 335735 [details] [diff] [review]
Lastest patch

Right.  Code auditing says this is good.  I think.  ;)
Comment 39 Mike Beltzner [:beltzner, not reading bugmail] 2008-08-27 11:21:31 PDT
Comment on attachment 335735 [details] [diff] [review]
Lastest patch

a=beltzner, please land ASAP
Comment 40 Blake Kaplan (:mrbkap) 2008-08-27 11:27:28 PDT
Landed: new revision: 1.31.2.22
Comment 41 Al Billings [:abillings] 2008-09-02 16:38:24 PDT
What is a good way to test this in 1.8.1.17? It isn't immediately clear.
Comment 42 Al Billings [:abillings] 2008-09-02 16:44:51 PDT
I've verified bug 444073 but it isn't clear that it covers this here.
Comment 43 Blake Kaplan (:mrbkap) 2008-09-02 16:56:03 PDT
Al, take a look at the first sentence in the initial description of bug 444073. The reporter there is spot-on (as always).
Comment 44 Al Billings [:abillings] 2008-09-02 16:57:46 PDT
I saw that before I commented. The reporter's (valued) opinion is not a specific confirmation that verifying it in that bug covers this as well. If you agree that it does, I will mark this bug appropriately since I checked that one already.
Comment 45 Blake Kaplan (:mrbkap) 2008-09-02 17:34:43 PDT
Sorry, I meant to be more explicit in my previous statement. The two bugs in question are exactly the same.
Comment 46 Al Billings [:abillings] 2008-09-02 17:37:06 PDT
This is verified then with build 2 of 2.0.0.17.
Comment 47 Al Billings [:abillings] 2008-09-05 16:58:56 PDT
Verified for 1.9.0.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2) Gecko/2008090212 Firefox/3.0.2.
Comment 48 Martin Stránský 2008-09-15 03:29:44 PDT
Created attachment 338614 [details] [diff] [review]
a backport to 1.8.0 line

Igor, can you please check this backport? Thanks.
Comment 49 Alexander Sack 2008-09-18 02:16:54 PDT
Created attachment 339213 [details] [diff] [review]
1.8.0 (by stransky)

a=asac for 1.8.0.15
Comment 50 Benjamin Smedberg [:bsmedberg] 2009-01-27 17:26:13 PST
Landed before branching for 1.9.1
Comment 51 Tony Chung [:tchung] 2009-02-05 22:33:56 PST
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090205 Shiretoko/3.1b3pre Ubiquity/0.1.5

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