ADB 'connect' command may trigger a device to be owned by the user!

denis99999 commented 6 years ago

What is the issue or idea you have? If the remote debug URL of a device is known while this device is free that is owned by no one, a STF user, that is an authenticated one, may run an adb connect command on this URL which triggers the acquisition of control of this device by this user.

This behaviour is abnormal and is compounded by the latest version of ADB (1.0.40 Version 4986621) which comes with this new feature:

Add support for reconnection of TCP connections. Upon disconnection, adb will attempt to reconnect for up to 60 seconds before abandoning a connection

resulting to an automatic owning of the device every time the user try to release the device either through the Stop Using button or the API or automatic timeout (i.e. which all disconnect the TCP connection), until the user kills the adb server or runs an adb disconnect command on this URL.

The worst part is that in case of a STF machine restarting, the TCP port of the Remote debug URL may change for the considered device, and even can be assigned to another device of the same stf-provider, leading the automatic connection to be made to the latter, I have seen this behaviour by making tests.

Does it only happen on a specific device? Please run adb devices -l and paste the corresponding row. no device specific.

Please provide the steps to reproduce the issue.

get the remote debug URL url-1 of the device device-1 using UI or API, and then keep free device-1

run the command : adb connect url-1

tale a look at UI and observe that device-1 is now owned by the user

run the command : adb devices to check that device-1 is well connected

try to release device-1 using UI or API or automatic timeout, and in case of latest ADB binary is used, observe that device-1 is owned and connected again just after being released

What is the expected behavior? At step 2 above, ADB should return an error like :

unable to connect to device-1: Connection refused

What is your proposal? In fact, I felt some problems in the code:

the API remoteDisconnectUserDeviceBySerial does not work properly because it does not avoid the user to ADB connect to the target device

the API deleteUserDeviceBySerial as well as kicking actions triggered at UI side and automatic timeout should include a processing to stop unexpected ADB connections.

So, for test purpose only, I have made some simple changes in a test branch (i.e. see below), and I have tested it with success, I hope that will help you to bring the complete and final changes into the master branch.

diff --git a/lib/units/device/plugins/connect.js b/lib/units/device/plugins/connect.js
index 90bf3f7..2bc728c 100644
--- a/lib/units/device/plugins/connect.js
+++ b/lib/units/device/plugins/connect.js
@@ -117,6 +117,11 @@ module.exports = syrup.serial()
       if (plugin.isRunning()) {
         activeServer.close()
         activeServer.end()
+
+/******************* added ******************/
+        activeServer = null
+
+/*****************************************************/
       }
     })

@@ -131,8 +136,12 @@ module.exports = syrup.serial()
     }

     lifecycle.observe(plugin.stop)
-    group.on('leave', plugin.end)

