From 3a0a7f630205082c9610bb0cb8e5e5c139070b0a Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 5 Oct 2026 02:44:26 +0100 Subject: [PATCH] Fail closed on oversized or corrupt passive VM reads Bound the existing passive preflight before Bash can discard NULs or accept unlimited configuration output. Preserve producer errors independently, isolate the byte reader from Python startup hooks, and keep the copied helper and mirrored help private and off the guest-agent channel. Contract-Neutral: Operator diagnostics and existing help only; Proxmox polling, guest-agent coordination, APIs, storage and service behaviour are unchanged. Change-source: pulse-maintainer --- docs/VM_DISK_MONITORING.md | 9 +- .../public/docs/VM_DISK_MONITORING.md | 9 +- scripts/test-vm-disk.sh | 78 +++++++-- scripts/tests/test_vm_disk_diagnostics.py | 165 ++++++++++++++++-- 4 files changed, 230 insertions(+), 31 deletions(-) diff --git a/docs/VM_DISK_MONITORING.md b/docs/VM_DISK_MONITORING.md index 5b3272aba2..0ab7bbd28d 100644 --- a/docs/VM_DISK_MONITORING.md +++ b/docs/VM_DISK_MONITORING.md @@ -151,8 +151,9 @@ custom monitoring role if the required read privileges are absent. ### Passive host preflight The current `scripts/test-vm-disk.sh` helper reads only local VM status and -current configuration, with bounded timeouts. It sends **no guest-agent -commands**, reads no guest files, and changes no service, lock or ACL. Use it +current configuration, with bounded timeouts and byte limits. It sends **no guest-agent +commands**, reads no guest files, and changes no service, lock or ACL. It requires +local `qm`, `timeout` and Python 3; it downloads nothing. Use it on the VM's owning Proxmox host with an account permitted to read that local configuration (normally through `sudo`). @@ -170,6 +171,10 @@ a reported backup/other lock without dumping the full VM configuration. A missing or unreadable lock is not permission to probe the guest: a backup can start immediately after the read. A non-zero exit means a required read failed or the response could not be interpreted; it is not an empty or healthy result. +Status replies are limited to 256 bytes and configuration to 64 KiB. Oversized +or corrupt replies fail without interpreting a truncated prefix, printing raw +configuration or saving it to disk. Inspect the configuration privately rather +than retrying with guest-agent probes. **A successful preflight does not verify disk freshness, guest-agent responsiveness or thaw.** diff --git a/frontend-modern/public/docs/VM_DISK_MONITORING.md b/frontend-modern/public/docs/VM_DISK_MONITORING.md index 5b3272aba2..0ab7bbd28d 100644 --- a/frontend-modern/public/docs/VM_DISK_MONITORING.md +++ b/frontend-modern/public/docs/VM_DISK_MONITORING.md @@ -151,8 +151,9 @@ custom monitoring role if the required read privileges are absent. ### Passive host preflight The current `scripts/test-vm-disk.sh` helper reads only local VM status and -current configuration, with bounded timeouts. It sends **no guest-agent -commands**, reads no guest files, and changes no service, lock or ACL. Use it +current configuration, with bounded timeouts and byte limits. It sends **no guest-agent +commands**, reads no guest files, and changes no service, lock or ACL. It requires +local `qm`, `timeout` and Python 3; it downloads nothing. Use it on the VM's owning Proxmox host with an account permitted to read that local configuration (normally through `sudo`). @@ -170,6 +171,10 @@ a reported backup/other lock without dumping the full VM configuration. A missing or unreadable lock is not permission to probe the guest: a backup can start immediately after the read. A non-zero exit means a required read failed or the response could not be interpreted; it is not an empty or healthy result. +Status replies are limited to 256 bytes and configuration to 64 KiB. Oversized +or corrupt replies fail without interpreting a truncated prefix, printing raw +configuration or saving it to disk. Inspect the configuration privately rather +than retrying with guest-agent probes. **A successful preflight does not verify disk freshness, guest-agent responsiveness or thaw.** diff --git a/scripts/test-vm-disk.sh b/scripts/test-vm-disk.sh index b386aa8c4c..af753de681 100755 --- a/scripts/test-vm-disk.sh +++ b/scripts/test-vm-disk.sh @@ -20,7 +20,7 @@ if [[ $# -ne 1 || ! $1 =~ ^[1-9][0-9]{0,8}$ ]]; then fi vmid=$1 -for command in qm timeout; do +for command in qm timeout python3; do if ! command -v "$command" >/dev/null 2>&1; then printf 'Required command unavailable: %s. Run on the owning Proxmox host.\n' "$command" >&2 exit 1 @@ -38,27 +38,71 @@ read_failure() { exit 1 } +read_bounded() { + local operation=$1 destination=$2 limit=$3 response reader_code producer_code + shift 3 + # Validate bytes before Bash command substitution can discard NULs or + # trailing newlines. Close oversized replies after limit + 1 bytes; never + # parse a prefix as a complete configuration or retain raw config on disk. + # The final private status suffix preserves producer and reader failures + # independently, even when they happen to use the same exit code. + response=$( + set +e + timeout --signal=TERM --kill-after=2s 10s "$@" 2>/dev/null | + python3 -I -c ' +import sys +limit = int(sys.argv[1]) +data = sys.stdin.buffer.read(limit + 1) +if len(data) > limit: + sys.exit(80) +try: + data.decode("utf-8", errors="strict") +except UnicodeDecodeError: + sys.exit(81) +if b"\0" in data: + sys.exit(81) +sys.stdout.buffer.write(data) +' "$limit" 2>/dev/null + codes=("${PIPESTATUS[@]}") + printf '\nVM_READ_STATUS %s %s' "${codes[0]}" "${codes[1]}" + ) + read -r producer_code reader_code <<< "${response##*$'\nVM_READ_STATUS '}" + case "$reader_code" in + 80) + printf 'VM %s response exceeded the %s-byte limit; preflight incomplete.\n' "$operation" "$limit" >&2 + exit 1 + ;; + 81) + printf 'VM %s read contained non-text bytes; preflight incomplete.\n' "$operation" >&2 + exit 1 + ;; + 0) ;; + *) read_failure "$operation" "$reader_code" ;; + esac + if [[ $producer_code -ne 0 ]]; then + read_failure "$operation" "$producer_code" + fi + printf -v "$destination" '%s' "${response%$'\nVM_READ_STATUS '*}" +} + printf '%s\n' 'Passive VM preflight: no guest-agent commands will be sent.' # Bound even host-local reads. Do not print raw config or command errors: # those can contain private guest names, paths and other infrastructure data. -if status_output=$(timeout --signal=TERM --kill-after=2s 10s qm status "$vmid" 2>/dev/null); then - case "$status_output" in - 'status: running') status=running ;; - 'status: stopped') status=stopped ;; - *) printf '%s\n' 'VM status response was not understood; preflight incomplete.' >&2; exit 1 ;; - esac -else - read_failure status "$?" -fi +read_bounded status status_output 256 qm status "$vmid" +# qm emits a newline-terminated status. Preserve the existing acceptance of +# trailing newlines, but only after the complete bounded byte read succeeds. +while [[ $status_output == *$'\n' ]]; do status_output=${status_output%$'\n'}; done +case "$status_output" in + 'status: running') status=running ;; + 'status: stopped') status=stopped ;; + *) printf '%s\n' 'VM status response was not understood; preflight incomplete.' >&2; exit 1 ;; +esac printf 'VM status: %s\n' "$status" -if config=$(timeout --signal=TERM --kill-after=2s 10s qm config "$vmid" --current 2>/dev/null); then - if [[ -z $config ]]; then - printf '%s\n' 'VM current configuration was empty; preflight incomplete.' >&2 - exit 1 - fi -else - read_failure configuration "$?" +read_bounded configuration config 65536 qm config "$vmid" --current +if [[ -z $config ]]; then + printf '%s\n' 'VM current configuration was empty; preflight incomplete.' >&2 + exit 1 fi agent_seen=false diff --git a/scripts/tests/test_vm_disk_diagnostics.py b/scripts/tests/test_vm_disk_diagnostics.py index 29aab08b84..dd58ee5641 100644 --- a/scripts/tests/test_vm_disk_diagnostics.py +++ b/scripts/tests/test_vm_disk_diagnostics.py @@ -24,8 +24,11 @@ SCRIPT = ROOT / "scripts/test-vm-disk.sh" DOC = ROOT / "docs/VM_DISK_MONITORING.md" BASH = shutil.which("bash") REAL_TIMEOUT = shutil.which("timeout") +REAL_PYTHON = os.sys.executable PRIVATE = "synthetic-private-infrastructure-marker" READS = [["status", "100"], ["config", "100", "--current"]] +STATUS_LIMIT = 256 +CONFIG_LIMIT = 64 * 1024 def guide_section(guide, heading): @@ -83,9 +86,27 @@ elif name == 'qm': sys.exit(90) if settings.get('sleep_operation') == operation: time.sleep(20) - sys.stdout.write(settings.get(operation + '_output', default)) + # Write bytes so corrupt transport controls are not silently repaired by + # the fixture's text encoding before they reach the actual helper. + output = settings.get(operation + '_output', default).encode('utf-8') + if settings.get(operation + '_invalid_utf8'): + output += b'\\xff' + try: + sys.stdout.buffer.write(output) + sys.stdout.buffer.flush() + except BrokenPipeError: + # A bounded reader is entitled to close the pipe before the producer + # finishes. Do not turn this fixture's expected stop into raw output. + os._exit(1) sys.stderr.write('synthetic-private-infrastructure-marker\\n') sys.exit(settings.get(operation + '_exit', 0)) +elif name == 'python3': + # The non-isolated form exists only for the startup-code negative control. + assert (len(args) == 4 and args[:2] == ['-I', '-c'] or + len(args) == 3 and args[0] == '-c') and args[-1] in ('256', '65536') + if settings.get('reader_exit') is not None: + sys.exit(settings['reader_exit']) + os.execv(os.environ['REAL_PYTHON'], [os.environ['REAL_PYTHON'], *args]) elif name == 'sudo': assert args == ['bash', './scripts/test-vm-disk.sh', '100'] os.execv(os.environ['REAL_BASH'], [os.environ['REAL_BASH'], *args[1:]]) @@ -100,22 +121,32 @@ class VMDiskDiagnosticsTest(unittest.TestCase): with tempfile.TemporaryDirectory() as temporary: root = Path(temporary) (root / "settings.json").write_text(json.dumps(settings or {})) - for name in ("qm", "timeout", "sudo", "pveum", "pveversion", "curl", "systemctl"): + for name in ("qm", "timeout", "python3", "sudo", "pveum", "pveversion", "curl", "systemctl"): if name == missing: continue tool = root / name - tool.write_text(TOOL) + # The collector adapter is itself named python3. Pin the + # fixture interpreter so /usr/bin/env cannot recurse through + # that adapter instead of ever executing the fake qm. + tool.write_text(TOOL.replace("/usr/bin/env python3", f"{REAL_PYTHON} -I")) tool.chmod(0o700) env = dict(os.environ, VM_FIXTURE=str(root), REAL_BASH=BASH, - REAL_TIMEOUT=REAL_TIMEOUT, PATH=f"{root}:{os.environ['PATH']}") + REAL_TIMEOUT=REAL_TIMEOUT, REAL_PYTHON=REAL_PYTHON, + PATH=f"{root}:{os.environ['PATH']}") if missing: # The adapters use an absolute interpreter, so an empty # fallback PATH can genuinely model unavailable dependencies. env["PATH"] = str(root) - for tool in root.iterdir(): - if tool.name in ("settings.json",): - continue - tool.write_text(TOOL.replace("/usr/bin/env python3", os.sys.executable)) + startup_marker = root / "startup-executed" + if (settings or {}).get("poison_pythonpath"): + startup = root / "python-startup" + startup.mkdir() + (startup / "sitecustomize.py").write_text( + "from pathlib import Path\n" + f"Path({str(startup_marker)!r}).write_text('synthetic startup')\n" + "raise SystemExit(29)\n" + ) + env["PYTHONPATH"] = str(startup) script = SCRIPT if source is not None: script = root / "candidate.sh" @@ -127,15 +158,121 @@ class VMDiskDiagnosticsTest(unittest.TestCase): capture_output=True, text=True, timeout=16) calls_file = root / "calls.jsonl" calls = [json.loads(line) for line in calls_file.read_text().splitlines()] if calls_file.exists() else [] + self.assertFalse(startup_marker.exists(), "untrusted Python startup code executed") self.assertNotIn(PRIVATE, result.stdout + result.stderr) return result, calls def assert_passive(self, calls, *, operations=READS): self.assertEqual([c["args"] for c in calls if c["name"] == "qm"], operations) - self.assertTrue(all(c["name"] in ("timeout", "qm", "sudo") for c in calls)) + self.assertTrue(all(c["name"] in ("timeout", "qm", "python3", "sudo") for c in calls)) + self.assertEqual([c["args"][-1] for c in calls if c["name"] == "python3"], + [str(STATUS_LIMIT if operation[0] == "status" else CONFIG_LIMIT) + for operation in operations]) for call in calls: if call["name"] == "timeout": self.assertEqual(call["args"][:4], ["--signal=TERM", "--kill-after=2s", "10s", "qm"]) + if call["name"] == "python3": + self.assertEqual(call["args"][:2], ["-I", "-c"]) + self.assertIn(call["args"][3], (str(STATUS_LIMIT), str(CONFIG_LIMIT))) + + def test_oversized_reads_fail_before_interpreting_partial_configuration(self): + cases = [ + {"status_output": "status: running\n" + PRIVATE + "x" * (1024 * 1024)}, + {"config_output": "agent: 1\nname: " + PRIVATE + "\ndescription: " + + "x" * (1024 * 1024) + "\nlock: backup\n"}, + ] + for settings in cases: + with self.subTest(operation=next(iter(settings))): + result, calls = self.exercise(settings=settings) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn("exceeded the", result.stderr) + self.assertIn("byte limit; preflight incomplete", result.stderr) + self.assertNotIn("Guest agent configured:", result.stdout) + self.assertNotIn("VM lock:", result.stdout) + self.assertNotIn("preflight completed", result.stdout) + self.assertLess(len(result.stdout + result.stderr), 600) + self.assert_passive(calls, operations=READS[:1] if "status_output" in settings else READS) + + def test_config_limit_is_measured_in_bytes_and_includes_trailing_newlines(self): + prefix = "agent: 1\nname: " + PRIVATE + "\ndescription: " + padding = CONFIG_LIMIT - len(prefix.encode()) - 1 + exact = prefix + "x" * padding + "\n" + self.assertEqual(len(exact.encode()), CONFIG_LIMIT) + result, calls = self.exercise(settings={"config_output": exact}) + self.assertEqual(result.returncode, 0, result.stderr) + self.assert_passive(calls) + # Shell command substitution removes trailing newlines. Neither that + # nor a multi-byte name may make an oversized response look complete. + for oversized in (exact + "\n", prefix + "é" * padding + "\n"): + with self.subTest(bytes=len(oversized.encode())): + result, calls = self.exercise(settings={"config_output": oversized}) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn("byte limit; preflight incomplete", result.stderr) + self.assertNotIn("VM lock:", result.stdout) + self.assertNotIn("preflight completed", result.stdout) + self.assert_passive(calls) + + def test_status_limit_includes_trailing_newlines_before_interpretation(self): + exact = "status: running" + "\n" * (STATUS_LIMIT - len("status: running")) + result, calls = self.exercise(settings={"status_output": exact}) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("VM status: running", result.stdout) + self.assert_passive(calls) + result, calls = self.exercise(settings={"status_output": exact + "\n"}) + self.assertEqual(result.returncode, 1) + self.assertIn("256-byte limit; preflight incomplete", result.stderr) + self.assertNotIn("VM status:", result.stdout) + self.assert_passive(calls, operations=READS[:1]) + + def test_corrupt_reads_cannot_be_silently_normalised_into_a_success(self): + for settings in ( + {"status_output": "status: run\u0000ning\n"}, + {"config_output": "agent: \u00001\nname: " + PRIVATE + "\n"}, + {"config_output": "agent: 1\nname: " + PRIVATE + "\n", "config_invalid_utf8": True}, + ): + with self.subTest(settings=settings): + result, calls = self.exercise(settings=settings) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn("non-text bytes; preflight incomplete", result.stderr) + self.assertNotIn("Guest agent configured:", result.stdout) + self.assertNotIn("VM lock:", result.stdout) + self.assertNotIn("preflight completed", result.stdout) + self.assert_passive(calls, operations=READS[:1] if "status_output" in settings else READS) + + def test_valid_unicode_configuration_remains_passive(self): + result, calls = self.exercise(settings={"config_output": "agent: 1\nname: café\nlock: backup\n"}) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("Guest agent configured: enabled", result.stdout) + self.assertIn("VM lock: backup", result.stdout) + self.assertNotIn("café", result.stdout + result.stderr) + self.assert_passive(calls) + + def test_producer_failures_are_independent_of_reader_result_codes(self): + # Producer exit codes are independent of bounded-reader result codes. + for code in (80, 81): + with self.subTest(code=code): + result, calls = self.exercise(settings={"config_exit": code}) + self.assertEqual(result.returncode, 1) + self.assertIn(f"configuration read failed (exit {code})", result.stderr) + self.assertNotIn("preflight completed", result.stdout) + self.assert_passive(calls) + + def test_reader_failure_is_incomplete_not_an_empty_or_successful_response(self): + result, calls = self.exercise(settings={"reader_exit": 23}) + self.assertEqual(result.returncode, 1) + self.assertIn("status read failed (exit 23)", result.stderr) + self.assertNotIn("VM status:", result.stdout) + self.assertNotIn("preflight completed", result.stdout) + self.assert_passive(calls, operations=READS[:1]) + + def test_reader_does_not_execute_python_environment_startup_code(self): + result, calls = self.exercise(settings={"poison_pythonpath": True}) + self.assertEqual(result.returncode, 0, result.stderr) + self.assert_passive(calls) + unsafe = SCRIPT.read_text().replace("python3 -I -c", "python3 -c") + self.assertNotEqual(unsafe, SCRIPT.read_text()) + with self.assertRaisesRegex(AssertionError, "untrusted Python startup"): + self.exercise(settings={"poison_pythonpath": True}, source=unsafe) def test_running_vm_preflight_is_passive_even_without_a_lock(self): result, calls = self.exercise() @@ -259,7 +396,7 @@ class VMDiskDiagnosticsTest(unittest.TestCase): self.assertEqual(calls, []) def test_missing_dependencies_do_not_run_a_partial_preflight(self): - for missing in ("qm", "timeout"): + for missing in ("qm", "timeout", "python3"): with self.subTest(missing=missing): result, calls = self.exercise(missing=missing) self.assertEqual(result.returncode, 1) @@ -299,6 +436,14 @@ class VMDiskDiagnosticsTest(unittest.TestCase): class VMDiskHelpTest(unittest.TestCase): + def test_documented_read_limits_and_failure_meaning_match_the_helper(self): + section = guide_section(DOC.read_text(), "Passive host preflight") + for phrase in ("256 bytes", "64 KiB", "Python 3", "truncated prefix", + "printing raw", "saving it to disk", "Inspect the configuration privately"): + self.assertIn(phrase, section) + self.assertIn(f"read_bounded status status_output {STATUS_LIMIT}", SCRIPT.read_text()) + self.assertIn(f"read_bounded configuration config {CONFIG_LIMIT}", SCRIPT.read_text()) + def test_section_selection_preserves_shell_comments_and_rejects_missing_or_duplicate_headings(self): content = "```bash\n# From the reviewed checkout\n### Shell comment, not a heading\ntrue\n```\n" guide = "### Passive host preflight\n" + content + "## Next\nOther text\n"