Closed Bug 396142 Opened 17 years ago Closed 12 years ago

Venkman: attack vectors that use getWrappedValue()

Categories

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

x86
Windows XP

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: moz_bug_r_a4, Assigned: Gijs)

References

Details

(Whiteboard: [sg:moderate] keep hidden until bug 720619 fixed)

Attachments

(2 files, 18 obsolete files)

1.63 KB, text/html
Details
1.56 KB, text/html
Details
Please see bug 345305.

I'll attach two testcases for each attack vector.  One uses document.write() to
run code, which works both on trunk and 1.8 branch, the other uses eval(),
which can show callers but does not work on trunk.

* testcase 1 and 2
jsdExecutionHook -> getBaseWindowFromWindow
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-debugger.js&rev=1.61&mark=306-310#306
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-utils.js&rev=1.35&mark=608-613#602

* testcase 3 and 4
testBreakpoint -> formatException
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-debugger.js&rev=1.61&mark=1484-1502#1484
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-static.js&rev=1.72&mark=489#478

* testcase 5 and 6
cmdChangeValue
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-commands.js&rev=1.44&mark=364,412#362

* testcase 7 and 8
ss_setThisObj
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-views.js&rev=1.38&mark=1570-1581#1570
cmdEval
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-commands.js&rev=1.44&mark=813-820#813

* testcase 9 and 10
cmdSetEvalObj assigns an unsafe object to console.currentEvalObject.  Actual
exploit takes place when "set-eval-obj" menuitem's checkedif attribute is
eval()d.
cmdSetEvalObj
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-commands.js&rev=1.44&mark=1882,1894#1878
"set-eval-obj" checkedif
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-views.js&rev=1.38&mark=540-543#540
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/menu-manager.js&rev=1.5&mark=119-131,194#119

* testcase 11 and 12
"inspect" enabledif -> isDOMThing -> isinstance
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-views.js&rev=1.38&mark=547-549#547
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-static.js&rev=1.72&mark=165#161
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-utils.js&rev=1.35&mark=1057#1048
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/menu-manager.js&rev=1.5&mark=119-131,178#119
cmdInspect -> isDOMThing -> isinstance
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/venkman/resources/content/venkman-commands.js&rev=1.44&mark=1218,1223,1225#1212
Attached file testcase 2: jsdExecutionHook - eval() (obsolete) —
Attached file testcase 4: testBreakpoint - eval() (obsolete) —
Attached file testcase 6: cmdChangeValue - eval() (obsolete) —
Attached file testcase 10: set-eval-obj - eval() (obsolete) —
Attached file testcase 12: inspect - eval() (obsolete) —
Also, venkman-dev.js uses getWrappedValue() and seems to be unsafe.  But, I
cannot test it.
Severity: normal → major
Flags: blocking1.9?
Whiteboard: [sg:moderate]
Assignee: rginda → mrbkap
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
gota try to get this for fx3.  up'ing pri
Priority: P3 → P2
Adding this back on the nom list as it was missed during the massive 1.9 tracking triage.
Flags: tracking1.9+ → blocking1.9?
Flags: blocking1.9? → blocking1.9+
I can't reproduce any exploits on a current trunk build.
Brandon or moz_bug_r_a4 can you try to repro on recent trunk?
Ah, exploits that use document.write no longer work on trunk.  But, this bug is
not fixed (I confirmed that content code can still be called by chrome code).
I'll try to create new testcases next week.
Tested with JavaScript Debugger 0.9.87.3 and DOM Inspector 2.0.0 on
fx-3.0pre-2008-03-30-05 and fx-2.0.0.14pre-2008-03-30-04.

Testcases that use eval don't work on either trunk or fx2.
Testcases that use document.write work on fx2.
Upcoming testcases that use location setter work on both trunk and fx2.
Attachment #280859 - Attachment is obsolete: true
Attachment #280861 - Attachment is obsolete: true
Attachment #280863 - Attachment is obsolete: true
Attachment #280865 - Attachment is obsolete: true
Attachment #280867 - Attachment is obsolete: true
Attachment #280869 - Attachment is obsolete: true
Attached file testcase 18: inspect - location.href (obsolete) —
This requires DOM Inspector.
Brandon/Brendan/DVeditz - since this exists in both 1.8 and 1.9 can we take a fix in a 1.9.0.x dot release?
I've been poking around in this code a bit and I see no evidence that this is something that needs to, or even could, be fixed in the Firefox code, it's all Venkman code that's running w/o safety wrapper protection. I would recommend we take this off the blocker list, and this really should be fixed in the Venkman code unless doing so turns out to be extremely painful, but as of yet I don't see anything that suggests that it would be, or that any of the pain could easily be lessened by changes to the Firefox code in any case.

