810092 - ADB should not be allowed to connect when device is locked

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Lucas Adamski [:ladamski]]

ADB can be used to make arbitrary changes to the device, including low-level OS functionality, and steal data.  Currently it connects regardless of whether the device is locked or unlocked.  Device lock should not be trivially bypass-able.

Comment 1

[Dave Hylands [:dhylands]]

I think that this is the wrong solution.

We should have a setting on the phone to control whether ADB is enabled or not.

For production phones, this setting should default to ADB being off.
For debug builds the setting should default to being on.
For user builds (what we put on dogfooding phone, I could live with either way).

When adb is enabled, it should be enabled whether the phone is unlocked or not. It doesn't make sense to me to control it via lock/unlock.

It does make sense to me to control it via a setting.

Comment 2

[Dave Hylands [:dhylands]]

After talking with Lucas, I'd be happy with an "ADB Debugging" setting which has the following 3 values:

- Disabled
- Enabled when Unlocked
- Enabled Always

His concern is that stuff like your mozilla LDAP password is stored in cleartext on the phone and retrievable via adb.

Comment 3

[Andreas Gal :gal]

We can take a setting adb on/off for v1. The rest is out of scope for v1 I think.

Comment 4

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Lucas Adamski from comment #0)
> ADB can be used to make arbitrary changes to the device, including low-level
> OS functionality, and steal data.  Currently it connects regardless of
> whether the device is locked or unlocked.  Device lock should not be
> trivially bypass-able.

You're mistaking our current build configuration for production config.  Currently adb shell has root privileges, which allows it to do anything.

In production builds, it's the "shell" user, and has barely more privileges (maybe no more) than app subprocesses.  There are still bad-ish things it can do but stealing data etc. is not among them.

I don't understand what "locked" means in this context.

Comment 6

[Dave Hylands [:dhylands]]

locked in this context means screen locked.

The dogfooding phones still have root access enabled, so any data on the phone can be retrieved.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The testdriver phones are not a production configuration.

Comment 8

[JP Rosevear [:jpr]]

Do we still need this at all for v1?  Sounds like from comment #5 and #7 this is not a critical issue.

Comment 9

[Lucas Adamski [:ladamski]]

Do we actually have a bug tracking changing this setting for production phones, then?

Comment 10

[Paul Theriault [:pauljt] (no longer reading bugmail)]

This was out-of-scope for v1. I would like it back on the table for Koi please since adb greatly increases the risk of user data compromise. An alternative may be bug 874484 (i.e. just disable adb after a period of inactivity, to reduce the risk that people will enable it, for example for sideloading, and forget to disable it).

Comment 11

[Andreas Gal :gal]

This is by far not the greatest attack surface (see s-g discussion). We can take a patch here when its ready. Definitely not a reason to block.

Comment 12

[Dave Hylands [:dhylands]]

s-g ?

Comment 14

[Dave Hylands [:dhylands]]

I made a comment over in bug 899489, so I'll repeat it here:

I expect that this bug will need a solution similar to the one used by UMS (which makes enabling UMS only happen when the phone is unlocked).

See: gaia/apps/system/js/storage.js to see what was done for UMS
See: gecko/b2g/chrome/content/settings.js for the code which enables/disables adb

Comment 16

[Wayne Chang [:wchang]]

Can a "shell" user unlock or bypass the lockscreen in anyway?

Do a quick search with "ADB lock screen" you'd get instructions on how to bypass the lockscreen. Would similar things be possible on FxOS?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)

> In production builds, it's the "shell" user, and has barely more privileges
> (maybe no more) than app subprocesses.  There are still bad-ish things it
> can do but stealing data etc. is not among them.
> 
> I don't understand what "locked" means in this context.

Comment 17

[Dave Hylands [:dhylands]]

I'd have to see the (In reply to Wayne Chang [:wchang] from comment #16)
> Can a "shell" user unlock or bypass the lockscreen in anyway?
> 
> Do a quick search with "ADB lock screen" you'd get instructions on how to
> bypass the lockscreen. Would similar things be possible on FxOS?

I'd have to see a particular set of instructions to determine if that would work. The one I looked at were android specific and wouldn't apply to FxOS.

The one and only directory that the shell user has write permission to is /data/local/tmp

All of the settings related to the lock screen should be stored in the settings database, which the shell user doesn't have read or write access to.

Comment 18

[Dave Hylands [:dhylands]]

Attachment: On a production phone (i.e. VARIANT=user) only allow adb to be enabled when the phone is unlocked.

This patch causes adb to be disabled (on a VARIANT=user build) when the phone is locked.

eng & userdebug builds are as before, with adb enabled all of the time.

If you disable the phone lock in settings, then adb will also stay enabled on a VARIANT=user build (useful if your phone is always plugged into your PC for development testing).

Comment 19

[Dave Hylands [:dhylands]]

Testing performed (I'm pretty sure that this is non-testable via any automated testing that we currently have).

For all of the steps below, "check that adb is enabled" translates to "phone shows up in 'adb devices' output" and "check that adb is disabled" translates to "phone does NOT show up in 'adb devices' output".

1 - For VARIANT=eng and VARIANT=userdebug
- start "adb logcat". You should get output without having to unlock the phone.
- Toggle remote debugging (Settings->Device Information->More Information->Developer) and verify logcat stays connected.
- lock and unlock the phone and verify that logcat stays connected.

2 - For VARIANT=user
- Boot phone. adb should be disabled by default (after flashing)
- Turn on remote debugging, verify adb enabled
- Lock phone, verify adb disabled
- Unlock phone, verify adb enabled
- Turn off remote debugging, verify adb disabled
- Turn on remote debugging, verify adb enabled
- Disable Phone Lock, (Settings->Phone Lock), verify adb enabled
- Press Power button (turns off screen), verify adb enabled
- Press Power button (turns on screen), verify adb enabled

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 789841 [details] [diff] [review]
On a production phone (i.e. VARIANT=user) only allow adb to be enabled when the phone is unlocked.

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

r=me

::: b2g/chrome/content/settings.js
@@ +200,5 @@
>  
> +// =================== Debugger / ADB ====================
> +
> +#ifdef MOZ_WIDGET_GONK
> +var AdbController = {

nit: s/var/let

@@ +215,5 @@
> +    this.updateState();
> +  },
> +
> +  updateState: function adbControllerUpdateState() {
> +    var enableAdb = this.remoteDebuggerEnabled && this.unlocked;

nit: s/var/let

@@ +243,5 @@
> +        }
> +      } else {
> +        // Remove adb from the list of functions, if present
> +        if (adbIndex >= 0) {
> +          configFuncs.splice(adbIndex,1);

nit: adbIndex, 1

@@ +251,5 @@
> +      if (newConfig != currentConfig) {
> +        libcutils.property_set("persist.sys.usb.config", newConfig);
> +      }
> +    } catch(e) {
> +      dump("Error configuring adb: " + e);

Why do we need a try{} catch() here?

@@ +258,5 @@
> +};
> +
> +SettingsListener.observe('lockscreen.locked', false, function(value) {
> +  AdbController.setLockscreenState(value);
> +});

You can simplify to : SettingsListener.observe('lockscreen.locked', false, AdbController.setLockscreenState)

Comment 21

[Dave Hylands [:dhylands]]

(In reply to Fabrice Desré [:fabrice] from comment #20)

> @@ +251,5 @@
> > +      if (newConfig != currentConfig) {
> > +        libcutils.property_set("persist.sys.usb.config", newConfig);
> > +      }
> > +    } catch(e) {
> > +      dump("Error configuring adb: " + e);
> 
> Why do we need a try{} catch() here?

property_set has a throw in it:
https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/systemlibs.js#94

So the try/catch probably only needs to be around the  call to property_set

Comment 22

[Dave Hylands [:dhylands]]

(In reply to Fabrice Desré [:fabrice] from comment #20)
> @@ +258,5 @@
> > +};
> > +
> > +SettingsListener.observe('lockscreen.locked', false, function(value) {
> > +  AdbController.setLockscreenState(value);
> > +});
> 
> You can simplify to : SettingsListener.observe('lockscreen.locked', false,
> AdbController.setLockscreenState)

Unfortunately, that doesn't work.

I figured out why. The correct way to simplify this would be:

SettingsListener.observe('lockscreen.locked', false,
    AdbController.setLockscreenState.bind(AdbController));

Comment 23

[Dave Hylands [:dhylands]]

Attachment: On a production phone (i.e. VARIANT=user) only allow adb to be enabled when the phone is unlocked. v2

I found some edge cases that didn't work properly with the initial patch. This version should address fabrice's comments from the previous review and also addresses the edge cases.

Comment 24

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8f567817ea3b

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8f567817ea3b

Comment 26

[Tony Chung [:tchung]]

Verified adb logging stops when screen is locked.

Comment 29

[Jason Smith [:jsmith]]

Already landed, no need to nom - it's already in 1.2.