Magisk: Security Concern

[ShellCode33]

Hi, recently a new type of vulnerability has been disclosed (it's called Man In The Disk) and I think Magisk suffers from that too.
I'm not really familiar with your code so may have misunderstood a few things, but I've found here that during the installation process, you are writing the new image to Download.EXTERNAL_PATH which is the user's Downloads folder.
A malicious app could take leverage of that by replacing the file while Magisk is updating, leading to the installation of a corrupted boot image. Basically, an unprivileged app can become root without even asking the permission to the user.
I also noticed that a checksum was involved, maybe I've missed something here, but I thing it's a typical case of race condition, the file just has to be replaced after the MD5 checksum (which by the way should be changed, it's quite easy to create MD5 collisions).
So let me know what you think ! Am I right to think our security is at risk ?

[topjohnwu]

I'm aware of this issue but it requires me to write new UI, so it isn't included in the current release

[ShellCode33]

Why is it needed to change the UI ? Can't you just use the app's internal storage instead of the external one ?

[shoeper]

Agree with @ShellCode33

Also think it would be good to get rid of the Magisk folder on the sdcard. It is very easy for an app to check against it an no "normal" user ever has a Magisk folder on its sdcard.

[SuperSandro2000]

Also think it would be good to get rid of the Magisk folder on the sdcard

would solve one problem with Fortnite and I need to agree with @ShellCode33

[topjohnwu]

@ShellCode33 you know why? Because Android's platform DownloadManager cannot download to an app's internal data. I'll have to manually write a notification for download progress. It is totally acceptable for me to just download silently in the background and suddenly pop up APK install request out of nowhere right (/sarcasm)?

@shoeper @SuperSandro2000 you might want to check the latest Magisk Manager...... /sdcard/MagiskManager is no longer used anywhere. You will have to manually delete the folder previously created if you want to prevent detection, I don't delete files on user's internal storage because it is not responsible for a developer to delete data from a user.

[mentalisttraceur]

@topjohnwu Can't you just copy the downloaded file to a privileged location before the checksum comparison?

Wouldn't that let you still use the normal download to the normal location and get the normal dialog while still safely aborting if anything modified or overwrote the updated file before actually using it?

[leosemilie]

como building native on zte blade vantage?

On Wed, Oct 24, 2018, 00:12 mentalisttraceur [e-mail]
wrote:

@topjohnwu https://github.com/topjohnwu Can't you just copy/move
the downloaded file to a privileged location before the checksum
comparison?

Wouldn't that let you still use the normal download to the normal location
using the normal download dialog while still safely aborting if anything
modified or overwrote the updated file before actually using it?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/topjohnwu/Magisk/issues/527#issuecomment-432466088,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ApFwDsLmXBMApgvDLo6rFqazwOrYwe94ks5un7B5gaJpZM4WWKNo
.

[topjohnwu]

Fixed in latest changes