1111725 - UMS (USB) mounting after reboot even without unlocking

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Clément Lefèvre]

For security purpose, you are supposed to need unlocking your phone to have access to storage content when USB share is enabled.

STR : Enable USB share and reboot the phone. Plug it to the computer before or after reboot, it does not matter. After reboot is complete do NOT unlock the phone.

What will happen : You will see that shared Volumes will mount on your computer.

What should have happened : You are not supposed to get access to the USB shared volumes if the phone have not been unlocked (which is like an authentication to get this access)
Considering anyone can reboot the phone, it gives access to phones content for everyone.

Additional informations: Just after reboot, if the screen switch off (manually or by idle time), the USB mount is stopped, but after some switch between on and off state, it finally stay mounted even if the screen stay at off state.
However, when the screen is shut down, it will switch on itself again until USB mount keep its state even with switched off screen.

Main tested phone and build:

Build ID               20141214040212
Build Type             user
Gaia Revision          e2a3e606675c346b6e6f35351a458040be599b09
Gaia Date              2014-12-12 21:39:46
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/f14dcd1c8c0b
Gecko Version          37.0a1
Device ID              flame
Device Name            msm8610
Firmware(Release)      4.4.2
Firmware(Incremental)  39
Firmware Date          Thu Oct 16 18:19:14 CST 2014
Bootloader             L1TC00011880

Reproduced on a 2.0 build, not reproduced on 1.3.

Comment 1

[Julien Wajsberg [:julienw]]

