Skip to content

fix(migtd): harden virtqueue, vsock, SPDM and policy against untruste…#935

Open
MichalTarnacki wants to merge 1 commit into
intel:mainfrom
MichalTarnacki:fix/rest
Open

fix(migtd): harden virtqueue, vsock, SPDM and policy against untruste…#935
MichalTarnacki wants to merge 1 commit into
intel:mainfrom
MichalTarnacki:fix/rest

Conversation

@MichalTarnacki

Copy link
Copy Markdown
Contributor
# Assessed Severity Bug Call Chain / Location Fix Classification Disposition Reason
1 High INV-1 violated: VMM shared-mem parsed in-place, not copied to private handle_pre_mig -> vsock_transport_dequeue -> vmcall_service_migtd_receive -> Response::new -> Response::data Copy response[..length] into private Vec immediately after Response::new validation; parse from private copy. Same fix for vmcall_raw process_buffer and session.rs:746. true_positive Both the send and receive poll_fn sanity checks read reply.data()[0], data()[1] and data()[4..12] with no prior length check, so a VMM setting the response length field to 24 makes data() empty and data()[0] panic the guest - a genuine host-triggered denial of service reachable directly from untrusted VMM input on every vmcall-vsock request. The broader "parse-in-place / re-read across polls" framing is weaker (the real payload is copied into a private Vec in recv_packet and bound to the request via mid), but the concrete out-of-bounds panic is a real, exploitable defect
2 Medium MigtdTransport::receive over-reads past frame boundary on stream transport rsp_handle_message -> process_message -> SpdmDeviceIo::receive -> MigtdTransport::receive -> AsyncRead::read First loop must read into &mut buffer[recvd..VMCALL_SPDM_MESSAGE_HEADER_SIZE]; second loop into &mut buffer[recvd..payload_size+12]; return payload_size+12 not recvd. true_positive The first loop reads into &mut buffer[recvd..] (the whole remaining buffer), so on a stream transport a coalesced read can return more than 12 bytes — recvd can already exceed payload_size+12. The second loop then never executes (or under-reads), and it returns recvd, handing the next frame's bytes to spdmlib as trailing garbage and desyncing the stream. Both reads must be capped: header to [recvd..12], payload to [recvd..payload+12], and return payload+12
3 Medium get_tcb_level_by_svn requires exact isvsvn match -> TdIdentity::get_tcb_level_by_svn Use highest level with isvsvn<=svn (floor match) per Intel TCB-Info semantics. true_positive The original exact-match (isvsvn == svn) misuses Intel TCB-Info/Enclave-Identity semantics, where tcbLevels are descending breakpoints and a reported svn must floor-match to the highest level with isvsvn <= svn; treating it as an exact lookup returns None for any svn between or above listed levels, which both rejects valid newer peers (availability) and lets an in-between svn skip the mandatory Revoked deny that it should have floored into (security). The floor-match fix is the correct, spec-conformant behavior and is backward-compatible for exact hits, so the finding is a genuine semantic defect.
4 Medium json_set_signature re-parses+re-serializes via serde_json::Value before embeddin -> json_set_signature Sign raw input bytes; embed signature without re-serializing the data block. weakness The behavior is real, but it is fail-closed and only affects tooling: canonicalization is required so the same JSON always maps to the same bytes before signing/embedding. With the guard in json_sign_detached, non-canonical input is rejected early, and verification uses the exact embedded bytes.
5 Low check-then-act across two separate spin::Mutex lock scopes -> wait_for_request Hold single lock across contains+insert; or use BTreeSet::insert return value. weakness There is one global EXECUTOR driven by a single poll_tasks() loop on the main thread, and the up-to-12 "concurrent" sessions are merely interleaved at .await points; since there is no .await between contains() and insert(), the whole window runs inside one task poll and no other session can interleave. REQUESTS is TD-private memory and request_id comes from the already-snapshotted private_mem, so the VMM cannot race or influence it.
6 Medium recycle_descriptors chain-walk via shared desc.next/flags -> recycle never-submitted slots, leak in-flight VirtioVsock::pop_used_rx -> VirtQueue::pop_used -> VirtQueue::recycle_descriptors Store the chain (head,len,next-links) in private memory at add() time and replay from there during recycle; ignore shared desc.next/flags. weakness The mechanism is real: recycle_descriptors walks the chain by reading host-writable desc.next/desc.flags from shared DMA, and the chain head comes from the host-controlled used-ring id, so the VMM can redirect the walk to never-submitted slots, drift num_used/free_head, and leak descriptors until I/O stalls. But it is not an enforceable security violation in the TDX threat model: the index is always bounds-checked via get_mut, the loop is bounded by num_used so there is no OOB or infinite loop, and forged addr/len are validated downstream in recv_pkt against the TD-private dma_record. The only un-neutralized effect is bookkeeping drift leading to descriptor exhaustion, which is a denial-of-service — and DoS from the VMM, which owns the entire virtio-vsock transport anyway, is out of model. So it is a legitimate robustness weakness worth hardening.
7 Low USED_PORT.lock().contains() then separate .lock().insert() in VsockStream::bind -- latent split VsockStream::bind Hold single guard: let mut g=USED_PORT.lock(); if g.contains(..){Err} else {g.insert(..);Ok}. weakness The split-lock is real — bind and get_unused_port released the USED_PORT guard between contains() and insert(), so a concurrent binder could race into the gap and two streams could share a local port, colliding in BINDING_PKT_QUEUES. But in the shipped single-vCPU cooperative executor there is no preemption and no .await between the check and the insert, so there is no interleaving point and it is unreachable today; the finding itself rates it T5/internal/HARDENING.
8 Low Rebind responder: op 0x03 (no variant check) -> op 0x05 (no variant check) -> write_msk(default) spdm_responder_rebind_new -> migtd_vdm_msg_rsp_dispatcher_ex -> handle_exchange_mig_attest_info_req -> migtd_vdm_msg_rsp_dispatcher_ex -> handle_exchange_mig_info_req -> write_msk Gate both migration-mode handlers on ResponderContextExInfo::MigrationInformation variant. weakness The op 0x05 handler handle_exchange_mig_info_req lacked the MigrationInformation variant guard that its three siblings have, and because vdm_message_transcript_before_finish is shared across modes, a legitimate rebind-attest could satisfy its only precondition — letting op 0x05 run in rebind mode against the default (zeroed) migration_info. However it stops short of a real key compromise: in rebind mode the buffer holds MigtdMigrationInformation::default(), so verify_servtd_attr(binding_handle=0, default_uuid) issues tdcall_servtd_rd against a non-bound handle and returns an attribute mismatch (or TDX error), failing before write_msk. So the chain is blocked by verify_servtd_attr, making it a weakness rather than a true positive — the added variant guard closes the mode-confusion gap early and restores symmetry with the other handlers, which is the correct hardening regardless.
9 Low SPDM migration responder stores peer-supplied servtd_ext BEFORE attestation veri Review the finding context and add appropriate input validation or guard checks. weakness In the production path the same peer-supplied servtd_ext bytes are verified inside rsp_verify_peer_attestation_v2; if that fails, op 0x03 returns Err, op 0x05 never runs, and write_approved_servtd_ext_hash is never reached, so no attacker-chosen hash ever reaches TDCS. The "write attacker-chosen APPROVED_SERVTD_EXT_HASH" primitive only materializes when attestation verification is compiled out (AzCVMEmu/test_mock_report/use-mock-quote), which are non-production test paths with no hostile peer. The original "store-before-verify" ordering was therefore a robustness/hardening defect rather than an exploitable flaw.
10 Info AzCVMEmu copy_to_private_shadow returns alias to buf -- no snapshot copy SharedMemory::copy_to_private_shadow Clone into a separate shadow Vec in the emu stub to preserve API contract parity with real td-payload, even if not strictly required for safety. weakness In AzCVMEmu builds buf is a process-private heap Vec with no concurrent host writer and the executor is single-threaded cooperative, so returning Some(&self.buf) instead of a copy grants no exploitable TOCTOU — the finding itself rates it Info/HARDENING/T5. The only real gap is fidelity: real td-payload returns a private snapshot decoupled from later shared-memory writes, while the emu stub returned a live alias, so a test relying on snapshot semantics could behave differently than production. That makes it a robustness weakness, not a vulnerability, and the trivial emu-only fix (copy into a separate shadow Vec) is justified for parity even though no security issue exists.

