NS_ERROR_XPC_SECURITY_MANAGER_VETO in Greasemonkey JavaScript eval containing object.property

VERIFIED FIXED

Status

()

Core
Security
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Cacycle, 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

9 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

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

Comment 2

9 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
(Assignee)

Comment 5

9 years ago
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?
(Assignee)

Comment 6

9 years ago
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.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #329475 - Flags: superreview?(jst)
Attachment #329475 - Flags: review?(jst)

Updated

9 years ago
Attachment #329475 - Flags: superreview?(jst)
Attachment #329475 - Flags: superreview+
Attachment #329475 - Flags: review?(jst)
Attachment #329475 - Flags: review+

Comment 7

9 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);
(Assignee)

Comment 8

9 years ago
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+
(Assignee)

Comment 9

9 years ago
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.
Attachment #329475 - Attachment is obsolete: true
Attachment #331171 - Flags: review?
(Assignee)

Updated

9 years ago
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+
(Assignee)

Updated

9 years ago
Attachment #331171 - Flags: superreview?(bzbarsky)
Flags: blocking1.8.1.17? → blocking1.8.1.17+

Comment 10

9 years ago
In what situation will the expando behavior change, exactly?
(Assignee)

Comment 11

9 years ago
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

9 years ago
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?

Comment 14

9 years ago
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

Comment 16

9 years ago
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)?

Comment 18

9 years ago
> 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?

Updated

9 years ago
Attachment #331171 - Flags: review?(jst) → review+
(Assignee)

Comment 19

9 years ago
Pushed as http://hg.mozilla.org/index.cgi/mozilla-central/rev/4593610d9715
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

9 years ago
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+
(Assignee)

Comment 22

9 years ago
Fix checked into the 1.9 branch.
Keywords: fixed1.9.0.2
(Assignee)

Comment 23

9 years ago
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.
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+
Blocks: 422113
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+
(Assignee)

Updated

9 years ago
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+
(Assignee)

Comment 27

9 years ago
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.
Attachment #334050 - Attachment is obsolete: true
(Assignee)

Comment 28

9 years ago
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1.17

Comment 29

9 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.

Comment 31

9 years ago
Unsetting branch fixed flag, per comment 29.
Keywords: fixed1.8.1.17
(Assignee)

Comment 32

9 years ago
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.
(Assignee)

Comment 34

9 years ago
(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

9 years ago
I think moz_bug_r's point was that we want != there instead of == to match what the 1.9 code does.
(Assignee)

Comment 36

9 years ago
Created attachment 335735 [details] [diff] [review]
Lastest patch

Sigh, yeah.
Attachment #335735 - Flags: superreview?(bzbarsky)
Attachment #335735 - Flags: review?(bzbarsky)
Attachment #335735 - Flags: approval1.9.0.2?
(Assignee)

Comment 37

9 years ago
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 38

9 years ago
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+
(Assignee)

Comment 40

9 years ago
Landed: new revision: 1.31.2.22
(Assignee)

Updated

9 years ago
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.
(Assignee)

Comment 43

9 years ago
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.
(Assignee)

Comment 45

9 years ago
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

Comment 48

9 years ago
Created attachment 338614 [details] [diff] [review]
a backport to 1.8.0 line

Igor, can you please check this backport? Thanks.
Attachment #338614 - Flags: review?

Updated

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

Updated

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

Comment 49

9 years ago
Created attachment 339213 [details] [diff] [review]
1.8.0 (by stransky)

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

Updated

9 years ago
Flags: blocking1.8.0.15+
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.