372075 - javascript: URI evaluation should use sandboxed context for toString, etc

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 368763 comment 9.  Basically, if someone manages to run an unprivileged javascript: URI against a privileged iframe they can possibly escalate their privileges.  I bet [e-mail] can provide an exploit if people need convincing... ;)

We should probably either duplicate the context-creation stuff in XPConnect or somehow expose it.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Would using a safe object wrapper work here? Can we use them from C++?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm...  That might work...  Especially if XPConnect returned that wrapper to start with (I bet XPConnect can use them from C++ if it wants!).

Comment 3

[Daniel Veditz [:dveditz]]

Wanted on the branches, but without an assignee or patch doesn't seem likely to happen any time soon. If we get a trunk solution renominate for a specific branch release at that time.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So I'm not sure whether a safe object wrapper would work here, to be honest.  I guess it might because we'll never be looking at the context...

Just creating a new JSContext won't work -- a JSContext without an nsIScriptContext is treated as all-powerful.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Or we could just compile a scripted function with the right principal and use that to stringify the value...  Do we need any sort of protection around JS_ReportPendingException, though?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #4)
> So I'm not sure whether a safe object wrapper would work here, to be honest.  I
> guess it might because we'll never be looking at the context...

Right, that was why I suggested it.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So is JS_ReportPendingException safe?  e.g. if toString() or valueOf() throws some object defined by the javascript: URI, what happens?

Comment 8

[moz_bug_r_a4]

(In reply to comment #7)

JS_ReportPendingException seems to be not safe.  Exploits work even with throw.

Comment 9

[moz_bug_r_a4]

Attachment: testcase 1 - without throwing

Comment 10

[moz_bug_r_a4]

Attachment: testcase 2 - toString() throws

Comment 11

[moz_bug_r_a4]

Attachment: testcase 3 - throws in the sandbox

Comment 12

[Boris Zbarsky [:bzbarsky]]

Hmm...  Throwing in the sandbox _also_ works?  How do those exceptions end up on our "outer" context anyway?

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/xpccomponents.cpp&rev=1.108&&mark=3518-3526&root=/cvsroot#3512

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ah, I see...

By the way, this seems to be basically a duplicate of bug 363597, no?

I wonder whether we could create the sandbox context together with the sandbox object and have xpconnect's API here return both or something?

Comment 15

[moz_bug_r_a4]

Old testcases no longer work on trunk.  They use strObj.eval.valueOf to access
Object.prototype.valueOf, but now strObj.eval is undefined on trunk. (see bug
382509.)  I'll attach new testcases, which use strObj.valueOf.valueOf to access
Object.prototype.valueOf.

Comment 16

[moz_bug_r_a4]

Attachment: testcase 4 - without throwing

Comment 17

[moz_bug_r_a4]

Attachment: testcase 5 - toString() throws

Comment 18

[moz_bug_r_a4]

Attachment: testcase 6 - throws in the sandbox

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

This should now be fixed by the checkin for bug 386635.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Should get this into a test suite.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

The patch that fixed this was backed out.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This patch makes the sandbox itself convert the return value (and exception) to a string, using good scope chains, etc.

The only "problem" with the patch, is that if the sandbox throws an object whose toString throws an exception, that exception will get lost. I'm pretty confident that this will only happen to those developers who are up to no good, so I'm fine with that.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272584 [details] [diff] [review]
Proposed fix

>+++ b/js/src/xpconnect/idl/nsIXPConnect.idl
>+                                         in PRBool returnString);

Please document.  Same for the declaration of  xpc_EvalInSandbox.

And maybe name it returnStringOnly?

r=bzbarsky other than that.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I'm about to check in

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 26

[Daniel Veditz [:dveditz]]

Is this fix appropriate for the 1.8.1.8 release or is it tied up with XOW?

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

In general, PAC doesn't interface with XOW. This should be easy to backport.

Comment 28

[Daniel Veditz [:dveditz]]

Blake: this didn't make 1.8.1.8, but please back-port when you get time so this doesn't miss 1.8.1.9 as well.

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, sorry about that. I have a backport, but wasn't able to test it.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Backport to 1.8

This backports this patch to the 1.8 branch. The conflicts come from API compat (nsIXPConnect_MOZILLA_1_8_BRANCH2) and the fact that the request stuff never landed on the 1.8 branch.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 283810 [details] [diff] [review]
Backport to 1.8

sr=bzbarsky

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

jst: Ping? The branch patch still needs review.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 283810 [details] [diff] [review]
Backport to 1.8

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branch.

Comment 35

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Screenshot of 2.0.0.11 on Vista

Comment 36

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Screenshot of 2.0.0.12rc1 on XP

Comment 37

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on the 1.8 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080128 Firefox/2.0.0.12.

Replacing fixed1.8.1.12 with verified1.8.1.12.

Comment 38

[moz_bug_r_a4]

I forgot to mention that all testcases in this bug (testcase 1-6) use bug
344495's trick.

Comment 39

[Alexander Sack]

distro patch blocking 1.8.0.15     

Comment 40

[Alexander Sack]

Comment on attachment 283810 [details] [diff] [review]
Backport to 1.8

unmodified distro patch. caillon, please sign off.

Comment 41

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 283810 [details] [diff] [review]
Backport to 1.8

a=caillon

Comment 42

[Reed Loden [:reed]]

MOZILLA_1_8_0_BRANCH:

Checking in dom/src/jsurl/nsJSProtocolHandler.cpp;
/cvsroot/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp,v  <--  nsJSProtocolHandler.cpp
new revision: 1.113.2.2.2.6; previous revision: 1.113.2.2.2.5
done
Checking in js/src/xpconnect/idl/nsIXPConnect.idl;
/cvsroot/mozilla/js/src/xpconnect/idl/nsIXPConnect.idl,v  <--  nsIXPConnect.idl
new revision: 1.35.4.1.4.3; previous revision: 1.35.4.1.4.2
done
Checking in js/src/xpconnect/src/nsXPConnect.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/nsXPConnect.cpp,v  <--  nsXPConnect.cpp
new revision: 1.73.4.3.2.5; previous revision: 1.73.4.3.2.4
done
Checking in js/src/xpconnect/src/xpccomponents.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpccomponents.cpp,v  <--  xpccomponents.cpp
new revision: 1.75.2.6.2.9; previous revision: 1.75.2.6.2.8
done
Checking in js/src/xpconnect/src/xpcprivate.h;
/cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v  <--  xpcprivate.h
new revision: 1.162.2.4.2.9; previous revision: 1.162.2.4.2.8
done