Closed Bug 959111 Opened 6 years ago Closed 6 years ago

Connecting dev-tools to the Firefox OS remote debugging allows chrome script execution

Categories

(DevTools :: Debugger, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

(blocking-b2g:koi+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
Firefox 29
blocking-b2g koi+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: arroway, Assigned: ochameau)

Details

(Keywords: sec-high, Whiteboard: [adv-main28-])

Attachments

(1 file, 1 obsolete file)

You need physical access to perform the following :
1. Start the server debugger on a production locked phone
2. Connect with the connection screen in the devtools
3. You now have access to the main console via a web console, with a root js console.
4. You can change the settings tools.debugger.forbid-certified-apps from true to false

Firefox OS 1.2 to 1.4 are impacted.
I can confirm 1.1 release phones are unaffected. Would this be the status-b2g18 Tracking Flag?
More detailed STR:

1. in Gaia setting, check "Remote Debugger"
2. on a laptop, "adb forward tcp:6000 localfilesystem:/data/local/debugger-socket"
3. In Firefox Desktop, turn on the "devtools.debugger.remote-enabled" pref to True
4. Load this page: chrome://browser/content/devtools/connect.xhtml
5. Connect to localhost:6000
6. Click "Main Process"

The console that shows up is connected to shell.js (top root process).
Renaming since the issue here is that remote debugging service on the phone allows access to connect to chrome (shell.js). Changing the tools.debugger.forbid-certified-apps pref is just one thing you can do, but iiuc you have chrome script, ie root access. 

The attack is trivial, but you do need local access to the phone. Having a PIN set on the lockscreen will prevent this attack since the remote debugger will not connect unless the lockscreen is unlocked. 

From a risk standpoint I would say this either sec-high or sec-critical. Its local only, but it is somewhat equivalent to shipping a phone with adb running as root, which makes me think this really should be sec-critical. We really should get this in 1.2 if at all possible at this point.
blocking-b2g: --- → koi?
Keywords: sec-high
Summary: On a production locked phone, tools.debugger.forbid-certified-apps can be modified → Connecting dev-tools to the Firefox OS remote debugging allows chrome script execution
Assignee: nobody → poirot.alex
Alex will work on that, Panos will review the code. We hope to get a fix soon.
In 1.1 we had no real devtools support, so we weren't loading tab actors in parent process on device
(ie we were loading it on desktop so that we can at least debug shell.xul).

In 1.2 we started enabling app debugging, and, in order to debug apps living in parent process (system and keyboard apps),
we had to load tab actors (console, inspector, debugger, ...) also in parent process [1].
The side effect of that, is that the root actor was also exposing tab actors for the main process,
even if we took care of preventing it to expose actors for the toplevel windows [2].

Ideally we would also explicitely prevent the root actor to expose the tab actors for main process,
even if the actors are loaded. But in this patch I just prevent them to be loaded, unless
the pref to allow certified apps debugging is set.

[1]
  http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#1075
[2]
  http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/dbg-browser-actors.js#30
blocking-b2g: koi? → koi+
Attachment #8359315 - Flags: review?(past)
Comment on attachment 8359315 [details] [diff] [review]
Prevent loading tab actors in parent process unless certified app pref is toggled

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

This looks good to me. The only observation I want to make is that if the debugger is already started and the user changes the forbid-certified-apps pref, stopping and starting remote debugging would be required for the actors to be loaded, right? We may want to mention that in the docs.

::: b2g/chrome/content/shell.js
@@ +1072,5 @@
>        DebuggerServer.init(this.prompt.bind(this));
>        DebuggerServer.chromeWindowType = "navigator:browser";
>        DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/webbrowser.js");
>        // Until we implement unix domain socket, we enable content actors
>        // only on development devices

Nit: this comment is no longer accurate. Better remove or reword it.

Also, if you feel like it, there is a similar comment in the webapps.js actor definition that needs an update or removal.

But in general, I'm not clear on whether enable-content-actors is still useful now that UNIX domain sockets are used. I don't mean to imply this is something that belongs to this bug, just wanted to raise the issue.
Attachment #8359315 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #6)
> This looks good to me. The only observation I want to make is that if the
> debugger is already started and the user changes the forbid-certified-apps
> pref, stopping and starting remote debugging would be required for the
> actors to be loaded, right? We may want to mention that in the docs.

There's no way to change the pref when fxos is running.

> But in general, I'm not clear on whether enable-content-actors is still
> useful now that UNIX domain sockets are used. I don't mean to imply this is
> something that belongs to this bug, just wanted to raise the issue.

This will disappear in bug 942756.
Attached patch patch v2Splinter Review
Updated comment and removed useless enable content actor code.
Attachment #8359315 - Attachment is obsolete: true
(In reply to Paul Rouget [:paul] from comment #7)
> (In reply to Panos Astithas [:past] from comment #6)
> > This looks good to me. The only observation I want to make is that if the
> > debugger is already started and the user changes the forbid-certified-apps
> > pref, stopping and starting remote debugging would be required for the
> > actors to be loaded, right? We may want to mention that in the docs.
> 
> There's no way to change the pref when fxos is running.

What about pulling prefs.js with adb, editing and pushing it back? Isn't this something developers would be accustomed to?
(In reply to Panos Astithas [:past] from comment #9)
> (In reply to Paul Rouget [:paul] from comment #7)
> > (In reply to Panos Astithas [:past] from comment #6)
> > > This looks good to me. The only observation I want to make is that if the
> > > debugger is already started and the user changes the forbid-certified-apps
> > > pref, stopping and starting remote debugging would be required for the
> > > actors to be loaded, right? We may want to mention that in the docs.
> > 
> > There's no way to change the pref when fxos is running.
> 
> What about pulling prefs.js with adb, editing and pushing it back? Isn't
> this something developers would be accustomed to?

In the documentation, way explicitly ask the user to restart B2G via adb shell.
Note that I'll be using bug 958043 to clean things up around actors on b2g.
Comment on attachment 8359775 [details] [diff] [review]
patch v2

Carrying r+ from previous patch.
Attachment #8359775 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/caf64bb3f566
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Do we have app manager support on 1.1.hd? I assume not, but I don't know too much about that branch.
(In reply to Paul Theriault [:pauljt] from comment #16)
> Do we have app manager support on 1.1.hd? I assume not, but I don't know too
> much about that branch.

No, only 1.2+ is supported by the App Manager.
Whiteboard: [adv-main28-]
Group: core-security
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.