Add the frame length validator, without calling it yet

The received frame header carries a payload length that nothing reads. This
adds the function that says what that field should hold for a given frame
kind, so the next change can compare the two and drop a frame whose header
disagrees with what actually arrived.

No call site yet, deliberately: this commit changes no behaviour, and the
function carries a dead-code annotation that the change adding the call
removes with it. I checked that the annotation is doing real work rather than
assuming so — removing it fails the lint build.

Each frame kind gets its own arm because there is no shared convention. The
handshake frames count everything after the common prefix; the established
frame counts only the inner plaintext. An unrecognised phase returns nothing
rather than a guess, so a caller skips the frame instead of rejecting it.

Three tests come with it, each broken to confirm it can fail. One catches the
wrong convention specifically, reproducing the value a validator written from
the field's looser description would have computed.
This commit is contained in:
Johnathan Corgan
2026-08-16 16:34:40 +00:00
parent ee1c624cef
commit 2d3ddf7188
2 changed files with 94 additions and 0 deletions
+73
View File
@@ -66,6 +66,43 @@ pub const FLAG_CE: u8 = 0x02;
/// Spin bit for RTT measurement.
pub const FLAG_SP: u8 = 0x04;
// ============================================================================
// Wire Length Validation
// ============================================================================
/// Expected `payload_len` for a packet of `phase` and total wire length
/// `total`, or `None` if the phase carries no fixed relationship.
///
/// Each frame kind states its own relationship; there is no single shared
/// convention. The handshake frames count everything after the 4-byte common
/// prefix. The established frame counts only the inner plaintext, excluding
/// both the 16-byte header and the AEAD tag, which is what
/// [`CommonPrefix::payload_len`] documents.
///
/// The handshake arms are built from the same constants the encoders use; the
/// established arm is not, since no encoder reads `ENCRYPTED_MIN_SIZE`. Neither
/// shape stops a one-sided edit from making the two disagree, which is what the
/// tests below exist to catch.
///
/// A `None` from the established arm means "no fixed relationship", so a caller
/// skips such a frame rather than rejecting it. That is the safe direction: a
/// truncating cast here would produce a false rejection instead.
// Nothing calls this yet. The dispatch point that calls it arrives in the
// following commit, which removes this annotation with it. `CommonPrefix::flags`
// and `EncryptedHeader::payload_len` below carry the annotation for the same
// reason.
#[allow(dead_code)]
pub fn expected_payload_len(phase: u8, total: usize) -> Option<u16> {
match phase {
PHASE_MSG1 => Some((MSG1_WIRE_SIZE - COMMON_PREFIX_SIZE) as u16),
PHASE_MSG2 => Some((MSG2_WIRE_SIZE - COMMON_PREFIX_SIZE) as u16),
PHASE_ESTABLISHED => total
.checked_sub(ENCRYPTED_MIN_SIZE)
.and_then(|n| u16::try_from(n).ok()),
_ => None,
}
}
// ============================================================================
// Common Prefix
// ============================================================================
@@ -623,4 +660,40 @@ mod tests {
// payload_len = sender_idx(4) + receiver_idx(4) + noise_msg2(57) = 65
assert_eq!(prefix.payload_len, 65);
}
#[test]
fn expected_payload_len_for_an_established_frame_excludes_header_and_tag() {
let header = build_established_header(SessionIndex::new(7), 0, 0, 40);
let frame = build_encrypted(&header, &[0u8; 40 + TAG_SIZE]);
// 16 header + 40 plaintext + 16 tag = 72 on the wire, declaring 40.
assert_eq!(frame.len(), ESTABLISHED_HEADER_SIZE + 40 + TAG_SIZE);
assert_eq!(
expected_payload_len(PHASE_ESTABLISHED, frame.len()),
Some(40)
);
}
#[test]
fn expected_payload_len_matches_what_build_msg1_and_build_msg2_actually_emit() {
// The Noise buffers are sized with literals rather than
// HANDSHAKE_MSG1_SIZE / HANDSHAKE_MSG2_SIZE, unlike the two tests
// above. With the constant on both sides, changing it would move the
// encoder's payload_len and the validator's expectation together and
// leave this test green. Pinning the input keeps the two sides
// independent so a one-sided change is caught.
let msg1 = build_msg1(SessionIndex::new(1), &[0u8; 106]);
let prefix = CommonPrefix::parse(&msg1).unwrap();
assert_eq!(
expected_payload_len(PHASE_MSG1, msg1.len()),
Some(prefix.payload_len)
);
let msg2 = build_msg2(SessionIndex::new(1), SessionIndex::new(2), &[0u8; 57]);
let prefix = CommonPrefix::parse(&msg2).unwrap();
assert_eq!(
expected_payload_len(PHASE_MSG2, msg2.len()),
Some(prefix.payload_len)
);
}
}
+21
View File
@@ -229,6 +229,27 @@ mod tests {
frame
}
/// The wire sizes above are written as literals, independently of the FMP
/// wire module, which derives the same values from the Noise message sizes.
/// Keeping them independent is deliberate: this module takes no dependency
/// on `crate::node`, and the import below exists only under `cfg(test)`.
/// The cost is that the two can drift. A wrong literal on this side also
/// breaks the TCP integration tests, since they move real frames through
/// this reader; a wrong value on the wire-module side does not reach here
/// at all, and this test is what catches that direction.
#[test]
fn stream_reader_constants_agree_with_the_fmp_wire_module() {
use crate::node::wire;
assert_eq!(MSG1_WIRE_SIZE, wire::MSG1_WIRE_SIZE);
assert_eq!(MSG2_WIRE_SIZE, wire::MSG2_WIRE_SIZE);
assert_eq!(PREFIX_SIZE, wire::COMMON_PREFIX_SIZE);
assert_eq!(
ESTABLISHED_REMAINING_HEADER + PREFIX_SIZE,
wire::ESTABLISHED_HEADER_SIZE
);
}
#[tokio::test]
async fn test_read_established_frame() {
let payload_len = 64u16;