345305 - Arbitrary code execution with Venkman JavaScript Debugger

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[moz_bug_r_a4]

See Bug 344494 and Bug 344751.

Venkman is vulnerable to attacks using the Array.prototype methods, too.


Steps to Reproduce:
1. Open JavaScript Debugger (Venkman).
2. Open testcase.
3. Set a breakpoint at function xxx.
4. Click a button to call xxx().
5. Step over until after |var yyy = {};|.
6. Open the tree item for |yyy| in the Local Variables view.

An alert dialog that shows Components.stack will appear.

Comment 1

[moz_bug_r_a4]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

So venkman absolutely _must_ be able to access random properties "safely" to be at all useful...  Maybe we should go with sicking's suggestion and if we detect an explicitly assigned getter we should list the getter code, not the value it would return?  Is there a way to detect this?  Are there other ways to make this work?

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> So venkman absolutely _must_ be able to access random properties "safely" to be
> at all useful...  Maybe we should go with sicking's suggestion and if we detect
> an explicitly assigned getter we should list the getter code, not the value it
> would return?  Is there a way to detect this?

Yes, but it takes some care.  The extension or XUL app has to call Object.prototype.__lookupGetter__ indirectly.  Suggest using a helper:

function lookupGetter(untrustedObj, id) {
    return Object.prototype.__lookupGetter__.call(untrustedObj, id);
}

/be

> Are there other ways to make this work?

Yes, we should not let any untrusted code gain privilege, no matter how indirect its influence on data or control flow.  The kind of data-tainting architecture needed to support this is a goal, but not for the near term.

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #2)
> Are there other ways to make this work?

Not from JavaScript proper. We'd need to expose some API written in C++ to avoid calling content-defined getters. (Is that a viable option)?

Comment 5

[Mike Shaver (:shaver emeritus)]

(In reply to comment #3)
> Yes, but it takes some care.  The extension or XUL app has to call
> Object.prototype.__lookupGetter__ indirectly.  Suggest using a helper:
> 
> function lookupGetter(untrustedObj, id) {
>     return Object.prototype.__lookupGetter__.call(untrustedObj, id);
> }

We'd need to take care that we don't call a booby-trapped .toString on the getter function either, of course...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Is calling JS_GetProperty from C++ any different from just getting the property in chrome JS, from a security standpoint?  That is, does the getter end up running with different principals in those two cases?

It really sounds to me like we might want a C++ class (similar to XPCNativeWrapper, and with similar auto-wrapping on gets, but not really XPConnect-related?) that various places that want to access content-set properties reasonably safely can use.  It'd do things like look up the getter, and if it finds one handle converting it to string in a safe way, etc...  At the very least, this is what we need in DOM Inspector and Venkman, looks like.

Comment 7

[Daniel Veditz [:dveditz]]

CC'ing Joe so he can keep an eye on the proposed new functionality which would probably prove useful for Firebug as well.

Comment 8

[James Ross]

FWIW, I believe this could be fixed entirely within Venkman, mainly by tracking the frame that a value came from and using jsdIStackFrame.eval instead of unwrapping the value and just carelessly getting properties off it. This, in my simple experimentation, causes the testcase to throw up a "Permission denied to get property UnnamedClass.stack" dialog on top of the webpage.

An alternative is to give JSD a "get property safely" method that did getter checks, etc. (but that would only protect new back-ends).

Comment 9

[moz_bug_r_a4]

Attachment: testcase 2 - for old branches

This works even on fx1.0.x and moz1.7.x.  (My 1st testcase does not work on fx1.0.x and moz1.7.x.)

Comment 10

[James Ross]

I've managed to protect Venkman from the issue here relatively easily, though it's not going to be fun to avoid using for..in on content objects (I have ideas, but they are not all that pretty). All the work relies extensively on having the debugger present, so none of it would benefit DOM Inspector.

My question right now is whether an application-specific fix or a more general core addition/change is preferred?

Comment 11

[Boris Zbarsky [:bzbarsky]]

For the 1.8.0 branch we should probably do "whatever works", unless the general fix is pretty easy.

For trunk, it might be worth thinking about a general fix.

For 1.8 branch, I'm not sure.

Comment 12

[moz_bug_r_a4]

It seems that the patch (attachment 229913 [details] [diff] [review]) in Bug 344495 also fixes the
privilege escalation bugs.

Comment 13

[Daniel Veditz [:dveditz]]

Appears fixed by the patch in bug 344495

Comment 14

[James Ross]

I'm going to go ahead with the app-specific fix for Venkman over the next week. If a general fix happens, good for it, but Venkman will not be depending on it.

Comment 15

[James Ross]

This should not be blocking1.8.1+ unless someone is planning a core code fix. I've got a Venkman patch for this, which I'll attach after tidying it up a bit, but as it is app-level it has no need to block any release.

Comment 16

[Mike Schroepfer]

Given comment #15 I'm removing this from the blocking list...  Thanks James

Comment 17

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 18

[James Ross]

Attachment: Replace unsafe operations with stackframe-evals

The aim is to never do:
  - wrappedValue.getWrappedValue()
  - unsafeObject["someProp"]
  - for..in on unsafeObject

This patch does not claim to remove all calls to getWrappedValue(), as it is not inherently unsafe, but the less it is used the safer we all are. This patch fixes the code execution exploits for Locals, Watches and any other tree view of debuggee objects. If the code does not have a stack frame with which to safely evaluate the desired expression, the code asserts (in the caller) and dies (in evalString).

The advantage to this particular change (besides the obvious security implications) is that Venkman will see the values returned by getters as if the debuggee itself had queried them - something I much prefer to just having the getter code and/or security errors.

Comment 19

[Daniel Veditz [:dveditz]]

We'll gladly consider this patch for the branches once it's reviewed (add the approval? request flags) but aren't going to block on it.

Comment 20

[:Gijs (he/him)]

>+    // Note: uses an inline function to avoid polluting the frame's scope.
>+    var code = "(function(obj){" +
>+               "    var string = '';" +
>+               "    for (var prop in obj) {" +
>+               "        if (string)" +
>+               "            string += ',';" +
>+               "        string += prop.quote();" +
>+               "    }" +
>+               "    return string;" +
>+               "})(" + this.expression + ")";
>+
>+    // list is undefined if an exception occured.
>+    var list = this.evalString(code);
>+    if (typeof list != "undefined") {
>+ <add stuff to propList>
>+    }

(I would have preferred to ask this on IRC, but connection problems are preventing me from doing that, so going here instead)

James: As far as I can tell, the above code means that if an object |obj| has a getter for some property that throws an exception, none of the properties are going to show up in the list anymore, because the value of |list| will be undefined and hence none of the properties get added to propList. If so, that'd seem like something which would need to be fixed before this patch gets r+. Please correct me if I'm misreading the patch or the consequences of it. :-)

