Closed
Bug 345305
Opened 18 years ago
Closed 17 years ago
Arbitrary code execution with Venkman JavaScript Debugger
Categories
(Other Applications Graveyard :: Venkman JS Debugger, defect)
Other Applications Graveyard
Venkman JS Debugger
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)
13.73 KB,
patch
|
Gijs
:
review+
dveditz
:
approval1.8.1.8+
|
Details | Diff | Splinter Review |
14.17 KB,
patch
|
caillon
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•18 years ago
|
||
![]() |
||
Comment 2•18 years ago
|
||
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?
Comment 3•18 years ago
|
||
(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+
Comment 4•18 years ago
|
||
(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•18 years ago
|
||
(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•18 years ago
|
||
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•18 years ago
|
||
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
Assignee | ||
Comment 8•18 years ago
|
||
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).
Reporter | ||
Comment 9•18 years ago
|
||
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.)
Assignee | ||
Comment 10•18 years ago
|
||
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•18 years ago
|
||
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.
Reporter | ||
Comment 12•18 years ago
|
||
It seems that the patch (attachment 229913 [details] [diff] [review]) in Bug 344495 also fixes the privilege escalation bugs.
Updated•18 years ago
|
Flags: blocking1.8.0.6? → blocking1.8.0.6+
Assignee | ||
Comment 14•18 years ago
|
||
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
Updated•18 years ago
|
Flags: blocking1.8.0.8?
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+
Assignee | ||
Comment 15•18 years ago
|
||
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
Comment 16•18 years ago
|
||
Given comment #15 I'm removing this from the blocking list... Thanks James
Flags: blocking1.8.1+ → blocking1.8.1-
Comment 17•18 years ago
|
||
Restoring lost blocking flag
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Assignee | ||
Updated•18 years ago
|
Whiteboard: [sg:moderate] critical on debugged pages → [sg:moderate] critical on debugged pages [vnk-0.9.88]
Assignee | ||
Comment 18•18 years ago
|
||
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)
Assignee | ||
Updated•18 years ago
|
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
Comment 19•18 years ago
|
||
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-
![]() |
||
Updated•18 years ago
|
Flags: blocking1.9a2? → blocking1.9?
Updated•18 years ago
|
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
Updated•18 years ago
|
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
Comment 20•17 years ago
|
||
>+ // 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. :-)
Assignee | ||
Comment 21•17 years ago
|
||
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•17 years ago
|
||
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+
Comment 23•17 years ago
|
||
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•17 years ago
|
||
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?
Updated•17 years ago
|
Attachment #243179 -
Flags: approval-seamonkey1.1.4? → approval-seamonkey1.1.5?
Comment 25•17 years ago
|
||
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•17 years ago
|
||
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?
Comment 27•17 years ago
|
||
(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•17 years ago
|
||
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+
Assignee | ||
Comment 29•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Keywords: fixed1.8.1.7
Reporter | ||
Comment 30•17 years ago
|
||
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•17 years ago
|
||
(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!
Reporter | ||
Comment 32•17 years ago
|
||
I filed bug 396142.
![]() |
||
Updated•17 years ago
|
Flags: blocking1.9?
Comment 33•17 years ago
|
||
Attachment #306272 -
Flags: approval1.8.0.15?
Comment 34•17 years ago
|
||
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+
Updated•16 years ago
|
Group: core-security
Updated•6 years ago
|
Product: Other Applications → Other Applications Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•