Closed Bug 810092 Opened 9 years ago Closed 8 years ago

ADB should not be allowed to connect when device is locked

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ladamski, Assigned: dhylands)

References

Details

(Keywords: late-l10n, sec-moderate)

Attachments

(1 file, 1 obsolete file)

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.
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.
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.
We can take a setting adb on/off for v1. The rest is out of scope for v1 I think.
Assignee: nobody → dhylands
Keywords: late-l10n
Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.
Priority: -- → P1
(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.
locked in this context means screen locked.

The dogfooding phones still have root access enabled, so any data on the phone can be retrieved.
The testdriver phones are not a production configuration.
Do we still need this at all for v1?  Sounds like from comment #5 and #7 this is not a critical issue.
blocking-basecamp: + → ?
blocking-basecamp: ? → -
Do we actually have a bug tracking changing this setting for production phones, then?
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).
blocking-b2g: --- → koi?
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.
blocking-b2g: koi? → -
s-g ?
Duplicate of this bug: 893826
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
Duplicate of this bug: 899489
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.
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.
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).
Attachment #789841 - Flags: review?(fabrice)
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 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)
Attachment #789841 - Flags: review?(fabrice) → review+
(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
(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));
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.
Attachment #789841 - Attachment is obsolete: true
Attachment #790887 - Flags: review?(fabrice)
Attachment #790887 - Flags: review?(fabrice) → review+
Blocks: 874484
https://hg.mozilla.org/mozilla-central/rev/8f567817ea3b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Verified adb logging stops when screen is locked.
Status: RESOLVED → VERIFIED
Depends on: 917331
blocking-b2g: - → koi?
blocking-basecamp: - → ---
Duplicate of this bug: 926072
Already landed, no need to nom - it's already in 1.2.
blocking-b2g: koi? → ---
You need to log in before you can comment on or make changes to this bug.