From 19bd88131d749b88544b6227b6bc2aa58c01b2a4 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Tue, 27 Oct 2026 14:46:14 +0100 Subject: [PATCH] fix: show outside text, never obey it Pack and list paths with a control character are refused, the names a list records, card labels, folders and events pass one filter, and a folder a configuration names with one is not taken. --- install.ps1 | 2 +- install.py | 62 ++++++++++++++++--- install.sh | 2 +- install/launchers/index.json | 16 ++--- tests/test_install_tui.py | 114 +++++++++++++++++++++++++++++++++++ wiki/installer.md | 14 ++++- 6 files changed, 188 insertions(+), 22 deletions(-) diff --git a/install.ps1 b/install.ps1 index 49ce42b0c..58abd67e3 100644 --- a/install.ps1 +++ b/install.ps1 @@ -11,7 +11,7 @@ $ErrorActionPreference = "Stop" $defaultInstallUrl = "https://raw.githubusercontent.com/Abdess/retrobios/main/install.py" - $defaultInstallSha256 = "f4f2d041d923e32d46c828222f43f4359c5241fcce7ccc29bfc9b3e8565bba0b" + $defaultInstallSha256 = "455524a94cfddee646331cc5b92358ef52c06f8ed23a0a73be35355efa6528ea" $maximumInstallerBytes = 2MB $installerExit = 0 $installer = if ($PSScriptRoot) { Join-Path $PSScriptRoot "install.py" } else { $null } diff --git a/install.py b/install.py index 6b6bc0fc3..26c7532b4 100644 --- a/install.py +++ b/install.py @@ -632,7 +632,11 @@ class EventStream: keys = EVENT_KEYS[kind] if set(fields) != set(keys): raise ValueError(f"event {kind}: keys {sorted(fields)} != {sorted(keys)}") - line = json.dumps({"event": kind, **{key: fields[key] for key in keys}}) + # Every string a script reads goes through the terminal's filter, + # a new line kept: a consumer printing a folder or a name obeyed + # the control characters a card label or a list held. + line = json.dumps({"event": kind, **{key: _clean_strings(fields[key], "\n") + for key in keys}}) with self._lock: self.stream.write(line + "\n") self.stream.flush() @@ -941,8 +945,11 @@ def shown_path(path: Path | str) -> str: match = (re.match(rf"^{re.escape(root)}([a-z])(?:/(.*))?$", text) if _WSL["active"] else None) if match: - return f"{match.group(1).upper()}:\\" + (match.group(2) or "").replace("/", "\\") - return text + text = f"{match.group(1).upper()}:\\" + (match.group(2) or "").replace("/", "\\") + # A folder's name comes from whoever named it: a card's label, a + # retroarch.cfg, a pack. Printed raw, a control character in it is a + # terminal command (code_audit_18 P2-1). + return screen_text(text) def _wsl_dest(args: argparse.Namespace) -> None: @@ -2009,6 +2016,8 @@ def launchbox_bios_dirs(root: Path) -> dict[str, Path]: # (folder, why in words) for each folder detection could not look in. _DETECTION_REFUSED: list[tuple[Path, str]] = [] +# The platforms whose configuration names a folder with a control character. +_UNNAMEABLE: list[str] = [] def refused_words(folder: Path, why: str, shown: str = "") -> str: @@ -2360,7 +2369,21 @@ def detect_platforms(os_type: str) -> list[tuple[str, Path]]: _detect_by_path(os_type, found) # An embedded system's config read again as the home folder's one (HOME is # /storage on the ELEC family) is the same folder. - return list(dict.fromkeys(found)) + kept = [] + for name, path in dict.fromkeys(found): + if _CONTROLS.search(str(path)): + # A config naming a folder with a control character: printed, + # the name is a terminal command, and the folder one nobody can + # type (code_audit_18 P2-1). Its default is then no answer: the + # setup reads the folder named. + if name not in _UNNAMEABLE: + _UNNAMEABLE.append(name) + print(f" {display_name(name)}: the folder its configuration names holds a " + f"control character ({shown_path(path)}). Name its BIOS folder with " + "--dest.", file=sys.stderr) + continue + kept.append((name, path)) + return kept def _emudeck_windows_bios() -> Path | None: @@ -2728,11 +2751,14 @@ def _safe_relative_path(value: object, field: str) -> PurePosixPath: raise ValueError(f"invalid {field}") if ( "\\" in value - or "\x00" in value + or _CONTROLS.search(value) or "//" in value or value.endswith("/") or re.match(r"^[A-Za-z]:", value) ): + # A control character is refused with NUL: a pack's member named + # x]0;PWN.bin set the window title in the progress line, + # and the file was written under that name (code_audit_18 P2-1). raise ValueError(f"unsafe {field}: {value!r}") path = PurePosixPath(value) if path.is_absolute() or any(part in ("", ".", "..") for part in path.parts): @@ -2759,7 +2785,7 @@ def _safe_target_dir(value: object, field: str) -> str: """ if not isinstance(value, str) or not value or len(value) > 2048: raise ValueError(f"invalid {field}") - if "\x00" in value: + if _CONTROLS.search(value): raise ValueError(f"unsafe {field}: {value!r}") parts = re.split(r"[\\/]", value) if any(part == ".." for part in parts): @@ -3126,6 +3152,21 @@ def _check_data_directory(entry: object, where: str) -> None: raise ValueError(f"{where}.files[{rel!r}] is invalid") +def _clean_strings(value: object, keep: str = "") -> object: + """value with a "?" for each control character of its strings, keys + included, those of keep left: the one filter between a list, a card or + a config and the terminal, its paths refused before (_safe_relative_path).""" + if isinstance(value, str): + return _CONTROLS.sub(lambda found: found.group() if found.group() in keep else "?", + value) + if isinstance(value, dict): + return {_clean_strings(key, keep): _clean_strings(item, keep) + for key, item in value.items()} + if isinstance(value, (list, tuple)): + return [_clean_strings(item, keep) for item in value] + return value + + def _validate_manifest(data: object, plat: str) -> dict: """Validate the untrusted install-manifest boundary using stdlib only.""" if not isinstance(data, dict): @@ -3284,7 +3325,9 @@ def _validate_manifest(data: object, plat: str) -> dict: raise ValueError(f"invalid standalone copy targets at index {index}") for value in values: _safe_target_dir(value, f"standalone_copies[{index}].targets") - return data + # Its paths checked above, every other string (a system's title, a + # core's name) is shown as the terminal shows text. + return _clean_strings(data) def _validate_targets(data: object) -> dict[str, dict]: @@ -6771,7 +6814,7 @@ def _default_dest(os_type: str, platform_name: str) -> Path | None: C:\\userdata\\bios, reported done. """ default = _built_in_dest(os_type, platform_name) - if default is not None and _refused_near(default): + if default is not None and (_refused_near(default) or platform_name in _UNNAMEABLE): # The Flatpak's own folder at 000 hid the retroarch.cfg naming # ~/Games/bios, and 6.6 GB were offered to a folder it does not read. return None @@ -13584,7 +13627,8 @@ def _mount_name(mount: Path) -> str: if (_WSL["active"] and mount.parent == Path(_WSL["root"]) and len(mount.name) == 1 and mount.name.isalpha()): return f"{mount.name.upper()}:" - return mount.name or str(mount) + # The label is whoever formatted the card's: shown, never obeyed. + return screen_text(mount.name or str(mount)) def _mounted_cards() -> list[Path]: diff --git a/install.sh b/install.sh index 1a8428252..5a5e1da33 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="f4f2d041d923e32d46c828222f43f4359c5241fcce7ccc29bfc9b3e8565bba0b" +DEFAULT_INSTALL_SHA256="455524a94cfddee646331cc5b92358ef52c06f8ed23a0a73be35355efa6528ea" MAX_INSTALLER_BYTES=2097152 cleanup() { diff --git a/install/launchers/index.json b/install/launchers/index.json index 7710fa313..d49fbcb0e 100644 --- a/install/launchers/index.json +++ b/install/launchers/index.json @@ -27,8 +27,8 @@ { "dest": "roms/ports/retrobios/install.py", "repo_path": "install.py", - "size": 784255, - "sha256": "f4f2d041d923e32d46c828222f43f4359c5241fcce7ccc29bfc9b3e8565bba0b", + "size": 786759, + "sha256": "455524a94cfddee646331cc5b92358ef52c06f8ed23a0a73be35355efa6528ea", "executable": false }, { @@ -71,8 +71,8 @@ "muos": { "archive": { "dest": "ARCHIVE/RetroBIOS.muxapp", - "size": 836804, - "sha256": "2a46dfb5f18a6535fed35fc828078657fa09440abeafd3219ba099ca3ef4e580" + "size": 839308, + "sha256": "4fef6336d600ff82f1b0d5040d537775df190c23d9ae97e022160e60189e127a" }, "members": [ { @@ -92,8 +92,8 @@ { "name": "RetroBIOS/install.py", "repo_path": "install.py", - "size": 784255, - "sha256": "f4f2d041d923e32d46c828222f43f4359c5241fcce7ccc29bfc9b3e8565bba0b", + "size": 786759, + "sha256": "455524a94cfddee646331cc5b92358ef52c06f8ed23a0a73be35355efa6528ea", "executable": false }, { @@ -166,8 +166,8 @@ { "dest": ".config/modules/retrobios/install.py", "repo_path": "install.py", - "size": 784255, - "sha256": "f4f2d041d923e32d46c828222f43f4359c5241fcce7ccc29bfc9b3e8565bba0b", + "size": 786759, + "sha256": "455524a94cfddee646331cc5b92358ef52c06f8ed23a0a73be35355efa6528ea", "executable": false }, { diff --git a/tests/test_install_tui.py b/tests/test_install_tui.py index f2c8fe72d..113ff3601 100644 --- a/tests/test_install_tui.py +++ b/tests/test_install_tui.py @@ -8595,5 +8595,119 @@ class AuditEighteenAndEleven(unittest.TestCase): self.assertIn("Recalbox SHARE drive", block) self.assertEqual(generate_installer_doc.render(page), page) +class NothingFromOutsideReachesTheTerminalRaw(unittest.TestCase): + """code_audit_18 P2-1: a pack's member path, a --manifest dest, the + names a list records, a card's label and a retroarch.cfg folder reached + line mode, the events and the disk with their control characters. Each + source is fed escape sequences, and none comes out raw.""" + + OSC = "\x1b]0;PWNED\x07" + CSI = "\x1b[41m" + + def setUp(self): + (REPO_ROOT / "tmp").mkdir(exist_ok=True) + self.root = Path(tempfile.mkdtemp(dir=REPO_ROOT / "tmp")) + self.addCleanup(shutil.rmtree, self.root, True) + install.LOCAL.reset() + self.addCleanup(install.LOCAL.reset) + install._UNNAMEABLE.clear() + self.addCleanup(install._UNNAMEABLE.clear) + self.dest = self.root / "dest" + self.dest.mkdir() + + def assertClean(self, text: str, where: str) -> None: + self.assertNotIn("\x1b", text, where) + self.assertNotIn("\x07", text, where) + + def assertNothingRawOnDisk(self) -> None: + for path in self.root.rglob("*"): + self.assertIsNone(install._CONTROLS.search(path.name), path) + + def run_install(self, args: argparse.Namespace, mounts: list = (), + env: dict | None = None) -> str: + events = io.StringIO() + with mock.patch.object(install, "_mounted_cards", return_value=list(mounts)), \ + mock.patch.object(install, "_mount_points", return_value=frozenset()), \ + mock.patch.object(install, "detect_frontends", return_value=[]), \ + mock.patch.object(install, "detect_os", return_value="linux"), \ + mock.patch.object(install.EVENTS, "stream", events), \ + mock.patch.dict(os.environ, {install.OFFLINE_VARIABLE: "1", **(env or {})}), \ + contextlib.redirect_stdout(io.StringIO()) as out, \ + contextlib.redirect_stderr(io.StringIO()) as err, \ + contextlib.suppress(SystemExit): + install._install(args, io.StringIO()) + return out.getvalue() + err.getvalue() + events.getvalue() + + def test_a_pack_member_path_is_refused(self): + from test_install_offline import _pack, GBA # noqa: PLC0415 + + pack = _pack(self.root, "RetroArch_v1_BIOS_Pack.zip", {f"x{self.OSC}.bin": GBA}) + text = self.run_install(_args(platform="retroarch", dest=self.dest, + from_pack=[pack], no_input=True, declared_only=False)) + self.assertClean(text, "pack") + self.assertIn("is not valid: unsafe", text) + self.assertEqual(list(self.dest.iterdir()), []) + self.assertNothingRawOnDisk() + + def test_a_list_dest_and_its_names_are_refused_or_shown(self): + files = [{"dest": f"x{self.CSI}BAD.bin", "sha1": "0" * 40, "size": 1, + "repo_path": "bios/x", "cores": None, "systems": ["a"]}] + listing = self.root / "list.json" + listing.write_text(json.dumps({"manifest_version": 2, "platform": "retroarch", + "files": files})) + text = self.run_install(_args(platform="retroarch", dest=self.dest, manifest=listing, + no_input=True, dry_run=True, verbose=True, + declared_only=False)) + self.assertClean(text, "--manifest dest") + self.assertIn("unsafe", text) + named = {"manifest_version": 2, "platform": "retroarch", "files": [ + {**files[0], "dest": "a.bin", "systems": [f"a{self.CSI}"], + "cores": [f"core{self.OSC}"]}], + "system_titles": {f"a{self.CSI}": f"Title{self.OSC}"}} + cleaned = install._validate_manifest(named, "retroarch") + self.assertClean(json.dumps(cleaned, ensure_ascii=False), "names") + self.assertNothingRawOnDisk() + + def test_a_card_label_is_shown_and_never_obeyed(self): + card = self.root / "media" / f"S{self.CSI}RED" + (card / "system").mkdir(parents=True) + (card / "system" / "batocera.conf").write_text("system.hostname=BATOCERA\n") + listing = self.root / "batocera.json" + listing.write_text(json.dumps({"manifest_version": 2, "platform": "batocera", + "files": []})) + with mock.patch.object(install, "detect_platforms", return_value=[]): + text = self.run_install(_args(platform="batocera", manifest=listing, + no_input=True, dry_run=True, events=True, + declared_only=False), [card]) + self.assertIn("S?[41mRED", text) + self.assertClean(text, "card label") + self.assertClean(install.card_line(card), "card row") + + def test_a_folder_a_config_names_is_refused(self): + home = self.root / "home" + weird = home / f"sys{self.OSC}x" + weird.mkdir(parents=True) + cfg = home / ".config" / "retroarch" / "retroarch.cfg" + cfg.parent.mkdir(parents=True) + cfg.write_text(f'system_directory = "{weird}"\n') + listing = self.root / "retroarch.json" + listing.write_text(json.dumps({"manifest_version": 2, "platform": "retroarch", + "files": []})) + text = self.run_install( + _args(manifest=listing, no_input=True, dry_run=True, events=True, + declared_only=False), + env={"HOME": str(home), "XDG_CONFIG_HOME": str(home / ".config"), + "RETROBIOS_OS": "linux"}) + self.assertClean(text, "retroarch.cfg") + self.assertIn("holds a control character", text) + self.assertNotIn("using default path", text) + self.assertNotIn(str(home / ".config" / "retroarch" / "system"), text) + + def test_a_path_with_a_control_character_is_no_relative_path(self): + for value in ("a\x1bb.bin", "a\x07.bin", "a\x9bb.bin", "a\nb.bin"): + with self.subTest(value=value), self.assertRaises(ValueError): + install._safe_relative_path(value, "dest") + self.assertEqual(install.shown_path(Path("/a/b\x1b[2Jc")), "/a/b?[2Jc") + if __name__ == "__main__": unittest.main() diff --git a/wiki/installer.md b/wiki/installer.md index 1ded72ce0..842a3a52e 100644 --- a/wiki/installer.md +++ b/wiki/installer.md @@ -1382,12 +1382,20 @@ That keeps the installer's questions working when the script is piped from The manifest is fetched over the network and treated as untrusted input. Every path it carries goes through one validator that refuses an absolute path, a -drive letter, a backslash, a null byte, a doubled separator, a trailing slash -and any `..` component, so a destination cannot climb out of the BIOS -directory. Manifest, target list and payloads each have a size cap, the file +drive letter, a backslash, a null byte or any other control character, a +doubled separator, a trailing slash and any `..` component, so a destination +cannot climb out of the BIOS directory and no file is named with a terminal +command. Manifest, target list and payloads each have a size cap, the file list a count cap, and the sum of the declared sizes is checked against the total the manifest claims. +Every other string from outside is shown, never obeyed: the names a list or +a pack records, a card's label and a folder a configuration names reach the +terminal, line by line or on the screen, and the `--events` stream with a `?` +for each control character. A folder a configuration names with one is not +taken: the run says so and asks for `--dest`, its default being no answer +for a setup that reads another folder. + Symbolic links met under the BIOS root are followed on purpose: they are the user's own layout, and EmuDeck links `bios/shadps4/sys_modules` into shadPS4's data directory where the emulator actually reads it. The boundary is lexical,