search

TeamWin / Team-Win-Recovery-Project

Core recovery files for the Team Win Recovery Project (T.W.R.P) - this is not up to date, please see https://github.com/TeamWin/android_bootable_recovery/
http://twrp.me
1.95k stars 741 forks source link

Option to disable OpenRecoveryScript or ask for confirmation #1597

Open LoganDark opened 3 years ago

LoganDark commented 3 years ago

Device codename: guacamole TWRP version: 3.4.0-0

WHAT STEPS WILL REPRODUCE THE PROBLEM?

Placing an OpenRecoveryScript in the cache directory that wipes the internal storage

WHAT IS THE EXPECTED RESULT?

TWRP should give you an opportunity to cancel before running an OpenRecoveryScript

WHAT HAPPENS INSTEAD?

For some reason it just goes ahead and wipes the device without asking

ADDITIONAL INFORMATION

I need to know I can trust TWRP to help me, well, recover from boot issues! Having it wipe my phone immediately without any confirmation is an extremely bad user experience. I lost all of my data because restoring the backup caused a bootloop and then booting back into TWRP erased the backup. I thought I had a copy of the backup on one of my computers, but it turns out that was no longer the case.

LoganDark commented 3 years ago

Please fix this issue. I cannot safely use my phone if there is a constant threat of TWRP wiping my data without confirmation. It's already happened once.

LoganDark commented 3 years ago

cc @bigbiff you recently posted some comments here

LoganDark commented 3 years ago

@mauronofrio Are you the maintainer of TWRP for guacamole? https://gerrit.twrp.me/q/project:android_device_oneplus_guacamole

Please, anyone, I need this option implemented. This is not a convenience feature. My phone should not wipe itself if I try to boot into TWRP during a bootloop!

LoganDark commented 3 years ago

I can't believe the lack of attention this issue is getting. It looks like the TWRP team does not care about making this optional. @bigbiff @mauronofrio I'm giving you both another ping to stress the importance of this issue. Even though TWRP makes no guarantees about the integrity of your data, it should not purposefully wipe it without permission.

Since OpenRecoveryScript is most useful for remote deployments, what about a popup dialog that says, "An OpenRecoveryScript will be executed in 15 seconds. Would you like to cancel?", which gives the user an opportunity to cancel a script they didn't authorize, while at the same time allowing unattended control of TWRP just like how it works today.

bigbiff commented 3 years ago

Can you tell me how openrecoveryrescript was added to /cache?

LoganDark commented 3 years ago

It was most likely the installation script of a Magisk module. It's highly likely that this was accidental rather than malicious, as you need root access anyway and if you have that you can wreck a phone in a millisecond. I took a cursory look at a few of the Magisk modules I use but couldn't find anything mentioning cache or formatting data.

Regardless of how the script finds its way there, if my phone is in a bootloop TWRP is my only option for removing it, and TWRP should give me the opportunity to cancel its execution if I don't mean to run that script.

CaptainThrowback commented 3 years ago

But then this raises a security issue in cases where someone wants to remote wipe their device because it's been stolen, but now the thief will have an option to cancel wiping of the data, so they can steal whatever they want?

Just because you have an issue doesn't mean there aren't reasons that this is the default behavior. Putting an ORS in /cache that wipes internal data isn't something I've ever seen before, so I think you should focus on figuring out how it got there rather than blaming TWRP for executing it.

LoganDark commented 3 years ago

But then this raises a security issue in cases where someone wants to remote wipe their device because it's been stolen, but now the thief will have an option to cancel wiping of the data, so they can steal whatever they want?

@CaptainThrowback That's a stretch at best. You're saying that someone obtains privileged remote execution on their stolen phone, but instead of wiping it then and there, they place a script in /cache, and wait for the thief to reboot into TWRP and enter their lockscreen password. And that's why TWRP needs to make it impossible for the user to abort script execution?...

Getting something to run remotely that can place scripts in /cache would allow you to wipe the phone without relying on TWRP, so I'm not sure why TWRP would have to support this weird use case.

TWRP could offer a setting "don't prompt before execution of OpenRecoveryScripts", but it should still be turned off by default. :/

It's far more likely a script ends up in there by accident, the user forgets it's there, reboots into TWRP, and doesn't expect a sudden wipe of all their data.

Putting an ORS in /cache that wipes internal data isn't something I've ever seen before, so I think you should focus on figuring out how it got there rather than blaming TWRP for executing it.

That's what I've been doing. Would you like to help? Solving this in TWRP will prevent things like this from happening in the future, but it's not the only measure that can be taken, sure. I'm doing multiple things and this is one of them.

LoganDark commented 3 years ago

Hey, it's been around a month since the last activity on this issue, and I still think it's quite important to add user choice here. While I understand my specific situation may raise more questions than answers, the specifics aren't really important.

Right now there is no way to stop TWRP from executing an OpenRecoveryScript once it sees one. It really doesn't matter how it got there. The lack of choice that TWRP offers here can and has caused loss of personal data.

