From 39d47b8108702d898cb00db5ba4b654350be144d Mon Sep 17 00:00:00 2001 From: didericis Date: Fri, 17 Jul 2026 00:44:06 -0400 Subject: [PATCH] fix(firecracker): harden committed-snapshot resume against guest-controlled data Address the codex review on #398: - P1: inject_guest_boot no longer follows a symlink at bb-init/bb-dropbear. A committed snapshot is guest-controlled and could plant those paths as symlinks aimed at a host file (e.g. bb-init -> ~/.bashrc); write_text / copy2 would then overwrite the target as the host user during resume. Replace any pre-existing entry and create the files with O_EXCL|O_NOFOLLOW so the write stays inside the staging tree. - P2: write the snapshot tar owner-only (0600). It can contain the bottle's private workspace; it was being created world-readable (0644). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UoEZHDjv84ChoZbozQERhJ --- bot_bottle/backend/firecracker/freezer.py | 7 ++- bot_bottle/backend/firecracker/util.py | 34 +++++++++++--- tests/unit/test_firecracker_helpers.py | 55 +++++++++++++++++++++++ 3 files changed, 89 insertions(+), 7 deletions(-) diff --git a/bot_bottle/backend/firecracker/freezer.py b/bot_bottle/backend/firecracker/freezer.py index e1f3735..55f0a1c 100644 --- a/bot_bottle/backend/firecracker/freezer.py +++ b/bot_bottle/backend/firecracker/freezer.py @@ -63,7 +63,11 @@ def _commit_rootfs_via_ssh(private_key: Path, guest_ip: str, tar_path: Path) -> tar_path.parent.mkdir(parents=True, exist_ok=True) partial = tar_path.with_name(tar_path.name + ".partial") ssh = util.ssh_base_argv(private_key, guest_ip) - with open(partial, "wb") as tar_out: + # The snapshot can contain the bottle's private workspace, so keep it + # owner-only (0600) — create it that way and re-assert after the rename + # (os.replace carries the source mode, but be explicit). + fd = os.open(partial, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, "wb") as tar_out: result = subprocess.run( [*ssh, "--", "tar", "--create", "--one-file-system", "--exclude=./proc", "--exclude=./sys", "--exclude=./dev", @@ -75,3 +79,4 @@ def _commit_rootfs_via_ssh(private_key: Path, guest_ip: str, tar_path: Path) -> die(f"ssh tar for {guest_ip} failed: " f"{(result.stderr or b'').decode().strip() or ''}") os.replace(partial, tar_path) + os.chmod(tar_path, 0o600) diff --git a/bot_bottle/backend/firecracker/util.py b/bot_bottle/backend/firecracker/util.py index 4421714..425addd 100644 --- a/bot_bottle/backend/firecracker/util.py +++ b/bot_bottle/backend/firecracker/util.py @@ -259,12 +259,34 @@ def build_committed_rootfs_dir(tar_path: Path) -> Path: def inject_guest_boot(rootfs: Path, init_script: str | None = None) -> None: """Drop the static dropbear and the PID-1 init into the rootfs. `init_script` defaults to the SSH-only agent init; the infra VM - passes its own (control plane + gateway) init.""" - shutil.copy2(dropbear_path(), rootfs / "bb-dropbear") - os.chmod(rootfs / "bb-dropbear", 0o755) - init = rootfs / "bb-init" - init.write_text(init_script or _GUEST_INIT) - os.chmod(init, 0o755) + passes its own (control plane + gateway) init. + + A committed snapshot is guest-controlled, so `bb-dropbear`/`bb-init` + may already exist as symlinks aimed at a host file (e.g. bb-init -> + ~/.bashrc). Replace whatever is there and create the files with + O_EXCL|O_NOFOLLOW so the write always lands a fresh regular file in + the staging tree and never follows a planted symlink out of it.""" + _write_staged_file(rootfs / "bb-dropbear", dropbear_path().read_bytes()) + _write_staged_file(rootfs / "bb-init", (init_script or _GUEST_INIT).encode()) + + +def _write_staged_file(path: Path, data: bytes) -> None: + """Write `data` to `path` (mode 0755) as a fresh regular file inside a + staging rootfs, replacing any pre-existing entry without following a + symlink at `path`. Fails closed on anything unexpected there.""" + if path.is_symlink() or path.exists(): + if path.is_dir() and not path.is_symlink(): + shutil.rmtree(path) + else: + path.unlink() + fd = os.open( + path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW, 0o755 + ) + try: + os.write(fd, data) + finally: + os.close(fd) + os.chmod(path, 0o755) def build_rootfs_ext4(base_dir: Path, out_path: Path, *, slack_mib: int = 1024) -> None: diff --git a/tests/unit/test_firecracker_helpers.py b/tests/unit/test_firecracker_helpers.py index 76f97d8..85113d1 100644 --- a/tests/unit/test_firecracker_helpers.py +++ b/tests/unit/test_firecracker_helpers.py @@ -7,6 +7,7 @@ from __future__ import annotations import json import os +import stat import subprocess import tempfile import unittest @@ -218,5 +219,59 @@ class TestBuildCommittedRootfsDir(unittest.TestCase): self.assertEqual(2, calls["n"]) +class TestInjectGuestBootSymlinkSafe(unittest.TestCase): + """A committed snapshot is guest-controlled: inject_guest_boot must not + follow a planted symlink and overwrite a host file during resume.""" + + def test_planted_symlink_does_not_escape_staging_tree(self): + with tempfile.TemporaryDirectory(prefix="fc-inject.") as d: + tmp = Path(d) + dropbear = tmp / "dropbear" + dropbear.write_bytes(b"DROPBEAR") + # A host file the malicious snapshot tries to clobber. + victim = tmp / "victim" + victim.write_text("original") + + rootfs = tmp / "rootfs" + rootfs.mkdir() + # The snapshot planted bb-init/bb-dropbear as symlinks to it. + (rootfs / "bb-init").symlink_to(victim) + (rootfs / "bb-dropbear").symlink_to(victim) + + with patch.object(util, "dropbear_path", return_value=dropbear): + util.inject_guest_boot(rootfs, init_script="#!/bin/sh\nreal\n") + + # Host file untouched; the staged paths are fresh regular files. + self.assertEqual("original", victim.read_text()) + self.assertFalse((rootfs / "bb-init").is_symlink()) + self.assertFalse((rootfs / "bb-dropbear").is_symlink()) + self.assertEqual("#!/bin/sh\nreal\n", (rootfs / "bb-init").read_text()) + self.assertEqual(b"DROPBEAR", (rootfs / "bb-dropbear").read_bytes()) + + +class TestCommitRootfsPermissions(unittest.TestCase): + """The snapshot tar can hold the bottle's private workspace, so the + freezer must write it owner-only (0600).""" + + def test_snapshot_created_owner_only(self): + with tempfile.TemporaryDirectory(prefix="fc-freeze.") as d: + tmp = Path(d) + key = tmp / "key" + key.write_text("K") + tar_path = tmp / "state" / "committed-rootfs.tar" + + def fake_run(argv: list[str], *a: Any, **k: Any) -> Any: + # Emulate the ssh|tar pipe streaming the snapshot to stdout. + k["stdout"].write(b"TARDATA") + return subprocess.CompletedProcess(argv, 0, b"", b"") + + with patch.object(freezer.util, "ssh_base_argv", return_value=["ssh"]), \ + patch.object(freezer.subprocess, "run", side_effect=fake_run): + freezer._commit_rootfs_via_ssh(key, "10.0.0.1", tar_path) + + self.assertEqual(b"TARDATA", tar_path.read_bytes()) + self.assertEqual(0o600, stat.S_IMODE(tar_path.stat().st_mode)) + + if __name__ == "__main__": unittest.main()