Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce --root option for (rooted) secure display support on Android 12+ #4127

Draft
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

RiggiG
Copy link

@RiggiG RiggiG commented Jun 29, 2023

Resolves #3049, launches the server as AID_SYSTEM when --root is passed.

Functionality confirmations with --root:

  • Video (13)
  • Device audio (13)
  • Clipboard from device to PC (13, 10) No error output, nor INFO: Device clipboard copied
  • Clipboard from PC to device (13, 10) Works when package name matches UID
  • Stay awake ()
  • Screen off ()

Android 14 fails to create a secure display, but the virtual display is owned by AID_SYSTEM.

@yume-chan
Copy link
Contributor

yume-chan commented Jun 29, 2023

The option name is --root but it actually runs as uid 1000 might be a little mis-leading.

I reserved the --uid flag for you #3729 :-)

IIRC audio capture should also work with uid 0/1000 on all versions #4094

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.R) {
Ln.w("Audio disabled: it is not supported before Android 11");
streamer.writeDisableStream(false);
return;
}

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.R) {
Ln.w("Audio disabled: it is not supported before Android 11");
streamer.writeDisableStream(false);
return;
}

I remember clipboard syncing was having issues when running as uid 0, not sure about 1000. #1606

@RiggiG
Copy link
Author

RiggiG commented Jun 29, 2023

The option name is --root but it actually runs as uid 1000 might be a little mis-leading.

Yeah, I figured since 0 is an unnecessary elevation but root privs are required to elevate to 1000 for system privileges anyhow, --root would mean a lot more to the end user than --AID_SYSTEM-via-root-auth, and --uid=[value] would be more of a debug flag than a useful/friendly-to-the-end-user flag. We can just note that the flag executes it as AID_SYSTEM in docs/help output. I certainly defer to the consensus here on that though. This was briefly discussed here.

IIRC audio capture should also work with uid 0/1000 on all versions

That's a good point and makes sense, unfortunately I don't have anything configured for testing other than my current Android 13 device - I'll scrounge up my emulator and give that a shot on some older versions when I've got some time.

I remember clipboard syncing was having issues when running as uid 0, not sure about 1000.