Silver, would you have any time to look into these issues? Or if not, who else is enough in the know to fix this on the Venkman side?
I don't have the time to look into these in detail, although I could probably answer questions about them if needed; Gijs would be my first suggestion for people who are around and know the code, but I believe he's pretty busy at the moment.

I'd also agree with you that it's pretty much up to Venkman to be exceptionally careful of the unwrapped values, but I think there are cases where things may be really hard to do without help from native code (but only JSD, I expect).
Given Comment 27 and Comment 28 taking this off the 1.9 blocker list.
Flags: blocking1.9+ → blocking1.9-
So... I just started looking at this again (I know, long time...), but it seems like some part of this has been fixed outside of Vnk. In particular, I tried the last two testcases, and no alerts appear. For the last one (testcase 18) I do get a security message in the console saying that the content on the attachment can't link to content at chrome://global/content/mozilla.xhtml . However, in both cases, the webpage is able to load about:mozilla into its frame *when I open the context menu in Venkman*, which I find disconcerting. Is the half-breakage of these testcases just because of contentaccessible now making about:mozilla out-of-bounds for webpages, while the fundamental issue remains (thus requiring Venkman to take more steps still)? Or did something more substantial change the game here?
Did you run testcases in b.m.o?  If so, the half-breakage of these testcases is
due to the b.m.o redirection feature for security-sensitive attachments.

By running testcases in a local server, I can reproduce testcases 13-18 with
Venkman 0.9.87.4 on trunk, fx3.5 and fx3.0.
Attached patch Patch for 4 out of 6 issues (obsolete) — Splinter Review
OK, I obsoleted the document.write testcases because they don't work on 3.5 (see also the previous comments). The issues are still there, obviously, and can be tested with the location.href tests.

This patch fixes (at least attempts to fix...) the jsdExecutionHook, testBreakpoint, cmdChangeValue and inspect issues. The currentEvalObject functionality, IMHO, needs a full rewrite, because right now it's a sieve. I want to have a pair of eyes over this patch before I attempt that (as it is more complicated, as far as I can tell, and I don't want to be making the same mistakes twice).

Some notes on this patch:
- XPCNativeWrapper on the window (which should work if it is a proper window, which is what we're concerned with in getBaseWindowFromWindow anyway) patches the jsdExecutionHook hole.
- XPCSafeJSObjectWrapper on the exception we're trying to show. Doing that may throw, in which case we need to catch that exception, too. This fixes testBreakpoint. :-)
- cmdChangeValue was also a sieve, but a simpler one. I have completely done away with making the command at all usable from the commandline - this is not necessary as you can just evaluate stuff without using any commands (foo = 5 will change the value of foo). I've used the 'expression' property on e instead, which is safe because of the work in bug 345305, and simply assign to it using the user-given expression. I no longer unwrap any jsdValues.
- inspect was my own doing originally... I'm using XPCNativeWrapper again, which should work for proper DOM nodes, and guarantees us we're QI-ing the right things. In fact, it breaks the constructor checking (as the xpcnativewrapper seems to prohibit access to that altogether). The instanceof still works, but that makes me wonder if there are (a) cases where it doesn't (it has worked in my tests...); (b) if we need some other way to make sure this is safe but works.
Assignee: mrbkap → gijskruitbosch+bugs
Attachment #280858 - Attachment is obsolete: true
Attachment #280860 - Attachment is obsolete: true
Attachment #280862 - Attachment is obsolete: true
Attachment #280864 - Attachment is obsolete: true
Attachment #280866 - Attachment is obsolete: true
Attachment #280868 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #389922 - Flags: review?(mrbkap)
Attachment #389922 - Flags: review?(silver)
Attachment #389922 - Flags: review?(silver) → review-
Comment on attachment 389922 [details] [diff] [review]
Patch for 4 out of 6 issues

>+msg.err.condition.security = The webpage's code caused a security error when evaluating the breakpoint condition.

I'd go for something shorter and sweeter, like "Security error reporting breakpoint error." - msg.err.condition.failed will be displayed right before it.

>+msg.err.eval.security    = Security exception thrown when safely evaluating an expression:

Like with the other message, this is a problem reporting the error; "Security error reporting evaluation error.".

If you want to include the security exception in the message, do it consistently with msg.err.condition.security (I don't know which arrangement is preferred: include in message or separate).

>-    obj[e.propertyName] = e.newValue;
>-    dispatch ("hook-eval-done");
>+    

Nit: trailing whitespace.

>+    evalInTargetScope(e.expression + " = " + e.newValue, false);
>+    dispatch("hook-eval-done");


>@@ -1221,13 +1190,13 @@ function cmdInspect(e)
>         key = e.propertyName;
>         value = e.jsdValue.getWrappedValue();
>     }
>     else if ("expression" in e)
>     {
>         key = e.expression;
>-        value = evalInTargetScope(e.expression, true).getWrappedValue();
>+        value = evalInTargetScope(e.expression, false).getWrappedValue();

Can you explain this change? If you're not going to let it rethrow, you get null from evalInTargetScope() and you'll get a JavaScript error trying to call getWrappedValue() on null, as far as I can see.


>     // While debugging, we need to avoid any activity in both the current
>     // execution context and the target window (if present - JS modules usually
>     // don't have one!). Of course, we must not disable the debugger window...
>-    console.targetWindow = getBaseWindowFromWindow(glob.getWrappedValue());
>+    // The ordinary way to run this is:
>+    // console.targetWindow = getBaseWindowFromWindow(glob.getWrappedValue());
>+    // Unfortunately, that is not safe. So, we employ smoke and mirrors:
>+    var unsafeWindow = glob.getWrappedValue();
>+    try {

Nit: Newline before open brace.

>+        var safeWindow = new XPCNativeWrapper(unsafeWindow);

Any reason not to use |new XPCNativeWrapper(glob.getWrappedValue())|? It seems safer to not have the dangerous value lying around in a local variable.

>+        console.targetWindow = getBaseWindowFromWindow(safeWindow);
>+    }
>+    catch (ex) {console.targetWindow = null; }

Nit: Three newlines needed here for code style.


>@@ -1503,14 +1511,22 @@ function testBreakpoint(currentFrame, rv
>             if (breakpoint.passExceptions)
>             {
>                 rv.value = result.value;
>                 return RETURN_THROW;
>             }
>                 
>-            display (MSG_ERR_CONDITION_FAILED, MT_ERROR);
>-            display (formatException(result.value.getWrappedValue()), MT_ERROR);
>+            display(MSG_ERR_CONDITION_FAILED, MT_ERROR);
>+            try {

Nit: Newline before open brace.

>+                var safeVal = XPCSafeJSObjectWrapper(result.value.getWrappedValue());

Should this be |new XPCSafeJSObjectWrapper|?

>+                display(formatException(safeVal), MT_ERROR);
>+            }
>+            catch (ex)
>+            {
>+                display(MSG_ERR_CONDITION_SECURITY, MT_ERROR);
>+                display(formatException(ex), MT_ERROR);
>+            }


> function isDOMThing(o)
> {
>     const nsIDOMNode     = Components.interfaces.nsIDOMNode;
>     const nsIDOMDocument = Components.interfaces.nsIDOMDocument;
>-    return isinstance(o, nsIDOMNode);
>+    // Need to wrap our object because it is somewhere on the web and might
>+    // do nasty things when we QI it:
>+    try
>+    {
>+        return isinstance(XPCNativeWrapper(o), nsIDOMNode);

Should be |new XPCNativeWrapper|?


>@@ -465,13 +475,21 @@ function evalInTargetScope (script, reth
>     if (!getCurrentFrame().eval (script, MSG_VAL_CONSOLE, 1, rval))
>     {
>         if (rethrow)
>             throw rval.value;
>         
>         //dd ("exception: " + dumpObjectTree(rval.value));
>-        display (formatEvalException (rval.value), MT_ERROR);
>+        try
>+        {
>+            var safeVal = XPCSafeJSObjectWrapper(rval.value.getWrappedValue());

Should be |new XPCSafeJSObjectWrapper|?

>+            display(formatException(safeVal), MT_ERROR);
>+        }
>+        catch (ex)
>+        {
>+            display(getMsg(MSN_ERR_EVAL_SECURITY, ex), MT_ERROR);
>+        }

MSN_ERR_EVAL_SECURITY is undefined; you defined MSG_ERR_EVAL_SECURITY, with no replacements, so you'll be throwing |ex| away here even with the right message name.


> console.views.locals.onRowCommand =
> function lv_rowcommand(rec)
> {
>     if (this.isContainerEmpty(rec.childIndex) && "value" in rec.parentRecord)
>     {
>         dispatch ("change-value", 
>-                  {parentValue: rec.parentRecord.value, 
>+                  {jsdValue: rec.value,
>+                   expression: rec.expression,
>                    propertyName: rec.displayName});
>     }

I believe you've missed the watches's onRowCommand from this patch.

r-
New patch. Fixes all the issues mentioned in comment #33.

I reverted the change in cmdInspect for the exception. As for all the lacking "new"s, they were not strictly necessary (added them now) - some of the wrappers (maybe all of them - mrbkap?) will throw less if constructed without "new". No idea why. However, in all these cases, the objects themselves cause security errors, so all the calls had to be wrapped in a try ... catch *anyway*.

We should file a followup bug to have a good condition for cmdChangeValue in the watches window. In its current state, it is always disabled except for members of watched objects, which is silly. If I watch a local variable, I am perfectly entitled to change it. However, the condition should check whether the expression is a valid lhs for an assignment. Not sure how to do that, but that is definitely a separate bug. I've made this change anyway because it is an improvement - trying to change expressions which are obviously invalid as LHS ("foo" + "bar") will just give a JS syntax error, no harm done. :-)
Attachment #389922 - Attachment is obsolete: true
Attachment #391835 - Flags: review?(silver)
Attachment #389922 - Flags: review?(mrbkap)
Attachment #391835 - Flags: review?(mrbkap)
Comment on attachment 391835 [details] [diff] [review]
Patch v2, fixes 4 out of 6 issues.

>     console.menuSpecs["context:locals"] = {
>         getContext: this.getContext,
>         items:
>         [
>-         ["change-value", {enabledif: "cx.parentValue"}],
>+         ["change-value", {enabledif: "has('expression')"}],

Given the new syntax for /change-value requires the 'expression' argument, is the enabledif needed at all? (The MenuManager will ask the CommandManager isCommandSatisfied() so it should do all the work for you.)

> console.views.locals.onRowCommand =

As discussed on IRC, replace this with the same code as console.views.watches.onRowCommand.

>     console.menuSpecs["context:watches"] = {
>         getContext: this.getContext,
>         items:
>         [
>-         ["change-value", {enabledif: "cx.parentValue"}],
>+         ["change-value", {enabledif: "has('expression')"}],

As for context:locals.

> console.views.watches.onRowCommand =
> function wv_rowcommand(rec)
> {
>-    if ("value" in rec.parentRecord)
>-    {
>-        dispatch ("change-value", 
>-                  {parentValue: rec.parentRecord.value,
>-                   propertyName: rec.displayName});
>-    }
>+    var cx = this.getContext({});
>+    if ("expression" in cx)
>+        dispatch("change-value", cx);

  var cx = this.getContext({});
  var cm = console.commandManager;
  if (cm.isCommandSatisfied(cx, cm.commands["change-value"]))
      dispatch("change-value", cx);

If you can drop the has('expression') and use the isCommandSatisfied() version of locales.onRowCommand/watches.onRowCommand, I feel they'd be better. If not stick to the new version of watches.onRowCommand.

r=silver with either version.
Attachment #391835 - Flags: review?(silver) → review+
Attachment #391835 - Flags: review?(mrbkap) → review+
Comment on attachment 391835 [details] [diff] [review]
Patch v2, fixes 4 out of 6 issues.

The wrapper stuff looks good here. There's some talk in other bugs of automatically wrapping inside jsdIValue, but this is a decent solution in the meantime.

One thing I'm worried about is if someone adds a new use of getWrappedValue they will be default-vulnerable. I guess Venkman code doesn't change much, though, so maybe it isn't worth worrying about.
(In reply to comment #35)
> (From update of attachment 391835 [details] [diff] [review])
> >     console.menuSpecs["context:locals"] = {
> >         getContext: this.getContext,
> >         items:
> >         [
> >-         ["change-value", {enabledif: "cx.parentValue"}],
> >+         ["change-value", {enabledif: "has('expression')"}],
> 
> Given the new syntax for /change-value requires the 'expression' argument, is
> the enabledif needed at all? (The MenuManager will ask the CommandManager
> isCommandSatisfied() so it should do all the work for you.)

No, but we need a different check so as not to have it enabled for |this| and |scope|. I added this instead.

> > console.views.locals.onRowCommand =
> 
> As discussed on IRC, replace this with the same code as
> console.views.watches.onRowCommand.

Done, but with the same check mentioned above.

> >     console.menuSpecs["context:watches"] = {
> >         getContext: this.getContext,
> >         items:
> >         [
> >-         ["change-value", {enabledif: "cx.parentValue"}],
> >+         ["change-value", {enabledif: "has('expression')"}],
> 
> As for context:locals.

Indeed, got rid of this one entirely.

> > console.views.watches.onRowCommand =
> > function wv_rowcommand(rec)
> > {
> >-    if ("value" in rec.parentRecord)
> >-    {
> >-        dispatch ("change-value", 
> >-                  {parentValue: rec.parentRecord.value,
> >-                   propertyName: rec.displayName});
> >-    }
> >+    var cx = this.getContext({});
> >+    if ("expression" in cx)
> >+        dispatch("change-value", cx);
> 
>   var cx = this.getContext({});
>   var cm = console.commandManager;
>   if (cm.isCommandSatisfied(cx, cm.commands["change-value"]))
>       dispatch("change-value", cx);
> 
> If you can drop the has('expression') and use the isCommandSatisfied() version
> of locales.onRowCommand/watches.onRowCommand, I feel they'd be better. If not
> stick to the new version of watches.onRowCommand.
> 
> r=silver with either version.

Done.

Checked in, this is http://hg.mozilla.org/venkman/rev/406aa2c44ff8
Attachment #391835 - Attachment is private: true
Attachment #391835 - Attachment is private: false
What is the status here - is there still a bug that needs to be dealt with? Does this need to remain hidden?
(In reply to Josh Aas (Mozilla Corporation) from comment #38)
> What is the status here - is there still a bug that needs to be dealt with?
> Does this need to remain hidden?

I can't be sure because I think more backend security fixes have happened on trunk since this was filed. In any case, I'll try to look at this on Monday latest. You're absolutely right that this bug needs to die; please ping me if I haven't gotten to it by Tuesday.
Attachment #312693 - Attachment is obsolete: true
Attachment #312694 - Attachment is obsolete: true
Attachment #312695 - Attachment is obsolete: true
Attachment #312698 - Attachment is obsolete: true
Attachment #391835 - Attachment is obsolete: true
AIUI, these two test cases (16/17) should theoretically still be able to exploit vulnerabilities. Unfortunately the testcases are broken. First off because they have a trivial JS error (the length() setter should have at least one parameter, something Spidermonkey now enforces, breaking the entire script). Second because both use setting location.href to a JS string, which no longer works. Testcase 16 also seems to no longer work from a more general perspective (no exploit code seems to be running at all, while in 17 it at least opens about:mozilla in the iframe), and I'm not really sure why. I'll look at improving the current eval object code shortly, after which I think we can close this.
Assignee: gijskruitbosch+bugs → nobody
Component: Venkman JS Debugger → XUL Explorer
QA Contact: venkman → xul-explorer
Assignee: nobody → gijskruitbosch+bugs
Component: XUL Explorer → Venkman JS Debugger
QA Contact: xul-explorer → venkman
After checking with the JS folks, AIUI, the last few things here should be fine on Fx4 and later, because of the transparent wrappers being done by spidermonkey. What with JSDv1 going away, I think the best will be to close it, and leave it private until JSD1 is removed.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
(In reply to Gijs Kruitbosch from comment #40)
> while in 17 it at least opens about:mozilla in the iframe

For the record, this is due to bug 720619. Once bug 720619 is fixed, testcase 17 will not load the chrome page in the iframe.
Whiteboard: [sg:moderate] → [sg:moderate] keep hidden until bug 720619 fixed
Depends on: CVE-2012-4193
Group: core-security → core-security-release
Product: Other Applications → Other Applications Graveyard
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: