NeoApplications / Neo-Backup

backup manager for android
GNU Affero General Public License v3.0
2.52k stars 124 forks source link

[Bug] KernelSU fails root check with ksu Custom App Profile #796

Open CyanChanges opened 1 year ago

CyanChanges commented 1 year ago

Description when using KernelSU App Profile, and set uid != 1 App shows Non-Root. I think I grant enough permission for backup apps.

Steps To Reproduce

  1. Enable SU for NeoBackup in KernelSU
  2. Configure App Profile with uid != 0 like 100
  3. (Also you can add group should be required for backup like app, package_info, and install groups)
  4. Shows Non-Root page

Expected behavior Go to Main Page

Screenshots Screenshot_20230927-173040 Screenshot_20230927-173038

System Information(please complete the following information):


Additional Notes

Thanks for reporting.

hg42 commented 11 months ago

well, when kernelSU appeared on the scene, we changed the logic to

if pref useMountMaster is set and the shell has --mount-master (according to libsu): use echo "some command" | su --mount-master else if pref useSu0 is set: use echo "some command" | su 0 (so running as uid 0) else: use echo "some command" | su

I now use kernelSU myself (but without special settings).

I see it has --mount-master, so su 0 shouldn't be used. However we rely on libsu from the Magisk author. So, it seems it uses su 0 internally (and as far as I know, that's the way all superuser solutions worked before kernelSU).

Just curious, why do you need the uid different from 0?

hg42 commented 11 months ago

that said, I think I have a fairly current kernelSU implementation. But you may have another version (even a newer one).

Maybe yours doesn't provide the --mount-master option or it's disabled in some way.

I would say "please try if disabling useMountMaster and useSu0 changes the behavior". But you are not getting far enough to do that...

You could edit the xml file in shared_prefs folder inside NBs data folder.

Maybe we should disable both options by default. We already use nsenter if available, so --mount-master shouldn't be necessary and su should default to the appropriate uid.

hg42 commented 11 months ago

if pref useMountMaster is set and the shell has --mount-master (according to libsu): use echo "some command" | su --mount-master else if pref useSu0 is set: use echo "some command" | su 0 (so running as uid 0) else: use echo "some command" | su

as I said, we changed the code at that time...

But currently it's only using su --mount-master or su. The two preferences don't exist any more.

if mount-master exists is checked by this command:

echo true | su --mount-master 0

The 0 should probably be removed.

But I think, it never gets to that one, because the root check already fails and that's done by libsu.

hg42 commented 11 months ago

the latest libsu code uses this:

    public synchronized static Boolean isAppGrantedRoot() {
        if (currentRootState < 0) {
            if (Process.myUid() == 0) {
                // The current process is a root service
                currentRootState = 2;
                return true;
            }
            // noinspection ConstantConditions
            for (String path : System.getenv("PATH").split(":")) {
                File su = new File(path, "su");
                if (su.canExecute()) {
                    // We don't actually know whether the app has been granted root access.
                    // Do NOT set the value as a confirmed state.
                    currentRootState = 1;
                    return null;
                }
            }
            currentRootState = 0;
            return false;
        }
        switch (currentRootState) {
            case 0 : return false;
            case 2 : return true;
            default: return null;
        }
    }

so, it first looks if it is running as uid 0, which return true, then it tries to find an executable su, no su -> returns false otherwise the result is null

well, at least I didn't expect this, we simply return the result...which should create an exception, that is probably catched somewhere.

hg42 commented 10 months ago

I now have kernelSu myself...

i wonder how this works:

Configure App Profile with uid != 0 like 100

there are several aspects... e.g.

it obviously cannot work if the uid doesn't have enough permissions for NB. Did you already investigate that whole topic? I don't have time and motivation to add another topic to my big list.

Also, why do you want to change the uid at all? NB was created with full control in mind, nobody ever thought about granular permission control like KSU can do.

When I got KSU, I tried to grant granular permissions, and at the end I had granted all of them. I'm sure this could be reduced, but it's not easy to find out.

CyanChanges commented 10 months ago

well, when kernelSU appeared on the scene, we changed the logic to

if pref useMountMaster is set and the shell has --mount-master (according to libsu): use echo "some command" | su --mount-master else if pref useSu0 is set: use echo "some command" | su 0 (so running as uid 0) else: use echo "some command" | su

I now use kernelSU myself (but without special settings).

I see it has --mount-master, so su 0 shouldn't be used. However we rely on libsu from the Magisk author. So, it seems it uses su 0 internally (and as far as I know, that's the way all superuser solutions worked before kernelSU).

Just curious, why do you need the uid different from 0?

If uid is zero, you can simply use su again to grant non-limited full root permission. i want minimal permission granted. This is why I configured this Profile.

In the mainland of China, many apps abuse and their permissions. Probably it isn't happening here. But nobody know when root app have dangerous code can brick my device or something else.

For root apps full root may be dangerous and it's too much. But no strong force to supervise and securing the app about what did they done to my device.

CyanChanges commented 10 months ago

I now have kernelSu myself...

i wonder how this works:

Configure App Profile with uid != 0 like 100

there are several aspects... e.g.

  • can you use any uid?
  • should it exist before?
  • is it reused if it exists?
  • is it created by KSU if it doesn't exist?
  • are the permissions taken from an existing uid?
  • are they changed by KSU?
  • what about the selinux context (can also be configured in KSU)?

it obviously cannot work if the uid doesn't have enough permissions for NB. Did you already investigate that whole topic? I don't have time and motivation to add another topic to my big list.

Also, why do you want to change the uid at all? NB was created with full control in mind, nobody ever thought about granular permission control like KSU can do.

When I got KSU, I tried to grant granular permissions, and at the end I had granted all of them. I'm sure this could be reduced, but it's not easy to find out.

App can just pass the check if su is able to use. You don't want to tweak the options. It's okay. It is surely complex to figure out what these all options going to do. Just let the others which want to do it can tweak them.

You can just keep this issue. When you get free time, change a few lines to make uid!=0 pass the check.

hg42 commented 10 months ago

well, not using su 0 but something else (not sure what?) is the first step. After you pass the check, you probably want that it works, right?

I asked those questions for two reasons:

  1. I want to test it on a device and need to configure it the same like you do

  2. I want to understand how NB must proceed when uid!=0

It's a bit difficult with that su command. As far as I know the kernelSU developer has somehow redefined it's nature. Normally su changes the uid to someone else, so su $uid ... changes to that uid and executes the given command, or it swithces to that uid if no command given. KernelSU redefines it as su==sh.

With standard su su 0 is the same as su, because 0 is the default uid.

With a standard su, if not using 0, you would need to know the other uid and use that explicitely. E.g. if uid is set to 100 , you need to execute commands with su 100, to run the commands on that uid. If you would use su, it would run the command as uid=0 instead, so not what you want.

What's the behavior of kernelSU su command without the uid parameter? Does it default to the configured uid?

I tried to test this:

hg42 commented 10 months ago

I am debugging now...

as profile I use adb, which is uid/gid=2000, no capabilities (not sure which are needed)

before any checking takes place, libsu creates a shell first and this fails with:

java.io.IOException: Cannot run program "su": error=13, Permission denied

If I assign adb profile to Shell (com.android.shell) it behaves the same. It cannot execute su. Not sure why that adb profile exists. It's not something I would want for an adb shell.

If I use a custom kernelSU profile, based on adb but with all capabilities added, libsu still cannot create a shell.

So for now it's not NB's fault. If libsu cannot create a privileged shell there is no chance to proceed.

Currently, I don't understand, why libsu cannot create a shell...

hg42 commented 10 months ago

btw. you see this in the log:

libsu shell status = _status_ = UNKNOWN|NON_ROOT_SHELL|ROOT_SHELL|ROOT_MOUNT_MASTER|unknown code

(note: you need to enable logToSystemLogcat before switching the profile, otherwise you want get a log, something on my list)

I get 0 = NON_ROOT_SHELL, because libsu cannot create a privileged shell.