Regardless of how I discovered the problem, it should be a strong enough use case on its own (don't execute scripts without permission) to warrant a solution of some kind.

You should just be able to add a dialog that stays on-screen for something like 10 seconds or so, saying "Would you like to execute this script we found in /cache? It will be executed in 10 seconds unless you tap 'No'.". That would preserve the existing use-case of OpenRecoveryScript, automation, while adding user choice.

Can I get some communication here? What are the blockers on this issue? Are there any complications with the implementation I'm proposing?

CaptainThrowback commented 3 years ago

But then this raises a security issue in cases where someone wants to remote wipe their device because it's been stolen, but now the thief will have an option to cancel wiping of the data, so they can steal whatever they want?

@CaptainThrowback That's a stretch at best. You're saying that someone obtains privileged remote execution on their stolen phone, but instead of wiping it then and there, they place a script in /cache, and wait for the thief to reboot into TWRP and enter their lockscreen password. And that's why TWRP needs to make it impossible for the user to abort script execution?...

I don't think you understand how remote wiping works...the device reboots to recovery, where the factory reset is processed. That's how all device wipes work in Android. I'm not saying anything about a thief voluntarily rebooting into TWRP.

With your proposed change, when the remote wipe is triggered, and the device reboots to recovery to process the wipe command, the thief will have the option to cancel the wipe, leaving the user's data exposed.

LoganDark commented 3 years ago

I don't think you understand how remote wiping works...the device reboots to recovery, where the factory reset is processed. That's how all device wipes work in Android. I'm not saying anything about a thief voluntarily rebooting into TWRP.

Why would you rely on OpenRecoveryScript to do the wipe if Android has a built-in feature for it? This wouldn't affect that. TWRP would still wipe your device if Android told it to do so.

But if the user manually reboots into TWRP and there just happens to be a script somewhere, there should be a choice.

CaptainThrowback commented 3 years ago

I don't think you understand how remote wiping works...the device reboots to recovery, where the factory reset is processed. That's how all device wipes work in Android. I'm not saying anything about a thief voluntarily rebooting into TWRP.

Why would you rely on OpenRecoveryScript to do the wipe if Android has a built-in feature for it? This wouldn't affect that. TWRP would still wipe your device if Android told it to do so.

But if the user manually reboots into TWRP and there just happens to be a script somewhere, there should be a choice.

The TWRP ORS code runs both AOSP recovery scripts (like the kind for remote wiping) and manually placed ORS the same way, since in both cases, the commands should be run on recovery boot.

Additionally, how would TWRP know whether it was manually booted or booted into automatically? It has no way of knowing that.

LoganDark commented 3 years ago

I don't think you understand how remote wiping works...the device reboots to recovery, where the factory reset is processed. That's how all device wipes work in Android. I'm not saying anything about a thief voluntarily rebooting into TWRP.

Why would you rely on OpenRecoveryScript to do the wipe if Android has a built-in feature for it? This wouldn't affect that. TWRP would still wipe your device if Android told it to do so. But if the user manually reboots into TWRP and there just happens to be a script somewhere, there should be a choice.

The TWRP ORS code runs both AOSP recovery scripts (like the kind for remote wiping) and manually placed ORS the same way, since in both cases, the commands should be run on recovery boot.

Additionally, how would TWRP know whether it was manually booted or booted into automatically? It has no way of knowing that.

We're just wasting time going back and forth on a trivial topic. TWRP isn't usually resistant to physical attacks, although there are hardened versions that work on locked bootloaders.

If in doubt make it a setting. Probably off by default because TWRP settings can't be loaded until the phone is decrypted. But at least make it a setting that I can turn on to be sure that my phone won't randomly wipe itself if I get stuck in a bootloop and reboot into TWRP. Since ORS isn't executed until after I enter my password and allow the phone to be decrypted anyway.

bigbiff commented 3 years ago

I am leaving this open. Not many people are contributing to TWRP so I don't think this will be implemented any time soon. I am focused on core A11/A12 support. Please feel free to publish a patchset on https://gerrit.twrp.me referencing this issue.

AndroiableDroid commented 3 years ago

@CaptainThrowback @bigbiff I think it will be fixed by adding an additional option in settings to get something like get notified while running ORS, so that there will be no responsibility of TWRP if person enable this and take care of their devices.

LoganDark commented 3 years ago

@CaptainThrowback @bigbiff I think it will be fixed by adding an additional option in settings to get something like get notified while running ORS, so that there will be no responsibility of TWRP if person enable this and take care of their devices.

What do you mean get notified? You're already notified - OpenRecoveryScript shows up in the logs that you can plainly see while it executes.

CaptainThrowback commented 3 years ago

@CaptainThrowback @bigbiff I think it will be fixed by adding an additional option in settings to get something like get notified while running ORS, so that there will be no responsibility of TWRP if person enable this and take care of their devices.

That's fine with me, as long as it's only applied to TWRP ORS and AOSP Recovery commands continue to be processed without said option.

AndroiableDroid commented 3 years ago

@CaptainThrowback @bigbiff I think it will be fixed by adding an additional option in settings to get something like get notified while running ORS, so that there will be no responsibility of TWRP if person enable this and take care of their devices.

What do you mean get notified? You're already notified - OpenRecoveryScript shows up in the logs that you can plainly see while it executes.

Means to cancel if running an ORS Script

AndroiableDroid commented 3 years ago

@CaptainThrowback @bigbiff I think it will be fixed by adding an additional option in settings to get something like get notified while running ORS, so that there will be no responsibility of TWRP if person enable this and take care of their devices.

That's fine with me, as long as it's only applied to TWRP ORS and AOSP Recovery commands continue to be processed without said option.

Ya it will only show cancel button if enabled from TWRP settings

LoganDark commented 2 years ago

@AndroiableDroid What's the status on this?

AndroiableDroid commented 2 years ago

@AndroiableDroid What's the status on this?

I will definitely work on it as soon as I free from my personal stuff

LoganDark commented 2 years ago

Any progress on this?

LoganDark commented 2 years ago

To the best of my knowledge, this is still a major issue affecting the most recent version of TWRP.

LoganDark commented 1 year ago

This is still completely unsolved.

LoganDark commented 1 year ago

There is still no option to disable OpenRecoveryScript for devices that will not be operated autonomously. I, still, do not want TWRP to blindly run any random script it finds without asking for confirmation or prompting in any way.

CaptainThrowback commented 1 year ago

Well, as you might have noticed, we're all very busy. This is why we rely on people to help with the coding. It doesn't appear to be a big issue for most users, so that's probably why it's a low priority. I'm sorry if this negatively impacts your user experience, but it is what it is.

LoganDark commented 1 year ago

It still actively discourages me from ever booting into TWRP out of fear that it could find another script and wipe my phone again, but I guess if you had a nickel for every user that accidentally wiped their phone you'd be rich. Sigh.

(I'm sure there's some argument to be made that TWRP should allow you to turn off pieces of code that proactively check if they should immediately wipe your- oh.)

Anyway, I'm not trying to pester you on a daily basis or anything, I just didn't want this issue to be forgotten.

CaptainThrowback commented 11 months ago

It still actively discourages me from ever booting into TWRP out of fear that it could find another script and wipe my phone again, but I guess if you had a nickel for every user that accidentally wiped their phone you'd be rich. Sigh.

(I'm sure there's some argument to be made that TWRP should allow you to turn off pieces of code that proactively check if they should immediately wipe your- oh.)

Anyway, I'm not trying to pester you on a daily basis or anything, I just didn't want this issue to be forgotten.

It presently has a bug in it, but at least it's in progress now. So there should be no need to post about it again: https://gerrit.twrp.me/c/android_bootable_recovery/+/6726

LoganDark commented 11 months ago

I think there should be a timeout on that dialog (something like 15 seconds) so that automation can still be done, but there's just also an opportunity for a user to override it if they're not expecting, for instance, their entire device to be wiped.

CaptainThrowback commented 11 months ago

I think there should be a timeout on that dialog (something like 15 seconds) so that automation can still be done, but there's just also an opportunity for a user to override it if they're not expecting, for instance, their entire device to be wiped.

Feel free to add the code for what you want to the Gerrit patch. Otherwise, if the option is set, the script will just need to be manually run by swiping.

Or we can just abandon the patch as it sounds like it's not good enough for you...

LoganDark commented 11 months ago

Or we can just abandon the patch as it sounds like it's not good enough for you...

Is this necessary? I'll take anything. I just also like to point out improvement opportunities. Sorry if that came off rudely at all.

CaptainThrowback commented 7 months ago

@LoganDark can you test the patch and see if it works well enough for you?

https://gerrit.twrp.me/#/c/6726

LoganDark commented 7 months ago

@LoganDark can you test the patch and see if it works well enough for you?

https://gerrit.twrp.me/#/c/6726

I can't test it myself because I can't build TWRP from source, but anything that would allow me to cancel ORS before it runs would solve this issue. I just read the diff and it looks good to me.

Thanks for putting something out to fix this, this was the main reason I stopped modding. Wouldn't want anyone else to get burned by it.

CaptainThrowback commented 7 months ago

@LoganDark can you test the patch and see if it works well enough for you?

https://gerrit.twrp.me/#/c/6726

I can't test it myself because I can't build TWRP from source, but anything that would allow me to cancel ORS before it runs would solve this issue. I just read the diff and it looks good to me.

Thanks for putting something out to fix this, this was the main reason I stopped modding. Wouldn't want anyone else to get burned by it.

Unfortunately if you can't test it, it will likely never be merged. Patches don't just get merged without feedback from multiple sources, and since most people aren't interested in this feature, they won't take the time to test it. Why exactly can't you build TWRP from source? There is an official TWRP tree for your device, and you can use an online builder like this one to build and even include the patch from Gerrit that you want to test.

If you can find a way to test it and confirm it's functionity, then there may be a chance that it will be included at some point. Otherwise, you may just have to find a way to live with it as it currently is.