441087 - NS_ERROR_XPC_SECURITY_MANAGER_VETO in Greasemonkey JavaScript eval containing object.property

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Cacycle]

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]

There is the same problem with object properties in "square bracket notation".

Comment 2

[Cacycle]

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

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

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

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

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

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

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

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

In what situation will the expando behavior change, exactly?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

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

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]

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

Given the silly structure of the Firefox chrome, yes.  Said structure needs to be fixed.

Comment 15

[georgi - hopefully not receiving bugspam]

i can't imagine *any* legitimate use of content accessing chrome scripts except maybe for xbl

Comment 16

[Boris Zbarsky [:bzbarsky]]

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]

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

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

Pushed as http://hg.mozilla.org/index.cgi/mozilla-central/rev/4593610d9715

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

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

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

Fix checked into the 1.9 branch.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

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

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

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

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

Attachment: 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) (inactive)]

Fix checked into the 1.8 branch.

Comment 29

[moz_bug_r_a4]

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]

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

Unsetting branch fixed flag, per comment 29.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

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

Blake, let's get a fix asap.

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

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

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

Attachment: Lastest patch

Sigh, yeah.

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 335735 [details] [diff] [review]
Lastest patch

Clearly, I hate the 1.8 branch.

Comment 38

[Boris Zbarsky [:bzbarsky]]

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

Comment on attachment 335735 [details] [diff] [review]
Lastest patch

a=beltzner, please land ASAP

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Landed: new revision: 1.31.2.22

Comment 41

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

What is a good way to test this in 1.8.1.17? It isn't immediately clear.

Comment 42

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

I've verified bug 444073 but it isn't clear that it covers this here.

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

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

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

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

This is verified then with build 2 of 2.0.0.17.

Comment 47

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

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

Attachment: a backport to 1.8.0 line

Igor, can you please check this backport? Thanks.

Comment 49

[Alexander Sack]

Attachment: 1.8.0 (by stransky)

a=asac for 1.8.0.15

Comment 50

[Benjamin Smedberg]

Landed before branching for 1.9.1

Comment 51

[Tony Chung [:tchung]]

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