Skip to content

tower: fix serdes bounds checks#8629

Draft
nlgripto wants to merge 1 commit intofiredancer-io:mainfrom
nlgripto:fix/tower-serdes-fuzzer
Draft

tower: fix serdes bounds checks#8629
nlgripto wants to merge 1 commit intofiredancer-io:mainfrom
nlgripto:fix/tower-serdes-fuzzer

Conversation

@nlgripto
Copy link
Contributor

@nlgripto nlgripto commented Mar 2, 2026

Add bounds-safe de_short_u16/de_var_int with src_sz parameter to prevent OOB reads on untrusted input.

Copilot AI review requested due to automatic review settings March 2, 2026 20:52
@nlgripto nlgripto changed the title tower: fix serdes bounds checks;add roundtrip tower: fix serdes bounds checks Mar 2, 2026
@nlgripto nlgripto force-pushed the fix/tower-serdes-fuzzer branch from ac458ba to 34bbb05 Compare March 2, 2026 20:55
@nlgripto
Copy link
Contributor Author

nlgripto commented Mar 2, 2026

@ripatel-fd since you wrote the fuzzer

Add bounds-safe de_short_u16/de_var_int with src_sz parameter to
prevent OOB reads on untrusted input.
@nlgripto nlgripto force-pushed the fix/tower-serdes-fuzzer branch from 34bbb05 to 619f89f Compare March 2, 2026 20:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the CompactTowerSync (tower) serializer/deserializer against out-of-bounds reads on untrusted input and strengthens the fuzz harness by validating a deserialize→serialize→deserialize roundtrip.

Changes:

  • Add src_sz bounds checks to de_short_u16 and update fd_compact_tower_sync_de to fail on truncated inputs.
  • Expand the fuzz harness buffer and add roundtrip equality assertions to improve serdes validation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/choreo/tower/fuzz_tower_serdes.c Adds roundtrip verification and increases serialization buffer size for fuzzing.
src/choreo/tower/fd_tower_serdes.c Makes ShortU16 decoding length-aware and propagates decode failures instead of reading past buffer end.
Comments suppressed due to low confidence (1)

src/choreo/tower/fuzz_tower_serdes.c:42

  • Add a guard/assert that out_sz <= sizeof(buf) after serialization and before using out_sz as the size for the second deserialize. If the serializer ever returns success while producing an out_sz larger than the provided buffer, this harness would pass an invalid buf_sz to fd_compact_tower_sync_de (which assumes the buffer is at least buf_sz bytes), potentially causing an out-of-bounds read in the fuzzer itself.
  uchar buf[4096];
  ulong out_sz = 0;

  int ser_err = fd_compact_tower_sync_ser( serde, buf, sizeof(buf), &out_sz );
  assert( !ser_err );

if( FD_UNLIKELY( src_sz<2UL ) ) return 0;
else if( FD_LIKELY( !(0x80U & src[1]) ) ) { *dst = (ushort)((ulong)(src[0]&0x7FUL) + (((ulong)src[1])<<7)); return 2; }
if( FD_UNLIKELY( src_sz<3UL ) ) return 0;
else { *dst = (ushort)((ulong)(src[0]&0x7FUL) + (((ulong)(src[1]&0x7FUL))<<7) + (((ulong)src[2])<<14)); return 3; }
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the 3-byte decode path, src[2] is incorporated without masking/validating and the result is cast to ushort. Inputs where src[2] has bits outside the valid ShortU16 range (e.g. src[2]>3 or src[2]&0x80) will wrap/truncate into an unrelated lockouts_cnt value and can bypass the later max check. Consider enforcing canonical ShortU16 constraints (final byte must terminate and the overall value must fit in ushort).

Suggested change
else { *dst = (ushort)((ulong)(src[0]&0x7FUL) + (((ulong)(src[1]&0x7FUL))<<7) + (((ulong)src[2])<<14)); return 3; }
else {
/* Third byte must terminate (MSB clear) and only use low 2 bits
to keep the value within the 16-bit range and enforce a
canonical ShortU16 encoding. */
if( FD_UNLIKELY( src[2] & 0xFCU ) ) return 0;
ulong val = (ulong)(src[0] & 0x7FUL)
+ (((ulong)(src[1] & 0x7FUL)) << 7)
+ (((ulong)src[2]) << 14);
if( FD_UNLIKELY( val>65535UL ) ) return 0;
*dst = (ushort)val;
return 3;
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants