OpenPDroid / OpenPDroidPatches

Framework patches for OpenPDroid permission management
GNU General Public License v3.0
38 stars 24 forks source link

Iptables command permission check #4

Closed dkurochkin closed 11 years ago

dkurochkin commented 11 years ago

Hello.

First of all, I would like to say that I would love to have the ability to control application permissions on my device. IMO such functionality must be included in the AOSP. Unfortunately, it is not. So I am looking for alternative solutions. OpenPDroid looks like a promising project. Thank you for working on it! (BTW I use Nexus 4 with stock image, I hope you would support patching it one day)

I decided to take a closer look at the sources, just to get an idea of how complex and invasive the modifications are. Obviously, such project touches some core parts of the system and I would like to know that the modifications are sound.

I stumbled upon iptables command check implementation in openpdroid_4.2.1_libcore.patch. I think this check is an ugly hack. Moreover it simply does not work: it cannot catch all uses of iptables and it can detect false uses of iptables. I would definitely not want this particular code running on my system. And now I am trying to understand the reason behind these changes. IMO this code should be removed: it is better not to implement a feature at all than implement it in a way that does not work.

I hope this does not offend anyone. I apologize if it does.

Sorry, if this is a wrong place for the discussion.

Regards, Dmitry

Lekensteyn commented 11 years ago

I second this, the Java stuff should not check iptables. Let apps like SuperUser control who have access to iptables. PDroid is definitely not the place to implement it. The current code is fully broken. It breaks legitimate programs and does not prevent malicious programs from executing code.

Think of this, a malicious program will likely use some form of packing. It is not that stupid to use plain "iptables" in their commands or scripts. Example of how to break the current filter:

#!/bin/sh
ip''tables -vnL

Not to mention the possibility to use JNI or apps shipping binaries.

Let's drop iptables checking completely? See also issue #8

mateor commented 11 years ago

I tend to agree on the IPtables, but not so much because I have personally vetted this code as just a philosophical issue. I think that Java should leave it alone as well, and that SU is better suited.

Lekensteyn commented 11 years ago

So, do you agree on dropping iptables from the pdroid patches for now? The only reason to block iptables in Java is because you are root, but when an app has root privileges, you are hosed anyway. You cannot run iptables as non-root because of missing CAP_NET_ADMIN capabilities:

shell@android:/ $ iptables -vnL                                                
iptables v1.4.14: can't initialize iptables table `filter': Permission denied (you must be root)
Perhaps iptables or your kernel needs to be upgraded.

Let me reiterate the boring stuff again: the current form of blocking iptables is useless and gives the user a false sense of security. Oh, and the permission contains a typo (see the WTF line in my patches, it is "iptables", not "IpTable")

mateor commented 11 years ago

Sure. I have always thought it was unnecessary, but haven't had it as a priority to discuss. Nor had I given it the once over that you have. I am only one vote, though and it's not really my area, honestly. The two principle guys working on the core are wsot and CollegeDev. I imagine wsot will chime in here in a day or so.

However, I am unsuprised to find that there are holes or problems in the code. The Pdroid projects were being worked on and maintained by several independent devs, and was really only opened to review for the first time since Gingerbread when we recently opened this project.

Some of the issues you bring up are legacy issues from the very first iteration of PDroid, like the privacy.x509.pem and basically the entirety of the libcore patch, both of which have been around since before Svyat left. There is a lot of reviewing and editing needed, so people like you donating time and energy is crucial.

mateor commented 11 years ago

This is removed in upcoming version. I think those commits may be merged already. But as I move forward, I will root out any further instances. CollegeDev, let me know if you see me miss something.

davidmartinzeus commented 11 years ago

Could you post an URL for the updated patches without the iptables check hack?

mateor commented 11 years ago

That stuff is in the process of being weeded out of several branches, but patches are not ready for those yet. You can remove it yourself if you wish. If successful, send it back to us and I will compare it to the removal in my development branches.