From 3164a815bc5f344688c6a16cd29beb6d5571dd59 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Thu, 22 Oct 2026 14:10:30 +0200 Subject: [PATCH] fix: confirm the backup holder after writing Between the check and the creation of the run folder, retrobios-backup could be swapped for a link. The holder's device and inode are confirmed unchanged once the run folder exists. Directory file descriptors would close the window entirely but do not exist on Windows; the remaining window is the instant between that confirmation and the move. --- install.ps1 | 2 +- install.py | 25 ++++++++++++++++++++----- install.sh | 2 +- tests/test_install.py | 12 ++++++++++++ 4 files changed, 34 insertions(+), 7 deletions(-) diff --git a/install.ps1 b/install.ps1 index b0c0f1fe..b873d39c 100644 --- a/install.ps1 +++ b/install.ps1 @@ -11,7 +11,7 @@ $ErrorActionPreference = "Stop" $defaultInstallUrl = "https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" - $defaultInstallSha256 = "c595032e68eec7573ad418cac8b906b0fc84a2253367f02e480d21eef94e1980" + $defaultInstallSha256 = "abc5b672e1d4b2f507b6d8c3d5c23a120a7dffea6e583559bdcc92c9cbbc7692" $maximumInstallerBytes = 2MB $installerExit = 0 $installer = if ($PSScriptRoot) { Join-Path $PSScriptRoot "install.py" } else { $null } diff --git a/install.py b/install.py index 44a0a9dd..8ead9404 100644 --- a/install.py +++ b/install.py @@ -3443,19 +3443,29 @@ BACKUP_DIR = "retrobios-backup" BACKUP_MARKER = ".retrobios-backup" -def _check_holder(holder: Path) -> None: +def _check_holder(holder: Path) -> os.stat_result: """Refuse a backup holder the user's files must not go into. A link would send them wherever it points. On a system with user ids, a folder another user owns was not made by this user's runs, and its owner - could read or swap what is moved in. + could read or swap what is moved in. Returns what was checked, so the + caller can confirm the same folder is still there once it has written + into it. """ info = os.lstat(holder) - if stat.S_ISLNK(info.st_mode): - raise OSError(errno.ELOOP, f"{holder} is a link") + if not stat.S_ISDIR(info.st_mode): + raise OSError(errno.ELOOP, f"{holder} is a link or not a folder") getuid = getattr(os, "getuid", None) if getuid is not None and info.st_uid != getuid(): raise OSError(errno.EPERM, f"{holder} belongs to another user") + return info + + +def _same_folder(path: Path, checked: os.stat_result) -> bool: + """Whether path is still the folder checked, not a link swapped in since.""" + now = os.lstat(path) + return (stat.S_ISDIR(now.st_mode) + and (now.st_dev, now.st_ino) == (checked.st_dev, checked.st_ino)) class _NotKept(Exception): @@ -3511,7 +3521,7 @@ class KeptFiles: if run is None: try: holder.mkdir(parents=True, exist_ok=True) - _check_holder(holder) + checked = _check_holder(holder) for n in range(1, 1000): run = holder / (stamp if n == 1 else f"{stamp}-{n}") try: @@ -3523,6 +3533,11 @@ class KeptFiles: # Every name taken: never reuse a folder this run # did not create. raise OSError(errno.EEXIST, f"no free folder name in {holder}") + # The holder checked before is still the one the run + # folder was made in, and the run folder is a folder. + if not (_same_folder(holder, checked) + and stat.S_ISDIR(os.lstat(run).st_mode)): + raise OSError(errno.ELOOP, f"{holder} changed while in use") (run / BACKUP_MARKER).write_text( "Files the RetroBIOS installer replaced.\n", encoding="utf-8") except OSError as exc: diff --git a/install.sh b/install.sh index 269d3f84..e2ff9b09 100755 --- a/install.sh +++ b/install.sh @@ -18,7 +18,7 @@ esac TEMP_INSTALLER="" TEMP_DIRECTORY="" DEFAULT_INSTALL_URL="https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" -DEFAULT_INSTALL_SHA256="c595032e68eec7573ad418cac8b906b0fc84a2253367f02e480d21eef94e1980" +DEFAULT_INSTALL_SHA256="abc5b672e1d4b2f507b6d8c3d5c23a120a7dffea6e583559bdcc92c9cbbc7692" MAX_INSTALLER_BYTES=2097152 cleanup() { diff --git a/tests/test_install.py b/tests/test_install.py index 6dc20bef..4bde3446 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -5233,6 +5233,18 @@ class AFileYouHadIsKept(unittest.TestCase): self.assertEqual(len(inside), 1) self.assertEqual((inside[0] / "x.bin").read_bytes(), self.OLD) + def test_a_holder_swapped_after_the_check_is_noticed(self): + holder = self.home / install.BACKUP_DIR + holder.mkdir() + checked = install._check_holder(holder) + self.assertTrue(install._same_folder(holder, checked)) + holder.rename(self.home / "moved") + (self.home / "elsewhere").mkdir() + holder.symlink_to(self.home / "elsewhere") + self.assertFalse(install._same_folder(holder, checked)) + with self.assertRaises(OSError): + install._check_holder(holder) + @unittest.skipUnless(hasattr(os, "getuid"), "user ids are POSIX") def test_a_holder_another_user_owns_is_refused(self): (self.home / install.BACKUP_DIR).mkdir()