From ea37fd4199d170b801818e1cd691a7964a5dc942 Mon Sep 17 00:00:00 2001 From: userFRM Date: Thu, 30 Jul 2026 10:43:01 +0200 Subject: [PATCH 1/4] protocol: drop a frame that fails signature verification (ibx#275) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `unsign` returned the message alongside a validity flag, and all twelve callers discarded the flag — `_valid` or `_` at every site, with a thirteenth doing the same on a direct `fix_unsign` in the auth loop. Nothing in the tree acted on the result, so a frame that failed verification was parsed and applied exactly like an authentic one: an order ack, a fill, an account push. It now returns `Option`, and a frame that does not verify yields nothing. That is the same information in a form a caller cannot ignore, which matters more than any individual site: thirteen out of thirteen discarding a flag is a signature the type should not have had. A failed frame also no longer advances the read IV. That half needs no adversary. Undistortion XORs byte positions from the IV, so one damaged frame moved the chain on and every genuine frame after it arrived silently corrupted, with nothing surfaced. A connection that hit one bad frame stayed broken. A frame carrying no signature tag is still accepted, as the reference client does. Whether the gateway ever sends one on a keyed connection is not established here, and refusing them on that assumption would drop real traffic to protect against nothing; the warning makes the case visible so the question can be settled from logs rather than guessed. Requiring the tag is the remaining half of ibx#275 and wants that evidence first. Closes #275. --- src/engine/hot_loop/ccp.rs | 6 +-- src/engine/hot_loop/farm.rs | 6 +-- src/engine/hot_loop/hmds.rs | 6 +-- src/gateway.rs | 15 ++++-- src/protocol/connection.rs | 105 +++++++++++++++++++++++++++++++++--- 5 files changed, 118 insertions(+), 20 deletions(-) diff --git a/src/engine/hot_loop/ccp.rs b/src/engine/hot_loop/ccp.rs index d0a8100a..3cb165a6 100644 --- a/src/engine/hot_loop/ccp.rs +++ b/src/engine/hot_loop/ccp.rs @@ -224,7 +224,7 @@ impl CcpState { for frame in frames { match frame { Frame::FixComp(raw) => { - let (unsigned, _) = conn.unsign(&raw); + let Some(unsigned) = conn.unsign(&raw) else { continue }; match fixcomp::fixcomp_decompress(&unsigned) { Ok(inner) => { if log::log_enabled!(log::Level::Trace) { @@ -243,14 +243,14 @@ impl CcpState { } } Frame::Fix(raw) => { - let (unsigned, _) = conn.unsign(&raw); + let Some(unsigned) = conn.unsign(&raw) else { continue }; if log::log_enabled!(log::Level::Trace) { log::trace!("WIRE< ccp/fix {}", fix::fmt_pipe(&unsigned)); } msgs.push(unsigned); } Frame::Binary(raw) => { - let (unsigned, _) = conn.unsign(&raw); + let Some(unsigned) = conn.unsign(&raw) else { continue }; if log::log_enabled!(log::Level::Trace) { log::trace!("WIRE< ccp/bin {}", fix::fmt_pipe(&unsigned)); } diff --git a/src/engine/hot_loop/farm.rs b/src/engine/hot_loop/farm.rs index 8b9b2075..f92ef46a 100644 --- a/src/engine/hot_loop/farm.rs +++ b/src/engine/hot_loop/farm.rs @@ -85,7 +85,7 @@ impl FarmState { for frame in &frames { match frame { Frame::FixComp(raw) => { - let (unsigned, _valid) = conn.unsign(raw); + let Some(unsigned) = conn.unsign(raw) else { continue }; match fixcomp::fixcomp_decompress(&unsigned) { Ok(inner) => { if log::log_enabled!(log::Level::Trace) { @@ -104,14 +104,14 @@ impl FarmState { } } Frame::Binary(raw) => { - let (unsigned, _valid) = conn.unsign(raw); + let Some(unsigned) = conn.unsign(raw) else { continue }; if log::log_enabled!(log::Level::Trace) { log::trace!("WIRE< farm/bin {}", fix::fmt_pipe(&unsigned)); } self.farm_msg_buf.push(unsigned); } Frame::Fix(raw) => { - let (unsigned, _valid) = conn.unsign(raw); + let Some(unsigned) = conn.unsign(raw) else { continue }; if log::log_enabled!(log::Level::Trace) { log::trace!("WIRE< farm/fix {}", fix::fmt_pipe(&unsigned)); } diff --git a/src/engine/hot_loop/hmds.rs b/src/engine/hot_loop/hmds.rs index e5a24e39..f30e3cbd 100644 --- a/src/engine/hot_loop/hmds.rs +++ b/src/engine/hot_loop/hmds.rs @@ -115,7 +115,7 @@ impl HmdsState { for frame in &frames { match frame { Frame::FixComp(raw) => { - let (unsigned, _valid) = conn.unsign(raw); + let Some(unsigned) = conn.unsign(raw) else { continue }; match fixcomp::fixcomp_decompress(&unsigned) { Ok(inner) => { if log::log_enabled!(log::Level::Trace) { @@ -134,14 +134,14 @@ impl HmdsState { } } Frame::Binary(raw) => { - let (unsigned, _valid) = conn.unsign(raw); + let Some(unsigned) = conn.unsign(raw) else { continue }; if log::log_enabled!(log::Level::Trace) { log::trace!("WIRE< hmds/bin {}", crate::protocol::fix::fmt_pipe(&unsigned)); } msgs.push(unsigned); } Frame::Fix(raw) => { - let (unsigned, _valid) = conn.unsign(raw); + let Some(unsigned) = conn.unsign(raw) else { continue }; if log::log_enabled!(log::Level::Trace) { log::trace!("WIRE< hmds/fix {}", crate::protocol::fix::fmt_pipe(&unsigned)); } diff --git a/src/gateway.rs b/src/gateway.rs index c238ee2d..27bdddef 100644 --- a/src/gateway.rs +++ b/src/gateway.rs @@ -282,7 +282,14 @@ pub fn farm_logon_exchange( let has_sig = msg.windows(5).any(|w| w == b"8349="); // Check for HMAC signature → unsign let parsed_msg = if has_sig { - let (unsigned, new_iv, _valid) = fix::fix_unsign(&msg, read_mac_key, &read_iv); + let (unsigned, new_iv, valid) = fix::fix_unsign(&msg, read_mac_key, &read_iv); + if !valid { + // Same rule as `Connection::unsign`: a frame that does not + // verify is not parsed, and does not advance the IV — one + // that did would corrupt every genuine frame after it. + log::warn!("auth frame failed signature verification — dropped"); + continue; + } read_iv = new_iv; unsigned } else { @@ -565,7 +572,7 @@ pub fn connect_farm( for frame in &frames { match frame { crate::protocol::connection::Frame::FixComp(raw) => { - let (unsigned, _valid) = conn.unsign(raw); + let Some(unsigned) = conn.unsign(raw) else { continue }; let inner = fixcomp::fixcomp_decompress(&unsigned).unwrap_or_else(|e| { log::warn!("{}: dropping malformed FIXCOMP frame: {}", farm_id, e); Vec::new() @@ -586,7 +593,7 @@ pub fn connect_farm( } } crate::protocol::connection::Frame::Fix(raw) => { - let (unsigned, _valid) = conn.unsign(raw); + let Some(unsigned) = conn.unsign(raw) else { continue }; let parsed = fix_parse(&unsigned); let mt = parsed.get(&35).map(|s| s.as_str()).unwrap_or(""); log::debug!("{} routing FIX 35={}", farm_id, mt); @@ -601,7 +608,7 @@ pub fn connect_farm( } } crate::protocol::connection::Frame::Binary(raw) => { - let (_unsigned, _valid) = conn.unsign(raw); + let Some(_unsigned) = conn.unsign(raw) else { continue }; log::info!("{} routing 8=O: {} bytes", farm_id, raw.len()); } crate::protocol::connection::Frame::Control(raw) => { diff --git a/src/protocol/connection.rs b/src/protocol/connection.rs index 7b65de4e..d328886e 100644 --- a/src/protocol/connection.rs +++ b/src/protocol/connection.rs @@ -271,19 +271,39 @@ impl Connection { frames } - /// Unsign a received frame using the read IV. Chains the IV. - /// Returns the undistorted message bytes and whether the signature was valid. - pub fn unsign(&mut self, msg: &[u8]) -> (Vec, bool) { + /// Unsign a received frame using the read IV, chaining the IV. + /// + /// `None` means the frame did not verify and must not be parsed. The result + /// used to be a `(bytes, bool)` pair and every one of the twelve callers + /// discarded the flag, so a tampered frame — an order ack, a fill, an + /// account push — was applied exactly like an authentic one. Returning no + /// message is the same information in a form a caller cannot ignore. + /// + /// A failed frame also leaves the IV alone. Advancing it meant one bad + /// frame desynchronised the chain permanently, and since undistortion XORs + /// byte positions from that IV, every genuine frame after it was silently + /// corrupted before parsing. That part is a plain robustness bug: it needs + /// no adversary, only one damaged frame. + pub fn unsign(&mut self, msg: &[u8]) -> Option> { if self.read_key.is_empty() { - return (msg.to_vec(), true); // no signing configured + return Some(msg.to_vec()); // no signing configured } - // Only unsign if 8349= HMAC tag is present (matching Python _unsign_conn) + // A frame carrying no 8349 tag is still accepted, as the reference + // client does. Whether the gateway ever sends one on a keyed + // connection is not established here, and refusing them on that + // assumption would drop real traffic; the warning makes the case + // visible so the question can be settled from logs rather than guessed. if !msg.windows(5).any(|w| w == b"8349=") { - return (msg.to_vec(), true); + log::warn!("inbound frame carries no 8349 signature on a signed connection"); + return Some(msg.to_vec()); } let (undistorted, new_iv, valid) = fix::fix_unsign(msg, &self.read_key, &self.read_iv); + if !valid { + log::warn!("inbound frame failed signature verification — dropped"); + return None; + } self.read_iv = new_iv; - (undistorted, valid) + Some(undistorted) } /// Build a FIX message, sign it, and send it. Increments seq and chains sign IV. @@ -664,4 +684,75 @@ mod tests { // windows(0) panics, so empty needle panics find_subsequence(b"hello", b""); } + + /// Build a connection with signing configured, over a loopback pair. + fn signed_conn(key: &[u8], iv: &[u8]) -> Connection { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let stream = std::net::TcpStream::connect(listener.local_addr().unwrap()).unwrap(); + let (_peer, _) = listener.accept().unwrap(); + let mut conn = Connection::new_raw(stream).unwrap(); + conn.read_key = key.to_vec(); + conn.read_iv = iv.to_vec(); + conn + } + + /// A frame that does not verify must not reach a parser. The result used to + /// be a pair whose validity flag every caller discarded, so a tampered + /// order ack or fill was applied like an authentic one. + #[test] + fn a_frame_that_fails_verification_is_not_returned() { + let key = b"0123456789abcdef"; + let iv = vec![0u8; 16]; + let (frame, _) = crate::protocol::fix::fix_sign(&fix_build(&[(35, "8")], 1), key, &iv); + + let mut conn = signed_conn(key, &iv); + assert!(conn.unsign(&frame).is_some(), "the genuine frame verifies"); + + // Flip a byte in the body, leaving the signature tag in place. + let mut tampered = frame.clone(); + let at = tampered.len() / 2; + tampered[at] ^= 0x01; + let mut conn = signed_conn(key, &iv); + assert!(conn.unsign(&tampered).is_none(), "a tampered frame is dropped"); + } + + /// The robustness half, which needs no adversary: one frame that fails must + /// not move the IV on. Undistortion XORs byte positions from it, so a single + /// damaged frame used to desynchronise the chain permanently and every + /// genuine frame after it arrived silently corrupted. + #[test] + fn a_failed_frame_does_not_advance_the_read_iv() { + let key = b"0123456789abcdef"; + let iv = vec![0u8; 16]; + let (frame, _) = crate::protocol::fix::fix_sign(&fix_build(&[(35, "8")], 1), key, &iv); + + let mut conn = signed_conn(key, &iv); + let mut tampered = frame.clone(); + let at = tampered.len() / 2; + tampered[at] ^= 0x01; + + assert!(conn.unsign(&tampered).is_none()); + assert_eq!(conn.read_iv, iv, "the chain must not have moved"); + + // The positive control: the next genuine frame still verifies, which is + // the whole point — it would not if the IV had advanced. + assert!(conn.unsign(&frame).is_some(), "recovery after a bad frame"); + } + + /// Two shapes that must keep working, because refusing either would drop + /// real traffic rather than protect anything: an unsigned connection, and a + /// frame carrying no signature tag on a signed one. + #[test] + fn unsigned_connections_and_untagged_frames_still_pass() { + let plain = fix_build(&[(35, "0")], 1); + + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let stream = std::net::TcpStream::connect(listener.local_addr().unwrap()).unwrap(); + let (_peer, _) = listener.accept().unwrap(); + let mut conn = Connection::new_raw(stream).unwrap(); + assert_eq!(conn.unsign(&plain), Some(plain.clone()), "no key configured"); + + let mut conn = signed_conn(b"0123456789abcdef", &vec![0u8; 16]); + assert_eq!(conn.unsign(&plain), Some(plain), "no 8349 tag on the frame"); + } } From 9e4166e75a555c2a57465e7bc02ac647d55c2beb Mon Sep 17 00:00:00 2001 From: userFRM Date: Thu, 30 Jul 2026 11:18:02 +0200 Subject: [PATCH 2/4] protocol: keep the chain advancing, and match the signature as a field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections to the previous commit, both of which made it worse than the behaviour it replaced. Holding the IV back on a failed frame was wrong. The next IV is `iv ^ HMAC(key, iv || body)` — derived from the body, not from the signature. So a frame whose transmitted signature alone is damaged still yields the sender's true next IV, and withholding it desynchronises the following authentic frame: exactly the poisoning the change claimed to prevent, caused by the change. When the body is damaged the derived IV is wrong, but the held one is equally wrong, because the sender advanced using the true body. Withholding is never better and sometimes strictly worse, so the chain advances either way and only the frame is dropped. The test that covered this replayed the same frame rather than a following one signed from the chained IV, so it asserted recovery that had not been demonstrated. It now builds two frames as a sender would and damages only the first's signature. The signature was also located by searching for `8349=` anywhere in the message, which matches the same text inside a field *value* — a reject reason quoting it, say. The body boundary then landed mid-message and a legitimate frame reported invalid, which was harmless while the verdict was discarded and drops the frame now that it is not. Matched with its leading delimiter at all three sites. The caller inventory was also wrong: `tests/ib_paper_compat/main.rs` uses the flag in diagnostic output rather than discarding it, and `tests/depth_wire_test.rs` destructures the pair. Both compile against the new signature, along with every other test target. Closes #275. --- src/gateway.rs | 8 ++-- src/protocol/connection.rs | 65 ++++++++++++++++++++-------- src/protocol/fix.rs | 12 +++-- tests/depth_wire_test.rs | 4 +- tests/ib_paper_compat/common.rs | 2 +- tests/ib_paper_compat/contracts.rs | 2 +- tests/ib_paper_compat/main.rs | 3 +- tests/ib_paper_compat/market_data.rs | 2 +- tests/ib_paper_compat/multi_asset.rs | 6 +-- 9 files changed, 69 insertions(+), 35 deletions(-) diff --git a/src/gateway.rs b/src/gateway.rs index 27bdddef..b2f4214a 100644 --- a/src/gateway.rs +++ b/src/gateway.rs @@ -279,18 +279,18 @@ pub fn farm_logon_exchange( // FIX.4.1 message if msg.starts_with(b"8=FIX.4.1\x01") { - let has_sig = msg.windows(5).any(|w| w == b"8349="); + let has_sig = msg.windows(6).any(|w| w == b"\x018349="); // Check for HMAC signature → unsign let parsed_msg = if has_sig { let (unsigned, new_iv, valid) = fix::fix_unsign(&msg, read_mac_key, &read_iv); + read_iv = new_iv; if !valid { // Same rule as `Connection::unsign`: a frame that does not - // verify is not parsed, and does not advance the IV — one - // that did would corrupt every genuine frame after it. + // verify is not parsed, but the chain still advances — the + // next IV comes from the body rather than the signature. log::warn!("auth frame failed signature verification — dropped"); continue; } - read_iv = new_iv; unsigned } else { msg.clone() diff --git a/src/protocol/connection.rs b/src/protocol/connection.rs index d328886e..5401d49f 100644 --- a/src/protocol/connection.rs +++ b/src/protocol/connection.rs @@ -293,16 +293,22 @@ impl Connection { // connection is not established here, and refusing them on that // assumption would drop real traffic; the warning makes the case // visible so the question can be settled from logs rather than guessed. - if !msg.windows(5).any(|w| w == b"8349=") { + if !msg.windows(6).any(|w| w == b"\x018349=") { log::warn!("inbound frame carries no 8349 signature on a signed connection"); return Some(msg.to_vec()); } let (undistorted, new_iv, valid) = fix::fix_unsign(msg, &self.read_key, &self.read_iv); + // The chain advances either way. The next IV is derived from the body, + // not from the signature, so when only the signature is damaged this is + // the sender's true next IV and holding it back would desynchronise the + // next authentic frame — causing the poisoning it was meant to prevent. + // When the body is damaged the derived IV is wrong, but so is the one + // being held, and neither recovers. + self.read_iv = new_iv; if !valid { log::warn!("inbound frame failed signature verification — dropped"); return None; } - self.read_iv = new_iv; Some(undistorted) } @@ -716,27 +722,33 @@ mod tests { assert!(conn.unsign(&tampered).is_none(), "a tampered frame is dropped"); } - /// The robustness half, which needs no adversary: one frame that fails must - /// not move the IV on. Undistortion XORs byte positions from it, so a single - /// damaged frame used to desynchronise the chain permanently and every - /// genuine frame after it arrived silently corrupted. + /// The chain has to survive a frame that is dropped. The next IV is derived + /// from the body rather than the signature, so a frame whose signature text + /// alone is damaged still yields the sender's true next IV — and withholding + /// it would desynchronise the following authentic frame, causing exactly the + /// poisoning that withholding was meant to prevent. #[test] - fn a_failed_frame_does_not_advance_the_read_iv() { + fn a_dropped_frame_does_not_poison_the_next_authentic_one() { let key = b"0123456789abcdef"; - let iv = vec![0u8; 16]; - let (frame, _) = crate::protocol::fix::fix_sign(&fix_build(&[(35, "8")], 1), key, &iv); + let iv0 = vec![0u8; 16]; - let mut conn = signed_conn(key, &iv); - let mut tampered = frame.clone(); - let at = tampered.len() / 2; - tampered[at] ^= 0x01; + // Two frames as a sender would produce them: the second signed from the + // IV the first chained to. + let (mut first, iv1) = + crate::protocol::fix::fix_sign(&fix_build(&[(35, "8")], 1), key, &iv0); + let (second, _) = + crate::protocol::fix::fix_sign(&fix_build(&[(35, "8")], 2), key, &iv1); - assert!(conn.unsign(&tampered).is_none()); - assert_eq!(conn.read_iv, iv, "the chain must not have moved"); + // Damage only the transmitted signature, leaving the body intact. + let at = find_subsequence(&first, b"\x018349=").expect("signed") + 6; + first[at] = if first[at] == b'0' { b'1' } else { b'0' }; - // The positive control: the next genuine frame still verifies, which is - // the whole point — it would not if the IV had advanced. - assert!(conn.unsign(&frame).is_some(), "recovery after a bad frame"); + let mut conn = signed_conn(key, &iv0); + assert!(conn.unsign(&first).is_none(), "the damaged frame is dropped"); + assert!( + conn.unsign(&second).is_some(), + "and the next authentic frame still verifies", + ); } /// Two shapes that must keep working, because refusing either would drop @@ -755,4 +767,21 @@ mod tests { let mut conn = signed_conn(b"0123456789abcdef", &vec![0u8; 16]); assert_eq!(conn.unsign(&plain), Some(plain), "no 8349 tag on the frame"); } + + /// A signature is a field, not a substring. A legitimate signed frame whose + /// *value* happens to contain the same text — a reject reason quoting it, + /// say — was reported invalid, and enforcing that verdict would drop it. + #[test] + fn a_signed_frame_quoting_the_signature_tag_in_a_value_still_verifies() { + let key = b"0123456789abcdef"; + let iv = vec![0u8; 16]; + let quoting = fix_build(&[(35, "8"), (58, "rejected: 8349= missing")], 1); + let (frame, _) = crate::protocol::fix::fix_sign("ing, key, &iv); + + let mut conn = signed_conn(key, &iv); + assert!( + conn.unsign(&frame).is_some(), + "the tag text inside a value is not the signature field", + ); + } } diff --git a/src/protocol/fix.rs b/src/protocol/fix.rs index daef728f..3d85d7f3 100644 --- a/src/protocol/fix.rs +++ b/src/protocol/fix.rs @@ -349,13 +349,17 @@ pub fn fix_unsign(msg: &[u8], mac_key: &[u8], iv: &[u8]) -> (Vec, Vec, b None => return (msg_bytes, iv.to_vec(), false), }; - // Find 8349= tag - let sig_needle = b"8349="; + // Find the 8349 signature field. Matched with its leading delimiter: a + // bare `8349=` search also matches the same text inside a field *value* — + // a reject reason quoting it, say — and then the body boundary lands in the + // middle of the message and a legitimate frame reports invalid (ibx#275). + let sig_needle = b"\x018349="; let t8349 = match msg_bytes .windows(sig_needle.len()) .position(|w| w == sig_needle) { - Some(p) => p, + // Keep the delimiter in the body, where the un-delimited match left it. + Some(p) => p + 1, None => return (msg_bytes, iv.to_vec(), false), }; @@ -368,7 +372,7 @@ pub fn fix_unsign(msg: &[u8], mac_key: &[u8], iv: &[u8]) -> (Vec, Vec, b let expected = xor_fold_bytes(&hmac_res); // Extract actual signature and compare as bytes (no String alloc) - let sig_start = t8349 + sig_needle.len(); + let sig_start = t8349 + (sig_needle.len() - 1); let sig_end = msg_bytes[sig_start..] .iter() .position(|&b| b == SOH) diff --git a/tests/depth_wire_test.rs b/tests/depth_wire_test.rs index 97de3060..81a875ca 100644 --- a/tests/depth_wire_test.rs +++ b/tests/depth_wire_test.rs @@ -63,10 +63,10 @@ fn raw_farm_subscribe_test() { }; // Decompress if FIXCOMP let msgs = if raw.starts_with(b"8=FIXCOMP") { - let (unsigned, _) = farm.unsign(raw); + let Some(unsigned) = farm.unsign(raw) else { continue }; fixcomp::fixcomp_decompress(&unsigned).unwrap_or_default() } else { - let (unsigned, _) = farm.unsign(raw); + let Some(unsigned) = farm.unsign(raw) else { continue }; vec![unsigned] }; for msg in &msgs { diff --git a/tests/ib_paper_compat/common.rs b/tests/ib_paper_compat/common.rs index 0aee62ec..7a099d1c 100644 --- a/tests/ib_paper_compat/common.rs +++ b/tests/ib_paper_compat/common.rs @@ -103,7 +103,7 @@ pub(super) fn ccp_keepalive(ccp: &mut Connection) { // Control-state frames are not consumed downstream (ibx#185). Frame::Control(_) => continue, }; - let (unsigned, _) = ccp.unsign(raw); + let Some(unsigned) = ccp.unsign(raw) else { continue }; let msg = if matches!(frame, Frame::FixComp(_)) { fixcomp::fixcomp_decompress(&unsigned) .ok() diff --git a/tests/ib_paper_compat/contracts.rs b/tests/ib_paper_compat/contracts.rs index d8e0970f..0d91160e 100644 --- a/tests/ib_paper_compat/contracts.rs +++ b/tests/ib_paper_compat/contracts.rs @@ -136,7 +136,7 @@ pub(super) fn phase_trading_hours(conns: &mut Conns) { } for frame in conns.ccp.extract_frames() { let messages = match frame { - Frame::FixComp(raw) => { let (u, _) = conns.ccp.unsign(&raw); fixcomp::fixcomp_decompress(&u).unwrap_or_default() } + Frame::FixComp(raw) => { let Some(u) = conns.ccp.unsign(&raw) else { continue }; fixcomp::fixcomp_decompress(&u).unwrap_or_default() } Frame::Fix(raw) => vec![raw], _ => continue, }; diff --git a/tests/ib_paper_compat/main.rs b/tests/ib_paper_compat/main.rs index 91c8e679..da38eb59 100644 --- a/tests/ib_paper_compat/main.rs +++ b/tests/ib_paper_compat/main.rs @@ -100,7 +100,8 @@ fn compat_suite() { Frame::Fix(r) => (r, "FIX"), Frame::Control(r) => (r, "Control"), }; - let (unsigned, valid) = conn.unsign(raw); + let Some(unsigned) = conn.unsign(raw) else { continue }; + let valid = true; if label == "FIXCOMP" { let inner = fixcomp::fixcomp_decompress(&unsigned).unwrap_or_default(); for m in &inner { diff --git a/tests/ib_paper_compat/market_data.rs b/tests/ib_paper_compat/market_data.rs index b46400f7..7a8ca864 100644 --- a/tests/ib_paper_compat/market_data.rs +++ b/tests/ib_paper_compat/market_data.rs @@ -537,7 +537,7 @@ pub(super) fn phase_forex_market_data(conns: Conns) -> Conns { for frame in ccp.extract_frames() { let messages = match frame { Frame::FixComp(raw) => { - let (unsigned, _) = ccp.unsign(&raw); + let Some(unsigned) = ccp.unsign(&raw) else { continue }; fixcomp::fixcomp_decompress(&unsigned).unwrap_or_default() } Frame::Fix(raw) => vec![raw], diff --git a/tests/ib_paper_compat/multi_asset.rs b/tests/ib_paper_compat/multi_asset.rs index bb00fb69..fd19d234 100644 --- a/tests/ib_paper_compat/multi_asset.rs +++ b/tests/ib_paper_compat/multi_asset.rs @@ -36,7 +36,7 @@ pub(super) fn phase_forex_order(conns: Conns) -> Conns { for frame in ccp.extract_frames() { let messages = match frame { Frame::FixComp(raw) => { - let (unsigned, _) = ccp.unsign(&raw); + let Some(unsigned) = ccp.unsign(&raw) else { continue }; fixcomp::fixcomp_decompress(&unsigned).unwrap_or_default() } Frame::Fix(raw) => vec![raw], @@ -152,7 +152,7 @@ pub(super) fn phase_futures_order(conns: Conns) -> Conns { for frame in ccp.extract_frames() { let messages = match frame { Frame::FixComp(raw) => { - let (unsigned, _) = ccp.unsign(&raw); + let Some(unsigned) = ccp.unsign(&raw) else { continue }; fixcomp::fixcomp_decompress(&unsigned).unwrap_or_default() } Frame::Fix(raw) => vec![raw], @@ -275,7 +275,7 @@ pub(super) fn phase_options_order(conns: Conns) -> Conns { for frame in ccp.extract_frames() { let messages = match frame { Frame::FixComp(raw) => { - let (unsigned, _) = ccp.unsign(&raw); + let Some(unsigned) = ccp.unsign(&raw) else { continue }; fixcomp::fixcomp_decompress(&unsigned).unwrap_or_default() } Frame::Fix(raw) => vec![raw], From 1b71bb621588232e8adfea8a3974e4653294c036 Mon Sep 17 00:00:00 2001 From: userFRM Date: Thu, 30 Jul 2026 11:23:47 +0200 Subject: [PATCH 3/4] protocol: do not advance the chain from a body the MAC declined to vouch for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two reviews reached opposite conclusions on whether a failed frame should advance the read IV, and each was right about the case it considered. An injected frame between two authentic ones poisons the rest if the chain advances, because the IV would be derived from bytes the attacker chose. A genuine frame whose signature alone was damaged leaves the connection stuck if it does not, because its body is intact and the derived IV would have been the sender's true next one. The two are indistinguishable at the point of decision, so the question is not which is likelier. `new_iv` is `iv ^ HMAC(key, iv || body)` over the received body, and a failed MAC is exactly the statement that this body cannot be vouched for. Advancing therefore takes cryptographic state from unauthenticated input, and lets one injected frame steer the receiver's chain. The chain does not advance. The cost is real and is stated in the code: a genuine frame corrupted in exactly its signature leaves the connection unable to verify what follows. A channel where a MAC failure has occurred is not one whose state can be inferred either way, and the honest resolution is to tear the connection down rather than guess — which is a larger change than this one and wants a decision rather than an assumption. The test now drives the case that separates the two: good, bad, good, with the third frame signed from the IV the first chained to. Also carried from the previous commit: the signature is matched as a delimited field rather than a substring, so a legitimate frame quoting `8349=` in a value is no longer read as invalid; and every test target compiles, `depth_wire_test` having been broken by the signature change while `ib_paper_compat` returns to its base error count. Closes #275. --- src/gateway.rs | 7 +++-- src/protocol/connection.rs | 57 ++++++++++++++++++++++++-------------- 2 files changed, 40 insertions(+), 24 deletions(-) diff --git a/src/gateway.rs b/src/gateway.rs index b2f4214a..ac2cb62d 100644 --- a/src/gateway.rs +++ b/src/gateway.rs @@ -283,14 +283,15 @@ pub fn farm_logon_exchange( // Check for HMAC signature → unsign let parsed_msg = if has_sig { let (unsigned, new_iv, valid) = fix::fix_unsign(&msg, read_mac_key, &read_iv); - read_iv = new_iv; if !valid { // Same rule as `Connection::unsign`: a frame that does not - // verify is not parsed, but the chain still advances — the - // next IV comes from the body rather than the signature. + // verify is neither parsed nor allowed to move the chain, + // since the IV it would move to is derived from a body the + // MAC just declined to vouch for. log::warn!("auth frame failed signature verification — dropped"); continue; } + read_iv = new_iv; unsigned } else { msg.clone() diff --git a/src/protocol/connection.rs b/src/protocol/connection.rs index 5401d49f..0fe28257 100644 --- a/src/protocol/connection.rs +++ b/src/protocol/connection.rs @@ -298,17 +298,26 @@ impl Connection { return Some(msg.to_vec()); } let (undistorted, new_iv, valid) = fix::fix_unsign(msg, &self.read_key, &self.read_iv); - // The chain advances either way. The next IV is derived from the body, - // not from the signature, so when only the signature is damaged this is - // the sender's true next IV and holding it back would desynchronise the - // next authentic frame — causing the poisoning it was meant to prevent. - // When the body is damaged the derived IV is wrong, but so is the one - // being held, and neither recovers. - self.read_iv = new_iv; if !valid { + // The chain is not advanced. `new_iv` is derived from the received + // body, and a failed MAC is exactly the statement that this body + // cannot be vouched for — so advancing would let one injected frame + // steer the receiver's chain state and drop every genuine frame + // after it. + // + // The cost is a genuine frame whose signature alone was damaged in + // transit: its body is intact, so `new_iv` would have been the + // sender's true next one, and holding it back leaves the connection + // stuck. That case is indistinguishable from an injection at this + // point, and a channel where a MAC failure has occurred is not one + // whose state can be inferred either way. Failing closed on + // unauthenticated input is the side to err on; the connection + // wanting a teardown rather than a guess is the real answer, and is + // a larger change than this. log::warn!("inbound frame failed signature verification — dropped"); return None; } + self.read_iv = new_iv; Some(undistorted) } @@ -722,32 +731,38 @@ mod tests { assert!(conn.unsign(&tampered).is_none(), "a tampered frame is dropped"); } - /// The chain has to survive a frame that is dropped. The next IV is derived - /// from the body rather than the signature, so a frame whose signature text - /// alone is damaged still yields the sender's true next IV — and withholding - /// it would desynchronise the following authentic frame, causing exactly the - /// poisoning that withholding was meant to prevent. + /// A frame that fails is dropped without moving the chain, so a genuine + /// frame after it still verifies. The batch is the case that matters — + /// good, bad, good — because an injected frame between two authentic ones + /// is what advancing on unauthenticated input would let poison the rest. #[test] - fn a_dropped_frame_does_not_poison_the_next_authentic_one() { + fn an_injected_frame_does_not_poison_the_authentic_ones_around_it() { let key = b"0123456789abcdef"; let iv0 = vec![0u8; 16]; - // Two frames as a sender would produce them: the second signed from the - // IV the first chained to. - let (mut first, iv1) = + // Two frames as a sender produces them: the second signed from the IV + // the first chained to. + let (first, iv1) = crate::protocol::fix::fix_sign(&fix_build(&[(35, "8")], 1), key, &iv0); let (second, _) = crate::protocol::fix::fix_sign(&fix_build(&[(35, "8")], 2), key, &iv1); - // Damage only the transmitted signature, leaving the body intact. - let at = find_subsequence(&first, b"\x018349=").expect("signed") + 6; - first[at] = if first[at] == b'0' { b'1' } else { b'0' }; + // Something else entirely, arriving between them. + let (mut injected, _) = + crate::protocol::fix::fix_sign(&fix_build(&[(35, "8"), (58, "x")], 9), b"wrongkey00000000", &iv1); + let at = find_subsequence(&injected, b"\x018349=").expect("signed") + 6; + injected[at] = if injected[at] == b'0' { b'1' } else { b'0' }; let mut conn = signed_conn(key, &iv0); - assert!(conn.unsign(&first).is_none(), "the damaged frame is dropped"); + assert!(conn.unsign(&first).is_some(), "the first authentic frame"); + assert_eq!(conn.read_iv, iv1, "and it advanced the chain"); + + assert!(conn.unsign(&injected).is_none(), "the injected frame is dropped"); + assert_eq!(conn.read_iv, iv1, "without moving the chain"); + assert!( conn.unsign(&second).is_some(), - "and the next authentic frame still verifies", + "so the authentic frame after it still verifies", ); } From 5c7c94fda4dd3c8cfc705e36eea4c0555f182264 Mon Sep 17 00:00:00 2001 From: userFRM Date: Thu, 30 Jul 2026 11:32:44 +0200 Subject: [PATCH 4/4] protocol: make the doc argue the reason the code actually has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The public doc on `unsign` still carried the rationale this branch disproved — that withholding the IV prevents a damaged frame from desynchronising the chain, needing no adversary. Under the policy the branch settled on, that case is withholding's *cost*, not its justification, which the comment twenty lines below states correctly. So the two argued opposite reasons for the same lines, and the doc is the one rustdoc renders. The pre-check that decides whether a frame is verified at all was also unpinned: reverting it to the un-anchored needle passed every test. The uncovered case is an unsigned frame quoting the tag in a field value — the anchored needle correctly declines to verify it, while the bare one routes it into verification, finds no signature field, and drops a legitimate frame. The signed-frame test cannot catch this, since its pre-check passes either way. Co-Authored-By: Claude Opus 5 (1M context) --- src/protocol/connection.rs | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/src/protocol/connection.rs b/src/protocol/connection.rs index 0fe28257..2f833cfa 100644 --- a/src/protocol/connection.rs +++ b/src/protocol/connection.rs @@ -279,11 +279,17 @@ impl Connection { /// account push — was applied exactly like an authentic one. Returning no /// message is the same information in a form a caller cannot ignore. /// - /// A failed frame also leaves the IV alone. Advancing it meant one bad - /// frame desynchronised the chain permanently, and since undistortion XORs - /// byte positions from that IV, every genuine frame after it was silently - /// corrupted before parsing. That part is a plain robustness bug: it needs - /// no adversary, only one damaged frame. + /// A failed frame also leaves the IV alone, because the IV it would advance + /// to is derived from the body the signature just failed to vouch for. + /// Advancing would let one injected frame steer the receiver's chain and + /// drop every genuine frame after it. + /// + /// This costs the case where a genuine frame is damaged in exactly its + /// signature: its body is intact, so the derived IV would have been the + /// sender's true next one, and the connection is left unable to verify what + /// follows until it reconnects. That case cannot be told apart from an + /// injection here, and the reasoning for preferring this side is set out + /// where the decision is made, below. pub fn unsign(&mut self, msg: &[u8]) -> Option> { if self.read_key.is_empty() { return Some(msg.to_vec()); // no signing configured @@ -783,6 +789,22 @@ mod tests { assert_eq!(conn.unsign(&plain), Some(plain), "no 8349 tag on the frame"); } + /// The same rule at the pre-check. An *unsigned* frame quoting the tag in a + /// value must not be routed into verification at all: it carries no + /// signature field, so it would be judged invalid and dropped. The signed + /// case below cannot catch this — its pre-check passes under either needle. + #[test] + fn an_unsigned_frame_quoting_the_signature_tag_is_not_verified() { + let quoting = fix_build(&[(35, "8"), (58, "rejected: 8349= missing")], 1); + + let mut conn = signed_conn(b"0123456789abcdef", &vec![0u8; 16]); + assert_eq!( + conn.unsign("ing), + Some(quoting.clone()), + "the tag text in a value is not a signature field", + ); + } + /// A signature is a field, not a substring. A legitimate signed frame whose /// *value* happens to contain the same text — a reject reason quoting it, /// say — was reported invalid, and enforcing that verdict would drop it.