ChainsDD / Superuser

Android superuser permissions app (from Zinx)
http://cyanogenmod.com
567 stars 362 forks source link

Please sign, fetch or otherwise securely transfer update su binaries #52

Open d1b opened 11 years ago

d1b commented 11 years ago

It seems that at the present time the updater service found in su.apk fetches new su binaries over http and checks that the md5sum of the downloaded binary matches that defined in the json descriptor. [0] I would like to suggest that future upgrades hashsums are distributed through su.apk via updating su.apk in the android market (and or the binary as well) which can be used to verify a su binary is legit.

[0] https://github.com/ChainsDD/Superuser/blob/master/src/com/noshufou/android/su/service/UpdaterService.java

koush commented 11 years ago

Using https instead of http would also address this issue.

thejh commented 11 years ago

Oh, and by the way, out-of-the-box HTTPS won't fix this either. You don't want to trust CAs.

koush commented 11 years ago

@thejh Why is that? The CA store lives in /system, which is read only. You would need root to change the CA store. So, the system would already need to be compromised for su to be replaced.

However, if this is a philosophical argument about how CAs are inherently scummy organizations, that's another story :)

kaniini commented 11 years ago

I think the larger issue here is that md5sums are used instead of something more collision-resistant such as whirlpool or sha512, not whether or not HTTPS is used, as HTTPS won't save you from the machine being compromised on the other end, while using a more resilient hash function would at least give you a fighting chance.

koush commented 11 years ago

@nenolod The hash only guarantees file integrity, not security. This is because the binaries and the hashes for the binaries are stored on the server. Furthermore, the su manifest json format allows you to specify the full url to the su binary. Currently, one only needs to compromise the manifest server to completely compromise everything. A case could be made for security, if the binary and manifest were on different servers, and the manifest did not specify a su binary server. That should be hardcoded into the Superuser.apk. Doing what @d1b suggested may work as well, though that may have get messy. Frankly, I do not like the idea of su binary being updatable at all, since it seems to open a gaping security hole. It should just be packaged with Superuser recovery zip or the ROM.

Signing the binary would also guarantee security.

http://downloads.androidsu.com/superuser/su/manifestv2.json:


[
    {
        "min-apk-version": 47,
        "version-code": 18,

        "version": "3.2",
        "armeabi": {
            "binary": "http://downloads.androidsu.com/superuser/su/su-3.2-armv5",
            "binary-md5sum": "3f4fb4ecc5ff247d805f67715952e5de"
            },
        "x86" : {
            "binary": "http://downloads.androidsu.com/superuser/su/su-3.2-x86",
            "binary-md5sum": "122068847ad1dcb2a8f072776e3da20a"
            }
    },
    {
        "min-apk-version": 42,
        "version-code": 17,

        "version": "3.1.1",
        "armeabi": {
            "binary": "http://downloads.androidsu.com/superuser/su/su-3.1.1-armv5",
            "binary-md5sum": "054c9a22d8900d50ce6172fd56bbf414"
            },
        "x86" : {
            "binary": "http://downloads.androidsu.com/superuser/su/su-3.1.1-x86",
            "binary-md5sum": "33ea8cd6e7c7a23eaa9ad97bb115ea55"
            }
    }
]
kaniini commented 11 years ago

@koush File integrity is a component of ensuring the entire system is secure. While I agree that su shouldn't be upgradable by the app (and it certainly was not in the good old days), if the manifest server is secured, and strong hashing is used, then this shouldn't be a huge problem. HTTPS won't guarantee that the manifest server is any more or any less secure. Further, signing in and of itself only guarantees file integrity as well, even if additional semantic meaning is implied by the presence of a signature... the signature is only meaningful if you trust the person who signed it's key has not been compromised.

I guess what I am trying to say is, we probably shouldn't allow su to be upgraded in the first place. Signing and stronger hash functions aside, this is kind of a stupid feature. And really, what is the difference in all of these versions? The whole point is that it asks if you want to give it root or not, so this is an app that should by this point be not necessary to upgrade... less than 100 lines of code really, and that's including IPC back to the Java-side app...

thejh commented 11 years ago

@koush Well, my point is that just too many people can act as a CA. For example, every german university has a CA certificate that all major browsers will accept. It'd be a lot more secure to use a specific signing key by the program author.

@nenolod IMO, it might still be necessary to update su - there's a lot you can do wrong in 100 lines of code.

d1b commented 11 years ago

IMHO the update code should be disabled until someone has written a 'secure' version.

thejh commented 11 years ago

On Thu, Aug 16, 2012 at 02:56:48AM -0700, David wrote:

IMHO the update code should be disabled until someone has written a 'secure' version.

+1

ChainsDD commented 11 years ago

I temporarily disabled the binary updater until I can find a better way to do things.

d1b commented 11 years ago

@ChainsDD awesome!