Enable/disable ADB when changing the "remote debugging" setting in the Settings App

VERIFIED FIXED in Firefox 21

Status

defect
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: pauljt, Assigned: fabrice)

Tracking

({late-l10n})

unspecified
B2G C4 (2jan on)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

ADB should be disabled by default in productions as it represents significant security risk, even if we run adb with lower risks as discussed in 810092.

However currently there is no setting in the UI to enable adb, so if we disable it by default, there is no way to ever turn it back on (afaik).

As discussed on IRC, we could link the existing "remote debugging" option under developer settings to enabled adb, in addition to enabling the Remote Debugger Service. This could be enabled by having this setting change the android system property assocaited with adb.
Priority: P1 → --
Summary: Provide a setting in the Settings App to enable/disable ADB → Enable/disable ADB when changing the "remote debugging" setting in the Settings App
What is the expected settings for the system props?

I think that the settings we want are
ro.secure=1
ro.debuggable=0|1 depending on "remote debugging setting"
persist.sys.usb.config=mass_storage,adb


Looking at shell.js, the settings can be manipulated with
libcutils.property_set(opt, value)
and put into the RemoteDebugger handleEvent
http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#888
Assignee

Comment 2

7 years ago
Posted patch wip (obsolete) — Splinter Review
Doesn't work unfortunately... I always get adb access.
So as far as I'm aware, you can't change ro.xxx settings.

2018 >adb shell 
root@android:/ # getprop ro.secure
0
root@android:/ # setprop ro.secure 1
root@android:/ # getprop ro.secure                                             
0

To disable adb, you need to remove the ,adb from the persist.sys.usb.config

The correct way would be to read the current setting, remove ,adb and write it out again. To enable, read the setting, add ,adb if it isn't already there and write it back out.

Manipulating the sys.usb.config can be tricky, general, as the string HAS to match one of the strings from init.qcom.rc. Fortunately, for adb, it's always at the end of the string so we can always append ,adb or remove it
Assignee

Comment 4

7 years ago
Posted patch patchSplinter Review
Thanks for the info Dave! that seems to work fine now.
Assignee: nobody → fabrice
Attachment #707933 - Attachment is obsolete: true
Attachment #707997 - Flags: review?(dhylands)
Comment on attachment 707997 [details] [diff] [review]
patch

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

r=me with dumps removed and no trailing comma on the value stored in persist.sys.usb.config

::: b2g/chrome/content/settings.js
@@ +199,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +  // Configure adb.
> +  try {
> +    let current = libcutils.property_get("persist.sys.usb.config");
> +    dump("Current value: " + current);

Everybody else complains when I leave dump statements in...

@@ +200,5 @@
> +  // Configure adb.
> +  try {
> +    let current = libcutils.property_get("persist.sys.usb.config");
> +    dump("Current value: " + current);
> +    let prefix = current.split(",adb");

So when I tried this in firefox scratchpad

"mass_storage,adb".split(",adb");

it leaves a trailing comma on the string, which I think is bad (it may be disabling adb, but I think you'll find its not enabling mass_storage).

Wouldn't it be better to use:

let prefix = current.replace(/,adb/, "");
Attachment #707997 - Flags: review?(dhylands) → review+
https://hg.mozilla.org/mozilla-central/rev/7cf999a95467
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You probably need to deal with the startup condition as well

(i.e. enabled/disable adb based on the setting at startup).

Or maybe the settings stuff generates a startup event? I can't remember.
blocking-b2g: tef? → tef+
Depends on: 836770
Verified fixed in 2013-02-06-11-05-09 pvt nightly b2g18 build
Status: RESOLVED → VERIFIED
Depends on: 839134
You need to log in before you can comment on or make changes to this bug.