Skip to content

fix(verifier): a DebugSwap guest verifies, and four more ways host bytes reach a measurement unchecked - #1275

Open
kvinwang wants to merge 8 commits into
nextfrom
fix/verifier-input-canonicalization
Open

kvinwang wants to merge 8 commits into
nextfrom
fix/verifier-input-canonicalization

Conversation

@kvinwang

Copy link
Copy Markdown
Collaborator

Problem

Eight defects in the verifier / measurement toolchain, all on the path that turns host-supplied
bytes into a trusted measurement. panic = "abort" in the release profile
(dstack/Cargo.toml:312) is the reason input canonicalization on these paths is worth more than
it looks: anything reachable from attacker bytes is a whole-process abort, not a caught error.

They are shipped as one PR because the eight items land in five files and two of those files
(dstack-mr/src/sev.rs, dstack-types/src/lib.rs) carry two items each. Splitting along the
canonicalization/hygiene line would have put two changes to each of those files on two branches and
guaranteed a self-conflict; the commits are one item each and are readable independently.

Every item below has a test that was run and observed failing on next (030fbb2) before the fix
existed. The pre-fix output is quoted verbatim.


1. guest_features was only checked != 0, and a DebugSwap guest passes verification today

dstack-mr/src/sev.rs:114 bailed only on zero, and :737-738 fed the value into both VMSA pages.
guest_features is the one launch parameter that is neither pinned by SevOsImageMeasurement nor
constrained by validate_measurement_input: it rides in the host-written SnpMeasurementDocument
next to vcpus/vcpu_type, outside the CBOR that os_image_hash commits to, and it lands verbatim
at VMSA offset 0x3B0.

The consequence is not "tampering is caught by the measurement comparison". Because the expected
launch digest is recomputed from the declared value, a host that really boots the guest with extra
feature bits and honestly declares them gets a matching digest and a key release. Nothing ever
checked the bits. The test below is exactly that: an internally consistent DebugSwap launch, not a
tampered one.

running 2 tests
test sev::tests::rejects_guest_features_outside_the_launch_allowlist ... FAILED
test sev::tests::verify_sev_launch_rejects_a_consistent_debugswap_guest ... FAILED

failures:

---- sev::tests::rejects_guest_features_outside_the_launch_allowlist stdout ----

thread 'sev::tests::rejects_guest_features_outside_the_launch_allowlist' (1865055) panicked at dstack-mr/src/sev.rs:1650:18:
feature bit outside the allowlist must not be accepted: ()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- sev::tests::verify_sev_launch_rejects_a_consistent_debugswap_guest stdout ----

thread 'sev::tests::verify_sev_launch_rejects_a_consistent_debugswap_guest' (1865056) panicked at dstack-mr/src/sev.rs:1626:28:
a DebugSwap launch verified; bound os_image_hash 3d7fe56dc936bde7c7d9045f44ed709f99c3f87ef007d1a5ff065a980b881466


failures:
    sev::tests::rejects_guest_features_outside_the_launch_allowlist
    sev::tests::verify_sev_launch_rejects_a_consistent_debugswap_guest

test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 30 filtered out; finished in 0.01s

Fix. ALLOWED_GUEST_FEATURES = SEV_FEATURE_SNP_ACTIVE (bit 0). validate_measurement_input
requires bit 0 and rejects every other bit by name.

The judgement call: which bits are permitted. The bit layout is the AMD64 APM vol. 2
SEV_FEATURES table, which the SEV-SNP ABI's GUEST_FEATURES field in SNP_LAUNCH_START mirrors:
0 SNPActive, 1 vTOM, 2 ReflectVC, 3 RestrictedInjection, 4 AlternateInjection, 5 DebugSwap,
6 PreventHostIBS, 7 BTBIsolation, 8 VmplSSS, 9 SecureTSC, 10 VmgexitParameter, 12 IbsVirtualization,
14 VmsaRegProt, 15 SmtProtection.

I derived the allowlist from what dstack's launch path actually sets rather than from the spec's
menu. dstack has exactly one launch path, dstack/vmm/src/app/qemu.rs:1072:

sev-snp-guest,id=sev0,policy=0x30000,sev-device=/dev/sev,kernel-hashes=on,host-data={host_data},...

It sets no feature property, so KVM starts the guest with SNPActive alone, and
dstack/vmm/src/app.rs:2244 writes the matching guest_features: 1 into the measurement document.
1 is the only value dstack has ever emitted; the captured real-hardware vector in
sev.rs's REAL_MEASUREMENT_DOC carries "guest_features":1 too.

Alternatives rejected:

  • Allow the protective bits too (RestrictedInjection, SecureTSC, VmsaRegProt). They are protective
    when set, so allowing them looks harmless. But dstack cannot produce them, so allowing them only
    ever admits a guest dstack did not configure — and the same MeasurementInput is what the KMS
    recomputes against, so "harmless bit" means "a VMSA shape the KMS was never asked to approve".
    Widening is a one-line change the day a launch path sets one.
  • Reject only the unambiguously dangerous bits (DebugSwap above all) and pass the rest. This was
    the fallback the audit allowed if no defensible allowlist could be established. It is not needed:
    the launch path pins the answer to a single value, so the allowlist is defensible and strictly
    tighter.

Compat. None. Any deployment whose real VMSA features differ from the declared value already
fails the measurement comparison, and the VMM only ever declares 1. The golden vectors
(sev::tests::measurement_vector_does_not_drift, sev::tests::real_fixture_recomputes_measurement)
pass unchanged — no measurement byte moved.

One existing case was moved, not deleted: verify_sev_launch_rejects_tampered_measured_inputs used
to tamper guest_features = 3 and assert the measurement comparison caught it. The allowlist now
rejects that value before the measurement is recomputed, so there is no tampered value left to reach
that gate; the coverage moved to the two new tests and a comment in the old list says so.
dstack-kms's rejects_unsafe_machine_config gained a DebugSwap case alongside its updated
zero case.


2. compute_pcr_digest had no per-PCR length check, and PcrValue.algorithm was never validated

tpm-qvl/src/verify.rs:307-314. compute_pcr_digest concatenates the quoted PCR values with nothing
marking where one ends and the next begins. The bank the quote attests is already resolved at
verify.rs:100-112, so both the expected digest length and the expected bank name were available and
neither was used.

The length check is not cosmetic. Given a genuine, AK-signed pcr_digest over PCR4 || PCR7, a
caller can re-split the same preimage into a 31-byte "PCR 4" and a 33-byte "PCR 7". The digest still
matches, the indices still match the signed selection, and VerifiedReport::get_pcr(4) then hands
out a value the TPM never held. Both tests below reach the AK certificate on next — i.e. both
forgeries pass every PCR gate, and in a real attack the certificate is genuine:

running 2 tests
test verify::tests::rejects_pcr_values_that_misname_the_attested_bank ... FAILED
test verify::tests::rejects_pcr_values_whose_lengths_do_not_match_the_attested_bank ... FAILED

failures:

---- verify::tests::rejects_pcr_values_that_misname_the_attested_bank stdout ----

thread 'verify::tests::rejects_pcr_values_that_misname_the_attested_bank' (1461757) panicked at tpm-qvl/src/verify.rs:806:9:
unexpected error: failed to extract AK public key from certificate

---- verify::tests::rejects_pcr_values_whose_lengths_do_not_match_the_attested_bank stdout ----

thread 'verify::tests::rejects_pcr_values_whose_lengths_do_not_match_the_attested_bank' (1461758) panicked at tpm-qvl/src/verify.rs:781:9:
unexpected error: failed to extract AK public key from certificate
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    verify::tests::rejects_pcr_values_that_misname_the_attested_bank
    verify::tests::rejects_pcr_values_whose_lengths_do_not_match_the_attested_bank

test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Fix. After the existing bank gate, every PcrValue must carry algorithm == "sha256" and a
32-byte value. Placed at the call site rather than inside compute_pcr_digest so the hunk does not
overlap #1267's (see the merge report below).

Compat. None. tpm-attest sets algorithm from the selection's bank name and the bank gate
above already restricts that to SHA-256, so every honest quote is unaffected. AWS NitroTPM uses
SHA-384 but does not go through this function — get_root_ca bails for Platform::AwsEc2.


3. CBOR decode accepted trailing bytes

cbor_from_slice at dstack-types/src/lib.rs:1527-1533 stopped at the end of the first item and
ignored the rest.

running 1 test
test cbor_canonicalization_tests::cbor_decoders_reject_trailing_bytes ... FAILED

failures:

---- cbor_canonicalization_tests::cbor_decoders_reject_trailing_bytes stdout ----

