Improve security and error handling for VM state and guest file operations; clarify permissions requirements in documentation.

DEVELOPER.md

@@ -733,7 +733,7 @@ ip route show default
 iptables -t nat -S POSTROUTING
 iptables -S FORWARD
 ls -la /var/lib/uvm/vms
-cat /var/lib/uvm/state.json
+sudo cat /var/lib/uvm/state.json  # Contains plaintext guest passwords; do not share.
 ```
 
 Do not use these commands to mutate UVM resources manually while a lifecycle

README.md

@@ -236,9 +236,9 @@ State writes are locked and atomic. Lifecycle operations share an additional
 operation lock so concurrent `create`, `stop`, `destroy`, and `install`
 commands cannot overwrite each other's state or binaries.
 
-The registry stores guest usernames and passwords in plaintext as requested,
+The registry stores guest usernames and passwords in plaintext, so `state.json`
-so `state.json` is mode `0600`. Treat it as a secret, do not include it in bug
+is mode `0600`. Treat it as a secret, do not include it in bug reports, and
-reports, and replace the default `root` password immediately.
+replace the default `root` password immediately.
 
 Credential provisioning applies only to VMs created by this version. Existing
 VM disks are not modified automatically.

tests/test_images.py

@@ -10,6 +10,7 @@ from uvm.config import Settings
 from uvm.errors import UvmError
 from uvm.images import (
     _FIRECRACKER_DEMO_PUBLIC_KEY,
+    _GuestFile,
     _enable_ssh_password_authentication,
     _set_shadow_password,
     _without_firecracker_demo_key,
@@ -96,6 +97,8 @@ class ImageStoreTests(unittest.TestCase):
                 "HostKey /etc/ssh/ssh_host_ecdsa_key\n"
                 "HostKey /etc/ssh/ssh_host_ed25519_key\n"
                 "PasswordAuthentication no\n"
+                "KbdInteractiveAuthentication no\n"
+                "ChallengeResponseAuthentication no\n"
                 "PermitRootLogin yes\n"
                 "Match User root\n"
                 "    PasswordAuthentication yes\n"
@@ -105,6 +108,16 @@ class ImageStoreTests(unittest.TestCase):
         self.assertNotIn("/insecure/shared-key", updated)
         self.assertNotIn("PermitRootLogin prohibit-password", updated)
 
+    def test_non_root_credentials_preserve_the_existing_root_login_policy(self) -> None:
+        updated = _enable_ssh_password_authentication(
+            "PermitRootLogin no\nChallengeResponseAuthentication yes\n",
+            "service",
+        )
+
+        self.assertIn("PermitRootLogin no", updated)
+        self.assertIn("Match User service\n    PasswordAuthentication yes", updated)
+        self.assertNotIn("ChallengeResponseAuthentication yes", updated)
+
     def test_removes_only_the_public_firecracker_demo_key(self) -> None:
         own_key = "ssh-ed25519 AAAA-own-key developer@example"
 
@@ -150,3 +163,27 @@ class ImageStoreTests(unittest.TestCase):
                     Path(temporary_directory) / "authorized_keys",
                     required=False,
                 )
+
+    def test_refuses_to_replace_files_with_unpreserved_security_metadata(self) -> None:
+        runner = PasswordHashRunner()
+        store = ImageStore(Settings(), runner=runner)  # type: ignore[arg-type]
+
+        for guest_file in (
+            _GuestFile(Path("/tmp/shadow"), 0o640, 0, 42, links=2),
+            _GuestFile(
+                Path("/tmp/shadow"),
+                0o640,
+                0,
+                42,
+                has_extended_attributes=True,
+            ),
+        ):
+            with self.subTest(guest_file=guest_file):
+                with self.assertRaises(UvmError):
+                    store._write_guest_file(
+                        Path("/tmp/rootfs.ext4"),
+                        "/etc/shadow",
+                        guest_file,
+                    )
+
+        self.assertEqual(runner.calls, [])

tests/test_state.py

@@ -6,9 +6,11 @@ import tempfile
 import unittest
 from dataclasses import replace
 from pathlib import Path
+from unittest.mock import patch
 
 from uvm.config import Settings
 from uvm.domain import VmRecord
+from uvm.errors import StateError
 from uvm.state import StateStore
 
 
@@ -97,3 +99,14 @@ class StateStoreTests(unittest.TestCase):
         self.store.load()
 
         self.assertEqual(stat.S_IMODE(self.settings.state_path.stat().st_mode), 0o600)
+
+    def test_load_explains_when_registry_permissions_require_root(self) -> None:
+        self.base.mkdir(parents=True)
+        self.settings.state_path.write_text('{"vms": {}}', encoding="utf-8")
+        self.settings.state_path.chmod(0o644)
+
+        with (
+            patch("uvm.state.os.chmod", side_effect=PermissionError),
+            self.assertRaisesRegex(StateError, "Run this command with sudo"),
+        ):
+            self.store.load()

uvm/images.py

@@ -24,6 +24,8 @@ _FIRECRACKER_DEMO_PUBLIC_KEY = (
 )
 _MODE_PATTERN = re.compile(r"Mode:\s+0*([0-7]+)")
 _OWNER_PATTERN = re.compile(r"User:\s+(\d+)\s+Group:\s+(\d+)")
+_FLAGS_PATTERN = re.compile(r"Flags:\s+0x([0-9a-fA-F]+)")
+_LINKS_PATTERN = re.compile(r"Links:\s+(\d+)")
 _HOST_KEY_TYPES = (
     ("rsa", "3072"),
     ("ecdsa", "256"),
@@ -43,6 +45,9 @@ class _GuestFile:
     mode: int
     uid: int
     gid: int
+    flags: int | None = None
+    links: int = 1
+    has_extended_attributes: bool = False
 
 
 class ImageStore:
@@ -184,7 +189,14 @@ class ImageStore:
         )
         mode_match = _MODE_PATTERN.search(stat_result.stdout)
         owner_match = _OWNER_PATTERN.search(stat_result.stdout)
-        if mode_match is None or owner_match is None:
+        flags_match = _FLAGS_PATTERN.search(stat_result.stdout)
+        links_match = _LINKS_PATTERN.search(stat_result.stdout)
+        if (
+            mode_match is None
+            or owner_match is None
+            or flags_match is None
+            or links_match is None
+        ):
             output = f"{stat_result.stdout}\n{stat_result.stderr}".lower()
             if not required and "file not found" in output:
                 return None
@@ -198,14 +210,35 @@ class ImageStore:
         )
         if not destination.is_file():
             raise UvmError(f"could not read guest rootfs file: {guest_path}")
+        attributes = self._runner.run(
+            ("debugfs", "-R", f"ea_list {guest_path}", disk),
+            check=False,
+            capture=True,
+        )
+        diagnostics = [
+            line
+            for line in attributes.stderr.splitlines()
+            if line and not line.startswith("debugfs ")
+        ]
+        if attributes.returncode != 0 or diagnostics:
+            raise UvmError(f"could not inspect guest rootfs metadata: {guest_path}")
         return _GuestFile(
             path=destination,
             mode=int(mode_match.group(1), 8),
             uid=int(owner_match.group(1)),
             gid=int(owner_match.group(2)),
+            flags=int(flags_match.group(1), 16),
+            links=int(links_match.group(1)),
+            has_extended_attributes=bool(attributes.stdout.strip()),
         )
 
     def _write_guest_file(self, disk: Path, guest_path: str, source: _GuestFile) -> None:
+        if source.links != 1:
+            raise UvmError(f"cannot safely replace hard-linked guest file: {guest_path}")
+        if source.has_extended_attributes:
+            raise UvmError(
+                f"cannot safely replace guest file with extended attributes: {guest_path}"
+            )
         self._runner.run(("debugfs", "-w", "-R", f"rm {guest_path}", disk), capture=True)
         self._runner.run(
             ("debugfs", "-w", "-R", f"write {source.path} {guest_path}", disk),
@@ -226,6 +259,17 @@ class ImageStore:
                 ),
                 capture=True,
             )
+        if source.flags is not None:
+            self._runner.run(
+                (
+                    "debugfs",
+                    "-w",
+                    "-R",
+                    f"set_inode_field {guest_path} flags 0x{source.flags:x}",
+                    disk,
+                ),
+                capture=True,
+            )
 
         verification = source.path.with_name(f"{source.path.name}.verify")
         written = self._read_guest_file(disk, guest_path, verification)
@@ -235,6 +279,7 @@ class ImageStore:
             or written.mode != source.mode
             or written.uid != source.uid
             or written.gid != source.gid
+            or (source.flags is not None and written.flags != source.flags)
         ):
             raise UvmError(f"could not verify updated guest rootfs file: {guest_path}")
 
@@ -315,7 +360,14 @@ def _set_shadow_password(contents: str, username: str, password_hash: str) -> st
 
 
 def _enable_ssh_password_authentication(contents: str, username: str) -> str:
-    managed_directives = {"hostkey", "passwordauthentication", "permitrootlogin"}
+    managed_directives = {
+        "challengeresponseauthentication",
+        "hostkey",
+        "kbdinteractiveauthentication",
+        "passwordauthentication",
+    }
+    if username == "root":
+        managed_directives.add("permitrootlogin")
     unmanaged_lines = []
     for line in contents.splitlines():
         stripped = line.lstrip()
@@ -328,13 +380,17 @@ def _enable_ssh_password_authentication(contents: str, username: str) -> str:
         "HostKey /etc/ssh/ssh_host_ecdsa_key",
         "HostKey /etc/ssh/ssh_host_ed25519_key",
         "PasswordAuthentication no",
+        "KbdInteractiveAuthentication no",
+        "ChallengeResponseAuthentication no",
     ]
     if username == "root":
         directives.append("PermitRootLogin yes")
     return (
         "# Managed by uvm\n"
         + "\n".join(directives)
-        + f"\nMatch User {username}\n    PasswordAuthentication yes\nMatch all\n"
+        + f"\nMatch User {username}\n"
+        + "    PasswordAuthentication yes\n"
+        + "Match all\n"
         + "\n".join(unmanaged_lines).rstrip()
         + "\n"
     )

uvm/state.py

@@ -98,6 +98,10 @@ class StateStore:
                 os.chmod(path, 0o600)
             with path.open(encoding="utf-8") as state_file:
                 raw = json.load(state_file)
+        except PermissionError as error:
+            raise StateError(
+                f"cannot access protected VM registry {path}. Run this command with sudo."
+            ) from error
         except (OSError, json.JSONDecodeError) as error:
             raise StateError(f"cannot read {path}: {error}") from error
         return State.from_dict(raw)