1659129 - (CVE-2021-23985) It is not apparent when Devtools remote debugging is enabled [ZDI-CAN-11725]

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Tom Ritter [:tjr]]

Attachment: firefox.pdf

The attached file comes via ZDI, and reports that when the Devtools dbugging is in an insecure configuration, such as devtools.debugger.prompt-connection is false and remote-enabled is true; there is no visual indicator to the user; and that this has been abused by malware in the past.

Comment 1

[Daniel Veditz [:dveditz]]

Under a similar "testing" configuration, marionette/puppeteer (and I hope WebDriver) show something like a robot-head in the URL bar when the browser is under remote control. We could re-use that, or something similar for this state? Might be good enough to do when you're actually under remote control, though ZDI is asking that the insecure state be flagged whether there's a connection or not.

Comment 2

[:Gijs (he/him)]

This isn't specific to the debugger.

Comment 3

[Daniel Veditz [:dveditz]]

Note that the PDF claims they've seen malware actually use this trick in the wild.

Since turning off the prompt is only intended for an automated-testing scenario it probably makes sense to show the UI change whenever the prompt is disabled whether or not there's a remote connection.

Comment 4

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #3)

Note that the PDF claims they've seen malware actually use this trick in the wild.

Since turning off the prompt is only intended for an automated-testing scenario it probably makes sense to show the UI change whenever the prompt is disabled whether or not there's a remote connection.

I think we also turn off the prompt for local builds? From a frontend perspective, I'd like to keep that.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Daniel Veditz [:dveditz] from comment #1)

Under a similar "testing" configuration, marionette/puppeteer (and I hope WebDriver) show something like a robot-head in the URL bar when the browser is under remote control. We could re-use that, or something similar for this state? Might be good enough to do when you're actually under remote control, though ZDI is asking that the insecure state be flagged whether there's a connection or not.

Maja, can you please provide more info about how DevTools could reuse this?