thread 'cbor_canonicalization_tests::cbor_decoders_reject_trailing_bytes' (1490495) panicked at dstack-types/src/lib.rs:2914:14:
a document with trailing bytes must not decode: SevOsImageMeasurement { base_cmdline: "console=ttyS0 dstack.rootfs_hash=11", ovmf_hash: [68, 68, ...], kernel_hash: [85, 85, ...], initrd_hash: [102, 102, ...], sev_hashes_table_gpa: 8392704, sev_es_reset_eip: 4294967280, ovmf_sections: [OvmfSection { gpa: 1048576, size: 4096, section_type: 1 }] }

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 55 filtered out; finished in 0.00s

I checked the "currently unexploitable" claim rather than repeating it. It holds, and the reason
is narrower than it sounds. The binding is always verify_measurement_material
(lib.rs:1588-1611), which hashes the raw document.measurement bytes against the
sha256sum.txt entry; trailing bytes therefore ride along in os_image_hash and produce an image
identity that has to be in the allowlist anyway. I walked every decode site to confirm nothing
compares a re-encoded hash to a checksum entry:

  • dstack-mr/src/sev.rs:1061SevOsImageMeasurementDocument::verify(&os_image_hash) runs
    before measurement_input_from_snp_document, so the SNP path is raw-bytes bound.
  • verifier/src/verification.rs:1037, :1200, :1239 — same, .verify(&vm_config.os_image_hash)
    precedes each decode_measurement().
  • vmm/src/app.rs:2215 decodes and compares two decoded structs, not hashes.
  • The three measurement_hash() methods (lib.rs:1691, :1962, :2232) re-encode, but their only
    callers are sev_measurement_hash_for_image_dir / tdx_measurement_hash_for_image_dir, driven
    from dstack-mr's CLI over a freshly built image directory — never over an untrusted document.

So the severity is unchanged from the backlog's assessment: no live bypass, and the fix is
cheap-and-bounded hardening that removes the trap before the first measurement_hash()-vs-sha256sum
comparison creates one.

Fix. Decode through a Cursor and reject if cursor.position() != bytes.len().

Compat. None. Every document this codebase produces is exactly one CBOR item.


4. MrConfigV3::version defaulted to 3

dstack-types/src/mr_config.rs:109 carried #[serde(default = "mr_config_v3_version")], so an
absent version silently satisfied the version != 3 gate in validate_mr_config
(dstack-mr/src/sev.rs:960) and verify_mr_config_id
(dstack-util/src/system_setup/config_id_verifier.rs:166).

running 1 test
test mr_config::tests::a_document_without_a_version_is_not_a_v3_document ... FAILED

failures:

---- mr_config::tests::a_document_without_a_version_is_not_a_v3_document stdout ----

thread 'mr_config::tests::a_document_without_a_version_is_not_a_v3_document' (1508414) panicked at dstack-types/src/mr_config.rs:330:10:
a document without a version must not parse as v3: MrConfigV3 { version: 3, app_id: None, compose_hash: [34, 34, ...], gpu_policy_hash: None, key_provider: None, key_provider_id: None, instance_id: None, init_script_hashes: None }

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 56 filtered out; finished in 0.00s

Fix. Drop the default; version is required. The backlog was stale about deny_unknown_fields
— the struct already has it at :107 — and the Option-identity half of this item is closed by
#1264 and was not touched.

Compat. None. to_canonical_json() always emits version (it is not an Option, so
skip_serializing_none cannot drop it), HOST_DATA binds the exact document bytes, and no
non-Rust producer of the document exists (rg mr_config over *.py/*.js/*.ts/*.go is empty).
One sibling test that fed a version-less document was updated to include "version":3.


5. rootfs_hash_from_cmdline took the first match; the initramfs honours the last

dstack-mr/src/sev.rs:839-851 used find_map. No live bug (every caller discards the return value
and uses it as a presence gate), but the function is pub and reads authoritative.

running 1 test
test sev::tests::a_duplicated_rootfs_hash_is_not_silently_resolved ... FAILED

failures:

---- sev::tests::a_duplicated_rootfs_hash_is_not_silently_resolved stdout ----

thread 'sev::tests::a_duplicated_rootfs_hash_is_not_silently_resolved' (1549209) panicked at dstack-mr/src/sev.rs:1591:25:
a duplicated rootfs hash resolved to 1111111111111111111111111111111111111111111111111111111111111111

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 31 filtered out; finished in 0.00s

(The command line in the test names …1111… first and …2222… second. The function answered with
the first; the guest would have mounted the second.)

The judgement call: reject rather than take the last. Taking the last would agree with the
initramfs, which is the argument for it. I rejected it because it leaves a measured command line
asserting two different rootfs identities both accepted and silently resolved — the reader of a
measured cmdline still cannot tell which one the guest used without knowing the rule. The callers
here are validation gates on a command line an untrusted host supplies, no dstack image emits a
duplicate, and the house rule for a shape nothing produces is to fail closed. Cost of rejecting:
zero.

Compat. None for any image dstack builds. A hand-rolled command line with two
dstack.rootfs_hash= values now fails validate_measurement_input instead of resolving to the
first.


6. AwsOsImageMeasurement had no version

dstack-types/src/lib.rs:1743-1791 encoded its public struct directly, unlike Tdx/Sev/Gcp,
each of which encodes a Cbor* mirror whose first field is version and is checked on decode.
A future v2 AWS document would have decoded as a v1 one.

running 1 test
test cbor_canonicalization_tests::the_aws_measurement_document_names_and_checks_its_version ... FAILED

failures:

---- cbor_canonicalization_tests::the_aws_measurement_document_names_and_checks_its_version stdout ----

thread 'cbor_canonicalization_tests::the_aws_measurement_document_names_and_checks_its_version' (1585812) panicked at dstack-types/src/lib.rs:2965:14:
measurement.aws.cbor names a version, like its three siblings

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 57 filtered out; finished in 0.00s

Fix. CborAwsOsImageMeasurement { version, boot_pcr_digest } + VERSION: u32 = 1 + a version
gate in from_cbor_slice, matching CborGcpOsImageMeasurement field for field.
cbor_json_value_from_slice added for parity, so dstack-mr inspect-measurement aws now reports the
version like the other three kinds. AWS_MEASUREMENT_FILENAME moved to dstack-types next to its
three siblings and is used by AwsOsImageMeasurementDocument::verify and by
vmm/src/app/image.rs, which had a private duplicate.

Refutation, with the check that produced it: the deny_unknown_fields half of this item does not
hold.
The backlog says AWS lacks it "unlike its three siblings". grep -n deny_unknown_fields dstack/dstack-types/src/lib.rs returns lines 414, 453, 1251, 1260 — and none of
CborTdxOsImageMeasurement, CborSevOsImageMeasurement, CborGcpOsImageMeasurement is among them.
All four are consistent already. I did not add it to any of them, for a reason beyond consistency:
the version check runs after the full decode, so deny_unknown_fields would make a future v2
document fail with unknown field ... instead of TdxOsImageMeasurement: unsupported version 2, expected 1 — replacing the diagnostic the version field exists to give (see the existing
unknown_versions_are_rejected test and the deliberately worded error at lib.rs:2206). The
security value would be nil in any case: the raw bytes are hashed into os_image_hash, which is the
same reason item 3's trailing bytes are not exploitable today. The refutation is recorded durably as
a doc comment on CborAwsOsImageMeasurement, not only here.

Compat — the one thing in this PR that moves bytes, please read. Adding version changes the
encoding of measurement.aws.cbor, which changes its sha256sum.txt entry, which changes
os_image_hash for AWS NitroTPM UKI images. It does not touch the SEV/TDX/GCP golden vectors
(all pass unchanged). No AWS fixture or pinned AWS os_image_hash exists in the tree, and
os/image/assemble.sh regenerates the file via dstack-mr aws-measurement-cbor on every build, so
the cost is a rebuild and a re-pin. Doing it now is the cheap moment; once a v2 exists it becomes a
real compat event with no way to tell v1 from v2.


7. sev_snp_mr_config turned malformed into None

dstack-attest/src/v1.rs:158-161, .and_then(|d| MrConfigV3::from_document(d).ok()).

Confirmed zero callers repo-wide before deleting:

$ rg -n 'sev_snp_mr_config\b' --glob '!target' --glob '!*.lock' .
./dstack/dstack-attest/src/v1.rs:158:    pub fn sev_snp_mr_config(&self) -> Option<MrConfigV3> {

Fix. Deleted, along with the now-unused MrConfigV3 import. sev_snp_mr_config_document()
which returns the raw document and is what the real paths use — is unchanged. No behavioural test:
the compiler is the proof that nothing called it.

Compat. dstack-attest is an internal crate; removing an accessor nothing calls is invisible.


8. parse_amd_snp_report invited misuse

sev-snp-qvl/src/lib.rs:424. Renamed to parse_unverified_amd_snp_report with a doc comment saying
what it does not check. Both call sites updated:

  • dstack-util/src/system_setup/config_id_verifier.rs:51 — the guest reading its own report.
  • dstack-attest/src/attestation.rs:1660decode_app_info_sev_snp, where the signature is
    established by the caller.

Checked the SDKs: rg parse_amd_snp_report over the whole tree returns only those two plus the
definition; no Python/Go/JS binding exposes it. No behavioural test — this is a rename, and the
compiler is the proof the call sites are complete.

Compat. sev-snp-qvl is an internal crate (no dstack- prefix, not published), so no alias is
warranted.


Verification

cargo test -p dstack-mr -p dstack-types -p tpm-qvl -p dstack-attest -p sev-snp-qvl -p dstack-verifier   # pass
cargo test --workspace                                                                                  # 123 binaries, all ok
cargo clippy -- -D warnings --allow unused_variables                                                    # clean
cargo fmt --check                                                                                        # clean

cargo clippy --all-targets -- -D warnings --allow unused_variables does not pass, and does not
pass on next either. The failures are three clippy::type_complexity on
Vec<(&str, fn(&mut MeasurementInput))> test tables in dstack-mr/src/sev.rs (present on next at
lines 1380/1403/1563) and three items after a test module in tee-simulator/src/tpm.rs:662,
ra-rpc/src/client.rs:185 and dstack-attest/src/lib.rs:141 — all in files this PR does not touch.
The gate the repo documents (cargo clippy -- -D warnings --allow unused_variables, lib targets) is
clean.

Golden vectors did not move. sev::tests::measurement_vector_does_not_drift,
sev::tests::real_fixture_recomputes_measurement and the inline TDX lite vectors
(tdx::tests::rtmr2_replay_is_stable, rtmr2_command_line_event_digest_is_stable,
tdx_measurement_document_cbor_is_stable) all pass unchanged. The only encoding this PR changes is
measurement.aws.cbor (item 6), which has no golden vector.

Test-merges against the open PRs on these files

PR Branch Result
#1267 fix/tpm-replay-and-diagnose one conflict, in dstack/tpm-qvl/src/verify.rs
#1248 fix/parser-input-bounds clean; merged tests pass
#1251 fix/verifier-measurement-fidelity clean; merged tests pass
#1266 fix/verifier-result-contract clean; merged tests pass

#1267 — the functional hunks compose; only the test modules collide. #1267 inserts its
duplicate-PCR-index gate at verify.rs:97, before the bank check; this PR inserts the per-PCR
bank/length check after it. Git merges both with no conflict, and the ordering is the right one —
the duplicate-index gate runs first, so the bounded-work argument in #1267 still holds.

The sole conflict is add/add: neither next nor either branch has a #[cfg(test)] mod tests in
verify.rs, and both branches append one at EOF.

Recommended resolution (applied and verified locally): keep #1267's module as the outer one,
widen its import to use crate::{QuoteCollateral, GCP_ROOT_CA};, and append this PR's four items
(attest_message_for_bank, quote_of, empty_collateral, and the two tests) inside it. The only
real collision is the helper name attest_message, which the two branches give different signatures
— rename this PR's to attest_message_for_bank and nothing else changes. I resolved it that way in a
scratch merge:

test verify::tests::rejects_pcr_values_whose_lengths_do_not_match_the_attested_bank ... ok
test verify::tests::rejects_pcr_values_that_misname_the_attested_bank ... ok
test verify::tests::events_for_pcrs_outside_the_quote_are_ignored ... ok
test verify::tests::duplicate_pcr_indices_do_not_make_the_replay_quadratic ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.05s

Whichever of the two lands second should take the rename; there is nothing else to decide.

#1248 touches sev-snp-qvl/src/lib.rs at lines 27, 934 and 1160 (certificate-table bounds);
this PR's rename is at 424 plus two call sites. No overlap. #1248 and #1251 both rework
sha256sum_entry_hash/verify_measurement_material around dstack-types/src/lib.rs:1545-1650 and
will conflict with each other; this PR's edits there are at cbor_from_slice (1527) and in the
AWS block (1743+), clear of both.

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

Labels

P0 Highest priority: review or decide before anything else in the audit batch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant