363594 - [FIX]Arbitrary code execution by using javascript: url

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[moz_bug_r_a4]

This is trunk only.

When javascript: url is evaluated in the sandbox, and its resulting value is an
object, the object's toString and valueOf methods defined by sandboxed script
can be called on the web page's context.

While the toString and valueOf methods are being executed, some of access
checks don't work properly.  Thus, sandboxed script can access xbl.method's
clone parent and xbl compilation scope to run arbitrary code with chrome
privileges.

This seems to have been caused by Bug 351633.

fx3.0a1-2006-10-16-04 is not exploitable.
fx3.0a1-2006-10-17-04 is exploitable.

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-10-16+04&maxdate=2006-10-17+04

Comment 1

[moz_bug_r_a4]

Attachment: testcase

Comment 2

[Daniel Veditz [:dveditz]]

bug 351633 does look like the most likely culprit --> bz for confirmation.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, this is all me.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 248495 [details] [diff] [review]
Fix

r+sr=jst

Comment 6

[Boris Zbarsky [:bzbarsky]]

Fixed.  Since this is not a problem in any shipping builds, should we just open the bug?  Or is the fact that it's a problem in the alpha preventing us from doing that?

Comment 7

[moz_bug_r_a4]

The testcase in this bug reveals a way that the sandboxed script gets a String
object from outside of the sandbox. (See also Bug 355214.)  I wonder if this
could be a reason for keeping this bug security-sensitive.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  In that case, when do we plan to open it?  What are the requirements in terms of other bugs being fixed and us shipping releases for that to happen?

I really dislike the idea of keeping bugs closed "just because".

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, so I'd kinda like to add a testcase for this to the regression tests so we don't break this again.  I guess I need to wait till the bug is opened up or something, since we're keeping it closed precisely because of the testcase?  So again, what's our plan for opening up this bug?

Comment 10

[moz_bug_r_a4]

FWIW, if this bug is public, but the fundamental issue of the sandbox and eval
is not yet fixed, then extensions authors who want to use the sandbox have to
learn the fact that the following code is exploitable even though it seems to
be safe.

<script src="prototype.js"></script>
<script>
var s = new Components.utils.Sandbox(url);
var x = Components.utils.evalInSandbox(untrusted_code, s);
if (x == 1) { ... }
</script>

Comment 11

[Daniel Veditz [:dveditz]]

"when to open" is complicated by the fact that we just shipped this in an alpha, unfortunately slashdotted as a "Firefox 3" alpha. Lots of potentially vulnerable folks who won't simply get the next trunk nightly, though I guess we can hope they'll either keep up or tire of it and go back to ff2.

That said, are we going to be able to fix the problem in comment 10 any time soon? If not we need to advertise the problem to prevent vulnerable extensions. (CC'ing sheppy who may have ideas on how to approach such an education/documentation issue.)

Comment 12

[Eric Shepherd [:sheppy]]

This is a tricky one.  Since there's no official release with this problem, I don't think this necessarily belongs in the MDC wiki, and there's really not a good way to be sure that everyone that needs to know about it will find out, since odds are most of the people that grabbed the so-called Firefox 3 alpha aren't regular readers of the mailing lists here anyway (and the people that do read them know better already).

However, it does need to be disclosed, I think, so I guess the best we can really do is to post a notice in dev.apps.firefox, and maybe on the Firefox 3 For Developers article on the MDC wiki, at least temporarily.

If we're concerned that end users might have grabbed a build with this bug, it might be a good idea to post a note in an appropriate end-user mailing list as well (even if it's just a link to a web page with information about the problem, which would help avoid scaring Firefox 2 users).  Something like "If you're running a Minefield build from prior to <date>, please read this important security notice."

Comment 13

[Daniel Veditz [:dveditz]]

(In reply to comment #12)
> Since there's no official release with this problem [...]

Different problem. This specific bug is fixed, the documentation issue is about the thing described in comment 10. Extensions that do that were and still are vulnerable quite apart from this now-fixed bug. The only relation is that the comment 10 exploit relies on a trick also used in the testcase for this different problem.

Comment 14

[Eric Shepherd [:sheppy]]

OK, that wasn't clear to me from the discussion here, since comment 10 doesn't actually say it's not directly related to this bug.

In that case, I'll find an appropriate place on the MDC wiki to document this security concern.

Comment 15

[Eric Shepherd [:sheppy]]

What exactly makes the code in comment 10 unsafe?  I mean, I can see where it's running unsafe code, but what makes it unsafe to run unsafe code in a sandbox?

Working on writing this up and want to be sure I get it right.

Comment 16

[moz_bug_r_a4]

(In reply to comment #15)
> What exactly makes the code in comment 10 unsafe?

When (x == 1) is evaluated, x.valueOf() is called by chrome code.  And, when
chrome code calls x.valueOf(), unsafe code can get a String object created in
the chrome code's global scope.  (The testcase in this bug reveals this trick.)

Thus, if chrome code has added a function that has chrome privileges to
String.prototype, Object.prototype, or Function.prototype, then unsafe code can
abuse that function.  What unsafe code can do depends on what that function is.
For example, prototype.js adds bind() method to Function.prototype.  It can be
easily abused to run arbitrary code with chrome privileges.

Note that chrome-defined function stuff is not needed if an attacker knows
another trick described in bug 344495.  Until bug 344495 is fixed, if chrome
code accesses a property of an unsafe object, then unsafe code can run
arbitrary code with chrome privileges.

That is to say, once chrome code runs unsafe code in a sandbox, it is unsafe to
access properties of a resulting object and the sandbox.

  var sandbox = new Components.utils.Sandbox(url);
  var x = Components.utils.evalInSandbox(unsafe_code, sandbox);
  var y = x.y; // unsafe
  var z = sandbox.z; // unsafe
  if (x == 1) {} // unsafe; this calls x.valueOf()
  if (x === 1) {} // safe
  if (typeof x == "number") {} // safe

So, a suggestion in http://developer.mozilla.org/en/docs/JSON seems to be
insufficient.  Chrome code should make sure that untrusted JSON strings do not
contain any functions and expressions, when chrome code uses
Components.utils.evalInSandbox to parse JSON strings. (See also Bug 355625.)

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #16)
> So, a suggestion in http://developer.mozilla.org/en/docs/JSON seems to be
> insufficient.  Chrome code should make sure that untrusted JSON strings do not
> contain any functions and expressions, when chrome code uses
> Components.utils.evalInSandbox to parse JSON strings. (See also Bug 355625.)

The safe object wrapper introduced in bug 355766 also fixes these bugs, right? So an alternative would be to wrap all sandbox-produced objects in a safe object wrapper before actually using them.

Comment 18

[Eric Shepherd [:sheppy]]

I've added a security warning to the evalInSandbox() page in MDC's wiki.  Let me know if there are any issues with it, or if anyone can think of other places it should be mentioned.

http://developer.mozilla.org/en/docs/Components.utils.evalInSandbox

Comment 19

[Brandon Sterne (:bsterne)]

Interesting, this bug is still closed even though it landed back in December 2006.  We've also documented the security risk of writing unsafe code that can abuse this hole.  Does this need to remain hidden?

Comment 20

[Boris Zbarsky [:bzbarsky]]

I wouldn't think so, no.  But Daniel's the one who usually makes that call, iirc.