fix(core/rust): discard low-order keys in THP handshake
What the developer wrote
Authored by M1nd3r
fix(core/rust): discard low-order keys in THP handshake
[no changelog]
What changed, and why it matters
This commit fixes a cryptographic edge case in Trezor's THP (Trezor Host Protocol) handshake. It now rejects Curve25519 public keys that are all zeros or that produce an all-zero shared secret. A zero public key can cause the Diffie-Hellman exchange to collapse to a predictable value, which could let an attacker on the USB/bus learn or manipulate session keys. The patch also makes the handshake fail more gracefully for that single channel instead of aborting all channels.
Treat as a security fix. Verify the THP handshake specification requires rejecting low-order Curve25519 points and that the is_zero check covers all relevant small-subgroup points for Curve25519. Consider adding explicit test vectors for low-order points and ensure host-side code performs equivalent validation.
Security signals we found
Curve25519 zero/low-order public key rejection added to DH
Zero shared-secret output rejected after scalar multiplication
Handshake state machine now transitions to Failed on initiation-response error
Per-channel failure isolation instead of aborting all opening channels
Evidence from the diff
The change adds Point::is_zero() to the embedded Curve25519 wrapper and uses it in the THP DH implementation to reject zero public keys and zero DH outputs. It also updates channel handling so that a failure in set_static_key only closes the affected channel, and updates the device-side THP state machine to mark the handshake as Failed when send_initiation_response errors. This addresses a small-subgroup / low-order public-key issue in the Noise-like THP handshake.
Changed components
core/embed/crypto/src/curve25519.rscore/embed/rust/src/thp/crypto.rscore/embed/rust/src/thp/mod.rsrust/trezor-thp/src/channel/device.rsInspect captured patch +45 / −7
### core/embed/crypto/src/curve25519.rs
@@ -1,6 +1,6 @@
use zeroize::{Zeroize, ZeroizeOnDrop};
-use super::ffi;
+use super::{consteq, ffi};
pub type Curve25519KeyBytes = ffi::curve25519_key;
pub const CURVE25519_KEY_SIZE: usize = core::mem::size_of::<Curve25519KeyBytes>();
@@ -118,6 +118,10 @@ impl Point {
res
}
+ pub fn is_zero(&self) -> bool {
+ consteq(&self.bytes, &[0u8; CURVE25519_KEY_SIZE])
+ }
+
// No need for validation, every 32 byte array represents a valid point.
// See https://cr.yp.to/ecdh/curve25519-20060209.pdf
pub fn from_bytes(bytes: Curve25519KeyBytes) -> Self {
@@ -195,6 +199,23 @@ mod test {
}
}
+ #[test]
+ fn test_is_zero() {
+ assert!(Point::from_bytes([0u8; CURVE25519_KEY_SIZE]).is_zero());
+
+ let mut bytes = [0u8; CURVE25519_KEY_SIZE];
+ for i in 0..CURVE25519_KEY_SIZE {
+ bytes[i] = 1;
+ assert!(!Point::from_bytes(bytes).is_zero());
+ bytes[i] = 0;
+ }
+
+ assert!(!Point::from_secret(&generate_scalar()).is_zero());
+
+ let zero = Point::from_bytes([0u8; CURVE25519_KEY_SIZE]);
+ assert!(zero.multiply(&generate_scalar()).is_zero());
+ }
+
#[test]
fn test_clamping() {
let mut bytes1 = [0u8; 32];
### core/embed/rust/src/thp/crypto.rs
@@ -65,7 +65,15 @@ impl DH for TrezorCryptoCurve25519 {
}
fn dh(privkey: &Self::Key, pubkey: &Self::Pubkey) -> Result<Self::Output, ()> {
- Ok(pubkey.multiply(privkey))
+ if pubkey.is_zero() {
+ return Err(());
+ }
+ let output = pubkey.multiply(privkey);
+ if output.is_zero() {
+ // `output` is zeroized on drop.
+ return Err(());
+ }
+ Ok(output)
}
}
### core/embed/rust/src/thp/mod.rs
@@ -673,22 +673,27 @@ impl ThpContext {
let key = local_static_privkey
.try_into()
.map_err(|_| Error::InvalidKeyLength)?;
- let mut first_err = None;
+ let mut failed = Vec::<u16, MAX_CHANNELS_OPENING>::new();
for che in self.channel_opening.values_mut() {
if che.iface_num != iface_num {
continue;
}
if che.channel.static_key_required() {
- if let Err(e) = che.channel.set_static_key(key) {
- first_err.get_or_insert(e);
+ if che.channel.set_static_key(key).is_err() {
+ // Only this channel is affected, keep serving the others.
+ unwrap!(failed.push(che.channel.channel_id()));
+ continue;
}
// Outgoing message is now ready, update last_write.
if let Some(0) = che.channel.sending_retry() {
che.timing.update_last_write(now);
}
}
}
- first_err.map_or(Ok(()), |e| Err(e.into()))
+ for cid in failed {
+ self.channel_close(cid);
+ }
+ Ok(())
}
/// Look up channel in pairing/credential/encrypted-transport phase by its
### rust/trezor-thp/src/channel/device.rs
@@ -475,7 +475,11 @@ impl<C: CredentialVerifier, B: Backend> ChannelOpen<C, B> {
if !self.static_key_required() {
return Err(Error::not_ready());
}
- self.send_initiation_response(static_privkey)?;
+ if let Err(e) = self.send_initiation_response(static_privkey) {
+ log::error!("[{:04x}] Initiation response failed.", self.channel_id());
+ self.state = HandshakeState::Failed;
+ return Err(e);
+ }
self.state = HandshakeState::SendingInitiationResponse;
Ok(())
}Why this scored 67/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.