922161 - hide Browser Console JS input field if devtools.chrome.enabled is false

Status: VERIFIED FIXED

(DevTools :: Console, defect, P3)

Description

[:Gavin Sharp [email: [e-mail]]]

To protect against bug 664589-style attacks, disabling the browser console's chrome-privileged JS evaluator by default seems like the best tradeoff.

I propose just having its visibility controlled by the existing devtools.chrome.enabled pref.

Comment 2

[Henri Kinnunen]

I would like to take this one.

Comment 3

[Mihai Sucan [:msucan]]

Henri: thank you for your interest to help us with this bug!


Overview of the workflow:

1. Get the firefox source:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial

2. Build firefox:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

3. Make a patch:

https://developer.mozilla.org/en-US/docs/Creating_a_patch

4. Submit the patch:

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

Please make sure you ask for feedback from me. Set the patch flag feedback? and write my nick, :msucan.

If you read through the source code about something you do not know about, you may find documentation here:

1. Mozilla Developer Network - has a ton of info about XUL elements, HTML, JS, DOM, Gecko-specific APIs and more.

http://developer.mozilla.org/

2. MXR (Mozilla Cross-Reference) is a source code search engine - search for symbols you want to learn about, eg. nsIDocument.

http://mxr.mozilla.org/mozilla-central/

3. DXR is a smart source code search tool, similar to MXR but sometimes better.

http://dxr.mozilla.org


Here's an overview of how you can fix this bug:

Edit browser/devtools/webconsole/webconsole.js. Find the JST_init() function and hide the |inputNode| when devtools.chrome.enabled is false. To read the preference use Services.prefs.getBoolPref(). You can find examples in the same JS file which use Services.prefs. Also check if this.hud.owner._browserConsole is true - we only want to hide the input in the browser console. Last, but not least, when you hide the inputNode, make sure you change the code to not add any event listeners to the input. You might need to hide the completeNode as well, or just hide the parent container. To see the markup, look at webconsole.xul file, in the same folder.


If you have any questions, please ask me here. Use the needinfo flag and write my nick, :msucan.

I will explain how you can write a test once you have a working patch.

Thanks and good luck!

Comment 4

[Henri Kinnunen]

Attachment: disable browser console if devtools.chrome.enabled == false

Comment 5

[Henri Kinnunen]

Sould we somehow tell the user that the console input is not available unless the user checks the devtools.chrome.enabled checkbox? I think it's not very clear at the moment that why the console input field is missing but the console itself is visible.

Comment 6

[Mihai Sucan [:msucan]]

