possible `rm -rf /` during screen installation

Matszwe02 commented 3 months ago

Linux Distribution

rpiOS legacy 32bit on rpi3b

What happened

rm -rf /

What did you expect to happen

idk what should happen, raised an exception, install successful, either way, not rm -rf /

How to reproduce

connect to freshly installed rpios
git clone https://github.com/goodtft/LCD-show.git
chmod -R 755 LCD-show
cd LCD-show/
sudo ./MPI3508-show
[now reboot]
sudo ./LCD35-show
[now reboot]
git clone https://github.com/dw-0/kiauh
cd kiauh
./kiauh.sh
1 [install] 5 [klipperscreen] [as service] [X server]

Additional information

I don't have any logs, because, yeah...

dw-0 commented 3 months ago

When choosing to install KlipperScreen with KIAUH, it's actually KlipperScreens own install script that gets executed by KIAUH. I had a look at the script and the only line containing a rm -rf was this: https://github.com/KlipperScreen/KlipperScreen/blob/master/scripts/KlipperScreen-install.sh#L120 The referenced variable {KSENV} is defined here: https://github.com/KlipperScreen/KlipperScreen/blob/master/scripts/KlipperScreen-install.sh#L5

I am not sure if there is any constellation where the value of KSENV may be / at line 120 in the script. @alfrix Any idea if and how this could have happened?

alfrix commented 3 months ago

When choosing to install KlipperScreen with KIAUH, it's actually KlipperScreens own install script that gets executed by KIAUH.

Before KIAUH runs KlipperScreen's installation script, KIAUH removes any existing KlipperScreen directory [[ -d ${KLIPPERSCREEN_DIR} ]] && rm -rf "${KLIPPERSCREEN_DIR}" https://github.com/dw-0/kiauh/blob/a929c6983dd9e911d029270c63db0ac301a2fd59/scripts/klipperscreen.sh#L50

leading into a first possible case of this unintended behavior if KLIPPERSCREEN_DIR is evaluated to "/" for some reason

then KS install script is executed the install script will set:

KSENV="${KLIPPERSCREEN_VENV:-${HOME}/.KlipperScreen-env}"
...
    if [ -d $KSENV ]; then
        echo_text "Removing old virtual environment"
        rm -rf ${KSENV}
    fi

which means if KLIPPERSCREEN_VENV is set use that, and if that is evaluated into "/" you would have the same possible issue again

the else uses /home/username/.KlipperScreen-env, even if HOME is evaluated into nothing the tail /.KlipperScreen-env should ensure that nothing bad will happen with the default

It's worth noting that nor KLIPPERSCREEN_DIR or KLIPPERSCREEN_VENV are set by KlipperScreen or it's standalone installer, those are external variables for other installers

This leads me to the conclusion that every rm -rf should be double checked against "/" just in case, even if i find this scenario very unlikely

dw-0 commented 3 months ago

Before KIAUH runs KlipperScreen's installation script, KIAUH removes any existing KlipperScreen directory [[ -d ${KLIPPERSCREEN_DIR} ]] && rm -rf "${KLIPPERSCREEN_DIR}"

https://github.com/dw-0/kiauh/blob/a929c6983dd9e911d029270c63db0ac301a2fd59/scripts/klipperscreen.sh#L50

leading into a first possible case of this unintended behavior if KLIPPERSCREEN_DIR is evaluated to "/" for some reason

Ah yes you are correct, there is a rm -rf before executing the KlipperScreen script. KLIPPERSCREEN_DIR is defined here: https://github.com/dw-0/kiauh/blob/master/scripts/globals.sh#L51

From my perspective that cannot be "/" in any case, nor can the KSENV from your script be "/".

No idea what happened there...

Matszwe02 commented 3 months ago

I suspect that something might be off because I didn't have klipper installed, as the second time, that I installed ks after klipper, it worked.

I saw that it broke after or during installation of requirements for venv

dw-0 / kiauh