(In reply to :Gijs (he/him) from comment #4)

I think we also turn off the prompt for local builds? From a frontend perspective, I'd like to keep that.

Correct and I also vote for keeping it.
https://searchfox.org/mozilla-central/rev/2f9eacd9d3d995c937b4251a5557d95d494c9be1/modules/libpref/init/all.js#4703

Honza

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

The robot icon was added to the Remote Protocol implementation in Bug 1526364. Both it and Marionette fire a remote-listening observer notification, which triggers that visual cue among other things. So, I think DevTools could reuse this pretty easily.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the pointer Maja!

Can you please also add a screenshot showing how this looks like?

Comment 8

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Remote Protocol visual cue UX

Comment 9

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

You can see it by running ./mach run --remote-debugging-port=0

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks Maja!

Nicolas, Julian any opinions on this?

Honza

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

One thing that came to my mind when thinking that we would show a different UI is that it could impact Firefox engineers working on the URL bar, and wanting to use the Browser Toolbox. They wouldn't see the end result unless they don't have the pref, which would mean not having access to the inspector, which might make it a bit more difficult to work on that.

Comment 12

[:Gijs (he/him)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)

One thing that came to my mind when thinking that we would show a different UI is that it could impact Firefox engineers working on the URL bar, and wanting to use the Browser Toolbox. They wouldn't see the end result unless they don't have the pref, which would mean not having access to the inspector, which might make it a bit more difficult to work on that.

I guess we could avoid using this UI for local developer builds? For work on infra builds, I'm sure folks working on the URL bar can figure out how to remove the attribute that causes this styling.

Speaking of which... in principle, once there's an active debugger connection, surely there is no way to prevent said debugger connection to remove the UI attributes in question? In other words, how would we create a "visual indicator" that wasn't removable by the attacker, if they already have a debugger (or remote protocol) connection? :-\

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

Just attaching a screenshot of the suggested URL bar UI (the one used be Marionette/Puppeteer) so, anyone can quickly see it.

Honza

Comment 14

[Frederik Braun [:freddy]]

The reporter have send us an email whether we have a fix scheduled for this one.

Comment 15

[Frederik Braun [:freddy]]

Honza, you cleared the needinfo without providing any response.
As the triage owner, can you help us figure this one out?
The reporters have asked us for a timeline by sending an email to [e-mail].
We generally do not want to be unresponsive when security researchers reach out.

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Ah, sorry, that was not intended!

We were discussing this at our team meeting yesterday and we agree that (see attached screenshot of the suggested URL bar UI in comment #13) is the way to go here. We can also see how it's done for Marionette/Puppeteer, get some inspiration and perhaps even share code.

It isn't clear how we can completely prevent an attacker from removing UI attributes (see comment #12), but at least introducing this special UI would be another step in making this more secure.

This isn't on our roadmap in Q1/2021, but we could work on this in Q2/Q3

Honza

Comment 17

[Frederik Braun [:freddy]]

FYI, ZDI will disclose this issue to the public on February 4th.

Comment 18

[Julian Descottes [:jdescottes]]

Will try to work on this before the disclosure date.

(In reply to Frederik Braun [:freddy] from comment #17)

FYI, ZDI will disclose this issue to the public on February 4th.

I can't see any reference to February 4th earlier in the bug.
Freddy: Is there anyway to know about such deadlines earlier?

Comment 19

[Frederik Braun [:freddy]]

Not really, they just started poking us again earlier in the year (see comment 14).
Our noncommittal caused them to press for disclosure.

Remember that this is still flagged as low-severity.
I'd really like to see it fixed, but I also wouldn't suggest you drop everything for this minor bug.

Comment 20

[Julian Descottes [:jdescottes]]

(In reply to Frederik Braun [:freddy] from comment #19)

Not really, they just started poking us again earlier in the year (see comment 14).
Our noncommittal caused them to press for disclosure.

Remember that this is still flagged as low-severity.
I'd really like to see it fixed, but I also wouldn't suggest you drop everything for this minor bug.

Ok thanks for the clarification!

On the topic of the actual fix, I wonder if adding an icon in the URL bar is actually going to be that effective.
What about locking down the prefs to false on Release & Beta?
They could still be toggled manually from the DevTools UI.
We need to add a checkbox for devtools.debugger.prompt-connection, but that's a minor thing.

Attackers would have to trick users into going into DevTools settings and select the options.
Modifying prefs.js would not override the locked preferences.

Maybe we can do both.

Comment 21

[Frederik Braun [:freddy]]

Your idea sounds neat, but I don't know if there are tools out there that drive & automate Firefox Release & Beta by overwriting prefs.js...

Comment 22

[Julian Descottes [:jdescottes]]

Attachment: Bug 1659129 - [devtools] Show a visual cue in the URL bar when DevTools is listening for incoming connections

Comment 23

[Julian Descottes [:jdescottes]]

Attachment: Bug 1659129 - [devtools] Show a visual cue in the URL bar when DevTools has an opened socked

Alternate approach without a preference.
The visual is programmatically disabled for Browser Toolbox scenarios

Comment 24

[Julian Descottes [:jdescottes]]

Let's go ahead with the visual cue for now and keep discussing about pref locking (which will probably move to a follow up if we go for it)

My initial assumption with locked prefs was that you could unlock the pref, change the value, relock the pref. That would have been nice to make it only update-able from devtools UI. But it doesn't work exactly like that. When you relock the pref you actually set it back to its locked value.
So the way this would work is that the prefs would be locked until the user updates them in the devtools settings. After that they'd be effectively unlocked. That still sounds like a nice way to reduce the risk for potential attacks. Provided there are not too many workarounds (see below).

(In reply to Frederik Braun [:freddy] from comment #21)

Your idea sounds neat, but I don't know if there are tools out there that drive & automate Firefox Release & Beta by overwriting prefs.js...

I wanted to ask about potential limitations of locking prefs, thanks for jumping in :)

So, we'd be worried about attackers distributing unoffical Firefox versions with a different source code?
Or am I misunderstanding?

From what I tested, changing prefs.js in the user profile doesn't change the value of a locked preference.
They'd need to update the value set in https://searchfox.org/mozilla-central/source/modules/libpref/init/all.js and build Firefox to change the locked value?

Comment 25

[Frederik Braun [:freddy]]

No, my concern was that someone might use browser debugging for a QA/CI scenario (like Selenium) and we're breaking it.

TBH, I don't know much about "locked preferences" and how they work. Maybe I need a bit of a better explanation.

Comment 26

[Julian Descottes [:jdescottes]]

That's a good point, it could indeed be annoying on CI. And I don't know much about locked preferences either, I'm mostly exploring the option for now.

Comment 27

[Julian Descottes [:jdescottes]]

Hi freddy,

Quick nudge, in case you missed the review request. Let me know if you can't look at it for now. Note: the code has been reviewed by devtools peers, but I felt like it would be good to have a signoff from a security peer.

Thanks!

Comment 28

[Frederik Braun [:freddy]]

Sorry, I had indeed missed the review request.

Comment 29

[Julian Descottes [:jdescottes]]

no worries, thanks for the review!

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/9e3376ccd1da2b2b065b121e05dd77ffdcf862a6

Backed out for causing failure on test_devtools_socket_status.js:
https://hg.mozilla.org/integration/autoland/rev/65b713bb42dc7c4b1d1d201a357d56f6c9d2725e

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=9e3376ccd1da2b2b065b121e05dd77ffdcf862a6
Failure log: https://treeherder.mozilla.org/logviewer?job_id=330144562&repo=autoland

[task 2021-02-16T20:41:23.365Z] 20:41:23 INFO - TEST-PASS | devtools/shared/security/tests/xpcshell/test_devtools_socket_status.js | - No DevTools socket is opened for connections - true == true
[task 2021-02-16T20:41:23.365Z] 20:41:23 INFO - (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2021-02-16T20:41:23.365Z] 20:41:23 INFO - (xpcshell/head.js) | test finished (2)
[task 2021-02-16T20:41:23.365Z] 20:41:23 INFO - (xpcshell/head.js) | test run_next_test 1 finished (1)
[task 2021-02-16T20:41:23.365Z] 20:41:23 INFO - exiting test
[task 2021-02-16T20:41:23.365Z] 20:41:23 INFO - PID 16680 | [Parent 16680, Main Thread] WARNING: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS', file /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3365
[task 2021-02-16T20:41:23.366Z] 20:41:23 INFO - PID 16680 | \x07[Parent 16680, Main Thread] ###!!! ASSERTION: Component Manager being held past XPCOM shutdown.: 'cnt == 0', file /builds/worker/checkouts/gecko/xpcom/build/XPCOMInit.cpp:778
[task 2021-02-16T20:41:23.366Z] 20:41:23 INFO - PID 16680 | #01: XRE_XPCShellMain(int, char**, char**, XREShellData const*) [js/xpconnect/src/XPCShellImpl.cpp:1383]
[task 2021-02-16T20:41:23.367Z] 20:41:23 INFO - PID 16680 | #02: main [js/xpconnect/shell/xpcshell.cpp:76]
[task 2021-02-16T20:41:23.373Z] 20:41:23 INFO - PID 16680 | #03: __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6 + 0x21b97]
[task 2021-02-16T20:41:23.373Z] 20:41:23 INFO - PID 16680 | #04: ??? [/builds/worker/workspace/build/application/firefox/xpcshell + 0x3ff79]
[task 2021-02-16T20:41:23.373Z] 20:41:23 INFO - PID 16680 | #05: ??? (???:???)

Comment 31

[Julian Descottes [:jdescottes]]

Looks like the new test fails on debug builds. Investigating.
Edit: It seems that there is an issue linked to the following lazyRequireGetter in the head_dbg.js of the folder:

loader.lazyRequireGetter(
  this,
  "SocketListener",
  "devtools/shared/security/socket",
  true
);

If you don't consume the lazy getter before the end of the test, the test fails.

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/a0c2169b01002f05855dfb5e20b4bcf05d02dd68
https://hg.mozilla.org/mozilla-central/rev/a0c2169b0100

Comment 33

[:Gijs (he/him)]

I noticed on recent nightly that if I open the browser, then open the browser toolbox, it doesn't affect the window that's already open, though it does affect new windows that are opened. Is that intentional?

Comment 34

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #33)

I noticed on recent nightly that if I open the browser, then open the browser toolbox, it doesn't affect the window that's already open, though it does affect new windows that are opened. Is that intentional?

Oh, never mind, I see this is being dealt with in bug 1693626.

Comment 35

[Julian Descottes [:jdescottes]]

Yes sorry about this, it should not affect any window when only using the BrowserToolbox.

Comment 36

[Tom Ritter [:tjr]]

Attachment: advisory.txt