@MichalTarnacki MichalTarnacki force-pushed the fix/rest branch 2 times, most recently from 1db0bce to a3d8359 Compare July 6, 2026 10:58
…d input

Address the remaining security-audit findings across the virtio, vsock,
migtd, policy, json-signer and td-payload-emu crates.

- virtqueue: drive free-list traversal and chain recycling from private
  shadow state (desc_next, desc_write, chain_len) instead of the
  host-writable desc.next/desc.flags, and validate the host-reported
  used id against a chain we actually submitted, so the VMM cannot
  redirect recycling to never-submitted slots or leak in-flight
  descriptors.

- vsock vmcall transport: reject responses whose VMM-controlled data
  section is shorter than the parsed fields, preventing an
  out-of-bounds panic when indexing reply.data().

- spdm MigtdTransport::receive: cap both read loops to the frame
  boundary and return payload_size + header, so a coalesced stream read
  no longer over-reads into the next frame and desyncs the transport.

- spdm responder: gate handle_exchange_mig_info_req on the
  MigrationInformation variant to close a rebind-mode confusion, and
  persist peer-supplied SERVTD_EXT only after attestation verification
  succeeds.

- policy: floor-match get_tcb_level_by_svn per Intel TCB-Info semantics,
  and fail closed in enforce_mandatory_deny when the MigTD-engine TCB
  status cannot be established (None treated like Revoked).

- session/vsock: collapse check-then-act across separate lock scopes
  into a single BTreeSet::insert return-value check.

- json-signer: enforce canonical-form input in json_sign_detached so a
  detached signature cannot be produced for bytes that will never verify.

- td-payload-emu: copy into a separate shadow buffer in
  copy_to_private_shadow to match real td-payload snapshot semantics.

Signed-off-by: Michal Tarnacki <[email protected]>
Co-authored-by: GitHub Copilot <[email protected]>
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