This I can definitely test in a timely manner (if someone doesn't beat me to it), thank you.

@@ -74,12 +74,12 @@ private static void startWorkaroundAndroid11() {
Intent intent = new Intent(Intent.ACTION_MAIN);
intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
intent.addCategory(Intent.CATEGORY_LAUNCHER);
intent.setComponent(new ComponentName(FakeContext.PACKAGE_NAME, "com.android.shell.HeapDumpActivity"));
intent.setComponent(new ComponentName(FakeContext.getPackageNameStatic(), "com.android.shell.HeapDumpActivity"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This activity is in com.android.shell package, this line shouldn't change.

ServiceManager.getActivityManager().startActivityAsUserWithFeature(intent);
}

private static void stopWorkaroundAndroid11() {
ServiceManager.getActivityManager().forceStopPackage(FakeContext.PACKAGE_NAME);
ServiceManager.getActivityManager().forceStopPackage(FakeContext.getPackageNameStatic());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, we want to always kill com.android.shell app.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two should be cleared up

@@ -23,11 +24,24 @@ private FakeContext() {

@Override
public String getPackageName() {
if (Os.getuid() == 1000) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to call getPackageNameStatic here and in getOpPackageName to not duplicate code.

@yume-chan
Copy link
Contributor

I figured since 0 is an unnecessary elevation but root privs are required to elevate to 1000

Oh, sure. I was looking for past vulnerability reports that give uid 1000 without root in recent weeks (got some ideas, but haven't tested).

}

public static String getPackageNameStatic() {
if (Os.getuid() == 1000) {
return "android";
}
return PACKAGE_NAME;
return PACKAGE_SHELL;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably assign the PACKAGE_NAME constant without a method:

public static final String PACKAGE_NAME = Os.getuid() == 1000 ? "android" : "com.android.shell";

In practice, what is the problem if the package name is "com.android.shell" while the uid is 1000?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least the clipboard manager is entirely busted when UID and package don't match up -

Package com.android.shell does not belong to 1000
[server] ERROR: Could not invoke method
java.lang.reflect.InvocationTargetException
      at java.lang.reflect.Method.invoke(Native Method)
      at com.genymobile.scrcpy.wrappers.ClipboardManager.getPrimaryClip(ClipboardManager.java:87)
      at com.genymobile.scrcpy.wrappers.ClipboardManager.getText(ClipboardManager.java:118)
      at com.genymobile.scrcpy.Device.getClipboardText(Device.java:285)
      at com.genymobile.scrcpy.Device.setClipboardText(Device.java:298)
      at com.genymobile.scrcpy.Controller.setClipboard(Controller.java:398)
      at com.genymobile.scrcpy.Controller.handleEvent(Controller.java:164)
      at com.genymobile.scrcpy.Controller.control(Controller.java:83)
      at com.genymobile.scrcpy.Controller.lambda$start$0$com-genymobile-scrcpy-Controller(Controller.java:91)
      at com.genymobile.scrcpy.Controller$$ExternalSyntheticLambda1.run(Unknown Source:4)
      at java.lang.Thread.run(Thread.java:1012)
Caused by: java.lang.SecurityException: Package com.android.shell does not belong to 1000
      at android.os.Parcel.createExceptionOrNull(Parcel.java:3011)
      at android.os.Parcel.createException(Parcel.java:2995)
      at android.os.Parcel.readException(Parcel.java:2978)
      at android.os.Parcel.readException(Parcel.java:2920)
      at android.content.IClipboard$Stub$Proxy.getPrimaryClip(IClipboard.java:387)
      ... 11 more
Caused by: android.os.RemoteException: Remote stack trace:
      at android.app.AppOpsManager.checkPackage(AppOpsManager.java:8803)
      at com.android.server.clipboard.ClipboardService.clipboardAccessAllowed(ClipboardService.java:1054)
      at com.android.server.clipboard.ClipboardService.clipboardAccessAllowed(ClipboardService.java:1040)
      at com.android.server.clipboard.ClipboardService.-$$Nest$mclipboardAccessAllowed(Unknown Source:0)
      at com.android.server.clipboard.ClipboardService$ClipboardImpl.getPrimaryClip(ClipboardService.java:461)

[server] ERROR: Could not invoke method
java.lang.reflect.InvocationTargetException
      at java.lang.reflect.Method.invoke(Native Method)
      at com.genymobile.scrcpy.wrappers.ClipboardManager.setPrimaryClip(ClipboardManager.java:107)
      at com.genymobile.scrcpy.wrappers.ClipboardManager.setText(ClipboardManager.java:133)
      at com.genymobile.scrcpy.Device.setClipboardText(Device.java:308)
      at com.genymobile.scrcpy.Controller.setClipboard(Controller.java:398)
      at com.genymobile.scrcpy.Controller.handleEvent(Controller.java:164)
      at com.genymobile.scrcpy.Controller.control(Controller.java:83)
      at com.genymobile.scrcpy.Controller.lambda$start$0$com-genymobile-scrcpy-Controller(Controller.java:91)
      at com.genymobile.scrcpy.Controller$$ExternalSyntheticLambda1.run(Unknown Source:4)
      at java.lang.Thread.run(Thread.java:1012)
Caused by: java.lang.SecurityException: Package com.android.shell does not belong to 1000
      at android.os.Parcel.createExceptionOrNull(Parcel.java:3011)
      at android.os.Parcel.createException(Parcel.java:2995)
      at android.os.Parcel.readException(Parcel.java:2978)
      at android.os.Parcel.readException(Parcel.java:2920)
      at android.content.IClipboard$Stub$Proxy.setPrimaryClip(IClipboard.java:333)
      ... 10 more

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably assign the PACKAGE_NAME constant without a method:

Definitely yes, done

@RiggiG
Copy link
Author

RiggiG commented Jun 30, 2023

I briefly tested a rooted Android 10 emulator to see if the audio would Just Work™️, no dice with the right encoder set but the clipboard syncing worked in both directions without issue - interestingly, the INFO: Device clipboard copied message was still not printed but it still works just fine.

@yume-chan
Copy link
Contributor

I also only have rooted Android 10 emulators. No matter whether adbd is running as root, with --root I got:

> .\scrcpy.exe -Vverbose --audio-codec=aac --audio-encoder='OMX.google.aac.encoder' --root
scrcpy 2.1 <https://github.com/Genymobile/scrcpy>
DEBUG: ADB device found:
DEBUG:     --> (tcpip)         emulator-5554            device  Android_SDK_built_for_x86_64
DEBUG: Device serial: emulator-5554
DEBUG: Using server (portable): D:\dev\yume-chan\scrcpy-devcontainer\scrcpy\dist\scrcpy-win64-v2.1-root_final-5-gdae2c204\scrcpy-server
D:\dev\yume-chan\scrcpy-devcontainer\scrcpy\dist\scrcpy-wi... file pushed, 0 skipped. 67.4 MB/s (57055 bytes in 0.001s)
su: failed to exec -c: Permission denied
DEBUG: Interrupting socket
DEBUG: Server disconnected
DEBUG: Server terminated
ERROR: Server connection failed

But with adb root and no --root, audio works.

> .\scrcpy.exe -Vverbose --audio-codec=aac --audio-encoder='OMX.google.aac.encoder'
scrcpy 2.1 <https://github.com/Genymobile/scrcpy>
DEBUG: ADB device found:
DEBUG:     --> (tcpip)         emulator-5554            device  Android_SDK_built_for_x86_64
DEBUG: Device serial: emulator-5554
DEBUG: Using server (portable): D:\dev\yume-chan\scrcpy-devcontainer\scrcpy\dist\scrcpy-win64-v2.1-root_final-5-gdae2c204\scrcpy-server
D:\dev\yume-chan\scrcpy-devcontainer\scrcpy\dist\scrcpy-wi... file pushed, 0 skipped. 76.1 MB/s (57055 bytes in 0.001s)
[server] INFO: Device: [Google] google Android SDK built for x86_64 (Android 10)
[server] DEBUG: Creating audio encoder by name: 'OMX.google.aac.encoder'
DEBUG: Server connected
DEBUG: Starting controller thread
DEBUG: Starting receiver thread
[server] DEBUG: Using video encoder: 'OMX.google.h264.encoder'
INFO: Renderer: direct3d
DEBUG: Trilinear filtering disabled (not an OpenGL renderer
DEBUG: Using icon (portable): D:\dev\yume-chan\scrcpy-devcontainer\scrcpy\dist\scrcpy-win64-v2.1-root_final-5-gdae2c204\icon.png
DEBUG: Demuxer 'video': starting thread
DEBUG: Demuxer 'audio': starting thread
INFO: Texture: 1080x1920
DEBUG: [Audio] Buffer underflow, inserting silence: 224 samples
VERBOSE: [Audio] Buffering: target=2400 avg=1640.105957 cur=1440 compensation=759
VERBOSE: [Audio] Buffering: target=2400 avg=1857.855224 cur=1742 compensation=542
VERBOSE: [Audio] Buffering: target=2400 avg=1997.128784 cur=2007 compensation=402
DEBUG: User requested to quit
DEBUG: quit...
DEBUG: Demuxer 'video': en[server] DEBUG: Controller stopped
d of frames
D[server] DEBUG: Device message sender stopped
EBUG: Demuxer 'audio': end of frames
DEBUG: Receiver stopped
[server] DEBUG: Screen streaming stopped
[server] DEBUG: Audio encoder stopped
DEBUG: Server disconnected
DEBUG: Server terminated

In contrast, with adb unroot:

> .\scrcpy.exe -Vverbose --audio-codec=aac --audio-encoder='OMX.google.aac.encoder'
scrcpy 2.1 <https://github.com/Genymobile/scrcpy>
DEBUG: ADB device found:
DEBUG:     --> (tcpip)         emulator-5554            device  Android_SDK_built_for_x86_64
DEBUG: Device serial: emulator-5554
DEBUG: Using server (portable): D:\dev\yume-chan\scrcpy-devcontainer\scrcpy\dist\scrcpy-win64-v2.1-root_final-5-gdae2c204\scrcpy-server
D:\dev\yume-chan\scrcpy-devcontainer\scrcpy\dist\scrcpy-wi... file pushed, 0 skipped. 70.7 MB/s (57055 bytes in 0.001s)
[server] INFO: Device: [Google] google Android SDK built for x86_64 (Android 10)
[server] WARN: Audio disabled: it is not supported before Android 11
[server] DEBUG: Audio encoder stopped
DEBUG: Server connected
DEBUG: Starting controller thread
DEBUG: Starting [server] DEBUG: Using video encoder: 'OMX.google.h264.encoder'
receiver thread
INFO: Renderer: direct3d
DEBUG: Trilinear filtering disabled (not an OpenGL renderer
DEBUG: Using icon (portable): D:\dev\yume-chan\scrcpy-devcontainer\scrcpy\dist\scrcpy-win64-v2.1-root_final-5-gdae2c204\icon.png
DEBUG: Demuxer 'video': starting thread
DEBUG: Demuxer 'audio': starting thread
WARN: Demuxer 'audio': stream explicitly disabled by the device
INFO: Texture: 1080x1920
DEBUG: User requested to quit
DEBUG: quit...
DEBUG: Demuxer 'video': en[server] DEBUG: Controller stopped
d[server] DEBUG: Device message sender stopped
 of frames
DEBUG: Receiver stopped
WARN: Killing the server...
DEBUG: Server disconnected
DEBUG: Server terminated

@RiggiG
Copy link
Author

RiggiG commented Jul 1, 2023

I also only have rooted Android 10 emulators. No matter whether adbd is running as root, with --root I got:

su: failed to exec -c: Permission denied

Yeah, the standard su binary that you have access to in AVD is quite a bit different than those provided by SuperSU and Magisk and the like - -c isn't even a flag as it's the default behavior. I've temporarily replaced the binary while testing in my emulator via a tmp filesystem mounted at /system/xbin so it matches the behavior of an actual rooted device. I'm out of town this weekend but I can give clear steps on that when I get back.

@huyz-git
Copy link

Using --root option will cause --stay-awake option to show error:

[server] ERROR: Could not change "stay_on_while_plugged_in"
com.genymobile.scrcpy.SettingsException: Could not access settings: put global stay_on_while_plugged_in 7
        at com.genymobile.scrcpy.Settings.execSettingsPut(Settings.java:24)
        at com.genymobile.scrcpy.Settings.putValue(Settings.java:59)
        at com.genymobile.scrcpy.Settings.getAndPutValue(Settings.java:78)
        at com.genymobile.scrcpy.Server.initAndCleanUp(Server.java:63)
        at com.genymobile.scrcpy.Server.lambda$startInitThread$2(Server.java:168)
        at com.genymobile.scrcpy.Server$$ExternalSyntheticLambda3.run(Unknown Source:2)
        at java.lang.Thread.run(Thread.java:1012)
Caused by: java.io.IOException: Command [settings, put, global, stay_on_while_plugged_in, 7] returned with value 255
        at com.genymobile.scrcpy.Command.exec(Command.java:16)
        at com.genymobile.scrcpy.Settings.execSettingsPut(Settings.java:22)
        ... 6 more

@yume-chan yume-chan mentioned this pull request Aug 9, 2023
@UltraHQ
Copy link

UltraHQ commented Sep 18, 2023

Hi, is there any update when this gets merged?

@Giantvince1
Copy link

I wanted to let you guys know of three things regarding this ongoing project.

First and foremost, I have opened a PR on RiggiG's current up-to-date dev branch to fix a few things that caused build problems.

Second, I have had the chance to test out the latest work he has done, along with my few fixups, and I've found that the screen off functionality still works perfectly. However I cannot confirm if this is the case on all Android versions, as this is only done with my Moto G Stylus 2023 model running on Android 13. As such I can't say if earlier Android versions, or even Android 14, allow this to work properly; I don't have the means to test it at this time across many versions.

And last, I noticed the issue with the settings change not taking place, erroring out when attempting to set the device to stay awake with the -w flag. My suggestion for this is to adapt the settings change code to use actual root temporarily whilst it does so IF, and ONLY if, the --root flag is provided, otherwise use only the current, existing logic. This is because it seems the AID_SYSTEM user doesn't seem to have permissions to change settings on the global table, but AID_SHELL does, causing the error when running the server side as AID_SYSTEM. I haven't managed to write this into my fixup patch set yet, but I plan to start a PoC and test it myself to see how it does on a physical, rooted, device (where su -c is valid, unlike emulators).

@RiggiG
Copy link
Author

RiggiG commented Dec 14, 2023

Hey thanks @Giantvince1, the most recent changes are just from me doing a rebase late at night recently to include new scrcpy features, did not do any testing whatsoever (and it appears there was definitely some weirdness in a handful of the conflict resolutions). I'll take a look at your fixups and we can get those merged in the near future.

I haven't really looked at much else as far as discourse here in some time, unfortunately my free time has been lacking of late. As far as testing in an emulator, you can mount a tmpfs at /system/xbin and drop in a different su binary (e.g. from SuperSU or Magisk) so the fork actually runs as expected, though I think I might just add another flag to pass alongside for emulation that'll just omit the -c so it can be tested on emulators without additional effort - will have to test that as well.

@RiggiG RiggiG marked this pull request as draft December 14, 2023 02:57
@Giantvince1
Copy link

The main reason I cannot currently test via emulator is that my current PC doesn't have enough CPU or RAM available, and its aging hard disk is worsening the effects. In the next couple months I should be able to run an AVD emulator effectively as well as test on real A14 hardware, as I soon plan to replace my laptop and upgrade my Android. It depends on other external factors but that is the end goal. In the meantime I can still check out the code, test, and investigate things as needed, albeit with only a physical A13 device on hand my testing will be a bit limited as of now.

Once I have some time later I'll rebase my fixes onto your new commits if any have been made, and/or try to implement some of the changes in my fork directly and see how it goes. I thank you for your cooperation thus far and I look forward to helping this make it to the official project!

@Giantvince1
Copy link

Did a little bit of hacking together a quick and dirty method to get and put settings whilst acting as AID_SYSTEM. This workaround is set up specifically to ONLY trigger if rooted AND the program is run with the --root flag by checking the UID it is running as. Settings changes now work perfectly, provided AID_SYSTEM actually gets root access. The only thing left is to get the clipboard to be able to go from device to PC on A13 devices, and we should be in the all clear after that point!

The only reason I haven't merged upstream's most recent commits is because the fork this PR is tied to is what I forked from, and my PR is on said fork, not this one. That being said, I see no conflicts between the current state of official dev and my dev branch; no changed files overlap, at least to my knowledge. I checked through the diffs a bit and didn't find anything that would break my changes at the very least.

@Giantvince1
Copy link

Giantvince1 commented Jun 22, 2024

Hello everyone that is paying attention to this PR in hopes of getting the --root toggle added to scrcpy! I have some great news! I got the patches updated to work with the current status of dev, as of 6/22/2024 at like midnight my time, as well as tested the build I just generated. Everything works flawlessly, even the bidirectional clipboard (at least on Android 14!). I have a new PR #4977 made under my own account, giving the OP of this pull request credit for the idea and his contributions that allowed this to happen. Please check it out, but keep in mind most of the "commits" to it are just me not understanding how Git works as a version control system very well, and are duplicate commits from updating the PR branch up to the current state of dev in a roundabout way. Any commits authored and/or submitted by me or RiggiG are the actual commits to pay attention to, as those are where this PR and the official dev branch differ.

@rom1v
Copy link
Collaborator

rom1v commented Jun 22, 2024

Hi,

Thank you for your work. 👍

I don't know if the --root will be merged in scrcpy, but if it works fine, does not impact non-root usage and is not too intrusive, that is a possibility.

However, as a first step, it must be implemented in a clean patchset based on dev branch (linear history, i.e. without merge commits).

Thank you

@Giantvince1
Copy link

Alright. I'll try to isolate the actual patches done, and redo it so that the only commits on top of dev are the actual edits. This may take a bit.

@Giantvince1
Copy link

Alright, I've got it fixed; the only commits on my PR on Genymobile/scrcpy are those that actually differ from ongoing progress upstream that occurred during the stall of this PR. The new PR is #5014 and is ready for upstream to look at and test.

@RiggiG
Copy link
Author

RiggiG commented Jul 11, 2024

On android 14, this fails to create a secure display, but the virtual display is owned by AID_SYSTEM. I'm frankly ill-equipped to debug this behavior, and briefly looking at the AOSP source and spending a few hours debugging, cannot identify a reason why this would be the case (assuming we're still hitting SurfaceFlinger::createDisplay via reflection.

If @rom1v or @yume-chan might have some ideas, I'd be happy to look into it further if I had a direction to go, but as of right now I'm stumped.

@Giantvince1
Copy link

Yeah, the lack of debugging ability is what stumped me as well, on top of not knowing the bit order of the flags to be able to manually specify them at virtual display creation time. I tried digging through the documentation myself but it did not yield many results, and funnily enough, the display does actually act secure on A12 and A13, but not on A14+.

In any case, between that problem and the fact that upstream is probably going to continue to progress even further before we can get it figured out at all is why I closed my PR off. I still have my fork available if you want to try to PR on that as a way to have a more up-to-date base than your own current fork, but beyond that, its usefulness is rather limited.

@yume-chan
Copy link
Contributor

On android 14, this fails to create a secure display

What does this refer to?

I think the new virtual display API uses the DisplayManager.VIRTUAL_DISPLAY_FLAG_SECURE flag to create secure displays:

https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/core/java/android/hardware/display/DisplayManager.java;l=278;drc=b4e50bcc51dfdcb1c389aa73be46104ff73a8c41

@Giantvince1
Copy link

That is true, but during testing, we have found that scrcpy will not create a secure display inside A14, even when given the context of AID_SYSTEM. It can create secure displays under A12 and A13 using this context, and on A11 and below by default as AID_SHELL has the necessary permission to do so prior to A12. I don't know what may have broken this behavior on A14, but we cannot get it to obtain a secure display at all on A14, even on generic AOSP with Magisk installed, and no other changes made at all (no modules, no /system modification, etc).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants