Skip to content

fix: a LUKS area bound that overflows, a mountinfo decoder that unescapes in the wrong order, and four silent defaults - #1280

Open
kvinwang wants to merge 4 commits into
nextfrom
fix/config-parser-bounds
Open

kvinwang wants to merge 4 commits into
nextfrom
fix/config-parser-bounds

Conversation

@kvinwang

Copy link
Copy Markdown
Collaborator

Problem

A LUKS keyslot area whose end overflows u64 passes the bound that exists to stop it

validate_single_luks2_header pins where cryptsetup reads the encrypted key material from,
and the comment says why:

Pin where the encrypted key material is read from. The binary area must sit between the two
header copies and the encrypted payload; otherwise a host with raw disk access could redirect
it elsewhere.

The guard is area.offset() + area.size() > PAYLOAD_OFFSET over two u64s that the
host-supplied on-disk header provides. offset = 32768 passes the lower bound; adding
size = 2^64 - 32768 + 1000 wraps the sum to 1000, which is under PAYLOAD_OFFSET, so the
header is accepted.

This runs on every boot with an existing encrypted data disk, not only at format time:
mount_data_diskopen_encrypted_volumecryptsetup luksHeaderBackup
validate_luks2_headers. It runs under dstack-prepare.service, which carries
FailureAction=reboot.

The release profile sets panic = "abort" and leaves overflow checks off, so the two profiles
disagree about what happens. Both are wrong, in different ways:

$ cargo test -p dstack-util test_validate_luks2_header          # debug
---- system_setup::test_validate_luks2_header_rejects_keyslot_area_that_overflows stdout ----
thread '...' panicked at dstack-util/src/system_setup.rs:3349:44:
attempt to add with overflow

$ cargo test --release -p dstack-util test_validate_luks2_header   # the shipped profile
---- system_setup::test_validate_luks2_header_rejects_keyslot_area_that_overflows stdout ----
thread '...' panicked at dstack-util/src/system_setup.rs:3524:58:
called `Result::unwrap_err()` on an `Ok` value: ()

The second one is the real behaviour: validate_luks2_headers returns Ok.

The two /proc/self/mountinfo decoders disagree, and the simulator's is wrong

The kernel escapes a literal backslash in a mount point as \134. tee-simulator decoded with
successive substitutions, \134 first:

value.replace(r"\134", "\\").replace(r"\040", " ")...

so the backslash it just produced is re-read as the start of a space escape. A mount point whose
name literally contains the text \040 decodes to one containing a space, and is_mounted
then answers about a path it was not asked about — which is what the "refusing to mount over an
existing mount" guard is consulted for:

---- tests::a_literal_escape_sequence_in_a_path_stays_literal stdout ----
assertion `left == right` failed
  left: "/run/a b"
 right: "/run/a\\040b"

dstack-volume's copy gets the order right but computes (octal[0] - b'0') * 64 in a u8:

---- tests::a_three_digit_octal_escape_does_not_overflow stdout ----
thread '...' panicked at crates/dstack-volume/src/bin/dstack-volume.rs:421:30:
attempt to multiply with overflow

That one is not reachable from /proc — the kernel's mangle() escapes only " \t\n\\",
so nothing above \134 is ever emitted. It is fixed because it is free, not because it bites.

Two grammars for the veritysetup root hash

volume.rs accepted only Root hash: at column 0 with no trim; dstack-volume.rs trimmed and
accepted both spellings. Each is correct for the command it reads today, which is exactly how one
of them gets to break silently later. The stricter one also reports a label with nothing after it
as a root hash:

parse_root_hash("Root hash:\n")       -> Some("")
parse_root_hash("  Root hash: abc\n") -> None

Four silent defaults that hide the thing that actually failed

House rule #541: if code ignores malformed input to stay lenient, log a warning — silent skips
hide corruption.

  • blkid … .unwrap_or_default() on a verity volume yields "", which drops both -t <type> and
    ro,noload. An ext4 volume then makes the kernel want to replay a journal onto a read-only
    dm-verity device, and nothing says the probe failed.
  • let _ = run_cmd!(modprobe dm-verity) and let _ = run_cmd!(udevadm settle): the module never
    loads, the scan finds nothing, and there is no trace of why.
  • GatewayKeyStore::load_from uses .ok()? twice, so a truncated cache reads as absent. The
    boot regenerates the WireGuard key and re-registers as if it had never registered.
  • app-compose.sh:19 reads .sys-config.json as jq … || echo "". set -e does not fire,
    because || makes it a tested command:
$ printf '%s' '{"pccs_url": "https://pccs.example/' > sc.json
$ set -euo pipefail
$ CFG=$([ -f sc.json ] && jq -r '.pccs_url//""' sc.json || echo ""); echo "exit=$? CFG_PCCS_URL=[$CFG]"
jq: parse error: Unfinished string at EOF at line 1, column 35
exit=0 CFG_PCCS_URL=[]

A host that wrote a truncated sys-config is indistinguishable from a host that configured no
PCCS at all.

Fix

  1. checked_add for the LUKS keyslot area. A sum that wraps is an out-of-range area. New
    test patches the real luks_header_good fixture — both header copies, keeping the JSON
    region length by giving back NUL padding — and asserts the bound rejects it. Passes in both
    profiles after the fix.

  2. One left-to-right pass in both mountinfo decoders, so a byte the decoder produces is never
    re-read as the start of another escape. The octal digits are folded in u16 and
    range-checked, so a three-digit escape above \377 stays literal text instead of wrapping.
    tee-simulator now returns OsString rather than String, so a non-UTF-8 mount point is not
    mangled before the comparison. Each copy carries a doc comment naming the other.

  3. One dstack_volume::parse_verity_root_hash used by the builder and the guest. Accepts
    both spellings after trimming, and requires what follows the label to actually be hex. Pinned
    against output captured from veritysetup 2.7.0 for format (flush left, capital R) and the
    status shape (indented, lower-case).

  4. Say what failed. The four silent defaults now warn and state what the guest does next.
    These are log-only changes: there is no failing test for them, because the only observable
    difference is a log line and this tree has no log-capture harness.

  5. app-compose.sh distinguishes absent from unreadable. It still continues with an empty
    PCCS_URL — the PCCS is untrusted collateral transport, not a trust boundary, and
    dstack-prepare already fails closed on a sys-config that Rust cannot parse — but it says so.
    SYS_CONFIG_FILE and HOST_SHARED_DIR become overridable, matching every other path in the
    file, so the behaviour is testable.

How this was verified

  • cargo test -p dstack-util -p dstack-volume -p dstack-types -p size-parser -p serde-duration -p nvattest — all pass.
  • cargo clippy --workspace --lib -- -D warnings --allow unused_variables — clean.
    (--all-targets does not pass on next either; pre-existing lints.)
  • cargo fmt --all --check — clean.
  • ./os/mkosi/build.sh lint (= os/mkosi/tests/acceptance.sh) — passes, with the new
    os/tests/test-app-compose-sys-config.sh wired in next to test-kernel-header-normalization.sh.
  • The new shell test runs the real app-compose.sh with real jq over four sys-config states.
    Reverting only app-compose.sh to next and re-running it:
--- app-compose.sh reverted to next ---
FAIL: a malformed sys-config was read as absent, with no warning: Usage: .../app-compose.sh [start|stop]
exit=1
--- restored ---
ok: app-compose.sh distinguishes an absent sys-config from an unreadable one

Overlap with open PRs

Test-merged against every open PR that touches a file this branch changes:

PR result
#1233, #1243, #1251, #1254, #1264 clean
#1257, #1274 conflict in os/mkosi/tests/acceptance.sh

Both conflicts are the same shape and purely additive: #1257, #1274 and this branch each append
their own "$D/../tests/test-*.sh" invocation right after test-kernel-header-normalization.sh.
Take the union, in merge order, each keeping its own comment — the three scripts test
disjoint things.

dstack/dstack-util/src/system_setup.rs merges clean against all five PRs that touch it.

Deliberately not touched, to keep one concern per PR and avoid re-litigating work in flight:
the .decrypted-env escaping and the blkid data-disk probe (#1243), the docker-compose.yaml
project-name grammar (#1254), and the sha256sum.txt manifest grammar (#1251). One residual for
#1233: its rewritten copy helper still does stat-then-std::io::copy with no
.take(max_size), so the size bound remains a TOCTOU against a host that can grow the file on
the 9p/virtio store between the two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant