NS_ERROR_XPC_SECURITY_MANAGER_VETO in Greasemonkey JavaScript eval containing object.property

VERIFIED FIXED

Status

()

VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: cacyclewp, Assigned: mrbkap)

Tracking

(4 keywords)

1.9.0 Branch
x86
Windows XP
regression, verified1.8.1.17, verified1.9.0.2, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.2 +
wanted1.9.0.x +
blocking1.8.1.17 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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
Component: General → Security
Product: Firefox → Core
QA Contact: general → toolkit
Version: unspecified → 1.9.0 Branch
(Reporter)

Comment 1

11 years ago
There is the same problem with object properties in "square bracket notation".
(Reporter)

Comment 2

11 years ago
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.
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
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?
Posted patch Proposed fix (obsolete) — Splinter Review
Passing null as the filename causes us to use the codebase principal's codebase as the filename.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #329475 - Flags: superreview?(jst)
Attachment #329475 - Flags: review?(jst)
Attachment #329475 - Flags: superreview?(jst)
Attachment #329475 - Flags: superreview+
Attachment #329475 - Flags: review?(jst)
Attachment #329475 - Flags: review+

Comment 7

11 years ago
(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 on attachment 329475 [details] [diff] [review]
Proposed fix

Based on comment 7, we don't want this. I'll take a different tack next.
Attachment #329475 - Flags: superreview+
Attachment #329475 - Flags: review-
Attachment #329475 - Flags: review+
Posted patch Alternate fixSplinter Review
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.
Attachment #329475 - Attachment is obsolete: true
Attachment #331171 - Flags: review?
Attachment #331171 - Flags: review? → review?(jst)
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.2?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.17?
Flags: blocking1.9.0.2? → blocking1.9.0.2+
Attachment #331171 - Flags: superreview?(bzbarsky)
Flags: blocking1.8.1.17? → blocking1.8.1.17+
In what situation will the expando behavior change, exactly?
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 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.
Attachment #331171 - Flags: superreview?(bzbarsky) → superreview+
> content page did <script src=chrome://.../browser.js></script>, it'd alert "false."

should content page be able to do this at all?
Given the silly structure of the Firefox chrome, yes.  Said structure needs to be fixed.
i can't imagine *any* legitimate use of content accessing chrome scripts except maybe for xbl
I didn't say it's legitimate, just that that file is in a "accessible to content" chrome package.  It shouldn't be.
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)?
> 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?
Attachment #331171 - Flags: review?(jst) → review+
Pushed as http://hg.mozilla.org/index.cgi/mozilla-central/rev/4593610d9715
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Comment on attachment 331171 [details] [diff] [review]
Alternate fix

This patch applies as-is the the 1.9 branch.
Attachment #331171 - Flags: approval1.9.0.2?
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
Attachment #331171 - Flags: approval1.9.0.2? → approval1.9.0.2+
Fix checked into the 1.9 branch.
Keywords: fixed1.9.0.2
Posted patch Possible 1.8 branch patch (obsolete) — Splinter Review
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.
Attachment #334050 - Flags: superreview?(brendan)
Attachment #334050 - Flags: review?(jst)
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
Attachment #334050 - Flags: superreview?(brendan) → superreview+
Comment on attachment 334050 [details] [diff] [review]
Possible 1.8 branch patch

r=jst, sorry for not getting to this sooner.
Attachment #334050 - Flags: review?(jst) → review+
Attachment #334050 - Flags: approval1.8.1.17?
Comment on attachment 334050 [details] [diff] [review]
Possible 1.8 branch patch

Approved for 1.8.1.17. a=ss
Attachment #334050 - Flags: approval1.8.1.17? → approval1.8.1.17+
Posted patch Last patchSplinter Review
I'm going to assume {{,s}r,a}+. The only difference is this patch remembers to mark the callee object in the frame.
Attachment #334050 - Attachment is obsolete: true
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1.17

Comment 29

11 years ago
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|.
>>+  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.
Unsetting branch fixed flag, per comment 29.
Keywords: fixed1.8.1.17
Ugh, crap. Sorry. I was debugging a problem I saw trying to verify this bug and forgot to remove that :-/
Blake, let's get a fix asap.
(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.
I think moz_bug_r's point was that we want != there instead of == to match what the 1.9 code does.
Posted patch Lastest patchSplinter Review
Sigh, yeah.
Attachment #335735 - Flags: superreview?(bzbarsky)
Attachment #335735 - Flags: review?(bzbarsky)
Attachment #335735 - Flags: approval1.9.0.2?
Comment on attachment 335735 [details] [diff] [review]
Lastest patch

Clearly, I hate the 1.8 branch.
Attachment #335735 - Flags: approval1.9.0.2? → approval1.8.1.17?
Comment on attachment 335735 [details] [diff] [review]
Lastest patch

Right.  Code auditing says this is good.  I think.  ;)
Attachment #335735 - Flags: superreview?(bzbarsky)
Attachment #335735 - Flags: superreview+
Attachment #335735 - Flags: review?(bzbarsky)
Attachment #335735 - Flags: review+
Comment on attachment 335735 [details] [diff] [review]
Lastest patch

a=beltzner, please land ASAP
Attachment #335735 - Flags: approval1.8.1.17? → approval1.8.1.17+
Landed: new revision: 1.31.2.22
Keywords: fixed1.8.1.17
Depends on: 453310
What is a good way to test this in 1.8.1.17? It isn't immediately clear.
I've verified bug 444073 but it isn't clear that it covers this here.
Al, take a look at the first sentence in the initial description of bug 444073. The reporter there is spot-on (as always).
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.
Sorry, I meant to be more explicit in my previous statement. The two bugs in question are exactly the same.
This is verified then with build 2 of 2.0.0.17.
Keywords: fixed1.8.1.17 → verified1.8.1.17
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.2 → verified1.9.0.2
Igor, can you please check this backport? Thanks.
Attachment #338614 - Flags: review?
Attachment #338614 - Flags: review? → review?(igor)

Updated

11 years ago
Attachment #338614 - Flags: review?(igor) → review+

Comment 49

11 years ago
a=asac for 1.8.0.15
Attachment #339213 - Flags: approval1.8.0.15+

Updated

11 years ago
Flags: blocking1.8.0.15+

Comment 50

10 years ago
Landed before branching for 1.9.1
Flags: blocking1.9.1+
Keywords: fixed1.9.1
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
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.