topjohnwu / Magisk

The Magic Mask for Android
GNU General Public License v3.0
48.05k stars 12.22k forks source link

Clarify that the Superuser Log is not a security mechanism #2974

Closed strugee closed 4 years ago

strugee commented 4 years ago

The log view which shows a record of which applications have been granted or denied root access looks like a useful security mechanism, because it seems like if you suspect a malicious application has been granted root access (perhaps by an attacker with physical access to the device) you can go to this screen to confirm whether or not that's true.

However, this doesn't work because a malicious application can tamper with these logs (at least AFAIK). This screen should show a warning at the top that says something like:

Malicious applications with root can tamper with these logs.

I may try to prepare a patch for this if I have some time.

topjohnwu commented 4 years ago

Any process with root can literally do ANYTHING. It can very trivially read/write any file on your device. It can directly modify Magisk's database and GRANT root for any process.

And with more advanced: It can read arbitrary userspace memory. It can inject arbitrary code into any process.

This should be the first thing one should know before granting root access to anything. There is nothing special about superuser logs, heck even the root access management is just data somewhere on the device.

Thanks for the suggestion, but regarding root there is no such thing as "security measure", so I don't think there is need to warn user about superuser logs, it is just something "helpful". We do have a warning in the su request dialog, and IMO only that is necessary (and can actually prevent users from granting access to anything they ran into).

topjohnwu commented 4 years ago

My point here is that adding a clarification to the superuser log itself is not enough if user does not understand the implications of rooting itself. If the same logic applies, then we shall warn the user that ANYTHING they see on their screen can be manipulated once they allowed some process root access.

And I do mean it. There are root apps that inject code into the image processing pipeline to alter display output (check CF.Lumen), so you literally cannot trust anything you see anymore.

strugee commented 4 years ago

Makes sense. I figured it would just be a nice reminder for users but I understand if you don't want to go down that rabbit hole 👍