Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 thanit 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 thecanonicalization/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 fixexisted. The pre-fix output is quoted verbatim.
1.
guest_featureswas only checked!= 0, and a DebugSwap guest passes verification todaydstack-mr/src/sev.rs:114bailed only on zero, and:737-738fed the value into both VMSA pages.guest_featuresis the one launch parameter that is neither pinned bySevOsImageMeasurementnorconstrained by
validate_measurement_input: it rides in the host-writtenSnpMeasurementDocumentnext to
vcpus/vcpu_type, outside the CBOR thatos_image_hashcommits to, and it lands verbatimat 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.
Fix.
ALLOWED_GUEST_FEATURES = SEV_FEATURE_SNP_ACTIVE(bit 0).validate_measurement_inputrequires 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_FEATUREStable, which the SEV-SNP ABI'sGUEST_FEATURESfield inSNP_LAUNCH_STARTmirrors: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:It sets no feature property, so KVM starts the guest with
SNPActivealone, anddstack/vmm/src/app.rs:2244writes the matchingguest_features: 1into the measurement document.1is the only value dstack has ever emitted; the captured real-hardware vector insev.rs'sREAL_MEASUREMENT_DOCcarries"guest_features":1too.Alternatives rejected:
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
MeasurementInputis what the KMSrecomputes 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.
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_inputsusedto tamper
guest_features = 3and assert the measurement comparison caught it. The allowlist nowrejects 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'srejects_unsafe_machine_configgained a DebugSwap case alongside its updatedzero case.
2.
compute_pcr_digesthad no per-PCR length check, andPcrValue.algorithmwas never validatedtpm-qvl/src/verify.rs:307-314.compute_pcr_digestconcatenates the quoted PCR values with nothingmarking 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 andneither was used.
The length check is not cosmetic. Given a genuine, AK-signed
pcr_digestoverPCR4 || PCR7, acaller 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 handsout a value the TPM never held. Both tests below reach the AK certificate on
next— i.e. bothforgeries pass every PCR gate, and in a real attack the certificate is genuine:
Fix. After the existing bank gate, every
PcrValuemust carryalgorithm == "sha256"and a32-byte value. Placed at the call site rather than inside
compute_pcr_digestso the hunk does notoverlap #1267's (see the merge report below).
Compat. None.
tpm-attestsetsalgorithmfrom the selection's bank name and the bank gateabove 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_cabails forPlatform::AwsEc2.3. CBOR decode accepted trailing bytes
cbor_from_sliceatdstack-types/src/lib.rs:1527-1533stopped at the end of the first item andignored the rest.
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 rawdocument.measurementbytes against thesha256sum.txtentry; trailing bytes therefore ride along inos_image_hashand produce an imageidentity 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:1061—SevOsImageMeasurementDocument::verify(&os_image_hash)runsbefore
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:2215decodes and compares two decoded structs, not hashes.measurement_hash()methods (lib.rs:1691,:1962,:2232) re-encode, but their onlycallers are
sev_measurement_hash_for_image_dir/tdx_measurement_hash_for_image_dir, drivenfrom
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-sha256sumcomparison creates one.
Fix. Decode through a
Cursorand reject ifcursor.position() != bytes.len().Compat. None. Every document this codebase produces is exactly one CBOR item.
4.
MrConfigV3::versiondefaulted to 3dstack-types/src/mr_config.rs:109carried#[serde(default = "mr_config_v3_version")], so anabsent
versionsilently satisfied theversion != 3gate invalidate_mr_config(
dstack-mr/src/sev.rs:960) andverify_mr_config_id(
dstack-util/src/system_setup/config_id_verifier.rs:166).Fix. Drop the default;
versionis required. The backlog was stale aboutdeny_unknown_fields— the struct already has it at
:107— and theOption-identity half of this item is closed by#1264 and was not touched.
Compat. None.
to_canonical_json()always emitsversion(it is not anOption, soskip_serializing_nonecannot drop it), HOST_DATA binds the exact document bytes, and nonon-Rust producer of the document exists (
rg mr_configover*.py/*.js/*.ts/*.gois empty).One sibling test that fed a version-less document was updated to include
"version":3.5.
rootfs_hash_from_cmdlinetook the first match; the initramfs honours the lastdstack-mr/src/sev.rs:839-851usedfind_map. No live bug (every caller discards the return valueand uses it as a presence gate), but the function is
puband reads authoritative.(The command line in the test names
…1111…first and…2222…second. The function answered withthe 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 failsvalidate_measurement_inputinstead of resolving to thefirst.
6.
AwsOsImageMeasurementhad noversiondstack-types/src/lib.rs:1743-1791encoded its public struct directly, unlikeTdx/Sev/Gcp,each of which encodes a
Cbor*mirror whose first field isversionand is checked on decode.A future v2 AWS document would have decoded as a v1 one.
Fix.
CborAwsOsImageMeasurement { version, boot_pcr_digest }+VERSION: u32 = 1+ a versiongate in
from_cbor_slice, matchingCborGcpOsImageMeasurementfield for field.cbor_json_value_from_sliceadded for parity, sodstack-mr inspect-measurement awsnow reports theversion like the other three kinds.
AWS_MEASUREMENT_FILENAMEmoved todstack-typesnext to itsthree siblings and is used by
AwsOsImageMeasurementDocument::verifyand byvmm/src/app/image.rs, which had a private duplicate.Refutation, with the check that produced it: the
deny_unknown_fieldshalf of this item does nothold. The backlog says AWS lacks it "unlike its three siblings".
grep -n deny_unknown_fields dstack/dstack-types/src/lib.rsreturns lines 414, 453, 1251, 1260 — and none ofCborTdxOsImageMeasurement,CborSevOsImageMeasurement,CborGcpOsImageMeasurementis 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_fieldswould make a future v2document fail with
unknown field ...instead ofTdxOsImageMeasurement: unsupported version 2, expected 1— replacing the diagnostic the version field exists to give (see the existingunknown_versions_are_rejectedtest and the deliberately worded error atlib.rs:2206). Thesecurity value would be nil in any case: the raw bytes are hashed into
os_image_hash, which is thesame 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
versionchanges theencoding of
measurement.aws.cbor, which changes itssha256sum.txtentry, which changesos_image_hashfor 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_hashexists in the tree, andos/image/assemble.shregenerates the file viadstack-mr aws-measurement-cboron every build, sothe 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_configturned malformed intoNonedstack-attest/src/v1.rs:158-161,.and_then(|d| MrConfigV3::from_document(d).ok()).Confirmed zero callers repo-wide before deleting:
Fix. Deleted, along with the now-unused
MrConfigV3import.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-attestis an internal crate; removing an accessor nothing calls is invisible.8.
parse_amd_snp_reportinvited misusesev-snp-qvl/src/lib.rs:424. Renamed toparse_unverified_amd_snp_reportwith a doc comment sayingwhat 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:1660—decode_app_info_sev_snp, where the signature isestablished by the caller.
Checked the SDKs:
rg parse_amd_snp_reportover the whole tree returns only those two plus thedefinition; 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-qvlis an internal crate (nodstack-prefix, not published), so no alias iswarranted.
Verification
cargo clippy --all-targets -- -D warnings --allow unused_variablesdoes not pass, and does notpass on
nexteither. The failures are threeclippy::type_complexityonVec<(&str, fn(&mut MeasurementInput))>test tables indstack-mr/src/sev.rs(present onnextatlines 1380/1403/1563) and three
items after a test moduleintee-simulator/src/tpm.rs:662,ra-rpc/src/client.rs:185anddstack-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) isclean.
Golden vectors did not move.
sev::tests::measurement_vector_does_not_drift,sev::tests::real_fixture_recomputes_measurementand 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 ismeasurement.aws.cbor(item 6), which has no golden vector.Test-merges against the open PRs on these files
fix/tpm-replay-and-diagnosedstack/tpm-qvl/src/verify.rsfix/parser-input-boundsfix/verifier-measurement-fidelityfix/verifier-result-contract#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-PCRbank/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
nextnor either branch has a#[cfg(test)] mod testsinverify.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 onlyreal collision is the helper name
attest_message, which the two branches give different signatures— rename this PR's to
attest_message_for_bankand nothing else changes. I resolved it that way in ascratch merge:
Whichever of the two lands second should take the rename; there is nothing else to decide.
#1248 touches
sev-snp-qvl/src/lib.rsat 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_materialarounddstack-types/src/lib.rs:1545-1650andwill conflict with each other; this PR's edits there are at
cbor_from_slice(1527) and in theAWS block (1743+), clear of both.