Comment 21

[James Ross]

Gijs: The above code doesn't call the getters, it only enumerates the properties. On trunk, objects can have their own enumerators (or I think they can, anyway), so it provides safety from anything they might do and will not list anything if they broke the enumeration part-way through. Whether that matters is a separate issue, as the old code would behave the same (but worse).

Comment 22

[:Gijs (he/him)]

Comment on attachment 243179 [details] [diff] [review]
Replace unsafe operations with stackframe-evals

ok, then r=me.
So which of the 6 seemingly-applying approvals do I need? dveditz?

Comment 23

[Daniel Veditz [:dveditz]]

Which branch(es) does Venkman actually use? I'd imagine you need approval1.9? to land on the trunk these days, and approval1.8.1.7? to land on the MOZILLA_1_8_BRANCH.  If you ship a single version that works for all versions of Firefox/SeaMonkey then maybe you only need to land in one of those places, probably the trunk.

Comment 24

[:Gijs (he/him)]

Comment on attachment 243179 [details] [diff] [review]
Replace unsafe operations with stackframe-evals

We're NPOTB on trunk, so we don't need a1.9, I believe. As for branches, we ship with SeaMonkey and while we don't backport everything, this is a security fix without locale changes, so I'd imagine we want to have it on branch, if possible. Not sure which of the two a? I want in that case, so I'll just set both. Thanks, dveditz!

Comment 25

[Daniel Veditz [:dveditz]]

If you're not putting security fixes into the trunk version then I'd prefer if the code were cvs rm'd from trunk. Otherwise people will assume the trunk is the latest and greatest.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 243179 [details] [diff] [review]
Replace unsafe operations with stackframe-evals

Since it's not part of Firefox but is in SeaMonkey I'll let the seamonkey-council folks approve this one for the 1.8 branch

Comment 27

[:Gijs (he/him)]

(In reply to comment #25)
> If you're not putting security fixes into the trunk version then I'd prefer if
> the code were cvs rm'd from trunk. Otherwise people will assume the trunk is
> the latest and greatest.
> 

Eh? I'm confused. This patch is going into trunk fine. What gave you the impression it wouldn't? I was under the impression it would be better to cross-commit to trunk and branch, so I'm waiting on that approval before I commit anything. It's NPOTB because Venkman doesn't ship with Firefox, and that is why we don't need approval to land this code on trunk (similar to calendar, chatzilla and all the other projects that don't touch Firefox).

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 243179 [details] [diff] [review]
Replace unsafe operations with stackframe-evals

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 29

[James Ross]

The patch has been checked in to HEAD and MOZILLA_1_8_BRANCH.

I am marking this bug FIXED as - as far as I can recall - the patch eliminates the specific attack vectors mentioned, and closes the general attack vector used. I would love for someone to properly test this, however.

Note that, as I said in comment 18, this does not necessarilly make Venkman completely safe, only reduces the potential attack surface somewhat (focused on the areas used in this bug). If someone wishes to do a further audit of getWrappedValue(), for example, I suggest they file security-sensitive bugs on any areas they find which could do with improvement.

Comment 30

[moz_bug_r_a4]

It seems that most codes that use getWrappedValue() are exploitable. 
(Previously, I didn't understand that the problem was caused by
getWrappedValue(), therefore I couldn't find attack vectors other than the one
mentioned in this bug.)

Should I file separate bugs on each area?  (jsdExecutionHook, testBreakpoint,
ss_setThisObj, cmdChangeValue, cmdEval, ...)

Comment 31

[:Gijs (he/him)]

(In reply to comment #30)
> It seems that most codes that use getWrappedValue() are exploitable. 
> (Previously, I didn't understand that the problem was caused by
> getWrappedValue(), therefore I couldn't find attack vectors other than the one
> mentioned in this bug.)
> 
> Should I file separate bugs on each area?  (jsdExecutionHook, testBreakpoint,
> ss_setThisObj, cmdChangeValue, cmdEval, ...)
> 

Either that or file one bug with a clear list of affected places. Thanks!

Comment 32

[moz_bug_r_a4]

I filed bug 396142.

Comment 33

[Alexander Sack]

Attachment: same diffed against 1.8.0

Comment 34

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 306272 [details] [diff] [review]
same diffed against 1.8.0

a=caillon for 1.8.0.15

Comment 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

committed to 1.8.0 branch