To make it clearer: reproduced in 2.2 as well (that's the build information above).

Comment 2

[Clément Lefèvre]

After looking a bit more at this bug, I could find other linked problems depending on adb state (enabled or not):

If ADB is unabled:
USB mount will happen after reboot, the screen will not switch on and off alone and no unmount will happen.

If ADB is enabled : The switches I described previously will happen. While the screen is off and the USB volume unmounted, you can have access to adb even without unlocking the phone too. If the screen switch on, USB mount and adb disconnect.

But then if you switch on and off several time you will then be able to have USB volume mounted AND adb access witch switched off screen without any problem or interruption.

Comment 3

[Stephanie Ouillon [:arroway] (needinfo me)]

(In reply to Clément Lefèvre from comment #2)
> After looking a bit more at this bug, I could find other linked problems
> depending on adb state (enabled or not):
> 
> If ADB is unabled:
> USB mount will happen after reboot, the screen will not switch on and off
> alone and no unmount will happen.

what do you mean by "the screen will not witch on and off alone"? It only happens when you switch on the screen manually and goes off manually or when it goes idle, right?

> 
> If ADB is enabled : The switches I described previously will happen. While
> the screen is off and the USB volume unmounted, you can have access to adb
> even without unlocking the phone too. If the screen switch on, USB mount and
> adb disconnect.
> 
> But then if you switch on and off several time you will then be able to have
> USB volume mounted AND adb access witch switched off screen without any
> problem or interruption.

Te be more precise, given that Paul had noticed the same thing on a developer build some time ago but this was a normal behavior: do you have adb access even if you unplug the phone before or during rebooting? 
(and I precise it again just in case to be clearer: Clement is testing on a *user* build).

Comment 4

[Clément Lefèvre]

(In reply to Stephanie Ouillon [:arroway] from comment #3)
> (In reply to Clément Lefèvre from comment #2)
> > After looking a bit more at this bug, I could find other linked problems
> > depending on adb state (enabled or not):
> > 
> > If ADB is unabled:
> > USB mount will happen after reboot, the screen will not switch on and off
> > alone and no unmount will happen.
> 
> what do you mean by "the screen will not witch on and off alone"? It only
> happens when you switch on the screen manually and goes off manually or when
> it goes idle, right?
> 

I mean, if adb settings is enabled, after reboot, the phone can switch on the screen itself after this one turned off either because of idle time or because user manually do it.
When the screen switch off, USB mount is unmounted. When it switch on, it remount.
It appears it will do this until USB mount keep mounted state definitely (ie. even with switched off screen – see second part of this comment for step by step STR).
If adb is clearly disabled, these switches will not happen and USB mount will be definitively mounted: screen state will make no difference.

> > 
> > If ADB is enabled : The switches I described previously will happen. While
> > the screen is off and the USB volume unmounted, you can have access to adb
> > even without unlocking the phone too. If the screen switch on, USB mount and
> > adb disconnect.
> > 
> > But then if you switch on and off several time you will then be able to have
> > USB volume mounted AND adb access witch switched off screen without any
> > problem or interruption.
> 
> Te be more precise, given that Paul had noticed the same thing on a
> developer build some time ago but this was a normal behavior: do you have
> adb access even if you unplug the phone before or during rebooting? 
> (and I precise it again just in case to be clearer: Clement is testing on a
> *user* build).

Yes, it's happening whatever you did to plug the phone : before reboot, after reboot, during reboot.
While phone have not been unlocked, issue happen.

To define very clear STR:
- Reboot the phone and plug it whenever you want, do not unlock it.
- USB is mounting and you have no adb access.
- Switch off the screen.
- USB volume is unmounting, and you can now connect adb to the phone. Phone have not been unlocked.
- Adb will be disconnect if you switch on the screen again or after around five minutes: the screen will switch on itself and have the same effect.

This can alternate several times (USB mount/adb, depending on screen state), but finally after some switches, both USB share and adb will be active in the same time.

Comment 5

[Stephanie Ouillon [:arroway] (needinfo me)]

I did the following tests and I reproduce the issues described by Clement on a user build, central branch, flashing the flame-kk images from pvt:

* If ADB is disabled and USB sharing is on (while the phone is unplugged), when we restart the phone and plug it, we can get access to internal storage before unlocking the phone

* If ADB (only, not the devtools) is enabled and USB sharing is on (while the phone is unplugged), when we restart the phone and plug it, we can observe two behaviors:

1) at first the local storage is not mounted when the screen is off. It is mounting if the screen is turned on, unmounting when the screen goes off. We can get an ADB shell *only* when the local storage is mounted.

2) after turning off and on the screen a few times, as described by Clement, the local storage stays mounted whatever the state of the screen is. Then, we can always get an ADB shell too.

During my tests, I unplugged the phone, turned off and on again USB Sharing and ADB, then restart the phone and plugged it again to be sure there wasn't a remaining connection as Paul was suggesting offline.

So it looks like that the issue comes from the way the connection for USB sharing is handled.

@Paul: can you confirm this is NOT a wanted behavior on a user build?

Comment 6

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

I'm unclear about usb storage, don't know if we ever prevented that based on the lockscreen status. We may have but I don't know.

As for adb, the security requirement is  we should not be able to initiate a new adb connection to a locked phone. IE if you steal someones phone, even if they have edb debugging enabled, you musn't be able to get to it. 

To enforce this, I believe we currently disable adb via adb access via an android property when the phone is locked, and we are not currently debugging. 

Dave, any idea why we might be seeing this interaction?

Stephanie, I won't have time to dig into this further today, but if it was me, I would start by looking at all the code which references the persist.sys.usb.config android pref.

Comment 7

[Dave Hylands [:dhylands]]

This looks like some bad logic in gaia/apps/system/js/usb_storage.js

The file looks totally different from when I originally wrote it, so somebody has re-written it and it doesn't quite work properly.

If nobody else gets to this, I'll take a look once my review queue is down.

I'm changing the component to Gaia::System, since that's where the issue is.

Comment 8

[Dave Hylands [:dhylands]]

Attachment: log.txt

logcat during bootup while USB Mass Storage was enabled.

I added some prints (search for usb_storage), which shows _setMode being called to enable sharing while the screen is locked.

The blank lines at 11:55:37.366 is where I hit the power button and then unlocked the screen.

The call which is enabling sharing happens at: 11:53:29.296

It also looks like usb_storage is treating MTP differently from UMS. They should both be treated the same (as far as enabling is concerned).

Comment 9

[Julien Wajsberg [:julienw]]

[Blocking Requested - why for this release]:

Comment 10

[bhavana bajaj [:bajaj]]

Blocking as this is a sec-high, dhylands would be the best person to help with a patch here ? Or can you recommend anyone else who can grab this?

Comment 11

[Dave Hylands [:dhylands]]

I can take a look at it, and see if my JS is good enough...

Comment 12

[Dave Hylands [:dhylands]]

Attachment: Rewrite usb_storage.js to have the same logic that it had before (in regards to locking/unlocking)

Comment 13

[Dave Hylands [:dhylands]]

Comment on attachment 8550059 [details] [review]
Rewrite usb_storage.js to have the same logic that it had before (in regards to locking/unlocking)

I'm going to put alive and kgrandon down for review, but I think I only need one or the other.

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Paul Theriault [:pauljt] from comment #6)
> I'm unclear about usb storage, don't know if we ever prevented that based on
> the lockscreen status. We may have but I don't know.
> 
> As for adb, the security requirement is  we should not be able to initiate a
> new adb connection to a locked phone. IE if you steal someones phone, even
> if they have edb debugging enabled, you musn't be able to get to it. 
> 
> To enforce this, I believe we currently disable adb via adb access via an
> android property when the phone is locked, and we are not currently
> debugging. 
> 
> Dave, any idea why we might be seeing this interaction?
> 
> Stephanie, I won't have time to dig into this further today, but if it was
> me, I would start by looking at all the code which references the
> persist.sys.usb.config android pref.

Do we need to care passcode-enabled phone here only or even the passcode is off?

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8550059 [details] [review]
Rewrite usb_storage.js to have the same logic that it had before (in regards to locking/unlocking)

Dave, thanks for contributing.

My main concern here is that we should not observe lockscreen.enabled or lockscreen.locked because it is already observed by lockscreenWindowManager.

Could you tell me why the original System.locked and listen to lockscreen-appopened/lockscreen-appclosed does not work?

Maybe what we need is just put your update() function inside start(). Could you give it a try?
Also we may need to change usb_storage_test.js to avoid test failure.

Lemme know if you have trouble doing that.

Comment 16

[Kevin Grandon :kgrandon]

Comment on attachment 8550059 [details] [review]
Rewrite usb_storage.js to have the same logic that it had before (in regards to locking/unlocking)

I'll let Alive take this one since he's already made some good comments here.

Comment 17

[Dave Hylands [:dhylands]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> (In reply to Paul Theriault [:pauljt] from comment #6)
> > I'm unclear about usb storage, don't know if we ever prevented that based on
> > the lockscreen status. We may have but I don't know.

Yes - this used to work.

> > As for adb, the security requirement is  we should not be able to initiate a
> > new adb connection to a locked phone. IE if you steal someones phone, even
> > if they have edb debugging enabled, you musn't be able to get to it. 
> > 
> > To enforce this, I believe we currently disable adb via adb access via an
> > android property when the phone is locked, and we are not currently
> > debugging. 

The adb logic is found in b2g/chrome/content/devtools/adb.js

I think that it needs to be updated to use similar logic to what gets used here.

There is also some interaction between adb and UMS/MTP and RNDIS. These are all controlled through sys.usb.config. It turns out that making any chaneg to sys.usb.config will have the side effect of stopping and restarting abd, so in the event that UMS/MTP/RNDIS are being used we defer disabling adb until the usb services are also turned off (this would typically happen when the usb cable is unplugged, or when the user disables the service in question).

> > Dave, any idea why we might be seeing this interaction?
> > 
> > Stephanie, I won't have time to dig into this further today, but if it was
> > me, I would start by looking at all the code which references the
> > persist.sys.usb.config android pref.
> 
> Do we need to care passcode-enabled phone here only or even the passcode is
> off?

The adb logic looks at lockscreen.enabled.

I'm going to rework usb_storage.js so that all updated go through _updateMode, but go back to using Sevice.locked and the appopened/appclosed logic.

Comment 18

[Dave Hylands [:dhylands]]

Comment on attachment 8550059 [details] [review]
Rewrite usb_storage.js to have the same logic that it had before (in regards to locking/unlocking)

I reworked the code so that it continues to use Service.locked and the lockscreen-appopened/closed.

I also updated the unit test to work with the new code.

The biggest problem I discovered is that the old logic wasn't taking the current state into consideration.

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8550059 [details] [review]
Rewrite usb_storage.js to have the same logic that it had before (in regards to locking/unlocking)

Thanks for your hard work!

Comment 20

[Dave Hylands [:dhylands]]

https://github.com/mozilla-b2g/gaia/commit/19e985b3ff0874931233152a1ec31ad1407de707

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment 0 makes it sound like 2.0 is affected as well? What about 1.4?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

And reverted for the same Linter failures that were visible on your Gaia Try run.
Master: https://github.com/mozilla-b2g/gaia/commit/ee6b41098f0ad8c70f9db289627f9d215df3eff0

https://treeherder.mozilla.org/logviewer.html#?job_id=1210256&repo=b2g-inbound

Comment 23

[Ryan VanderMeulen [:RyanVM]]

And Gaia UI test failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=1210266&repo=b2g-inbound

Please verify that your Gaia Try run is passing before pushing next time to avoid inconveniencing other developers.

Comment 24

[Dave Hylands [:dhylands]]

Hmm. Gaia try showed green on Gaia unit tests here: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=2d43046720cb

I'll figure out what's wrong...

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Gaia *UI* tests - they show up under the Gip group. Gu is the Gaia unit tests.

Also, what branches are affected by this bug?

Comment 26

[Dave Hylands [:dhylands]]

I think that the error was in a routine that the unit tests intercept, which is why the unit tests passed.

The real error was lost in the sea of reds (my try run happened to run when the the gaia tree was closed) and I missed it. Sorry.

Comment 27

[Dave Hylands [:dhylands]]

Completely green gaia try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=d3384a681c1e

Now I just have to wait for gaia to reopen

Comment 28

[Dave Hylands [:dhylands]]

https://github.com/mozilla-b2g/gaia/commit/8012698afd2a38aaf9d085f7992603e172f35646

Comment 29

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/0518f4581a0925c0b703d730ef289ab15cbd1216
v2.1: https://github.com/mozilla-b2g/gaia/commit/54d92cc0755e5102223276ab23063b5eee74b514

Comment 30

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

(In reply to Alive Kuo@Paris~2/17 [:alive][NEEDINFO!] from comment #14)
> (In reply to Paul Theriault [:pauljt] from comment #6)
> > I'm unclear about usb storage, don't know if we ever prevented that based on
> > the lockscreen status. We may have but I don't know.
> > 
> > As for adb, the security requirement is  we should not be able to initiate a
> > new adb connection to a locked phone. IE if you steal someones phone, even
> > if they have edb debugging enabled, you musn't be able to get to it. 
> > 
> > To enforce this, I believe we currently disable adb via adb access via an
> > android property when the phone is locked, and we are not currently
> > debugging. 
> > 
> > Dave, any idea why we might be seeing this interaction?
> > 
> > Stephanie, I won't have time to dig into this further today, but if it was
> > me, I would start by looking at all the code which references the
> > persist.sys.usb.config android pref.
> 
> Do we need to care passcode-enabled phone here only or even the passcode is
> off?

Late reply, but only a passcode enabled phone. if the passcode is disabled, a local attacker can do whatever they want with the device, including enabling USB mounting.