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..ac2cb62d 100644 --- a/src/gateway.rs +++ b/src/gateway.rs @@ -279,10 +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); + 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 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 { @@ -565,7 +573,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 +594,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 +609,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..2f833cfa 100644 --- a/src/protocol/connection.rs +++ b/src/protocol/connection.rs @@ -271,19 +271,60 @@ 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, 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 (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) - if !msg.windows(5).any(|w| w == b"8349=") { - return (msg.to_vec(), true); + // 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(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); + 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; - (undistorted, valid) + Some(undistorted) } /// Build a FIX message, sign it, and send it. Increments seq and chains sign IV. @@ -664,4 +705,120 @@ 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"); + } + + /// 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 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 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); + + // 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_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(), + "so the authentic frame after it still verifies", + ); + } + + /// 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"); + } + + /// 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. + #[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],