git-core
commented
12 years ago Does pm work when invoked w/o su but w/ root privileges? i.e. from command line w/ root credentials.
Closed miracle2k closed 12 years ago
git-core
commented
12 years ago Does pm work when invoked w/o su but w/ root privileges? i.e. from command line w/ root credentials.
miracle2k
commented
12 years ago Yes, sorry for failing to being explicit about this. It also works when piping the command, as root, through the default su binary, or through the ChainsDD binary (again, it works if I'm already root).
git-core
commented
12 years ago The only difference I can imagine is in the environment. Look, you invoke su -c pm from /system/bin/ash, but su invoke /system/bin/sh. I guess this can be a culprit. Could you look at output from the env command in both cases? Could you try to run pm from /system/bin/sh?
miracle2k
commented
12 years ago I can't seem to run "env"; I get "env: not found" as root, and "env: permission denied" while su'd into another user. This doesn't work from sh, nor from ash.
The shell doesn't seem to make a difference (I'm using svc, because it has a shorter output, but same deal).
As root:
$ adb shell
# svc
Available commands:
help Show information about the subcommands
power Control the power manager
data Control mobile data connectivity
wifi Control the Wi-Fi manager
# /system/bin/sh
# svc
Available commands:
help Show information about the subcommands
power Control the power manager
data Control mobile data connectivity
wifi Control the Wi-Fi manager
# exit
# /system/bin/ash
# svc
Available commands:
help Show information about the subcommands
power Control the power manager
data Control mobile data connectivity
wifi Control the Wi-Fi manager
# exitAs an app:
# su app_57
$ svc
Available commands:
help Show information about the subcommands
power Control the power manager
data Control mobile data connectivity
wifi Control the Wi-Fi manager
$ /system/bin/ash
Available commands:
help Show information about the subcommands
power Control the power manager
data Control mobile data connectivity
wifi Control the Wi-Fi manager
$ exit
$ svc
Available commands:
help Show information about the subcommands
power Control the power manager
data Control mobile data connectivity
wifi Control the Wi-Fi manager$ exit
So no problems there, until you start calling "svc" via su.
This also does not work:
$ /system/bin/su -s /system/bin/ash -c svc
[1] Segmentation fault svcWhat I am wondering about: When running /system/bin/su while already being root, it works fine:
$ adb shell
# /system/bin/su -c svc
Available commands:
help Show information about the subcommands
power Control the power manager
data Control mobile data connectivity
wifi Control the Wi-Fi managerLooking at the source, this is simply a short-circuit. allow() is being called directly, skipping all the database reading and APK communication parts. But is it possible that these are the cause?
miracle2k
commented
12 years ago Actually, you were right; I managed to get the environment via:
$ cat /proc/$$/environIt turns out then when calling something via su, but only when doing so from a limited user account, then LD_LIBRARY_PATH is not set. Consequently, the following works:
$ su -c "svc"
[1] Segmentation fault svc
$ su -c "LD_LIBRARY_PATH=/vendor/lib:/system/lib svc"
Available commands:
help Show information about the subcommands
power Control the power manager
data Control mobile data connectivity
wifi Control the Wi-Fi managerMy app can work with that; though it's not clear to my why this would occur in these narrow circumstances, and whether it could be fixed(?) in su.
git-core
commented
12 years ago Definitely, not in su.
LD_LIBRARY_PATH contains list of directories where dynamic loader (ld.so) shall search for dynamic libraries. My expectations are /system/lib must be in the default list in Android. That is if LD_LIBRARY_PATH isn't set, ld.so searches in /system/lib only (and, perhaps, in /lib if the latter exists). I suppose google (?) (intentionally?) kept nonprivileged accounts out of running privileged commands. Another wild guess is that in your case vendor (or google?) put some system libs in /vendor/lib. What I can't understand is why so core stuff as java.lang.ClassLoader depends on vendor libs. It's not even Android specific, it's part of Java functionality.
miracle2k
commented
12 years ago I'm just wondering what is going on at a technical level; i.e. if the environment is inherited from the parent su process, then LD_LIBRARY_PATH should be there; if it is set via some shell init script, it should be there too, after all it works when calling su from the root account.
Anyway, thanks a lot for helping.
ChainsDD
commented
12 years ago Sorry, been away and missed this. If I'm not mistaken, this is the same issue that TitaniumBackup's dev and I worked through not too long ago. It seems that it comes down to not having the proper file mode for the su binary. He was able to fix it by using chmod 06755. Some scripts were setting it to 4755, which is not the recommended file mode for su.
git-core
commented
12 years ago File permission modes shall not be relevant here but who knows. Could you point me to the problem you mentioned? My understanding until your post was that 04755 is quite enough for su on android, which doesn't distinguish between uid and gid well.
ChainsDD
commented
12 years ago If you get me your email address I can forward you the traffic between him and a guy at google that was having issues with su on ICS before it was released. You may be able to glean more info from them than I was able to.
The discovery of chmod 06755 vs 4755 was mentioned on twitter, between him (matrixrewriter), PaulOBrien and myself.
miracle2k
commented
12 years ago FWIW, I did use 06755. As git-core explained to me, of course LD_LIBRARY_PATH would have to be unset, or one could inject code into su, so I'm quite sure this explains what I've been seeing.
git-core
commented
12 years ago I am not sure we aren't able to forward parent environment to the child and to keep su secure. Certainly, we need some discussion before implementing the patch. First of all, who added the requirement for LD_LIBRARY_PATH to all processes in ICS? Is it vendor/ROM creator specific or made by google in stock ICS? Is there a thing like /etc/ld.so.conf as in GNU libc or bionic hardcodes library paths? Finally, could anybody think up some scenario where inheriting all environment from the parent to the child may lead to secure vulnerabilities?
ChainsDD
commented
12 years ago As far as I know, the requirement is in stock ICS, as there were reports if the issue on pre-release builds.
You mentioned that many suid programs on various Unixes set up a sane environment when they are run, would it be possible to have su do that? I guess it would all depend on figuring out what a minimum sane environment is...
git-core
commented
12 years ago Unfortunately, we don't have a source of the sane environment. Original su has two forms: su (w/o arguments) and su -. The latter passes "-" to the shell, so it executes startup files. Android doesn't have accounts and, hence, startup files at all.
What we can do is to read /proc/
koush
commented
12 years ago I've fixed this in CM. This also is in fixed in gc-ics of this tree.
My fix is a bit different. Will push it to master shortly.
git-core
commented
12 years ago Did you mean commit 9b9a9cd24ae0e02f1a3b014739d40d22420ad06d in CyanogenMod/android_system_su? It fixes another issue and, btw, definitely needs some discussion. There is another idea of how to not depend on the intent which content is being constantly modified by google and others. su may send a transaction instead of full intent. The interfaces of transact() and Parcel seem to mature enough and the code of both is in a dynamic library. Superuser has to be modified to receive the transaction (via separate service?). As an additional advantage, su may send open fd in parcel. It's the only thing that is really required now anyway.
koush
commented
12 years ago @git-core There was some discussion on gerrit, cyanogenmod-priv, and other channels.
It fixes the issue at the top of this bug (seg fault when running any java processes from the CLI after su), as well as changes how the broadcast intent is sent to stabilize that code.
Using the Android internal C APIs seemed like a great idea in theory, but in practice has turned into a mess. For example, I just looked at the gc-ics branch and saw we had to add additional tweaks to support some weird HTC parcel errata on the Ville :(
Also, due to possible different prelink maps, missing/changing libs, having a dynamically linked su is not a great idea.
git-core
commented
12 years ago The commit is wrong. The worst part is invoking am w/o absolute path. Someone may easily force su to invoke his own version of am. The rest of the commit
For example, I just looked at the gc-ics branch and saw we had to add additional tweaks to support some weird HTC parcel errata on the Ville :(
Again, it's because we're playing dirty games with an intent. Unfortunately, there are no native interfaces for sending intents. Google sends simple transactions only from the native code. So should su. BTW, is the patch is good for CM? I mean, does CM uses own value for ro.com.google.clientidbase?
Also, due to possible different prelink maps, missing/changing libs, having a dynamically linked su is not a great idea.
Do you propose to link su statically??? There is some code in su, e.g. logging and sqlite handling, which might be in trouble, I'm afraid. su is dynamically linked even on Linux distros nowadays.
koush
commented
12 years ago Yeah, I should make sure am runs from /system/bin. Fixed in 7dd78d56ad5353b48f561d085e888490bee559ba.
Seems you are not understanding the code. The unsetting is not superfluous, I am intentionally matching the euid and eguid to uid and gid, which means linker_env_secure does not get called. Not doing so makes it get unset every time a binary is called from within a suid context, not just the first invocation. There is no way to start am (or any child process) without losing the LD_LIBRARY_PATH, unless eguid/euid are matched to uid/gid. The am call is just a script which execs app_process, which will lose LD_LIBRARY_PATH again upon invocation. I may need to restore the euid and eguid after the am call; looking into that actually to see how it affects child processes. Furthermore, that security fix is only in ICS, and it should be ported into su for pre-ics Android builds (for when calling into dynamically linked app_process/am). Can't rely on the OS sanitizing the LD_LIBRARY_PATH. This is also one of the reasons su is statically linked. So, the LD_LIBRARY_PATH setting is not superfluous, and most certainly should not be inherited until after "am" is called and access is granted. I'll probably use your code to copy the LD_LIBRARY_PATH environment after the su access has been granted.
Linux su is not copied and pasted between different linux distributions like our su is. Their use case is entirely different. Sqlite will work perfectly fine regardless of which sqlite version is accessing it. The data format is forward and backwards compatible between versions. Log format may change between versions (but it has not since v1), and it would be a nonissue if it did. It's more important to have a su that works than to have a su that logs improperly.
We are not google. Our su binary does not ship with the ROM. There are no guarantees that the intent format will be what it is expected to be. (I am speaking in regards to su installed aftermarket onto ROMs, not built into a source built ROM, need to support both)
koush
commented
12 years ago Btw, who are you? I do not recognize the handle.
git-core
commented
12 years ago I am a person who pushes commits in gc and gc-ics branches. But you can easily find my real identity in commit logs in the master branch. Feel free to use the email address from there.
Seems you are not understanding the code.
Perhaps. Let's check. The environment has been already sanitized on su invocation by the dynamic linker, your code tries to sanitize it again. For what reason? Well, in fact, you have to remove some environment variables, e.g. IFS (you're using system(3), aren't you? Or better stop using it at all.)
Furthermore, that security fix is only in ICS...
What security fix? If you're speaking about stripping LD_LIBRARY_PATH, it's from the time the dynamic library concept was invented. Or am I missed the point?
BTW, when I said "superfluous" I did really mean the code in bionic ld, which unsets a lot of garbage, not your code, which sets up LD_LIBRARY_PATH.
Can't rely on the OS sanitizing the LD_LIBRARY_PATH.
Could you explain why?
I'll probably use your code to copy the LD_LIBRARY_PATH environment after the su access has been granted.
Why do you need this ugly stuff? Teaching the dynamic linker to search in /system/lib and so on seems to be better.
Linux su is not copied and pasted between different linux distributions like our su is. Their use case is entirely different.
I don't agree. There are 3 or 4 not-so-different implementations of su for Linux. All of them do the same thing: control the change of the user identity. So does our su.
Sqlite will work perfectly fine regardless of which sqlite version is accessing it.
Think about different sqlite libs, e.g. w/ and w/o WAL. In general, static linking can't be considered a wise choice. The interface between the kernel and the userspace may change occasionally. In fact, this change has been occurred already several times during the Linux lifetime. For suid programs it also means the executable must be updated after every bufgix in every library.
We are not google.
And it's great. So why are you trying to inherit their bugs? Well, I proposed to change the linker already. CM may also change a way the system properties are read (and drop commit a0c1d809893126e3d281e625c8bcd1a3f3134523). BTW, why do you choose to use system properties there? I mean, you may just create a file or something like that as indication of the fact the user allows root access.
koush
commented
12 years ago The environment has been already sanitized on su invocation by the dynamic linker, your code tries to sanitize it again.
No, it hasn't been sanitized, since it is statically linked. The sanitization only happens if the binary is dynamically linked.
Could you explain why?
On Gingerbread, the linker does not sanitize the environment. There is no linker_secure_env call. The patch was only introduced in ICS.
Also, since I am setting euid, egid to match. linker_secure_env is not being called when invoking "am". The env must be secured prior to invocation.
So, to reiterate why you can't rely on the OS sanitizing LD_LIBRARY_PATH: In Gingerbread, there is no sanitization at all of dyamically linked libs. I am matching egid/euid to gid/uid, so the process is not setuid, so it won't invoke the linker_env_secure anyways. From linker.c:
if (program_is_setuid)
linker_env_secure();Well, in fact, you have to remove some environment variables, e.g. IFS
I've actually already switched off system() to fork/exec app_process. But sanitizing IFS may be an easier fix, since not invoking the shell/am requires making more assumptions about paths on the device.
Teaching the dynamic linker to search in /system/lib and so on seems to be better. How do you propose to "teach" the dynamic linker? You certainly can't just patch linker in any ROM you want.
I did not mean different implementations of su. I meant with regards to linking not working between different distributions of Linux. You can not drop a su in from Fedora and expect it to work on Ubuntu. That however must work with Android. Using statically linked binaries will ensure it continues working in the future.
Regarding WAL:
Backwards Compatibility - The database file format is unchanged for WAL mode. However, the WAL file and the wal-index are new concepts and so older versions of SQLite will not know how to recover a crashed SQLite database that was operating in WAL mode when the crash occurred. To prevent older versions of SQLite from trying to recover a WAL-mode database (and making matters worse) the database file format version numbers (bytes 18 and 19 in the database header) are increased from 1 to 2 in WAL mode. Thus, if an older version of SQLite attempts to connect to an SQLite database that is operating in WAL mode, it will report an error along the lines of "file is encrypted or is not a database".
Seems fine
So why are you trying to inherit their bugs?
That doesn't make sense?
That was just the half of it; the original reason I started looking into Superuser was due to a flurry of complaints of ROM Manager not being able to flash recovery. I pinpointed this to Superuser being flaky for a variety of reasons, which I also addressed in my commits. Incidentally, SuperSU, a competing Superuser app, has none of these issues. I had been telling people to just switch to that instead, since it just simply worked reliably. I don't have the source for SuperSU, but a quick strings/readelf just now shows it is doing the same thing I have done in my patch: am invocation and statically linked.
I'll just wait for @ChainsDD to return and chat with him about these changes. In the meantime, I'll prevent CM from autoupdating the su binary off the Superuser repos and use our repo instead and publish separate ones in ROM Manager.
hyperb1iss
commented
12 years ago @git-core I'm using system properties because there are specific behaviors I want to honor based on the build type. I considered using a custom file, but it still leads down the same path as I chose- reading files :)
git-core
commented
12 years ago @cyanogen Strictly speaking you don't have to read (and parse), just check for existence.
I thought CM would use own versions of Superuser/su and would update them via its dedicated channels. If I understood Koushik right, there was a willing to merge CM specific changes into ChainsDD repositories. Regarding commits proposed by Koushik, I'm still strictly against (If anyone interested in my opinion). But I definitely agree this part of su requires redesign. The commit a0c1d809893126e3d281e625c8bcd1a3f3134523 looks a lot more sane, but if you would change it a bit, e.g. just do stat(2) on a file, for example, /dev/CM/root_access_allowed or something like that, the merge would be much easier. (I still think the redesign of property_get() in more secure way is better for CM in general. That way other programs will benefit from it too.)
git-core
commented
12 years ago @koush OK. I realize why you need to sanitize the environment. For me, it looks like another argument that your approach isn't good. First, statically linked suid programs are evil. Second, using am opens a can of security issues. Taking that way we have to
Looks like enormous efforts, doesn't it? Quite frankly, I've deliberated on your code for two days only and have found two security holes. Believe me, I distinguish you as an experienced programmer, but nobody is experienced enough to invent new reliable concept for suid programs from scratch. I mean, it'll take years to find and fix all subtle security holes there.
Could you consider another way? Instead of guessing the proper content for an intent, send a parcel via transact(). This API is stable enough, as far as I can see, and, more importantly, is quite legal. So, my (wild) expectations are Google won't change it frequently. The additional advantage is that we have to pass only an open file descriptor to unnamed socket created by socketpair(2). The rest of communication between su and Superuser can be performed over this stream. The codebase for the latter is almost ready. You may see details of the protocol in the gc branch. ChainsDD has implemented (partially?) the other side of the connection in Superuser. Unfortunately, he is busy and I can't be considered a java programmer, so we're being stuck now. But if you would like to participate, we could implement this approach relatively quickly.
Considering SuperSU, I don't care. I personally can't trust suid programs with closed sources. If somebody is stupid enough to install suid black box on his device, well, the world is not perfect. And you explained enough for me to ensure security robustness of SupserSU is, say, not so good.
Seems fine
Unfortunately, it means the programs requiring root access on boot up will fail. su checks Superuser database for this reason only.
koush
commented
12 years ago Find the absolute path for am. If Google decides to change its location, we're in trouble.
Less likely than Google changing the parcel format. Google will not change this because the Eclipse plugins and adb scripts all look in /system/bin/am for development purposes. This path will not change, since that would mean Google breaking all the development plugins across various versions of Android. The very first version of Supuser (which I wrote 4 years ago, and uses am) still works today. Can't say the same for this one.
Craft parameters for am. We can't just pass the intent content as is, because it's visible widely.
No less visible than the arguments being passed to su itself or what is visible via "ps" after execution beigns. All the paths in the arguments are protected paths anyways. Can pass them in the socket if you want.
Check nobody can fool dalvik/app_process, e.g. by specifying his own debug channel
The process is only available for debugging if passed with "am -D". This is not being done. And that is only for activities. Debugging is not available on broadcasts. I'm not sure why you think sending a broadcast in this way will somehow enable debugging, but that won't happen when sending it via a transact.
Wait a bit after am starts and kill it, if USB debug is turned off. (Nooo, please, I just imagine this ugly code for a moment.)
Why do you need to kill am? It exits itself. A transact() call could just as easily get stuck, if that was your point.
Check changes in dalvik permanently ensuring there are no new covert channels.
What are you talking about?
The current su is exploitable via environment pollution on everything prior to ics. And it is not fixable. The am path security hole you mentioned were fixed, and my method does not suffer from the security hole in the existing.
The current implementation of su hasn't been security tested either. It's no more secure than SuperSu or am-via-system. You and ChainsDD aren't even the original authors of activity.cpp. That was zinx. I'd imagine no one else has even looked at the file. There have only been 2 commits since then on that file in master, none of which have been security related. So at the current time, this su is actually less secure than the changes I am proposing, since it has known exploits.
Unfortunately, it means the programs requiring root access on boot up will fail. su checks Superuser database for this reason only.
No. Reread the documentation. It just means that su without WAL can not recover corrupt databases with WAL (but I'm building with WAL, so I'm not sure what you are even talking about). This isn't even an issue. Let's not pretend it is.
As I said, this is mostly a reliability issue with me. If Superuser doesn't work reliably, I'm going to recommend people stop using it, or use a fork. I don't like SuperSU either, since it is closed source, but it works well. And I can't say the same for this su. But this seems essentially closed source due to the NIH attitude I am experiencing from you (though I'm not sure how much a role you play in this project). You brought up a legitimate security issue, but are basically stonewalling unreasonably otherwise.
The CM team has signed off on the changes, only reason I haven't pushed here, is because it is a non issue, and I'd rather wait for ChainsDD to return. Though I probably will push a change to the CM Superuser to use our own repo for su updates. That makes more sense anyways.
git-core
commented
12 years ago No less visible than the arguments being passed to su itself.
In fact, more. All arguments are exploitable via /proc. I agree that passing only protected socket path is safe.
Can't say the same for this one.
What's wrong with gc/gc-ics branches? Do you know bugs that aren't fixed yet? If you do, report them, please. I'll try to fix them.
The process is only available for debugging if passed with "am -D"
I'm not speaking for am, I'm speaking for dalvik vm that executes am. I suspect, it's exploitable via its debugging mechanisms. Perhaps, other exploitable things exist there or will be added in the future. That's why I mention "covert channels".
Why do you need to kill am? It exits itself.
Only, if USB debugging is turned on. At least, on some ROMs, e.g. on my Gingerbread HTC one.
The current su is exploitable via environment pollution on everything prior to ics
What pollution? LD_LIBRARY_PATH and LD_PRELOAD are ignored for suid binaries on every Android version. Well, they aren't stripped on Gingerbread, but su itself is safe. The rest depends on whether the user authorizes the action.
There have only been 2 commits since then on that file in master, none of which have been security related.
Check the gc branch. You'll find a few.
The current implementation of su hasn't been security tested either.
In fact, it has. I and my colleagues reviewed the code and I fixed some potential vulnerabilities. There was a report via a private email on an security issue. Also, I heard about other similar activities, but didn't read reports. Sure, the master isn't updated starting from some point. The development activity isn't high, it's true. Do you expect a lot of coding for such a tiny stuff like su?
You and ChainsDD aren't even the original authors of activity.cpp. That was zinx.
I can't understand your point at all. Well, zinx is the author, but, then, everybody may fix bugs there. It's open-sourced project.
ChainsDD
commented
12 years ago I've been watching this thread and I believe that using am to send the intent is the better way to do it.
As for the security side of things, I think that using am is no less secure than the current method. If we are worried about the arguments that su is passing to am, then we can cut that down to simply the socket path, then use the socket to pass the rest of the details. The system for passing all the details for the request are already in place in the dev branch of Superuser, as well as the gc branch of su-binary. I don't think that all of the code from the dev branch of Superuser is ready for release, but the socket communications portion seems to work well in my testing. I use the dev branch of Superuser and the gc branch of su-binary on my daily phone.
As for the sqlite problems, I don't think it would be an issue, as we are statically linking su against an sqlite library that understands WAL. The only thing I need to be assured of is that su won't turn WAL on if it's not already enabled, but I don't think that will be an issue, as we are only opening the database as read-only, and not touching the journaling mode.
If I'm misunderstading what the concers are, please let me know.
ChainsDD
commented
12 years ago These issues should be fixed with commit 382d1e37c1fec3d58f49a1ccb9cf63de046965df
ripporippoli
commented
11 years ago Thanks! Thanks! Thanks! Thanks! Great! Great! Great! Great!
Android ships with a number of scripts, like pm or svc, which are implemented in Java, and are run via app_process:
Using su 3.0.3 (Superuser.apk 3.0.7) I cannot run these scripts as root:
Logcat meanwhile says:
Notice the NullPointerException in Hashtable.put during loading of the system class loader; which is rather strange, isn't it.
I'm hoping someone here as an idea.