(In reply to Henri Kinnunen from comment #5)
> Sould we somehow tell the user that the console input is not available
> unless the user checks the devtools.chrome.enabled checkbox? I think it's
> not very clear at the moment that why the console input field is missing but
> the console itself is visible.

Good question. I would say yes. However, I am not sure if we should do this - it might defeat the purpose of this bug.

Gavin, should we add a message letting users know the JS input is disabled? What kind of wording would be acceptable? "The JavaScript input field is disabled. To enable it go to about:config and set devtools.chrome.enabled to true." ? Should we make the message a link with a callback that toggles the pref on click? Thank you!

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 8349022 [details] [diff] [review]
disable browser console if devtools.chrome.enabled == false

Review of attachment 8349022 [details] [diff] [review]:
-----------------------------------------------------------------

Henri, thank you for the patch!

With this patch, the Browser Console stops working if I set devtools.chrome.enabled to false. I get exceptions related to the inputNode.

This patch includes trailing whitespace. Please remove it.

::: browser/devtools/webconsole/webconsole.js
@@ +3054,3 @@
>      this.completeNode = doc.querySelector(".jsterm-complete-node");
> +
> +    this.jsTermInputContainer = doc.querySelector("hbox.jsterm-input-container")

The jsterm input container is not needed outside this function. Just make it a variable, let inputContainer = ...

@@ +3058,5 @@
> +    if (!Services.prefs.getBoolPref("devtools.chrome.enabled") && this.hud.owner._browserConsole === true) {
> +      this.jsTermInputContainer.style.display = "none"
> +    } else {
> +      this.jsTermInputContainer.style.display = "block"
> +      this.inputNode = doc.querySelector(".jsterm-input-node");

You should have this.inputNode set, irrespective of the preference. Just dont add the event listeners.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mihai Sucan [:msucan] from comment #6)
> Gavin, should we add a message letting users know the JS input is disabled?
> What kind of wording would be acceptable? "The JavaScript input field is
> disabled. To enable it go to about:config and set devtools.chrome.enabled to
> true." ? Should we make the message a link with a callback that toggles the
> pref on click? Thank you!

I don't see why we'd do that. The idea is that it shouldn't be easy to get a privileged console, and I don't really see understand why you think users would have an expectation of there being a console in the "Browser Console". Is the concern that they'd expect it because the similar-looking Web Console has one? The two are pretty clearly separately labeled, so I wouldn't really worry about that.

Comment 9

[Frederik Braun [:freddy]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #8)
> The idea is that it shouldn't be easy to get a privileged console...

Just a side-note: It's still easy to get one, if you pick a privileged chrome page, like about:about and open the devtools console

Comment 10

[Mihai Sucan [:msucan]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #8)
> (In reply to Mihai Sucan [:msucan] from comment #6)
> > Gavin, should we add a message letting users know the JS input is disabled?
> > What kind of wording would be acceptable? "The JavaScript input field is
> > disabled. To enable it go to about:config and set devtools.chrome.enabled to
> > true." ? Should we make the message a link with a callback that toggles the
> > pref on click? Thank you!
> 
> I don't see why we'd do that. The idea is that it shouldn't be easy to get a
> privileged console, and I don't really see understand why you think users
> would have an expectation of there being a console in the "Browser Console".
> Is the concern that they'd expect it because the similar-looking Web Console
> has one? The two are pretty clearly separately labeled, so I wouldn't really
> worry about that.

The question came because, yes, there's strong resemblance between the web console and the browser console, and we have releases of firefox that include the input - hiding it without notice might surprise users. Also, the error console had a js input, by default, which will make the browser console less useful than the error console was.

Simply hiding the js input in the browser console does not make it read-only either. We still have the object inspector, which shows up for any console.foo() method. Users can inspect and edit properties in objects. It seems we should also disallow editing of properties when devtools.chrome.enabled = false.

As Frederik points out, it's easy to bring back chrome privileges with the web console on a chrome privileged page. It is equally simple to just open the devtools, go to options and pick 'enable chrome debugging' (which turns devtools.chrome.enabled to true).

Comment 11

[Mihai Sucan [:msucan]]

(In reply to Henri Kinnunen from comment #5)
> Sould we somehow tell the user that the console input is not available
> unless the user checks the devtools.chrome.enabled checkbox? I think it's
> not very clear at the moment that why the console input field is missing but
> the console itself is visible.

Henri, please also disallow the editing/renaming/deleting of properties in the object inspector. Please update JST__updateVariablesView(). You will see we check if the variables view displays an object actor (aOptions.objectActor) and we set the |eval|, |switch| and |delete| functions, otherwise we set those to null so no changes can be made. Add a check for devtools.chrome.enabled, such that we get a read-only object inspector if the pref is false.

Thank you!

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to Frederik Braun [:freddyb] from comment #9)
> Just a side-note: It's still easy to get one, if you pick a privileged
> chrome page, like about:about and open the devtools console

That's harder than "Click Tools->Browser Console".

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #12)
> (In reply to Frederik Braun [:freddyb] from comment #9)
> > Just a side-note: It's still easy to get one, if you pick a privileged
> > chrome page, like about:about and open the devtools console
> 
> That's harder than "Click Tools->Browser Console".

Or even using the keyboard shortcut to open the browser console. IMHO that is the only attack scenario worthy of extra countermeasures:

"Pres Ctrl-Shift-J and then copy & paste the following s3cr3t 1337 code to get a million more chicken for your farm!"

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Fixing this bug as summarized addresses that case.

Comment 15

[Mihai Sucan [:msucan]]

Henri, sorry to bother you, do you have any progress with the patch? Please let us know if you need any help. Thanks!

Comment 16

[Henri Kinnunen]

No that's fine, I just haven't had time to work on this. I've had such a busy holidays but I hope that I can continue on this today.. :) 

I think that I have enough information to implement this feature but if not, then I'll ask more help. Thanks for asking. :)

Comment 17

[Henri Kinnunen]

Attachment: disable browser console and object inspector edit mode if devtools.chrome.enabled == false

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 8364575 [details] [diff] [review]
disable browser console and object inspector edit mode if devtools.chrome.enabled == false

Review of attachment 8364575 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updated patch.

Do you have time to write a test? To add a test, please learn about browser chrome tests:

https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

Go to browser/devtools/webconsole/test and copy one of the "browser_" JS test files. Name the new file as you see fit, add the new file name to the list of tests in browser.ini. Update the test to test the bug you are fixing. Run mach/make to update the local build, then run the test as explained in the provided link.

::: browser/devtools/webconsole/webconsole.js
@@ +3053,5 @@
> +    let inputContainer = doc.querySelector("hbox.jsterm-input-container");
> +
> +    this.inputNode = doc.querySelector(".jsterm-input-node");
> +
> +    if (!Services.prefs.getBoolPref("devtools.chrome.enabled")) {

This is hiding the input in the web console as well. We need to only hide the input in the browser console.

Please also check if this.owner._browserConsole is true.

(update both location where you read the pref)

Comment 19

[Henri Kinnunen]

Should I also disable the edit mode of object inspector only in the browser console or in both web and browser consoles?

Comment 20

[Mihai Sucan [:msucan]]

(In reply to Henri Kinnunen from comment #19)
> Should I also disable the edit mode of object inspector only in the browser
> console or in both web and browser consoles?

Yes, also disable edit mode of object inspector, only in the browser console. Thanks!

Comment 21

[Jesse Ruderman]

Please do the same for the browser console (⌘⌥K) when it is pointed at a privileged page such as about:home. Keeping in mind that you can navigate from an untrusted page to a privileged page or vice versa.

We should also do something for unprivileged pages to prevent account hijacking (bug 971613), but that will probably need a different solution and different pref(s).

Comment 22

[Joe Walker [:jwalker]]

(In reply to Jesse Ruderman from comment #21)
> Please do the same for the browser console (⌘⌥K) when it is pointed at a
> privileged page such as about:home. Keeping in mind that you can navigate
> from an untrusted page to a privileged page or vice versa.

The browser console is ⌘⌥J, and it points to the browser chrome window rather than a web page.

Assuming this is in relation to the Facebook self-xss problems, please could you explain what the self-xss instructions would look like which used about:home to attack a user? (Perhaps a separate bug would be best)

Comment 23

[Jesse Ruderman]

Self-XSS using about:home would look like:

1. ⌘T
2. ⌘⌥K
3. Components

Comment 24

[Henri Kinnunen]

Attachment: disables jsterm input and does not set object inspector properties when devtools.chrome.enabled == false tests included.

Comment 25

[Mihai Sucan [:msucan]]

Comment on attachment 8376999 [details] [diff] [review]
disables jsterm input and does not set object inspector properties when devtools.chrome.enabled == false  tests included.

Review of attachment 8376999 [details] [diff] [review]:
-----------------------------------------------------------------

The test for browser_console_dead_objects.js fails with this patch applied because the input field is hidden by default. I added a setBoolPref() call to enable the input and the test passes.

I sent the patch to the try servers: https://tbpl.mozilla.org/?tree=Try&rev=27a2014545aa

If this is green, I will land the patch. Thanks for your work!

::: browser/devtools/webconsole/test/browser_console_hide_jsterm_when_devtools_chrome_enabled_false.js
@@ +12,5 @@
> +*    -webconsole object inspector properties should be set.
> +*/
> +
> +function testObjectInspectorPropertiesAreNotSet(variablesView) {
> +  is(variablesView.eval, null);

For future reference: every is() call needs to take 3 arguments. the third gives the check a name that we can read in the logs after TEST-PASS.

Comment 26

[Mihai Sucan [:msucan]]

Attachment: bug922161-1.diff

Try push was green. This is the patch with minor changes (nits).

Landed: https://hg.mozilla.org/integration/fx-team/rev/23e724fda922

Thank you Henri!

Comment 27

[Henri Kinnunen]

(In reply to Mihai Sucan [:msucan] from comment #26)
> Created attachment 8378222 [details] [diff] [review]
> bug922161-1.diff
> 
> Try push was green. This is the patch with minor changes (nits).
> 
> Landed: https://hg.mozilla.org/integration/fx-team/rev/23e724fda922
> 
> Thank you Henri!

Woohoo! \o/ Thank you! :)

Comment 28

[Mihai Sucan [:msucan]]

Will: we need MDN docs for the browser console to mention to users that the devtools.chrome.enabled pref needs to be set to true, if they want the JS input. Thanks!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23e724fda922

Comment 30

[Paul Silaghi, QA [:pauly]]

Now the input field is displayed in the browser console only if devtools.chrome.enabled=TRUE.
Verified fixed FF 30.0a1 (2014-02-21), Win 7, Mac OS X 10.8.5, Ubuntu 13.04.

Comment 31

[Will Bamberg [:wbamberg]]

documented -> https://developer.mozilla.org/en-US/docs/Tools/Browser_Console#Browser_Console_command_line