From 3a243fd524fb23d37bd1d4f3f104764fd3e15733 Mon Sep 17 00:00:00 2001 From: Michal Tarnacki Date: Tue, 7 Jul 2026 09:51:14 +0200 Subject: [PATCH] fix(migtd): input validation and robustness from security audit - clamp pre-session payload length before alloc - parse_events safe indexing on peer event data - log_max_level/logarea header TOCTOU clamp - receive_pre_session_data treat n==0 as EOF - schedule_timeout validate CPUID.15H.EAX!=0 - pci::find_device propagate init failure gracefully - get_ccel/event_log_slice validate CCEL fields - VmcallServiceResponse::try_read truncate to length Signed-off-by: Michal Tarnacki Co-authored-by: GitHub Copilot --- src/migtd/src/driver/serial.rs | 3 +- src/migtd/src/driver/timer.rs | 9 ++++++ src/migtd/src/driver/vsock.rs | 3 +- src/migtd/src/event_log.rs | 33 +++++++++++---------- src/migtd/src/migration/data.rs | 4 ++- src/migtd/src/migration/logging.rs | 20 ++++++++++--- src/migtd/src/migration/pre_session_data.rs | 13 ++++++++ src/migtd/src/migration/rebinding.rs | 3 ++ src/migtd/src/ratls/server_client.rs | 22 +++++++++++++- 9 files changed, 87 insertions(+), 23 deletions(-) diff --git a/src/migtd/src/driver/serial.rs b/src/migtd/src/driver/serial.rs index b2e541478..38ff15d6a 100644 --- a/src/migtd/src/driver/serial.rs +++ b/src/migtd/src/driver/serial.rs @@ -54,7 +54,8 @@ pub fn virtio_serial_device_init() { pci::init_mmio(); // Enumerate the virtio device - let (_b, dev, _f) = pci::find_device(VIRTIO_PCI_VENDOR_ID, VIRTIO_PCI_DEVICE_ID).unwrap(); + let (_b, dev, _f) = pci::find_device(VIRTIO_PCI_VENDOR_ID, VIRTIO_PCI_DEVICE_ID) + .expect("Failed to find virtio-serial PCI device"); let pci_device = pci::PciDevice::new(0, dev, 0); diff --git a/src/migtd/src/driver/timer.rs b/src/migtd/src/driver/timer.rs index 856f92665..74f509d58 100644 --- a/src/migtd/src/driver/timer.rs +++ b/src/migtd/src/driver/timer.rs @@ -56,6 +56,15 @@ pub fn init_timer() { pub fn schedule_timeout(timeout: u32) -> Option { reset_timer(); let cpuid = unsafe { core::arch::x86_64::__cpuid_count(0x15, 0) }; + if cpuid.eax == 0 || cpuid.ebx == 0 || cpuid.ecx == 0 { + log::error!( + "schedule_timeout: CPUID.15H returned invalid values (eax={}, ebx={}, ecx={})\n", + cpuid.eax, + cpuid.ebx, + cpuid.ecx + ); + return None; + } let tsc_frequency = cpuid.ecx * (cpuid.ebx / cpuid.eax); let deadline = (tsc_frequency / 1000) as u64 * timeout as u64; diff --git a/src/migtd/src/driver/vsock.rs b/src/migtd/src/driver/vsock.rs index 2ad92b38b..e3a6f28e6 100644 --- a/src/migtd/src/driver/vsock.rs +++ b/src/migtd/src/driver/vsock.rs @@ -39,7 +39,8 @@ pub fn virtio_vsock_device_init() { pci::init_mmio(); // Enumerate the virtio device - let (_b, dev, _f) = pci::find_device(VIRTIO_PCI_VENDOR_ID, VIRTIO_PCI_DEVICE_ID).unwrap(); + let (_b, dev, _f) = pci::find_device(VIRTIO_PCI_VENDOR_ID, VIRTIO_PCI_DEVICE_ID) + .expect("Failed to find virtio-vsock PCI device"); let pci_device = pci::PciDevice::new(0, dev, 0); diff --git a/src/migtd/src/event_log.rs b/src/migtd/src/event_log.rs index 0b2c0928c..7793d56e6 100644 --- a/src/migtd/src/event_log.rs +++ b/src/migtd/src/event_log.rs @@ -62,19 +62,20 @@ impl TaggedEvent { } pub fn get_event_log_mut() -> Option<&'static mut [u8]> { - get_ccel().map(event_log_slice) + get_ccel().and_then(event_log_slice) } pub fn get_event_log() -> Option<&'static [u8]> { - let raw = get_ccel().map(event_log_slice)?; + let raw = get_ccel().and_then(event_log_slice)?; // The `+1` is required: `cc_measurement::log::CcEvents::next()` only // yields an event when `end_of_event < bytes.len()` (strict inequality). - // If we sliced to exactly `size`, the buffer would end at the last - // event boundary and that final event would be silently dropped by the - // iterator, breaking `parse_events()` (e.g. losing the policy tag) and - // any downstream RTMR replay. The runtime layout has trailing zeros in - // the CCEL area, so including one extra byte is safe. - event_log_size(raw).map(|size| &raw[..size + 1]) + // If we sliced to exactly `size`, the buffer would end at the last event + // boundary and that final event (the MigTdPolicy measurement) would be + // silently dropped by the iterator, breaking `parse_events()` and causing + // `check_policy_integrity()` to fail with `PolicyHashMismatch`. The runtime + // layout has trailing zeros in the CCEL area, so the extra byte is safe; + // clamp to the raw length as a defensive guard. + event_log_size(raw).map(|size| &raw[..core::cmp::min(size + 1, raw.len())]) } fn event_log_size(event_log: &[u8]) -> Option { @@ -90,16 +91,19 @@ fn event_log_size(event_log: &[u8]) -> Option { Some(size) } -fn event_log_slice(ccel: &Ccel) -> &'static mut [u8] { - unsafe { core::slice::from_raw_parts_mut(ccel.lasa as *mut u8, ccel.laml as usize) } +fn event_log_slice(ccel: &Ccel) -> Option<&'static mut [u8]> { + // Validate that lasa and laml are non-zero and laml is reasonable + if ccel.lasa == 0 || ccel.laml == 0 || ccel.laml as usize > 0x10_0000 { + return None; + } + Some(unsafe { core::slice::from_raw_parts_mut(ccel.lasa as *mut u8, ccel.laml as usize) }) } fn get_ccel() -> Option<&'static Ccel> { if !CCEL.is_completed() { // Parse out ACPI tables handoff from firmware and find the event log location let &ccel = get_acpi_tables() - .and_then(|tables| tables.iter().find(|&&t| t[..4] == *b"CCEL")) - .expect("Failed to find CCEL"); + .and_then(|tables| tables.iter().find(|&&t| t.get(..4) == Some(b"CCEL")))?; if ccel.len() < size_of::() { return None; @@ -181,9 +185,8 @@ pub(crate) fn parse_events(event_log: &[u8]) -> Option { - let desc_size = event_data[0] as usize; - let desc = event_data.get(1..1 + desc_size)?; - if desc == PLATFORM_FIRMWARE_BLOB2_PAYLOAD { + let desc_size = *event_data.get(0)? as usize; + if event_data.get(1..1 + desc_size)? == PLATFORM_FIRMWARE_BLOB2_PAYLOAD { map.insert(EventName::MigTdCore, CcEvent::new(event_header, None)); } } diff --git a/src/migtd/src/migration/data.rs b/src/migtd/src/migration/data.rs index 61d2c5ca0..02634f068 100644 --- a/src/migtd/src/migration/data.rs +++ b/src/migtd/src/migration/data.rs @@ -117,7 +117,9 @@ impl<'a> VmcallServiceResponse<'a> { if length < RESPONSE_HEADER_LENGTH || length > data.len() { return None; } - Some(Self { data }) + Some(Self { + data: &data[..length], + }) } pub fn new(response: &'a mut [u8], guid: Guid) -> Option { diff --git a/src/migtd/src/migration/logging.rs b/src/migtd/src/migration/logging.rs index be45b8322..734eef72a 100644 --- a/src/migtd/src/migration/logging.rs +++ b/src/migtd/src/migration/logging.rs @@ -374,12 +374,24 @@ pub fn entrylog(msg: &Vec, loglevel: Level, request_id: u64) { Some(if v == u64::MAX { 1 } else { v + 1 }) }) .unwrap(); - let start_offset: u64 = - u64::from_le_bytes(data_buffer[24..32].try_into().unwrap()); - let end_offset: u64 = - u64::from_le_bytes(data_buffer[32..40].try_into().unwrap()); + // Copy offsets from shared memory via volatile read to prevent TOCTOU + let start_offset: u64 = unsafe { + core::ptr::read_volatile(data_buffer[24..32].as_ptr() as *const u64) + }; + let end_offset: u64 = unsafe { + core::ptr::read_volatile(data_buffer[32..40].as_ptr() as *const u64) + }; let mut currentstartoffset: usize = start_offset as usize; let mut currentendoffset: usize = end_offset as usize; + // Clamp offsets to valid range to prevent VMM-controlled panic + if currentstartoffset < LOGAREABUFFERHEADERSIZE + || currentstartoffset >= PAGE_SIZE + { + currentstartoffset = LOGAREABUFFERHEADERSIZE; + } + if currentendoffset < LOGAREABUFFERHEADERSIZE || currentendoffset >= PAGE_SIZE { + currentendoffset = LOGAREABUFFERHEADERSIZE; + } if currentendoffset + LOGENTRYHEADERSIZE + msg.len() > PAGE_SIZE || currentendoffset < currentstartoffset { diff --git a/src/migtd/src/migration/pre_session_data.rs b/src/migtd/src/migration/pre_session_data.rs index 8603c6ec5..c6c767c2e 100644 --- a/src/migtd/src/migration/pre_session_data.rs +++ b/src/migtd/src/migration/pre_session_data.rs @@ -141,6 +141,10 @@ pub(super) async fn receive_pre_session_data( log::error!("receive_pre_session_data: Network error: {:?}\n", e); MigrationResult::NetworkError })?; + if n == 0 { + log::error!("receive_pre_session_data: EOF (peer closed connection)\n"); + return Err(MigrationResult::NetworkError); + } recvd += n; } Ok(()) @@ -194,6 +198,15 @@ pub(super) async fn receive_pre_session_data_packet MAX_PRE_SESSION_PAYLOAD { + log::error!( + "receive_pre_session_data_packet: payload size {} exceeds max {}\n", + pre_session_data_payload_size, + MAX_PRE_SESSION_PAYLOAD + ); + return Err(MigrationResult::InvalidParameter); + } let mut pre_session_data_payload = vec![0u8; pre_session_data_payload_size]; receive_pre_session_data(transport, &mut pre_session_data_payload) .await diff --git a/src/migtd/src/migration/rebinding.rs b/src/migtd/src/migration/rebinding.rs index 53a75991d..6ab81bca9 100644 --- a/src/migtd/src/migration/rebinding.rs +++ b/src/migtd/src/migration/rebinding.rs @@ -560,6 +560,9 @@ async fn tls_session_read_exact( .read(&mut data[recvd..]) .await .map_err(|_| MigrationResult::NetworkError)?; + if n == 0 { + return Err(MigrationResult::NetworkError); + } recvd += n; } Ok(()) diff --git a/src/migtd/src/ratls/server_client.rs b/src/migtd/src/ratls/server_client.rs index bb7c282d9..708cc914e 100644 --- a/src/migtd/src/ratls/server_client.rs +++ b/src/migtd/src/ratls/server_client.rs @@ -778,6 +778,16 @@ mod verify { cert: &[u8], quote_local: &[u8], ) -> core::result::Result<(), CryptoError> { + // Reject oversized certificates to prevent heap exhaustion from DER parsing + const MAX_CERT_SIZE: usize = 8192; + if cert.len() > MAX_CERT_SIZE { + log::error!( + "Certificate too large: {} bytes (max {})\n", + cert.len(), + MAX_CERT_SIZE + ); + return Err(CryptoError::ParseCertificate); + } let verified_report_local = attestation::verify_quote(quote_local).map_err(|e| { log::error!("Mutual attestation error {:?}.\n", e); CryptoError::TlsVerifyPeerCert(MUTUAL_ATTESTATION_ERROR.to_string()) @@ -1123,7 +1133,17 @@ mod verify { } const PUBLIC_KEY_HASH_SIZE: usize = 48; - let report_data = &verified_report[520..520 + PUBLIC_KEY_HASH_SIZE]; + let report_data = verified_report + .get(520..520 + PUBLIC_KEY_HASH_SIZE) + .ok_or_else(|| { + log::error!( + "verify_public_key: verified_report too short (len={})\n", + verified_report.len() + ); + CryptoError::TlsVerifyPeerCert( + "verified_report too short for public key hash".to_string(), + ) + })?; let digest = digest_sha384(public_key).map_err(|e| { log::error!("Failed to compute SHA384 digest: {:?}\n", e); e