From ea0bf35de101cdb9c75e10aadbbe11eb1889af9b Mon Sep 17 00:00:00 2001 From: Michal Tarnacki Date: Mon, 29 Jun 2026 11:12:12 +0200 Subject: [PATCH] fix(migtd): harden virtqueue, vsock, SPDM and policy against untrusted 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 Co-authored-by: GitHub Copilot --- .../td-payload-emu/src/mm/shared/mod.rs | 10 +- src/devices/virtio/src/virtqueue.rs | 96 ++++++++--- src/devices/vsock/src/stream.rs | 16 +- src/devices/vsock/src/transport/vmcall.rs | 30 +++- src/migtd/src/migration/session.rs | 7 +- src/migtd/src/spdm/mod.rs | 9 +- src/migtd/src/spdm/spdm_rsp.rs | 36 ++-- src/policy/src/v2/policy.rs | 159 +++++++++++++++--- src/policy/src/v2/servtd_collateral.rs | 6 +- tools/json-signer/src/lib.rs | 9 + 10 files changed, 291 insertions(+), 87 deletions(-) diff --git a/deps/td-shim-AzCVMEmu/td-payload-emu/src/mm/shared/mod.rs b/deps/td-shim-AzCVMEmu/td-payload-emu/src/mm/shared/mod.rs index 1794ec0f4..2b34ee873 100644 --- a/deps/td-shim-AzCVMEmu/td-payload-emu/src/mm/shared/mod.rs +++ b/deps/td-shim-AzCVMEmu/td-payload-emu/src/mm/shared/mod.rs @@ -10,6 +10,7 @@ const PAGE_SIZE: usize = 0x1000; pub struct SharedMemory { buf: Vec, + shadow: Vec, } impl SharedMemory { @@ -21,6 +22,7 @@ impl SharedMemory { let size = pages.checked_mul(4096)?; Some(Self { buf: Vec::from_iter(core::iter::repeat(0u8).take(size)), + shadow: Vec::new(), }) } @@ -34,9 +36,11 @@ impl SharedMemory { } pub fn copy_to_private_shadow(&mut self) -> Option<&[u8]> { - // In emulation mode, just return the buffer directly since we're not dealing with - // actual shared/private memory conversion like in real TDX - Some(&self.buf) + // Copy into a separate shadow buffer so the returned slice is an immutable + // snapshot, matching the real td-payload shared->private memcpy semantics. + self.shadow.clear(); + self.shadow.extend_from_slice(&self.buf); + Some(&self.shadow) } } diff --git a/src/devices/virtio/src/virtqueue.rs b/src/devices/virtio/src/virtqueue.rs index cb5c3806d..f1f8dcf6e 100644 --- a/src/devices/virtio/src/virtqueue.rs +++ b/src/devices/virtio/src/virtqueue.rs @@ -47,6 +47,20 @@ pub struct VirtQueue { free_head: u16, avail_idx: u16, last_used_idx: u16, + + /// Private shadow of the descriptor `next` links. This is the authoritative + /// source for free-list traversal and chain recycling; the host-writable + /// `desc[].next` field in shared DMA is never trusted to drive guest-side + /// accounting. + desc_next: [u16; MAX_QUEUE_SIZE], + /// Private shadow of each descriptor's direction (true = device-writable, + /// i.e. host-to-guest). Recorded at `add` time so recycling does not rely on + /// the host-writable `desc[].flags`. + desc_write: [bool; MAX_QUEUE_SIZE], + /// Number of descriptors in the in-flight chain headed by each index. A + /// value of 0 means the index is not the head of an in-flight chain, so a + /// host-reported used id that is not a real chain head is rejected. + chain_len: [u16; MAX_QUEUE_SIZE], } impl VirtQueue { @@ -82,8 +96,10 @@ impl VirtQueue { unsafe { &mut *((dma_addr as usize + layout.used_offset) as *mut UsedRing) }; // link descriptors together + let mut desc_next = [0u16; MAX_QUEUE_SIZE]; for i in 0..(queue_size - 1) { desc[i as usize].next.write(i + 1); + desc_next[i as usize] = i + 1; } Ok(VirtQueue { @@ -96,6 +112,9 @@ impl VirtQueue { free_head: 0, avail_idx: 0, last_used_idx: 0, + desc_next, + desc_write: [false; MAX_QUEUE_SIZE], + chain_len: [0; MAX_QUEUE_SIZE], }) } @@ -129,7 +148,11 @@ impl VirtQueue { flags.remove(DescFlags::NEXT); desc.flags.write(flags); - self.num_used += (g2h.len() + h2g.len()) as u16; + let chain_len = (g2h.len() + h2g.len()) as u16; + self.num_used += chain_len; + // Record the submitted chain length privately so recycling validates the + // host-reported used id against a chain we actually built. + self.chain_len[head as usize] = chain_len; let avail_slot = self.avail_idx & (self.queue_size - 1); self.avail @@ -160,18 +183,20 @@ impl VirtQueue { } fn add_descriptor(&mut self, buf: &VirtqueueBuf, flag: DescFlags) -> Result { + let index = self.free_head; let desc = self .desc - .get_mut(self.free_head as usize) + .get_mut(index as usize) .ok_or(VirtioError::InvalidDescriptorIndex)?; desc.set_buf(buf); desc.flags.write(flag); - // Update the free head - let last = self.free_head; - self.free_head = desc.next.read(); + // Advance the free head using the private shadow link instead of the + // host-writable `desc.next`, and record the buffer direction privately. + self.free_head = self.desc_next[index as usize]; + self.desc_write[index as usize] = flag.contains(DescFlags::WRITE); - Ok(last) + Ok(index) } /// Recycle descriptors in the list specified by head. @@ -179,42 +204,61 @@ impl VirtQueue { /// This will push all linked descriptors at the front of the free list. fn recycle_descriptors( &mut self, - mut head: u16, + head: u16, g2h: &mut Vec, h2g: &mut Vec, ) -> Result<()> { + // Validate, against private state, that `head` (reported by the host via + // the used ring) is the head of a chain we actually submitted. The walk + // below is driven entirely by the private shadow, never by the + // host-writable `desc.next`/`desc.flags`, so the host cannot redirect it + // to never-submitted slots or leak in-flight descriptors. + let len = self + .chain_len + .get(head as usize) + .copied() + .ok_or(VirtioError::InvalidDescriptorIndex)?; + if len == 0 || len > self.num_used { + return Err(VirtioError::InvalidDescriptor); + } + let origin_free_head = self.free_head; - self.free_head = head; + let mut cur = head; + for i in 0..len { + let index = cur as usize; + let next = self.desc_next[index]; + let is_write = self.desc_write[index]; + let is_last = i + 1 == len; - while self.num_used > 0 { let desc = self .desc - .get_mut(head as usize) + .get_mut(index) .ok_or(VirtioError::InvalidDescriptorIndex)?; - - let flags = desc.flags.read(); - self.num_used -= 1; - let addr = desc.addr.read(); - let len = desc.len.read(); + let buf_len = desc.len.read(); + if is_last { + // Relink the tail of the recycled chain to the previous free + // head in the shared table so the device still observes a + // consistent free list. + desc.next.write(origin_free_head); + } - if flags.contains(DescFlags::WRITE) { - h2g.push(VirtqueueBuf::new(addr, len)); + if is_write { + h2g.push(VirtqueueBuf::new(addr, buf_len)); } else { - g2h.push(VirtqueueBuf::new(addr, len)); + g2h.push(VirtqueueBuf::new(addr, buf_len)); } - if flags.contains(DescFlags::NEXT) { - if self.num_used == 0 { - return Err(VirtioError::InvalidDescriptor); - } - head = desc.next.read(); - } else { - desc.next.write(origin_free_head); - break; + if is_last { + self.desc_next[index] = origin_free_head; } + cur = next; } + self.free_head = head; + self.num_used -= len; + self.chain_len[head as usize] = 0; + Ok(()) } diff --git a/src/devices/vsock/src/stream.rs b/src/devices/vsock/src/stream.rs index ff705b0e2..0be1c290f 100644 --- a/src/devices/vsock/src/stream.rs +++ b/src/devices/vsock/src/stream.rs @@ -109,11 +109,11 @@ impl VsockStream { } pub fn bind(&mut self, addr: &VsockAddr) -> Result { - if USED_PORT.lock().contains(&addr.port()) { + let mut used_port = USED_PORT.lock(); + if !used_port.insert(addr.port()) { return Err(VsockError::AddressAlreadyUsed); } - - USED_PORT.lock().insert(addr.port()); + drop(used_port); self.addr.local.set_port(addr.port); Ok(()) } @@ -457,14 +457,16 @@ lazy_static! { } pub fn get_unused_port() -> Option { - let mut port = UNUSED_PORT_COUNTER.lock().checked_add(1)?; + let mut counter = UNUSED_PORT_COUNTER.lock(); + let mut used_port = USED_PORT.lock(); + let mut port = counter.checked_add(1)?; - while USED_PORT.lock().contains(&port) { + while used_port.contains(&port) { port = port.checked_add(1)?; } - USED_PORT.lock().insert(port); - *UNUSED_PORT_COUNTER.lock() = port; + used_port.insert(port); + *counter = port; Some(port) } diff --git a/src/devices/vsock/src/transport/vmcall.rs b/src/devices/vsock/src/transport/vmcall.rs index 470608ca6..1db107c75 100644 --- a/src/devices/vsock/src/transport/vmcall.rs +++ b/src/devices/vsock/src/transport/vmcall.rs @@ -192,12 +192,20 @@ async fn vmcall_service_migtd_send( return Poll::Pending; } + // The VMM controls the response length; reject any response whose data + // section is too short to hold the fields parsed below, otherwise the + // indexing into `reply.data()` would panic. + let reply_data = reply.data(); + if reply_data.len() < 12 { + return Poll::Ready(Err(VsockTransportError::InvalidParameter)); + } + // Do the sanity check if reply.guid() != VMCALL_SERVICE_MIGTD_GUID.as_bytes() || reply.status() != 0 - || reply.data()[0] != CURRENT_VERSION - || reply.data()[1] != COMMAND_SEND - || u64::from_le_bytes(reply.data()[4..12].try_into().unwrap()) != mid + || reply_data[0] != CURRENT_VERSION + || reply_data[1] != COMMAND_SEND + || u64::from_le_bytes(reply_data[4..12].try_into().unwrap()) != mid { return Poll::Ready(Err(VsockTransportError::InvalidParameter)); } @@ -230,17 +238,25 @@ async fn vmcall_service_migtd_receive( return Poll::Pending; } + // The VMM controls the response length; reject any response whose data + // section is too short to hold the fields parsed below, otherwise the + // indexing into `reply.data()` would panic. + let reply_data = reply.data(); + if reply_data.len() < 12 { + return Poll::Ready(Err(VsockTransportError::InvalidParameter)); + } + // Do the sanity check if reply.guid() != VMCALL_SERVICE_MIGTD_GUID.as_bytes() || reply.status() != 0 - || reply.data()[0] != CURRENT_VERSION - || reply.data()[1] != COMMAND_RECV - || u64::from_le_bytes(reply.data()[4..12].try_into().unwrap()) != mid + || reply_data[0] != CURRENT_VERSION + || reply_data[1] != COMMAND_RECV + || u64::from_le_bytes(reply_data[4..12].try_into().unwrap()) != mid { return Poll::Ready(Err(VsockTransportError::InvalidParameter)); } - recv_packet(&reply.data()[12..])?; + recv_packet(&reply_data[12..])?; Poll::Ready(pop_stream_queues(addrs).ok_or(VsockTransportError::InvalidVsockPacket)) }) .await diff --git a/src/migtd/src/migration/session.rs b/src/migtd/src/migration/session.rs index c7629f73e..de15baf19 100644 --- a/src/migtd/src/migration/session.rs +++ b/src/migtd/src/migration/session.rs @@ -585,11 +585,10 @@ pub async fn wait_for_request() -> Result { .ok_or(MigrationResult::InvalidParameter)?; let request_id = mig_info.mig_info.mig_request_id; - if REQUESTS.lock().contains(&request_id) { - Poll::Pending - } else { - REQUESTS.lock().insert(request_id); + if REQUESTS.lock().insert(request_id) { Poll::Ready(Ok(mig_info)) + } else { + Poll::Pending } } else if wfr.operation == 0 { Poll::Pending diff --git a/src/migtd/src/spdm/mod.rs b/src/migtd/src/spdm/mod.rs index 5c6c7d254..689acffe9 100644 --- a/src/migtd/src/spdm/mod.rs +++ b/src/migtd/src/spdm/mod.rs @@ -85,7 +85,7 @@ impl SpdmDeviceIo for MigtdTransport SpdmDeviceIo for MigtdTransport SpdmDeviceIo for MigtdTransport SpdmResult { diff --git a/src/migtd/src/spdm/spdm_rsp.rs b/src/migtd/src/spdm/spdm_rsp.rs index 2eb978f88..3bb95869c 100644 --- a/src/migtd/src/spdm/spdm_rsp.rs +++ b/src/migtd/src/spdm/spdm_rsp.rs @@ -147,15 +147,19 @@ pub fn spdm_responder<'a, T: AsyncRead + AsyncWrite + Unpin + Send + Sync + 'sta Ok((responder_context_ex, device_io_ref)) } -pub async fn spdm_responder_transfer_msk( - spdm_responder_ex: &mut ResponderContextEx<'_>, - mig_info: &MigtdMigrationInformation, +pub async fn spdm_responder_transfer_msk<'a>( + spdm_responder_ex: &mut ResponderContextEx<'a>, + mig_info: &'a MigtdMigrationInformation, #[cfg(feature = "policy_v2")] peer_data: Vec, ) -> Result<(), SpdmStatus> { #[cfg(not(feature = "policy_v2"))] let peer_data = Vec::new(); spdm_responder_ex.peer_data = peer_data; + // Mark this responder as operating in migration mode so the migration-mode + // handlers (mig-attest op 0x03, mig-info op 0x05) accept the request; their + // MigrationInformation variant guard rejects rebind-mode confusion. + spdm_responder_ex.info = ResponderContextExInfo::MigrationInformation(mig_info); let spdm_responder = &mut spdm_responder_ex.responder_context; let mut writer = Writer::init(&mut spdm_responder.common.app_context_data_buffer); @@ -520,13 +524,6 @@ pub fn handle_exchange_mig_attest_info_req( #[cfg(feature = "policy_v2")] let servtd_ext_bytes_vec = servtd_ext_bytes.to_vec(); - // Store SERVTD_EXT in ResponderContextEx for later use during MSK exchange - #[cfg(feature = "policy_v2")] - unsafe { - let spdm_responder_ex = upcast_mut(responder_context); - spdm_responder_ex.servtd_ext = ServtdExt::read_from_bytes(servtd_ext_bytes); - }; - // Init TDINFO from src (used for SERVTD_HASH verification) let vdm_element = VdmMessageElement::read(reader).ok_or(SPDM_STATUS_INVALID_MSG_SIZE)?; if vdm_element.element_type != VdmMessageElementType::TdReportInit { @@ -581,6 +578,14 @@ pub fn handle_exchange_mig_attest_info_req( responder_context, session_id, )?; + + // Persist SERVTD_EXT only after attestation verification succeeded, so + // unverified peer-supplied data is never stored in the responder context + // (its hash is later written to TDCS during MSK exchange in op 0x05). + unsafe { + let spdm_responder_ex = upcast_mut(responder_context); + spdm_responder_ex.servtd_ext = ServtdExt::read_from_bytes(&servtd_ext_bytes_vec); + }; } let mut writer = Writer::init(vendor_defined_rsp_payload); @@ -788,6 +793,17 @@ pub fn handle_exchange_mig_info_req( reader: &mut Reader<'_>, vendor_defined_rsp_payload: &mut [u8], ) -> SpdmResult { + // Reject if not in migration mode: a rebind session shares the + // transcript_before_finish precondition and must not reach write_msk. + let spdm_responder_ex = unsafe { upcast_mut(responder_context) }; + if !matches!( + &spdm_responder_ex.info, + ResponderContextExInfo::MigrationInformation(_) + ) { + error!("Migration info is not set in responder context.\n"); + return Err(SPDM_STATUS_INVALID_MSG_FIELD); + } + // The VDM message for secret migration info exchange MUST be sent after mutual attested session establishment. let session_id = if let Some(sid) = session_id { sid diff --git a/src/policy/src/v2/policy.rs b/src/policy/src/v2/policy.rs index 721fb2ba9..a6ff87416 100644 --- a/src/policy/src/v2/policy.rs +++ b/src/policy/src/v2/policy.rs @@ -305,12 +305,12 @@ impl<'a> PolicyData<'a> { relative_reference: &PolicyEvaluationInfo, skip_global: bool, ) -> Result<(), PolicyError> { - match self.forward_policy.as_ref() { - Some(policy) => { - Self::evaluate_policy_block(policy, value, relative_reference, skip_global) - } - None => Ok(()), - } + Self::evaluate_policy_block( + self.forward_policy.as_ref(), + value, + relative_reference, + skip_global, + ) } pub fn evaluate_policy_backward( @@ -319,12 +319,12 @@ impl<'a> PolicyData<'a> { relative_reference: &PolicyEvaluationInfo, skip_global: bool, ) -> Result<(), PolicyError> { - match self.backward_policy.as_ref() { - Some(policy) => { - Self::evaluate_policy_block(policy, value, relative_reference, skip_global) - } - None => Ok(()), - } + Self::evaluate_policy_block( + self.backward_policy.as_ref(), + value, + relative_reference, + skip_global, + ) } pub fn evaluate_policy_common( @@ -333,29 +333,60 @@ impl<'a> PolicyData<'a> { relative_reference: &PolicyEvaluationInfo, skip_global: bool, ) -> Result<(), PolicyError> { - match self.policy.as_ref() { - Some(policy) => { - Self::evaluate_policy_block(policy, value, relative_reference, skip_global) - } - None => Ok(()), - } + Self::evaluate_policy_block(self.policy.as_ref(), value, relative_reference, skip_global) } fn evaluate_policy_block( - block: &Vec, + block: Option<&Vec>, value: &PolicyEvaluationInfo, relative_reference: &PolicyEvaluationInfo, skip_global: bool, ) -> Result<(), PolicyError> { - for policy_type in block { - match policy_type { - PolicyTypes::Global(global) if !skip_global => { - global.evaluate(value, relative_reference)? + // Apply explicit policy constraints, if present. + if let Some(block) = block { + for policy_type in block { + match policy_type { + PolicyTypes::Global(global) if !skip_global => { + global.evaluate(value, relative_reference)? + } + PolicyTypes::Servtd(migtd) => migtd.evaluate(value, relative_reference)?, + _ => {} + } + } + } + + // Always enforce mandatory deny checks, even if a policy block is absent. + Self::enforce_mandatory_deny(value, skip_global)?; + + Ok(()) + } + + /// Enforce non-optional deny checks. + /// + /// Reject `Revoked` platform status when global checks are enabled. + /// Always check engine status; treat unknown (`None`) as denied. + fn enforce_mandatory_deny( + value: &PolicyEvaluationInfo, + skip_global: bool, + ) -> Result<(), PolicyError> { + if !skip_global { + if let Some(status) = value.tcb_status.as_deref() { + if TcbStatus::try_from(status)? == TcbStatus::Revoked { + return Err(PolicyError::TcbEvaluation); + } + } + } + + // Engine status must be known; fail closed on `None`. + match value.migtd_tcb_status.as_deref() { + Some(status) => { + if ServtdTcbStatus::try_from(status)? == ServtdTcbStatus::Revoked { + return Err(PolicyError::SvnMismatch); } - PolicyTypes::Servtd(migtd) => migtd.evaluate(value, relative_reference)?, - _ => {} } + None => return Err(PolicyError::UnqualifiedMigTdInfo), } + Ok(()) } @@ -1183,4 +1214,82 @@ mod test { .evaluate_integer(4, Some(relative_reference)) .unwrap()); } + + #[test] + fn test_absent_block_denies_revoked_platform() { + // No block: still deny `Revoked` platform status. + let value = PolicyEvaluationInfo { + tcb_status: Some("Revoked".to_string()), + ..PolicyEvaluationInfo::default() + }; + let relative = PolicyEvaluationInfo::default(); + + // `skip_global = false` evaluates platform status. + assert!( + PolicyData::<'static>::evaluate_policy_block(None, &value, &relative, false).is_err() + ); + } + + #[test] + fn test_absent_block_denies_revoked_engine() { + // No block: still deny `Revoked` engine status. + let value = PolicyEvaluationInfo { + migtd_tcb_status: Some("Revoked".to_string()), + ..PolicyEvaluationInfo::default() + }; + let relative = PolicyEvaluationInfo::default(); + + // Engine status is checked even with `skip_global = true`. + assert!( + PolicyData::<'static>::evaluate_policy_block(None, &value, &relative, true).is_err() + ); + } + + #[test] + fn test_absent_block_allows_non_revoked() { + // No block: non-`Revoked` status is allowed. + let value = PolicyEvaluationInfo { + tcb_status: Some("UpToDate".to_string()), + migtd_tcb_status: Some("UpToDate".to_string()), + ..PolicyEvaluationInfo::default() + }; + let relative = PolicyEvaluationInfo::default(); + + assert!( + PolicyData::<'static>::evaluate_policy_block(None, &value, &relative, false).is_ok() + ); + } + + #[test] + fn test_skip_global_ignores_platform_status() { + // In rebinding (`skip_global`), platform status is ignored. + let value = PolicyEvaluationInfo { + tcb_status: Some("Revoked".to_string()), + migtd_tcb_status: Some("UpToDate".to_string()), + ..PolicyEvaluationInfo::default() + }; + let relative = PolicyEvaluationInfo::default(); + + assert!( + PolicyData::<'static>::evaluate_policy_block(None, &value, &relative, true).is_ok() + ); + } + + #[test] + fn test_unclassifiable_engine_is_denied() { + // Fail closed: unknown engine status (`None`) is denied in all paths. + let value = PolicyEvaluationInfo { + tcb_status: Some("UpToDate".to_string()), + migtd_tcb_status: None, + ..PolicyEvaluationInfo::default() + }; + let relative = PolicyEvaluationInfo::default(); + + assert!( + PolicyData::<'static>::evaluate_policy_block(None, &value, &relative, false).is_err() + ); + assert!( + PolicyData::<'static>::evaluate_policy_block(None, &value, &relative, true).is_err() + ); + } } diff --git a/src/policy/src/v2/servtd_collateral.rs b/src/policy/src/v2/servtd_collateral.rs index e746ac3cc..60a4e7d78 100644 --- a/src/policy/src/v2/servtd_collateral.rs +++ b/src/policy/src/v2/servtd_collateral.rs @@ -83,9 +83,13 @@ impl TdIdentity { } pub fn get_tcb_level_by_svn(&self, svn: u16) -> Option<&TcbLevel> { + // TCB levels are breakpoints: pick the highest isvsvn <= reported svn + // (floor match), not an exact match. Exact match can reject valid + // newer svns and miss `Revoked` deny for in-between values. self.tcb_levels .iter() - .find(|&level| level.tcb.isvsvn == svn) + .filter(|level| level.tcb.isvsvn <= svn) + .max_by_key(|level| level.tcb.isvsvn) } } diff --git a/tools/json-signer/src/lib.rs b/tools/json-signer/src/lib.rs index a0b2acece..cd82bceff 100644 --- a/tools/json-signer/src/lib.rs +++ b/tools/json-signer/src/lib.rs @@ -49,6 +49,15 @@ pub fn json_sign(json_key: &str, data: &[u8], private_key: &[u8]) -> Result Result, Error> { + // Detached signing is finalized via `json_set_signature`, which embeds + // canonical serde bytes. Require canonical input here too, so signed bytes + // match embedded bytes and verification cannot fail due to reserialization. + let value: Value = serde_json::from_slice(data)?; + let canonical_data = serde_json::to_vec(&value)?; + if data != canonical_data { + return Err(Error::NotCanonical); + } + let private_key_der = crypto::ecdsa::pem_to_der_from_slice(private_key).map_err(|_| Error::InvalidKey)?; let signature = crypto::ecdsa::ecdsa_sign(&private_key_der, data).map_err(|_| Error::Sign)?;