From e8a4a03265101e2f376ff437a7edde673e17d8cb Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Sun, 20 Sep 2026 08:17:56 -0700 Subject: [PATCH 1/2] fix(attest): refuse a TEE variant the guest's own hardware contradicts --- dstack/dstack-attest/src/attestation.rs | 198 ++++++++++++++++++++++-- 1 file changed, 186 insertions(+), 12 deletions(-) diff --git a/dstack/dstack-attest/src/attestation.rs b/dstack/dstack-attest/src/attestation.rs index e55d63827..428a3a0fa 100644 --- a/dstack/dstack-attest/src/attestation.rs +++ b/dstack/dstack-attest/src/attestation.rs @@ -475,27 +475,75 @@ fn choose_dstack_tee_variant(has_tdx: bool, has_sev_snp: bool) -> Result Result { - let has_tdx = tdx_attest::is_tdx_available(); - let has_sev_snp = std::path::Path::new("/dev/sev-guest").exists() || has_sev_snp_tsm_provider(); +/// The TEE devices this guest actually exposes. +/// +/// Every field is a device or driver ABI the platform presents to the guest. +/// None of them is a DMI string, which is the point: see [`resolve_tee_variant`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct TeeDevices { + has_tdx: bool, + has_sev_snp: bool, + has_nsm: bool, + has_tpm: bool, +} + +impl TeeDevices { + fn detect() -> Self { + Self { + has_tdx: tdx_attest::is_tdx_available(), + has_sev_snp: std::path::Path::new("/dev/sev-guest").exists() + || has_sev_snp_tsm_provider(), + has_nsm: std::path::Path::new("/dev/nsm").exists(), + has_tpm: std::path::Path::new("/dev/tpmrm0").exists() + || std::path::Path::new("/dev/tpm0").exists(), + } + } +} - // First, try to detect platform from DMI product name - let platform = Platform::detect_or_dstack(); +/// Resolve the attestation variant from the platform hint and the devices the +/// guest can actually see. +/// +/// `Platform::detect` reads `/sys/class/dmi/id`, i.e. SMBIOS type 1. QEMU takes +/// those strings from host-side configuration (`vmm/src/app/qemu.rs` +/// `configure_smbios`) and no TDX or SEV-SNP measurement covers them, so they +/// are an untrusted host claim, not evidence. +/// +/// That matters because the variant decides whether two guest-side checks run +/// at all. `DstackNitroEnclave` has no TDX RTMR lane and no TPM PCR lane +/// (`TeeVariant::has_tdx`, `TeeVariant::tpm_event_pcr_and_bank`), so +/// `emit_runtime_event` measures nothing and still returns `Ok`; and +/// `verify_mr_config_id` returns `Ok` for it without reading MR_CONFIG_ID at +/// all. A host that could name the variant could therefore turn both off on +/// hardware that supports them. +/// +/// So the hint may only select among variants the hardware corroborates: each +/// arm below requires the device ABI its evidence comes from, and a hint that +/// contradicts a confidential-computing device that *is* present is refused +/// rather than believed. +fn resolve_tee_variant(platform: Platform, devices: TeeDevices) -> Result { match platform { - Platform::Dstack => choose_dstack_tee_variant(has_tdx, has_sev_snp), + Platform::Dstack => choose_dstack_tee_variant(devices.has_tdx, devices.has_sev_snp), Platform::Gcp => { // GCP platform: TDX + TPM dual mode - if has_tdx { + if devices.has_tdx { return Ok(TeeVariant::DstackGcpTdx); } bail!("Unsupported platform: GCP(-tdx)"); } - Platform::NitroEnclave => Ok(TeeVariant::DstackNitroEnclave), + Platform::NitroEnclave => { + if devices.has_tdx || devices.has_sev_snp { + bail!( + "refusing the Nitro Enclave platform claim: this guest exposes a {} device, and the Nitro Enclave variant measures no runtime event", + if devices.has_tdx { "TDX" } else { "SEV-SNP" } + ); + } + if !devices.has_nsm { + bail!("unsupported platform: Nitro Enclave without /dev/nsm"); + } + Ok(TeeVariant::DstackNitroEnclave) + } Platform::AwsEc2 => { - if std::path::Path::new("/dev/tpmrm0").exists() - || std::path::Path::new("/dev/tpm0").exists() - { + if devices.has_tpm { return Ok(TeeVariant::DstackAwsNitroTpm); } bail!("unsupported platform: AWS EC2 without NitroTPM"); @@ -503,6 +551,132 @@ pub fn detect_tee_variant() -> Result { } } +/// Detect the attestation variant exposed by the current guest environment. +pub fn detect_tee_variant() -> Result { + // The DMI platform is only a hint; `resolve_tee_variant` says why. + resolve_tee_variant(Platform::detect_or_dstack(), TeeDevices::detect()) +} + +#[cfg(test)] +mod tee_variant_resolution_tests { + use super::*; + + const NOTHING: TeeDevices = TeeDevices { + has_tdx: false, + has_sev_snp: false, + has_nsm: false, + has_tpm: false, + }; + + #[test] + fn a_nitro_enclave_claim_needs_the_nsm_device() { + let err = resolve_tee_variant(Platform::NitroEnclave, NOTHING) + .expect_err("a Nitro Enclave claim with no NSM device must be refused"); + assert!(err.to_string().contains("/dev/nsm"), "{err}"); + + assert_eq!( + resolve_tee_variant( + Platform::NitroEnclave, + TeeDevices { + has_nsm: true, + ..NOTHING + } + ) + .unwrap(), + TeeVariant::DstackNitroEnclave + ); + } + + #[test] + fn a_nitro_enclave_claim_does_not_downgrade_tdx_or_sev_snp_hardware() { + // The variant this claim selects measures no runtime event and skips + // verify_mr_config_id, so believing it on TDX or SEV-SNP hardware would + // let the host turn both off with an SMBIOS string. + for devices in [ + TeeDevices { + has_tdx: true, + ..NOTHING + }, + TeeDevices { + has_sev_snp: true, + ..NOTHING + }, + TeeDevices { + has_tdx: true, + has_nsm: true, + ..NOTHING + }, + ] { + let err = resolve_tee_variant(Platform::NitroEnclave, devices) + .expect_err("a Nitro Enclave claim must not override present TEE hardware"); + assert!(err.to_string().contains("refusing"), "{err}"); + } + } + + #[test] + fn hardware_backed_platforms_still_resolve() { + let tdx = TeeDevices { + has_tdx: true, + ..NOTHING + }; + assert_eq!( + resolve_tee_variant(Platform::Dstack, tdx).unwrap(), + TeeVariant::DstackTdx + ); + assert_eq!( + resolve_tee_variant(Platform::Gcp, tdx).unwrap(), + TeeVariant::DstackGcpTdx + ); + assert_eq!( + resolve_tee_variant( + Platform::Dstack, + TeeDevices { + has_sev_snp: true, + ..NOTHING + } + ) + .unwrap(), + TeeVariant::DstackAmdSevSnp + ); + assert_eq!( + resolve_tee_variant( + Platform::AwsEc2, + TeeDevices { + has_tpm: true, + ..NOTHING + } + ) + .unwrap(), + TeeVariant::DstackAwsNitroTpm + ); + assert!(resolve_tee_variant(Platform::Gcp, NOTHING).is_err()); + assert!(resolve_tee_variant(Platform::AwsEc2, NOTHING).is_err()); + assert!(resolve_tee_variant(Platform::Dstack, NOTHING).is_err()); + } + + /// Reproduces the host-supplied-DMI path end to end. + /// + /// Passes trivially on a normal machine. To see what it guards, run it with + /// the SMBIOS product name a host controls bound over the real one: + /// + /// ```text + /// printf 'Nitro Enclave\n' > /tmp/fake_product_name + /// unshare -Urm sh -c 'mount --bind /tmp/fake_product_name \ + /// /sys/class/dmi/id/product_name && cargo test -p dstack-attest --lib \ + /// a_dmi_string_alone_cannot_select_the_nitro_enclave_variant' + /// ``` + #[test] + fn a_dmi_string_alone_cannot_select_the_nitro_enclave_variant() { + if std::path::Path::new("/dev/nsm").exists() { + return; + } + assert!( + !matches!(detect_tee_variant(), Ok(TeeVariant::DstackNitroEnclave)), + "DMI alone selected DstackNitroEnclave with no NSM device present" + ); + } +} + /// The content type of a quote. A CVM should only generate quotes for these types. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum QuoteContentType<'a> { From 791e06f6ba69e5fef27711e128a7e5bf1b323fe3 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Sun, 20 Sep 2026 08:17:56 -0700 Subject: [PATCH 2/2] fix(dstack-util): bound the host-shared copy by what it reads, not by a prior stat --- dstack/dstack-util/src/system_setup.rs | 154 ++++++++++++++++++++++--- 1 file changed, 139 insertions(+), 15 deletions(-) diff --git a/dstack/dstack-util/src/system_setup.rs b/dstack/dstack-util/src/system_setup.rs index 66839b4fa..19717ce5f 100644 --- a/dstack/dstack-util/src/system_setup.rs +++ b/dstack/dstack-util/src/system_setup.rs @@ -247,6 +247,51 @@ struct HostShared { instance_info: InstanceInfo, } +/// Copy one host-supplied file into the guest's own copy of `.host-shared`. +/// +/// The source lives on a mount the host backs and can change under us: 9p is +/// the default `cvm.host_share_mode`, so the host serves every read live and +/// decides what kind of file it is serving. Three properties follow, and each +/// one is a guard here rather than a check on the path: +/// +/// - `O_NOFOLLOW | O_NONBLOCK` and a regular-file check on the *open +/// descriptor*. `Path::metadata` would describe whatever the path named a +/// moment ago; the descriptor is what we then read. `O_NONBLOCK` is what +/// makes the check reachable at all: opening a FIFO without it blocks until +/// a writer appears, which is a boot that never finishes rather than one +/// that fails. +/// - the size limit is applied to the *copy*, not to a prior `stat`. A +/// stat-then-copy pair is advisory against a live backing store, and against +/// a FIFO or a character device it is not a limit at all: both stat as zero +/// bytes and then read forever into the guest's tmpfs. +/// - reading exactly one byte past the limit is what distinguishes "fits" from +/// "was truncated", so an over-long file is refused instead of silently +/// arriving as its first `max_size` bytes. +fn copy_host_shared_file(src_path: &Path, dst_path: &Path, max_size: u64) -> Result<()> { + use fs::os::unix::fs::OpenOptionsExt; + use std::io::Read; + + let src_io = fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) + .open(src_path)?; + let file_type = src_io.metadata()?.file_type(); + if !file_type.is_file() { + bail!("Source file is not a regular file: {file_type:?}"); + } + let mut src_io = src_io.take(max_size.saturating_add(1)); + let mut dst_io = fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .open(dst_path)?; + let copied = std::io::copy(&mut src_io, &mut dst_io)?; + if copied > max_size { + bail!("Source file is too large, max size is {max_size} bytes"); + } + Ok(()) +} + impl HostShared { fn load(host_shared_dir: impl Into) -> Result { let host_shared_dir = host_shared_dir.into(); @@ -281,21 +326,8 @@ impl HostShared { } bail!("Source file {src} does not exist"); } - let src_size = src_path.metadata()?.len(); - if src_size > max_size { - bail!("Source file {src} is too large, max size is {max_size} bytes"); - } - use fs::os::unix::fs::OpenOptionsExt; - let mut src_io = fs::OpenOptions::new() - .read(true) - .custom_flags(libc::O_NOFOLLOW) - .open(src_path)?; - let mut dst_io = fs::OpenOptions::new() - .write(true) - .create(true) - .open(dst_path)?; - std::io::copy(&mut src_io, &mut dst_io)?; - Ok(()) + copy_host_shared_file(&src_path, &dst_path, max_size) + .with_context(|| format!("Failed to copy host-shared file {src}")) }; info!("Mounting host-shared"); mount_host_shared(host_shared_dir)?; @@ -4242,3 +4274,95 @@ Endpoint = [2001:db8::1]:51822 assert!(wireguard_endpoint_hosts("Endpoint = missing-port").is_err()); } } + +#[cfg(test)] +mod host_shared_copy_tests { + use super::*; + use std::os::unix::ffi::OsStrExt as _; + + fn temp_dir(name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("dstack-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + dir + } + + #[test] + fn a_regular_file_within_the_limit_is_copied() { + let dir = temp_dir("host-shared-ok"); + let src = dir.join("src"); + let dst = dir.join("dst"); + std::fs::write(&src, b"hello").unwrap(); + copy_host_shared_file(&src, &dst, 1024).unwrap(); + assert_eq!(std::fs::read(&dst).unwrap(), b"hello"); + let _ = std::fs::remove_dir_all(dir); + } + + #[test] + fn a_file_over_the_limit_is_refused_rather_than_truncated() { + let dir = temp_dir("host-shared-big"); + let src = dir.join("src"); + let dst = dir.join("dst"); + std::fs::write(&src, vec![b'a'; 64]).unwrap(); + let err = copy_host_shared_file(&src, &dst, 16).unwrap_err(); + assert!(err.to_string().contains("too large"), "{err}"); + let _ = std::fs::remove_dir_all(dir); + } + + /// A FIFO is what a 9p host share can name where a file is expected. It + /// stats as zero bytes, so a stat-then-copy pair reads it without any + /// limit, and opening it without `O_NONBLOCK` blocks until a writer shows + /// up -- a boot that hangs instead of one that fails. + #[test] + fn a_fifo_is_refused_without_blocking_or_reading_it() { + let dir = temp_dir("host-shared-fifo"); + let src = dir.join("src"); + let dst = dir.join("dst"); + let path = std::ffi::CString::new(src.as_os_str().as_bytes()).unwrap(); + // SAFETY: FFI call with a NUL-terminated path this test owns. + assert_eq!(unsafe { libc::mkfifo(path.as_ptr(), 0o600) }, 0); + + assert_eq!( + std::fs::metadata(&src).unwrap().len(), + 0, + "a FIFO stats as zero bytes, which is why the old size check passed it" + ); + + let (tx, rx) = std::sync::mpsc::channel(); + let src_for_thread = src.clone(); + let dst_for_thread = dst.clone(); + std::thread::spawn(move || { + let result = copy_host_shared_file(&src_for_thread, &dst_for_thread, 32); + let _ = tx.send(result.is_err()); + }); + // A writer, so an implementation that blocks on open gets past it and + // then streams without a limit. Best-effort: nothing must depend on it. + let src_for_writer = src.clone(); + std::thread::spawn(move || { + if let Ok(mut fifo) = std::fs::OpenOptions::new().write(true).open(src_for_writer) { + loop { + if fifo.write_all(&[b'a'; 4096]).is_err() { + break; + } + } + } + }); + + let refused = rx.recv_timeout(std::time::Duration::from_secs(10)).expect( + "the copy neither finished nor failed within 10s: it is blocked on the \ + FIFO open, or reading it with no limit", + ); + assert!(refused, "a FIFO must be refused, not read"); + let _ = std::fs::remove_dir_all(dir); + } + + #[test] + fn a_symlink_is_still_refused() { + let dir = temp_dir("host-shared-symlink"); + let src = dir.join("src"); + let dst = dir.join("dst"); + std::os::unix::fs::symlink("/etc/hostname", &src).unwrap(); + assert!(copy_host_shared_file(&src, &dst, 1024).is_err()); + let _ = std::fs::remove_dir_all(dir); + } +}