Closed Bug 345305 Opened 13 years ago Closed 12 years ago

Arbitrary code execution with Venkman JavaScript Debugger

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: moz_bug_r_a4, Assigned: bugzilla-mozilla-20000923)

References

Details

(Keywords: fixed1.8.0.15, fixed1.8.1.8, Whiteboard: [sg:moderate] critical on debugged pages [vnk-0.9.88])

Attachments

(2 files)

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.
Attached file testcase
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?
Flags: blocking1.9a2?
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
(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
Flags: blocking1.8.1? → blocking1.8.1+
(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)?
(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...
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.
CC'ing Joe so he can keep an eye on the proposed new functionality which would probably prove useful for Firebug as well.
Whiteboard: [sg:moderate] critical on debugged pages
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).
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.)
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?
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.
It seems that the patch (attachment 229913 [details] [diff] [review]) in Bug 344495 also fixes the
privilege escalation bugs.
Appears fixed by the patch in bug 344495
Depends on: 344495
Flags: blocking1.8.0.6? → blocking1.8.0.6+
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.
Assignee: rginda → silver
Flags: blocking1.8.0.8?
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+
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.
Status: NEW → ASSIGNED
Given comment #15 I'm removing this from the blocking list...  Thanks James
Flags: blocking1.8.1+ → blocking1.8.1-
Restoring lost blocking flag
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Whiteboard: [sg:moderate] critical on debugged pages → [sg:moderate] critical on debugged pages [vnk-0.9.88]
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.
Attachment #243179 - Flags: review?(rginda)
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
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.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1-
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9-
Flags: blocking1.9a2? → blocking1.9?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
>+    // 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. :-)
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 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?
Attachment #243179 - Flags: review?(rginda) → review+
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 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!
Attachment #243179 - Flags: approval1.8.1.7?
Attachment #243179 - Flags: approval-seamonkey1.1.4?
Attachment #243179 - Flags: approval-seamonkey1.1.4? → approval-seamonkey1.1.5?
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 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
Attachment #243179 - Flags: approval1.8.1.7?
(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 on attachment 243179 [details] [diff] [review]
Replace unsafe operations with stackframe-evals

approved for 1.8.1.7, a=dveditz for release-drivers
Attachment #243179 - Flags: approval-seamonkey1.1.5? → approval1.8.1.7+
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: fixed1.8.1.7
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, ...)
(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!
I filed bug 396142.
Flags: blocking1.9?
Attachment #306272 - Flags: approval1.8.0.15?
Comment on attachment 306272 [details] [diff] [review]
same diffed against 1.8.0

a=caillon for 1.8.0.15
Attachment #306272 - Flags: approval1.8.0.15? → approval1.8.0.15+
committed to 1.8.0 branch
Keywords: fixed1.8.0.15
Group: core-security
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.