+/******************* replaced ******************/
+    //group.on('leave', plugin.end)
+    group.on('leave', plugin.stop)
+
+/*****************************************************/
     router
       .on(wire.ConnectStartMessage, function(channel) {
         var reply = wireutil.reply(options.serial)
@@ -163,7 +172,12 @@ module.exports = syrup.serial()
       })
       .on(wire.ConnectStopMessage, function(channel) {
         var reply = wireutil.reply(options.serial)
-        plugin.end()
+
+/******************* replaced ******************/
+        // plugin.end()
+        plugin.stop()
+
+/********************************************************/
           .then(function() {
             push.send([
               channel

Nevertheless, using the latest version of ADB, a little problem remains as described hereafter:

user-1takes the control of device-1

user-1 ADB connects to device-1

user-1 releases device-1 at time T0

(ADB tries to connect to device-1 without success because the server for device-1 is stopped, so it's OK)

Before time T0+60s, user-2 takes the control of device-1

ADB always tries to connect to device-1 without success because it is unauthorized (i.e. bad ADB key), but since the server for device-1 is now started, a TCP connection is still set up about every 300 ms, and the server logs them at the same rate (i.e New remote ADB connection from ::ffff:192.168.1.127), until user-2 decides to release device-1, or user-1decides to ADB disconnect from device-1.

So, perhaps only authorized ADB connections should be logged ?

Do you see errors or warnings in the stf local output? If so, please paste them or the full log here. N/A

Please run stf doctor and paste the output here. debian@debian:~/stf-master$ stf doctor

2018-10-26T15:43:07.148Z INF/cli:doctor 30483 [*] OS Arch: x64
2018-10-26T15:43:07.149Z INF/cli:doctor 30483 [*] OS Platform: linux
2018-10-26T15:43:07.150Z INF/cli:doctor 30483 [*] OS Platform: 3.16.0-4-amd64
2018-10-26T15:43:07.150Z INF/cli:doctor 30483 [*] Using Node 8.9.3
2018-10-26T15:43:07.168Z INF/cli:doctor 30483 [*] Using ZeroMQ 4.0.5
2018-10-26T15:43:07.185Z INF/cli:doctor 30483 [*] Using ADB 1.0.36
2018-10-26T15:43:07.186Z ERR/cli:doctor 30483 [*] ProtoBuf is not installed (`protoc` is missing)
2018-10-26T15:43:07.211Z INF/cli:doctor 30483 [*] Using GraphicsMagick 1.3.20
2018-10-26T15:43:07.232Z INF/cli:doctor 30483 [*] Using RethinkDB 2.3.6~0jessie

thinkhy commented 6 years ago

@denis99999 your fix make my day! I merged your code into the device component, which resolved the re-connect problem during test automation(we implemented a similar auto-reconnt mechnism in test agent). For the excessive server logs, we may need to limiit the rate of "adb connect", which would release the problem.

@sorccu suggest to merge the code into master branch, it will fix a security problem more or less.

denis99999 commented 6 years ago

@thinkhy, thanks a lot !!!

sorccu commented 6 years ago

FYI it functions like that on purpose. If you connect to a device that is not being used, but you have an adb key registered, the device automatically becomes yours. This is not a security issue because the user’s adb key is verified during the handshake. If you attempt to connect to a device that is being used by someone else, or you have not registered your adb key, you do not get in.

denis99999 commented 6 years ago

Hi @sorccu , is it mean you will not include this fix into the master branch ?

thinkhy commented 6 years ago

make sense, but consider the following scenario:

user A registered adb public key into STF on test server S1, he can log into STF and connect any device on the server.
user B can also use the test server S1(it's common for test server), so he can connect any devices on S1 without logging into STF.
Even worse, user B may download the ADB public key from test server, so he may connect any devices anywhere ...

sorccu commented 6 years ago

The public key is not enough. During the handshake a challenge is represented and it must be signed by the private key of that public key. The result is then verified.

If someone takes the adb private key from e.g. a CI server, then yeah they can get in - but if they have that much access they probably could’ve used some other way to get in, too.

sorccu commented 6 years ago

The reconnection issue isn’t great though. We might have to remove this feature just because of that.

I’m just saying that it originally had a purpose.

denis99999 commented 6 years ago

Ok @sorccu, so do I understand you accept this fix although it does not fix a security issue ?

sorccu commented 6 years ago

I would suggest creating a pull request. Then it would be easier to review.

denis99999 commented 6 years ago

Yes @sorccu you are right I will create a pull request when I can, waiting for that let me explain hereafter why I feel it is important to merge this fix to the master branch.

I feel actually this issue is both a functional and consistency one, but perhaps in the future it could become a security issue in case of the adding of new high level features as a booking system for instance.

Functional issue

As described in my opening remarks, when a user - let us name him Bob - ADB connects to a device, he takes automatically the control of the device permanently, despite the releasing of the device through the UI, the API or an automatic timeout.

So, let us imagine Bob forgets to ADB disconnect from the device and then go on vacation, the only solutions for the admin to be sure to release the device are then the following:

(software) stop the STF provider, waiting for 60s, then start the STF provider
(hardware) USB disconnect the device from the STF provider, waiting for 60s, then USB connect the device to the STF provider

So, I feel from a functional point of view that STF should not meet such situation.

Consistency issue

I talk about consistency because the ADB connect action depends on the prior knowledge of the Remote debug URL, except that to get this URL - for example using API - the user has to make the API call POST /user/devices/{serial} remoteConnect, and before that to make the API call POST --data={serial} /user/devices to own the device, meaning that the device should be normally owned by the user before this one is able to ADB connect to the device.

Moreover, the user has no right to assume that the Remote debug URL will never change, for example storing this URL after a proper sequence of API calls (i.e. as described above) to use it next times to perform blind and direct ADB connect actions, simply because this URL may really change across provider restarting or device USB reconnections. The worst case being after a provider restarting in less than 60s, in which the URL would identify another device, triggering the automatic connection to this bad device!

Another inconsistency actually is that an authenticated ADB connection to a device triggers the owning of the device, but the corresponding ADB disconnection from the device does not release the device.

In synthesis, my general feeling is that ADB should be authorized to connect to a device if and only if the STF user (i.e. which has registered the corresponding ADB key), is currently owning the device and has opened the listen port (i.e. using UI or API), otherwise the attempt of connection should fail.

In the same way, the releasing of a device through the UI, the API or an automatic timeout, while an ADB connection is currently established, should not only close the connection but also close the listen port so as to forbid a subsequent attempt of connection from ADB.

This is why I proposed this fix with the main goal to ensure these strict conditions:

[1] owning of the device on: o clicking the Use button of UI o calling API POST --data={serial} /user/devices
[2] releasing of the device on: o clicking the Stop Using or Stop Automation buttons of UI o automatic timeout o calling API DELETE --data={serial} /user/devices
[3] opening of the device worker listen port and getting the Remote debug URL on: o clicking the Use button of UI o calling API POST /user/devices/{serial} remoteConnect
[4] closing of the device worker listen port on: o clicking the Stop Using or Stop Automation buttons of UI o automatic timeout
o calling API DELETE /user/devices/{serial} remoteConnect o calling API DELETE /user/devices/{serial}

So, for example using API, a typical sequence of actions a user should perform to ADB connect to a device should be:

own the device [1] : calling API POST --data={serial} /user/devices
open the device worker listen port and get the Remote debug URL [3] : calling API POST /user/devices/{serial} remoteConnect
ADB connect to the device

and, a typical sequence of actions a user should perform to ADB disconnect from a device and release it should be:

ADB disconnect from the device
release the device and close the device worker listen port [2][4] : calling API DELETE --data={serial} /user/devices

To ensure the provider does not open the listen port of each device worker at starting stage, I made an additional change in the connect plugin:

-    return plugin.start()
-      .return(plugin)
+/******************* replaced ******************/
+    //return plugin.start()
+    //  .return(plugin)
+    return(plugin)
+
+/********************************************************/

Security issue (perhaps in the future)

Actually, as said by @sorccu there is no security issue because the ADB connection is authenticated.

Nevertheless, this proposal which forces the prerequisite owning of the device before to access it through ADB could be also a prerequisite to host inside STF some value added features relying on a strict control of the access to the devices.

For instance, let us imagine a booking system allowing a user named Bob to reserve the device-1 at 11th of November from 04:00 pm to 05:00 pm, ensuring that no one except Bob will be able to own device-1 at this date.

Ok, but what will happen if another user named Lea decides anyway at this date to ADB connect to device-1 during a slot time when it is not owned by Bob ? The response in this case is that STF will face to a security issue!

So, in conclusion, I feel that enforcing the access control to the devices is not only a benefit for STF consistency, it is also probably a prerequisite to leverage the adding of value added features into the amazing STF tool.

jupe commented 4 years ago

@denis99999 any change to push PR for reviewing ? @sorccu is there unit/integration tests which verify current functionality related original behaviour ?

denis99999 commented 4 years ago

Hi @jupe , This issue I raised has been resolved in my global PR #1056 (and related #1057), this solution has been fully tested in internal Orange SA (my company) and it is also used by many external users, it works also fine with the support of Android 10 bring by @pcrepieux (see #1127 and so on) a member of my team.

amrsa1 commented 4 years ago

Hi @jupe , This issue I raised has been resolved in my global PR #1056 (and related #1057), this solution has been fully tested in internal Orange SA (my company) and it is also used by many external users, it works also fine with the support of Android 10 bring by @pcrepieux (see #1127 and so on) a member of my team.

hi @denis99999 does that mean that your PR regarding group feature supports Android 10 ? and can work fine with any android mobile version 10

denis99999 commented 4 years ago

Hi @Amrkamel1 ,

Yes, internally in our private STF platform ! But very soon (i.e. probably this day or this week) we (i.e. I and @pcrepieux) will update our Orange Open Source repository in order to have a ready to use solution integrating all our value-added features as Android 10 support and Booking/partitioning system.

pcrepieux commented 4 years ago

Hi @Amrkamel1 , I just merged the necessary changes on our public repos so that you can give them a try pretty easily. Android 10 support requires minicap, minitouch, STFService.apk as well as stf to be updated. The booking feature has also been merged. You can also check the corresponding PRs:

Hope this helps

amrsa1 commented 4 years ago

@denis99999 @pcrepieux

Guys congrats thats awesome job, will add great value to the platform cant wait to test it and use it very happy for these news

amrsa1 commented 4 years ago

Hi @Amrkamel1 , I just merged the necessary changes on our public repos so that you can give them a try pretty easily. Android 10 support requires minicap, minitouch, STFService.apk as well as stf to be updated. The booking feature has also been merged. You can also check the corresponding PRs:

1056

1057

1127

minicap#190

minitouch#50

STFService.apk#38

Hope this helps

So all what i have to do is just pull group-feature branch, run npm install,rm rethinkdb ,install manually the new STFService.apk ?

What else i should do?

denis99999 commented 4 years ago

Hi @Amrkamel1 ,

Not exactly, what @pcrepieux said is now the master branch of our Orange SA repository (i.e. https://github.com/Orange-OpenSource/stf/tree/master) is a ready to use solution integrating all our value-added features as Android 10 support and Booking/partitioning system, so you have just to pull this master branch! Of course after that, you have to make a clean npm install, etc. and due to the Group feature bringing, you have also to erase any existing rethink database before to launch STF simply because the database model has changed.

amrsa1 commented 4 years ago

Hi @Amrkamel1 ,

Not exactly, what @pcrepieux said is now the master branch of our Orange SA repository (i.e. https://github.com/Orange-OpenSource/stf/tree/master) is a ready to use solution integrating all our value-added features as Android 10 support and Booking/partitioning system, so you have just to pull this master branch! Of course after that, you have to make a clean npm install, etc. and due to the Group feature bringing, you have also to erase any existing rethink database before to launch STF simply because the database model has changed.

Great got you, about the new STFService.apk should i install it manually on each device or its already going to embedded within the master branch and gonna be installed automatically during preparing process

denis99999 commented 4 years ago

Yes, as said previously, you have just to pull the master branch, the new STFService.apk is inside and it is automatically installed onto the devices!

amrsa1 commented 4 years ago

Yes, as said previously, you have just to pull the master branch, the new STFService.apk is inside and it is automatically installed onto the devices!

Great thx

amrsa1 commented 4 years ago

@denis99999 , @pcrepieux i am getting this error when using Note10+ Android version 9 during preparing

amrka@warrior: ~_005

tried with samsung S8 android 9 it works fine, im still able to run the apk manually then connected to STF by running these commands

adb uninstall jp.co.cyberagent.stf adb install d:/xxx/STFService.apk adb shell am start -n jp.co.cyberagent.stf/.IdentityActivity adb shell am startservice -n jp.co.cyberagent.stf/.Service

Any idea why its happening with this particular device

pcrepieux commented 4 years ago

@Amrkamel1, Could you provide more logs ? I could not find the operation that timed out with your screenshot.

amrsa1 commented 4 years ago

@pcrepieux Sorry it seems that the old STF apk was working on background so when i launch the sever it didn't reinstall it, i manually uninstall the old apk and connect the device it install the new apk with no problem

but there is another issue is happening not so major but will be good if its fixed, when you plugged the android device to the usb port some notification pops up "Allow access to phone data " , if you click allow or deny the session will be terminated and the device will be disconnected this wasnt happening as far i remember with the previous version to avoid this issue just click on any other area in the screen and the message will disappear without interrupting the session Selection_006

Selection_007

denis99999 commented 4 years ago

@Amrkamel1 ,

The repository has been updated, can you try it to see if it fixes your issue ?

amrsa1 commented 4 years ago

@denis99999 thanks for the fix i will try it asap is it on master branch ?, also wanna let u know i have tried the new android 10 upgrade on huawei and its working fine.

but encountered issue with xiaomi android 9 it doesn't show any screen monitoring its blank i have reported an issue for that with all details and logs

https://github.com/openstf/STFService.apk/issues/41#issuecomment-566015594

https://github.com/openstf/STFService.apk/issues/40#issuecomment-566014437

denis99999 commented 4 years ago

@Amrkamel1 yes on master branch!

amrsa1 commented 4 years ago

@denis99999 hi denis thanks for the fix for rotating icon

shri-0509 commented 4 years ago

@denis99999 @pcrepieux Hi, I see on oneplus models while landscape mode click events are not working, only in portrait mode it works

shri-0509 commented 4 years ago

@Amrkamel1 r u not facing the above issue when in landscape mode. even i tried on pixel also same prob click events not working.

amrsa1 commented 4 years ago

@Amrkamel1 r u not facing the above issue when in landscape mode. even i tried on pixel also same prob click events not working.

Will check this scenario and let u know

pcrepieux commented 4 years ago

@shridharkabbur, @Amrkamel1 You are right, touch events are broken when putting the screen in landscape mode. When using the InputManager for Android 10 devices, I actually have to transform the coordinates in the STFService.apk. Will fix that.

amrsa1 commented 4 years ago

@pcrepieux Great thx i will retest it as well after u push, also i would like to ask you if you have noticed that serial number is appearing as unknown after recent security update in android 10

UPDATE : Verified on note 10+ working perfect

pcrepieux commented 4 years ago

screen rotation should now be properly handled for touch events (fixed in STFService.apk)

shri-0509 commented 4 years ago

thanks @pcrepieux screen orientation working..

amrsa1 commented 4 years ago

@pcrepieux i have noticed that pinch feature is not working, @shridharkabbur can you confirm if you have same issue, it was working properly before.

also find device icon in the info tab in not functioned any more

shri-0509 commented 4 years ago

@Amrkamel1 yup you are right its not working.

pcrepieux commented 4 years ago

Yep, it currently only support single touch events. As you have mentionned it, I just worked on it and got it working on a development branch. Should update it shortly. Stay tuned.

pcrepieux commented 4 years ago

An apk for testing purpose can be downloaded here Here is the related PR: https://github.com/openstf/STFService.apk/pull/42 NB: You might have to uninstall the previous one.

amrsa1 commented 4 years ago

@pcrepieux great thx will try it and let you know

openstf / stf