1021756 - Allow to keep running adbd as root for ro.secure=1 & ro.debuggable=1 devices

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Certification Suite, defect)

Description

[Jonathan Griffin (:jgriffin)]

According to the Taipei engineers, in some environments we should be running 'adb root' before attempting any operation that requires root access, to force adb on the phone into root mode.

We should probably be doing this every time we reboot the phone.

Comment 1

[Dave Hylands [:dhylands]]

So adb root is a command run on the host.

Phones come up in user mode and allow the adb root command from the host to work have:

ro.secure=1
ro.debuggable=1

in their /default.prop file.

If you want adb to come up in root mode, then you can change ro.secure=1 to be ro.secure=0

This is typically part of the kernel's initrd image.

Note that production phones are normally released with ro.secure=1 and ro.debuggable=0

Comment 2

[Jonathan Griffin (:jgriffin)]

We have no ability to affect the builds in this context.  We're trying to make the certification suite operate under the widest possible conditions, and the TAMs requested that we add this step.  I know it may not always have the desired effect.

Comment 3

[Dave Hylands [:dhylands]]

It seems that user builds (at least on the flame) are totally locked down, so there is no way to get a root shell.

I've been told that this is intentional.

Comment 4

[Armen [:armenzg]]

jgriffin, do you have more info as to which engineers? or which environments?

I assume that we can change the devicemanagerADB to call "adb root" on the host before rebooting.

Comment 5

[Jonathan Griffin (:jgriffin)]

This was specifically noted as being needed for the non-phone device, however, Dan has been working on that and I'm not sure he needed to do it.  Dan, did you need to run 'adb root' before running the cert suite on the non-phone device?

Comment 6

[Dan Minor [:dminor]]

(In reply to Jonathan Griffin (:jgriffin) from comment #5)
> This was specifically noted as being needed for the non-phone device,
> however, Dan has been working on that and I'm not sure he needed to do it. 
> Dan, did you need to run 'adb root' before running the cert suite on the
> non-phone device?

We need to call 'adb root' to be able to uninstall apps through marionette on the non-phone device.

Comment 7

[Armen [:armenzg]]

(In reply to Dan Minor [:dminor] from comment #6)
> (In reply to Jonathan Griffin (:jgriffin) from comment #5)
> > This was specifically noted as being needed for the non-phone device,
> > however, Dan has been working on that and I'm not sure he needed to do it. 
> > Dan, did you need to run 'adb root' before running the cert suite on the
> > non-phone device?
> 
> We need to call 'adb root' to be able to uninstall apps through marionette
> on the non-phone device.

It seems like the config for the non-phone device is (rwood sent it to me):
ro.secure=1
ro.debuggable=1

Would running "adb root" change the value from ro.secure=1 to ro.secure=0?
I have failed miserably at finding somewhere where it explains what adb root actually does.

dminor, has running "adb root" before starting the cert suite worked for you?

I've tried using the device this morning but rwood and I could not figure out how to make it list under adb devices. What do you have for your udev rules? This is what I have:
SUBSYSTEM=="usb", ATTR{idVendor}=="2207", ATTR{idProduct}=="0006", MODE="0666", OWNER="armenzg"

On another note, the uninstall code seems to be the one of fxos_appgen [1]
It runs some javacript through marionette IIUC.

[1] https://github.com/mozilla-b2g/fxos-appgen/blob/master/fxos_appgen/generator.py#L249

Comment 8

[Dan Minor [:dminor]]

Armen, I spent some time looking at this this afternoon, and I'm not getting consistent results, so I'm no longer sure if it is dependent upon 'adb root'. The first time I had problems, restarting as 'adb root' allowed the uninstall to work, but that doesn't seem to be the case now.

Did you add 0x2207 to your .android/adb_usb.ini file? I had to do that in order to get the device to work.

Comment 9

[Armen [:armenzg]]

(In reply to Dan Minor [:dminor] from comment #8)
> Did you add 0x2207 to your .android/adb_usb.ini file? I had to do that in
> order to get the device to work.
Yes, I did as well.

Comment 10

[Dave Hylands [:dhylands]]

ro.secure and ro.debuggable are read-only - so they never change.

When the system starts up with ro.secure=1 and ro.debuggable=1, then adb will be launched as the "shell" user. You can see this if you do:

> adb shell ps | grep adbd
> shell     392   1     4508   212   ffffffff 00000000 S /sbin/adbd
> adb shell
> shell@flame:/ $ 

The first column tells you the user that adbd is running as.

You'll also notice that your shell prompt ends with $. This is your indication that you have a "shell" user shell.

Now, if ro.debuggable is set to 1, then you're allowed to do 'adb root' and this will relaunch adbd under the root user:

> adb root
> restarting adbd as root
> adb shell ps | grep adbd
> root      844   1     4512   216   ffffffff 00016ce4 S /sbin/adbd
> adb shell
> root@flame:/ # 

You'll also notice that your shell prompt ends in #. This is your indication that you have a "root" user shell.

Note that when you run from a script, you should always put adb wait-for-device after doing adb root.

If you have ro.secure=1 and ro.debuggable=0 then adb root won't work.
If you have ro.secure=0 then adb is launched under the root user to start with.

Comment 11

[Dave Hylands [:dhylands]]

The results of adb root are lost each time the phone reboots (or whenever the adbd daemon is killed).

Comment 12

[Dan Minor [:dminor]]

It no longer appears that my uninstall problems were caused by not running 'adb root'. I have some that consistently work and some that consistently fail, regardless of whether 'adb root' was used.

Comment 13

[Dan Minor [:dminor]]

It looks like I might need 'adb root' for the first run on a newly flashed device. I'll try a few times to see if this is the case, or if I hit an intermittent success :)

Comment 14

[Armen [:armenzg]]

Attachment: If requested, run adbd as root

This is the code I used to test it [1] and here's the output:
> adbd user shell
> adbd user root

[1]
from mozdevice import DeviceManagerADB
if __name__ == "__main__":
    dm = DeviceManagerADB()
    print "adbd user %s" % dm.processOwner("adbd")
    dm = DeviceManagerADB(runAdbAsRoot=True)
    print "adbd user %s" % dm.processOwner("adbd")

Comment 15

[Armen [:armenzg]]

wlach, how do you feel about making this the default behaviour and escape it if a variable is set to not run as root?

+   logLevel=mozlog.ERROR, autoconnect=True, doNotRunAdbAsRoot=False, **kwargs):
...
+   if not doNotRunAdbAsRoot and self.processOwner("adbd") != "root":

Comment 16

[Armen [:armenzg]]

Attachment: Request adb to be run as root

Depending on the review, this patch might not be needed.

I would prefer adb to be always run as root. I think it will help people more in the long term rather than having them to realize that they should pass the extra parameter.

Comment 17

[William Lachance (:wlach)]

Comment on attachment 8464219 [details] [diff] [review]
If requested, run adbd as root

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

As discussed, let's put this code inside the certsuite, since it seems unlikely that we'll be using this functionality anywhere but there.

::: testing/mozbase/mozdevice/mozdevice/devicemanager.py
@@ +412,5 @@
>          """
>          if not isinstance(processName, basestring):
>              raise TypeError("Process name %s is not a string" % processName)
>  
> +        process_info = None

Use camel case for variables inside mozdevice.

Comment 18

[Armen [:armenzg]]

Attachment: allow to request processInfo

After a second thought, the processInfo function is useful to me.
Otherwise I would have to duplicate the code in the cert suite.

Comment 19

[William Lachance (:wlach)]

Comment on attachment 8464757 [details] [diff] [review]
allow to request processInfo

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

Makes sense. r+ with nits addressed.

::: testing/mozbase/mozdevice/mozdevice/devicemanager.py
@@ +408,2 @@
>          """
> +        Returns None if the process does not exist on the device, otherwise.

This comment's a bit hard to parse. I'd suggest something more like:

Returns information on the process with processName.

Information on process is in tuple format: (pid, process path, user)

If a process with the specified name does not exist this function will return None.

@@ +442,4 @@
>  
> +    def processExist(self, processName):
> +        """
> +        Returns None if process does not exist, otherwise, it will

Please reset this to the previous comment: Returns True if process with name processName is running on device.

@@ +447,5 @@
> +        """
> +        processInfo = self.processInfo(processName)
> +        if processInfo:
> +            return processInfo[0]
> +        else:

This 'else' clause is unnecessary and can safely be taken out, all python functions will return None if no value is given

Comment 20

[Armen [:armenzg]]

Attachment: [wip] subclassing devicemanagerADB

Comment 21

[Armen [:armenzg]]

Attachment: adb_root_mc.diff

Would this approach work for you?

I have noticed that _haveSu is exactly the case scenario that we are trying to fix.

Comment 22

[William Lachance (:wlach)]

Comment on attachment 8464949 [details] [diff] [review]
adb_root_mc.diff

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

As discussed, I don't think this is going to work. _haveSu refers to whether the device has a su binary that is setuid root.

Comment 23

[Armen [:armenzg]]

Attachment: adb_as_root_mc.diff

Comment 24

[William Lachance (:wlach)]

Comment on attachment 8465542 [details] [diff] [review]
adb_as_root_mc.diff

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

Ok, I guess we needed this after all.

Comment 25

[Armen [:armenzg]]

Thanks wlach!

Devices that start with adb as shell rather than root (ro.secure=1 & ro.debuggable=1) fail to pass the cert:permissions test.

Should we check for such type of devices and fail if the user does not use a flag? (e.g. run.sh --run-adb-as-root) Or should we make cert.py to always run adb as root?

You can see current patch in https://github.com/mozilla-b2g/fxos-certsuite/pull/183

Comment 26

[Jonathan Griffin (:jgriffin)]

If there aren't any side effects of this (and I don't think there are), I think we should just always do this.

Comment 27

[Armen [:armenzg]]

The adb tests have passed, however, I want to see it pass on try before landing tomorrow morning (I will be online a bit).
https://tbpl.mozilla.org/?tree=Try&rev=867dfc0e6700

After landing I will look into uploading it to pypi (perhaps a version bump is required or we can simply overwrite).

Comment 28

[Armen [:armenzg]]

(In reply to Jonathan Griffin (:jgriffin) from comment #26)
> If there aren't any side effects of this (and I don't think there are), I
> think we should just always do this.

I have not been able to think of one.

Comment 29

[Jonathan Griffin (:jgriffin)]

(In reply to Armen Zambrano [:armenzg] (EDT/UTC-4) from comment #27)
> The adb tests have passed, however, I want to see it pass on try before
> landing tomorrow morning (I will be online a bit).
> https://tbpl.mozilla.org/?tree=Try&rev=867dfc0e6700
> 
> After landing I will look into uploading it to pypi (perhaps a version bump
> is required or we can simply overwrite).

We should version bump; we never overwrite released versions.

Comment 30

[William Lachance (:wlach)]

(In reply to Jonathan Griffin (:jgriffin) from comment #26)
> If there aren't any side effects of this (and I don't think there are), I
> think we should just always do this.

As discussed I don't think we can do this as it will break using devicemanagerADB with unrooted Android devices. It should be kept optional.

Comment 31

[Armen [:armenzg]]

(In reply to William Lachance (:wlach) from comment #30)
> (In reply to Jonathan Griffin (:jgriffin) from comment #26)
> > If there aren't any side effects of this (and I don't think there are), I
> > think we should just always do this.
> 
> As discussed I don't think we can do this as it will break using
> devicemanagerADB with unrooted Android devices. It should be kept optional.

I don't recall that we agreed on that. As far as I know even if it was mandatory on mozdevice it should not break anything. We never figured out a breakage that I recall of.

Nevertheless, we're referring in this comment to the cert suite, not mozdevice. We will leave it as optional for mozdevice.

Comment 32

[William Lachance (:wlach)]

(In reply to Armen Zambrano [:armenzg] (EDT/UTC-4) from comment #31)
> (In reply to William Lachance (:wlach) from comment #30)
> > (In reply to Jonathan Griffin (:jgriffin) from comment #26)
> > > If there aren't any side effects of this (and I don't think there are), I
> > > think we should just always do this.
> > 
> > As discussed I don't think we can do this as it will break using
> > devicemanagerADB with unrooted Android devices. It should be kept optional.
> 
> I don't recall that we agreed on that. As far as I know even if it was
> mandatory on mozdevice it should not break anything. We never figured out a
> breakage that I recall of.

Running "adb root" on my Galaxy Nexus produces this error message and does not change state:

adbd cannot run as root in production builds

If we applied this logic unconditionally devicemanagerADB would always throw an error on these types of devices.

Comment 33

[Dave Hylands [:dhylands]]

(In reply to William Lachance (:wlach) from comment #32)
> Running "adb root" on my Galaxy Nexus produces this error message and does
> not change state:
> 
> adbd cannot run as root in production builds
> 
> If we applied this logic unconditionally devicemanagerADB would always throw
> an error on these types of devices.

Right - that means rc.secure=1 and ro.debuggable=0 which is how production devices are normally.

They need to be "rooted" and/or unlocked first.

Some definitions of rooting only involve installing su which allows you to get root access. However, to get adb to relaunched as the root user requires modifying those ro. properties which are on the initial ram disk that's bundled with the kernel.

Comment 34

[Armen [:armenzg]]

(In reply to William Lachance (:wlach) from comment #32)
> (In reply to Armen Zambrano [:armenzg] (EDT/UTC-4) from comment #31)
> > (In reply to William Lachance (:wlach) from comment #30)
> > > (In reply to Jonathan Griffin (:jgriffin) from comment #26)
> > > > If there aren't any side effects of this (and I don't think there are), I
> > > > think we should just always do this.
> > > 
> > > As discussed I don't think we can do this as it will break using
> > > devicemanagerADB with unrooted Android devices. It should be kept optional.
> > 
> > I don't recall that we agreed on that. As far as I know even if it was
> > mandatory on mozdevice it should not break anything. We never figured out a
> > breakage that I recall of.
> 
> Running "adb root" on my Galaxy Nexus produces this error message and does
> not change state:
> 
> adbd cannot run as root in production builds
> 
> If we applied this logic unconditionally devicemanagerADB would always throw
> an error on these types of devices.

I see what you mean now.

Anyone know if we're going to run the cert suite on non-rooted devices? (Android production phones mainly).
As far as I know all devices run through the cert suite are supposed to be Firefox OS phones which either start adb as root or they are allowed to run "adb root" to restart it.
If this is not the case, please let me know what we should do in such cases.

Comment 35

[Jonathan Griffin (:jgriffin)]

We will never run the cert suite on Android, and if it isn't rooted and 'adb root' isn't sufficient to root it, the tests will fail, so it should be ok to run this command always in the context of the cert suite.

Comment 36

[Armen [:armenzg]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0dff8ac3f629
http://hg.mozilla.org/integration/mozilla-inbound/rev/1cb5bd26726e

Comment 37

[Armen [:armenzg]]

For some reason critic did not pick up my pull request:
https://github.com/mozilla-b2g/fxos-certsuite/pull/183

dminor, does this look good to you?

Comment 38

[Dan Minor [:dminor]]

Merged :)

Comment 39

[Armen [:armenzg]]

Great!
After it merges to m-c, this will be done.

Comment 40

[Jonathan Griffin (:jgriffin)]

Merged to v1.3.

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1cb5bd26726e
https://hg.mozilla.org/mozilla-central/rev/0dff8ac3f629