ProcessManager.exec validation is possibly insecure

[Lekensteyn]

ProcessManager.exec is modified at https://github.com/OpenPDroid/OpenPDroidPatches/blob/4.2.1/openpdroid_4.2.1_libcore.patch#L311

The current modifications are possibly insecure. If taintedCommand[0] == null, a crash occurs. The checks are performed on an array that can still be modified by the caller (timing attacks?)

What is the rationale of changing the command from bash to su if the command is denied? Wouldn't it be better to throw an exception, return null or set the command to false if you want to have a no-op command?

Anyway, the taintedCommand != null lines can be dropped and to increase security, it should be put after the sanity checks and clone (just before setting up file descriptors).

(general question, would you like patches against the source tree or patches for the patches? Incremental or full?)

[Lekensteyn]

With disregard to the broken iptables handling (issue #4), I have added some tools and made some changes in https://github.com/Lekensteyn/OpenPDroidPatches/tree/4.2.1-cleanup.

Use the following command to check only for line changes:

diff -I '^[^+-]' -Nur orig/ cleanup-WIP-procman/

[mateor]

FFU5y and CollegeDev both work against the tree, so that would be best. Incremental patches would probably be best, although that can be incremental by feature, and full pulls won't be penalized for being so. We are working on getting together a gerrit instance, make this sort of thing a little more transparent.

As an aside, I use the AOSP tree to make the patches, but we will reformat any problem spots so that the patches can apply as widely as possible.

[wsot]

Thanks Lekensteyn: I currently only have quite intermittent access to a net connection in a personal context, so I have not had a chance to examine those changes yet. In terms of the Iptables permission check: I agree it needs either reworking or complete removal. It was inherited from PDroid 2.0 (no criticism to CollegeDev, mind you - it was one of many changes so it isn't surprising these sorts of things crept it).

It is really good to have someone else doing code review and providing feedback.

[Lekensteyn]

I have created a very simple app that uses JNI to bypass restrictions imposed by exec: http://lekensteyn.nl/files/android/TestCmd.tar.gz

On startup, it executes the id command. It could be changed to run any command, e.g. logcat -t 1 (providing that you have READ_LOGS permissions, but pdroid is "supposed" to block that). I guess that only kernel modifications (instead of Java ones) can provide full security.

Dropping groups like log (not possible when non-root atm) possibly cause programs to crash again when the permission is requested. I will try to build a list of ways to circumvent the pdroid limitations.

[nwf]

Relatedly, the logic for detecting "logcat" seems unnecessary; IIRC there's an Android permission that is reified in a UNIX group that logcat respects, yes?

[davidmartinzeus]

@Lekensteyn, did you build a list of ways to circumvent the PDroid limitations you said?

[Lekensteyn]

@davidmartinzeus Those can be found in #9