diff --git a/protos/rendezvous.proto b/protos/rendezvous.proto index f126714a8..ccdde0e79 100644 --- a/protos/rendezvous.proto +++ b/protos/rendezvous.proto @@ -9,7 +9,6 @@ message IdPk { // that swaps SDP fingerprints). Empty for non-WebRTC handshakes. string dtls_fingerprint = 3; // Newest key exchange version the signer speaks; absent, i.e. 0, is the original scheme. - // Inside the signed identity so a rendezvous or relay cannot lower it. uint32 kx_version = 4; } @@ -300,11 +299,7 @@ message RendezvousMessage { HttpProxyResponse http_proxy_response = 28; IceCandidate ice_candidate = 29; } - // From the server, on every message after a key exchange: the version it advertised, - // repeated inside the encrypted channel where nothing in transit can alter it. A client that - // was shown a different value in the clear knows the handshake was altered and refuses the - // stream. Every message and not the first alone, so that dropping one frame at a position - // that needs no key to find does not rid an attacker of the check; dropping them all is a - // plain DoS. Absent from servers that predate versions. + // From the server, on every message after a key exchange: the version it advertised. + // Absent from servers that predate versions. uint32 kx_advertised = 30; } diff --git a/src/stream.rs b/src/stream.rs index e10c7e3d2..ad08f0549 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -101,7 +101,7 @@ impl Stream { } /// Refuse the stream if an encrypted message from the server says it advertised a key - /// exchange version other than `seen`, the one this side received in the clear. + /// exchange version other than `seen`, the one this side read from the key exchange. #[inline] pub fn check_kx_advertised(&mut self, seen: u32) { match self { diff --git a/src/tcp.rs b/src/tcp.rs index 2204c1c18..0f2e27ed0 100644 --- a/src/tcp.rs +++ b/src/tcp.rs @@ -25,10 +25,9 @@ pub trait TcpStreamTrait: AsyncRead + AsyncWrite + Unpin {} pub struct DynTcpStream(pub Box); /// The newest key exchange version this build speaks. Version 0 is the original scheme, and -/// what an absent field means: one key for both directions, each direction counting its own -/// nonce from 1, so the n-th message each way is sealed under the same (key, nonce). Version 1 -/// splits the exchanged key into one per direction, the way Noise splits its cipher states -/// after the handshake, binds the handshake transcript into both, and keeps the nonce layout. +/// what an absent field means: one key for both directions. Version 1 splits the exchanged key +/// into one per direction, the way Noise splits its cipher states after the handshake, binds +/// the handshake transcript into both, and keeps the nonce layout. pub const KX_VERSION_LATEST: u32 = 1; /// The version to run against a peer that advertised `advertised`: the highest both sides @@ -42,9 +41,8 @@ const KX_SPLIT_CONTEXT: &[u8] = b"rdkx-spl"; const KX_SPLIT_INITIATOR: u8 = 1; const KX_SPLIT_RESPONDER: u8 = 2; -/// What both sides saw during the key exchange, each from its own side of the wire. Mixed into -/// the split keys, so a change to any of it in transit leaves the two sides with different keys -/// and the first message undecryptable, without a check per field. +/// What both sides saw during the key exchange, each from its own side of the wire, mixed +/// into the split keys. pub struct KxTranscript<'a> { /// The ephemeral public key of the side that sent the sealed key. pub initiator_pk: &'a [u8], @@ -349,18 +347,14 @@ impl Encrypt { } /// Arm the check of the server's advertisement echo, `seen` being the version this side - /// received in the clear. Call once the key is set and before the first frame is read. + /// read from the key exchange. Call once the key is set and before the first frame is read. pub fn check_kx_advertised(&mut self, seen: u32) { self.4 = Some(seen); } - /// The server repeats what it advertised inside an encrypted message, where it can be - /// neither forged nor stripped. A different value in the clear means the handshake was - /// altered on the way, and the stream is refused rather than run at the version it was - /// pushed down to. Returns whether the echo settled the question: a frame that carries - /// none says nothing either way, and must leave the check armed for the frames after it, - /// since a server tags what it chooses to and an attacker would otherwise only have to - /// let one untagged frame through to be rid of the check. + /// The server repeats what it advertised inside its encrypted messages. Returns whether the + /// echo settled the question: a frame that carries none says nothing either way, and leaves + /// the check armed for the frames after it. fn check_kx_advertised_echo(plain: &[u8], seen: u32) -> Result { let echoed = crate::rendezvous_proto::RendezvousMessage::parse_from_bytes(plain) .map(|m| m.kx_advertised) @@ -496,8 +490,7 @@ mod tests { let (mut initiator, mut responder) = (Encrypt::new(key.clone()), Encrypt::new(key)); let sent = seal_and_open(&mut initiator, &mut responder, b"hello"); let back = seal_and_open(&mut responder, &mut initiator, b"hello"); - // The n-th message each way is sealed under the same (key, nonce): the defect the split - // fixes, pinned here so the path a peer without versions relies on stays byte-for-byte. + // Pinned so the path a peer without versions relies on stays byte-for-byte. assert_eq!(sent, back); } @@ -539,7 +532,7 @@ mod tests { #[test] fn test_split_binds_the_transcript() { let key = secretbox::gen_key(); - // The responder advertised LATEST; the initiator was handed something else in transit. + // The two sides put different advertisements into the transcript. let mut initiator = Encrypt::new_split(key.clone(), true, &transcript(KX_VERSION_LATEST + 1)).unwrap(); let mut responder = Encrypt::new_split(key, false, &transcript(KX_VERSION_LATEST)).unwrap(); @@ -583,7 +576,7 @@ mod tests { assert!(first_frame_after(KX_VERSION_LATEST, KX_VERSION_LATEST).is_ok()); // A server from before versions echoes nothing, and advertised nothing. assert!(first_frame_after(0, 0).is_ok()); - // The advertisement was lowered on the way; the echo says otherwise. + // The echo differs from what this side saw. assert!(first_frame_after(0, KX_VERSION_LATEST).is_err()); assert!(first_frame_after(KX_VERSION_LATEST, KX_VERSION_LATEST + 1).is_err()); }