Do other apps using libsu work with your non-0 configuration?

hg42 commented 10 months ago

can you show the lower part of your configuration?

hg42 commented 10 months ago

libsu is explicitly programmed to check uid=0, otherwise it is not seen as a root shell:

private Integer shellCheck() throws IOException {
    ...
    this.STDIN.write("id\n".getBytes(StandardCharsets.UTF_8));
    this.STDIN.flush();
    s = br.readLine();
    if (!TextUtils.isEmpty(s) && s.contains("uid=0")) {
        status = 1;
        Utils.setConfirmedRootState(true);
    ...
hg42 commented 10 months ago

for me it works with this profile: image

the key is uid=0, apart from that, you can restrict a lot, but have to make sure NB's actions still work: E.g. I think removing capabilities for audition, oder network related etc. shoudn't be a problem.

Not sure what is different, when you use another uid, in most cases being part of the root group (like in your profile) should have similar effects. Usually permission management is done via groups.

Note, NB cannot be aware of all the possible combinations. If you use such a profile, you are on your own. If you want to report an issue, you should first reproduce it with the default profile. I guess, concrete suggestions (should be PRs) might be accepted.

hg42 commented 10 months ago

NB does very minimal checks, the status of libsu is more or less the only condition and this is depending on uid=0. NB itself does no more enforce uid=0. If root works with another user, libsu should be changed.

hg42 commented 10 months ago

we could theoretically replace the libsu check by checkiing other things like access to /data and maybe some commands we use like pm.

However, as far as I understand the code, libsu throws away the shell if it is not root = uid 0:

if (shell == null && !this.hasFlags(1)) {
    try {
        shell = this.build("su");
        if (!shell.isRoot()) {
            shell = null;
        }
    } catch (NoShellException var3) {
    }
}

and then creates a user shell instead:

if (shell == null) {
    if (!this.hasFlags(1)) {
        Utils.setConfirmedRootState(false);
    }
    shell = this.build("sh");
}

which should mean, it will not work with root commands later.

Though, I'm not sure how that kernelSU profile concept works. Would it also assign the given capabilities to non su commands? Does the whole app have the given rights without using su?

CyanChanges commented 10 months ago

well, not using su 0 but something else (not sure what?) is the first step. After you pass the check, you probably want that it works, right?

I asked those questions for two reasons:

  1. I want to test it on a device and need to configure it the same like you do
  2. I want to understand how NB must proceed when uid!=0

It's a bit difficult with that su command. As far as I know the kernelSU developer has somehow redefined it's nature. Normally su changes the uid to someone else, so su $uid ... changes to that uid and executes the given command, or it swithces to that uid if no command given. KernelSU redefines it as su==sh.

With standard su su 0 is the same as su, because 0 is the default uid.

With a standard su, if not using 0, you would need to know the other uid and use that explicitely. E.g. if uid is set to 100 , you need to execute commands with su 100, to run the commands on that uid. If you would use su, it would run the command as uid=0 instead, so not what you want.

What's the behavior of kernelSU su command without the uid parameter? Does it default to the configured uid?

I tried to test this:

  • I changed the Termux kernelSU profile to adb (which uses uid/gid=2000)
  • I tried to execute su via /system/bin/su (a simple su uses a command from the Termux installation), but I don't have permission to do so, it is owned by root:shell and uses rwxr-xr-x, so this part should work Termux is not a good test platform, it has too many specialties...I probably should use something simpler. Or may be directly use NB

If you cat the su in Termux, you will find:

#!/data/data/com.termux/files/usr/bin/sh

unset LD_LIBRARY_PATH LD_PRELOAD

for p in /sbin/su /system/sbin/su /system/bin/su /system/xbin/su /su/bin/su /magisk/.core/bin/su
do
        if [ -x $p ]; then
                # The su tool may require programs in PATH:
                PATH=/sbin:/sbin/su:/su/bin:/su/xbin:/system/bin:/system/xbin \
                        exec $p "$@"
        fi
done

echo "No su program found on this device. Termux"
echo "does not supply tools for rooting, see e.g."
echo "http://www.androidcentral.com/root for"
echo "information about rooting Android."
exit 1

It unset the library set by Termux. But if you use Termux su to execute su when I using system template, it will be no error.

But I can execute /system/bin/su directly when I use default profile.

CyanChanges commented 10 months ago

I am debugging now...

as profile I use adb, which is uid/gid=2000, no capabilities (not sure which are needed)

before any checking takes place, libsu creates a shell first and this fails with:

java.io.IOException: Cannot run program "su": error=13, Permission denied

If I assign adb profile to Shell (com.android.shell) it behaves the same. It cannot execute su. Not sure why that adb profile exists. It's not something I would want for an adb shell.

If I use a custom kernelSU profile, based on adb but with all capabilities added, libsu still cannot create a shell.

So for now it's not NB's fault. If libsu cannot create a privileged shell there is no chance to proceed.

Currently, I don't understand, why libsu cannot create a shell...

Screenshot_20231129-080312 I can execute the su that doesn't seemed to be existing? I know KernelSU do hook in somewhere like vfs read file, so?

hg42 commented 10 months ago

for me it works with this profile:

this was wrong...

the real profile I used in the test probably had all capabilities enabled.

For some reason changing the profile capabilities is not always accepted (I swear I saved them! :-) ). But in my current tests it was still like in the screen shot.

I guess that starting debug via IDE didn't always kill the app first.

I now did some manual tests without the IDE.

With capabilities like in the screenshot a backup creates an empty directory and there is no properties file. Then NB is hanging.

The reason is that nsenter doesn't work, because it doesn't see /proc/1.

If I enable all capabilities, it seems to work, at least I can create a backup. Though the first backup only had apk and external_data, not sure what exactly happened. While writing this I started NB again (which gave me the splash screen) and created the backup again and this time all parts were created. I'm sure I killed and restarted NB after switching to the full capabilities profile. Perhaps we need to wait after changing the profile...or another event must happen before it's really accepted by the system. Interesting, that apk and external worked, this probably means the mount didn't work, yet.

Someone heeds to find out which capabilities are necessary. Must be one that is not listed in the screenshot.

hg42 commented 10 months ago

interesting...

I generally think Termux is not a good environment to test such things, it probably plays a lot of tricks. I usually prefer the builtin terminal. Or may be an ssh login.

NB has it's own terminal just for that reason. This way you can execute commands exactly like they are executed by NB, including the wrapping in su and nsenter and with the kernelSU profile etc.

OTOH it's not suitable to find out how these root commands should work.

hg42 commented 10 months ago

change a few lines to make uid!=0 pass the check

it seems this would not help.

As analyzed here: https://github.com/NeoApplications/Neo-Backup/issues/796#issuecomment-1793219577

libsu checks explicitly for uid=0 and only then keeps the su shell. Otherwise it starts a simple sh shell instead, which cannot do root commands. Note, in the usual case commands are executed by that shell.

Theoretically it would be possible to reimplement all the things libsu does for us. But reinventing the wheel is not really what we want to do.

I think, you should convince the dev of libsu that uid > 0 can still be privileged with KSU. NB would automatically work then... Though I'm not sure how the alternative root check should work.

The whole thing is a result from the fact that kernelSU redefines the su command (at least if su without an uid does not switch to uid=0, not sure about that).

hg42 commented 10 months ago

I can execute the su that doesn't seemed to be existing?

please try /system/bin/su outside of Termux... Termux seems to inject libraries, that's why the corresponding environment vars are cleared in the script. The point is, Termux tries to provide a linux environment.

A simple terminal should not do such things. The behavior would be different.

Try the command /system,/bin/su -c id to see which uid the resulting shell uses, if you give the terminal uid > 0.

hg42 commented 10 months ago

I could create a pumpkin (=test) apk, that simply omits the root check. But as I said, I'm quite sure that libsu will not work then. I can also try to not use the usual libsu commands at all (not sure if I want to do that, maybe it's reinventing the whole thing, at least it will be slower, though that doesn't matter much for a proof of concept).