Closed Bug 423796 Opened 16 years ago Closed 16 years ago

Content can exploit FireBug through console.log

Categories

(Core :: Security, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Assigned: johnjbarton)

References

()

Details

Broken out of bug 344751. From moz_bug_r_a4:

* content can abuse console.log().  When content does |console.log(x);|,
Firebug code accesses properties of x.

Demo in attachment 307990 [details].

If this can be fixed in FireBug that would be ideal. Though it's possible that some new type of wrapper is needed here.
Flags: blocking1.9?
Blocks: 423949
(In reply to comment #0)

> * content can abuse console.log().  When content does |console.log(x);|,
> Firebug code accesses properties of x.
...
> If this can be fixed in FireBug that would be ideal. Though it's possible that
> some new type of wrapper is needed here.

Firebug accesses the properties of user-page objects everywhere, why is this case special?

Because the object passed in is a JS object, not a DOM object, and thus does not get any type of wrapper created.

At least I think that's what happens.

That said, what happens if someone overrides for example the .firstChild getter to return a JS-object? Will we not wrap that JS-object?
I have removed xpcnativewrappers=yes, and now I issue:
  win.wrappedJSObject.console = new FirebugConsole(context,win);
where win is the user-level window object.  Is it safe?
Removing xpcnativewrappers=yes from firebug does not solve this testcase. However I now get 
out of memory
XPCSafeJSObjectWrapper.cpp
Line 446
Please file that as a separate bug as it's unlikely related to anything we need to do in this bug.
> I have removed xpcnativewrappers=yes, and now I issue:
>   win.wrappedJSObject.console = new FirebugConsole(context,win);
> where win is the user-level window object.  Is it safe?

It's no less safe than just doing win.console = new FirebugConsole(context,win); with xpcnativewrappers=no, so it's definitely what you should do for now.

However that part isn't really what this bug is about. The issue here is how you deal with the object you are being passed when the log function is being called.

Blake/jst: Is fixing this bug as simple as having firebug manually wrap the passed in object in a SJOW? At least until we are able to do that automatically. Is there a way to do that currently?

Or even simpler, can you use Object.prototype.toString.apply(passedInArg)?
A modest proposal:

- console.log becomes a tiny function without privileges, evalInSandboxed into the window's scope so that it's visible to content
- console.log generates a "firebug-log" or "console-log" event and dispatches it with the appropriate string contents
- firebug listens for that event in chrome, and stick it in the console receptacle

That would mean that we don't have to put chrome-privileged console object or log function where content can see it.

This would also have the benefit that dispatching the event is harmless (other than some perf cost) even if FB isn't listening, so devs and toolkits could just leave it in there without breaking the site for non-Firebug users.
(In reply to comment #8)
> A modest proposal:
> ...

This is very much along the lines I have been thinking. (Though I was not planning to use evalInSandbox, no surprise I suppose). 

The Firebug Console panel gets input from user-level code in two ways: from dev-calls in the page to the "console" object and from command-line evaluations entered from Firebug. The results from these operations are similar, an arbitrary user-level object. So both paths need to communicate the user-level object to Firebug Console, securely and synchronously (since we are tracing). 

For security we want to "leave the object in the page", so we create an array of console objects and pass the index to the array through the event. On the Firebug side we handle the array and its objects through the wrapper.

For synchronous we need to investigate, I think a lot of window events are synchronous even if error console events seem to be off.

Assuming this works, the remaining bits are injecting the console object and the command-line expressions. My plan is to pre-/post-pend <script> tags, because I know that the semantics then must be exact the best we can do. 

For security this is only strings; for getting the time right, it is harder for the console-object prepend because I don't know how much control we have on when dynamic scripts are injected. 

Quite a bit of code to get right, so I want to test the assumptions before setting off.
(In reply to comment #8)
> A modest proposal:
> 
> - console.log becomes a tiny function without privileges, evalInSandboxed into
> the window's scope so that it's visible to content

Ok this part works.

> - console.log generates a "firebug-log" or "console-log" event and dispatches
> it with the appropriate string contents

How does that "with the appropriate string" bit work? event.foo does not survive the trip from the page to the extension.
(In reply to comment #10)
> > - console.log generates a "firebug-log" or "console-log" event and dispatches
> > it with the appropriate string contents
> 
> How does that "with the appropriate string" bit work? event.foo does not
> survive the trip from the page to the extension.

You have to use attributes, yeah, not properties:

http://developer.mozilla.org/en/docs/Code_snippets:Interaction_between_privileged_and_non-privileged_pages#Sending_data_from_unprivileged_document_to_chrome
(In reply to comment #11)
> You have to use attributes, yeah, not properties:

You have to use attributes on elements, that is, not on the event.  Sorry, not at my best tonight!

> http://developer.mozilla.org/en/docs/Code_snippets:Interaction_between_privileged_and_non-privileged_pages#Sending_data_from_unprivileged_document_to_chrome
> 

Depends on: 425139
(In reply to comment #8)
> A modest proposal:
Ok the new console prototype succeeded in one case.  The Demo in attachment 307990 [details] no longer succeeds. This is not as good as it could be, since I get a (bogus) out of memory error, 422137. So this needs to be retested. The results are on https://fbug.googlecode.com/svn/branches/firebug1.2/ at R452.
Assignee: nobody → johnjbarton
We want FB 1.x to work with FF3 - so marking this as blocking so we resolve one way or the other. 
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
The only block for console tests is one I posted to the newsgroup:

If in Firebug I do:
    if (o instanceof Window)
I sometimes get
    invalid 'instanceof' operand Window

Other places issue:
...    !(win.parent instanceof Window)
and get
    Window is not defined

With FF3b5 rc I now fail on
  this.notifyFirebug(Array.prototype.slice.call(arguments, 1), "assert");
because Array.prototype is undefined??

I tried the same code in FF2 and it works fine.
With further investigation I believe that both problem in comments 15 and 16 arise from using evalInSandbox to inject the console code.  So for the time being I am making this work depend upon progess on 421593
Depends on: 421593
No longer depends on: 425139
Flags: blocking1.9+
John: why did you remove the blocking1.9+ flag?
Flags: blocking1.9?
Gavin,  right now 425139 blocks Firebug 1.2, but if that is fixed Firebug can fix 421593 423947 423949  and this one, 423796. 

In particular, Firebug 1.2 has a new implementation of 'console.log' which prevents the exploit attached to this bug. I'll verify that with the release version before marking this fixed.
Flags: blocking1.9? → blocking1.9-
The error message that results when the test case for this bug is run is incorrect in two ways. 
  1) The file name and line number are wrong:[fileName]=XPCSafeJSObjectWrapper.cpp;
[lineNo]=445;
  2) The error level is marked as "info" rather than "error"
I apologize for assigning this bug to myself.  I don't know what will happen if I change the assignment value. Would someone who understands the assignment issues please change it?  Thanks.
Can we move the error-message issues to another bug (iirc, bugs already exist for this)?  This one is about a privilege escalation exploit.  JJB:  You should be the owner for this, if the fix must come from Firebug (as I think it does).  Is it fixed?
 Bug 443424 
This is just what I mean about 'systematic' bias against fixing error messages. The security bug will get top priority until it is half fixed. Then when its 'just' the error message that is wrong, file a new bug which never gets fixed.
(In reply to comment #23)
>  Bug 443424 
> This is just what I mean about 'systematic' bias against fixing error messages.
> The security bug will get top priority until it is half fixed. Then when its
> 'just' the error message that is wrong, file a new bug which never gets fixed.

That's not a "systematic" bias against fixing error messages, it's a bias against letting error reporting re-architecture block progress on security fixes. Surely you can understand that error reporting bugs won't always be given the same priority as security bugs.
> That's not a "systematic" bias against fixing error messages, it's a bias
> against letting error reporting re-architecture block progress on security
> fixes. Surely you can understand that error reporting bugs won't always be
> given the same priority as security bugs.
> 
The part I don't understand is why you separate error reporting from security function. By arbitrarily dividing this security feature into 'security-function' and 'error-reporting', combined with triage, we cause extra work for everyone by having poor quality error messages. 

To me there is one feature whose implementation is half done. By choosing to divide this into two bugs we create an 'important' part and a 'nice to have' part. Guess what, 'nice to have' often does not get fixed.

Now step back and see what happens when this is done systematically. A large number of features are divided into 'function' and 'error-reporting'. Given finite time, the long list is triaged.  When the line is drawn, large numbers of features have their 'error-reporting' half below the cut off. The result is a system with many error messages like -228 Download error or 'info' about XPCSafeJSObjectWrapper.cpp.

(In reply to comment #25)
> To me there is one feature whose implementation is half done. By choosing to
> divide this into two bugs we create an 'important' part and a 'nice to have'
> part. Guess what, 'nice to have' often does not get fixed.

If you really can't (or won't) distinguish the two problems, and aren't able to understand that it is reasonable that they would often be given different priorities, then I'm not sure there is merit to further debating the point.
(In reply to comment #25)
> > That's not a "systematic" bias against fixing error messages, it's a bias
> > against letting error reporting re-architecture block progress on security
> > fixes. Surely you can understand that error reporting bugs won't always be
> > given the same priority as security bugs.
> > 
> The part I don't understand is why you separate error reporting from security
> function. By arbitrarily dividing this security feature into
> 'security-function' and 'error-reporting', combined with triage, we cause extra
> work for everyone by having poor quality error messages. 

No, we cause security bugs to be fixed in a more timely manner than if they had to wait for gold-encrusted perfection in every related part of the architecture.

We separate security from error reporting because the former can result in *arbitrary code execution on the computers of every Firebug user*, and the latter makes for a more frustrating debugging experience.  I am frankly shocked that you don't understand why we believe that those problems should be treated differently in priority.

(I'll also note that our record on fixing error reporting bugs is pretty good once there is a minimal test case for them; see the "property denied" bug and others from the FF3 cycle -- why don't people make it a higher priority to build such test cases, and triage that effort against all the other things they could be doing with their time, omg?!?)
However, the security problem is fixed (timeliness is not an issue), the test case is attached to this bug report (no need to convince someone that it is important), and the people who understand the issue are all CC on the bug. By add a small effort to this bug, a problem is fixed. By creating a new report the problem joins a thousand others and I don't see any process -- other than my ranting -- that will cause it to ever be seen again.  

I completely understand the idea that, in a normal development process, the bug 'should' be split off and 'should' be recorded so its not lost. But realistically the scale of this project means that some of the desires of the development team are not realized. Many thousands of bugs cannot be fixed. By treating error and normal path code as one problem, many simple fixes can be rolled into work at a nominal cost and a long term improvement.

(And I'll try to get more testcases on error message bugs)
JJB:  If you're simply hoping to retain the audience from this bug, just use the "Clone This Bug" feature of bugzilla.  This bug, afaict, is fixed.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.