Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
d1b opened this issue Jul 14, 2012 · 11 comments
Open

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

d1b opened this issue Jul 14, 2012 · 11 comments

Comments

@d1b
Copy link

d1b commented Jul 14, 2012

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
Copy link
Contributor

koush commented Jul 14, 2012

Using https instead of http would also address this issue.

@thejh
Copy link

thejh commented Aug 12, 2012

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

@koush
Copy link
Contributor

koush commented Aug 12, 2012

@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
Copy link

kaniini commented Aug 13, 2012

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
Copy link
Contributor

koush commented Aug 13, 2012

@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
Copy link

kaniini commented Aug 13, 2012

@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
Copy link

thejh commented Aug 13, 2012

@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
Copy link
Author

d1b commented Aug 16, 2012

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

@thejh
Copy link

thejh commented Aug 16, 2012

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
Copy link
Owner

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

@d1b
Copy link
Author

d1b commented Aug 21, 2012

@ChainsDD awesome!

koush added a commit to CyanogenMod/android_packages_apps_Superuser that referenced this issue Mar 9, 2013
Add Traditional Chinese translation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants