Skip to content

🛡️ Sentinel: [HIGH] Buffer Overflow in Network Encoding due to missing bounds check #67

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
igorls merged 1 commit into from
Mar 9, 2026
Merged

🛡️ Sentinel: [HIGH] Buffer Overflow in Network Encoding due to missing bounds check #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,8 @@
**Vulnerability:** Intermediate key material (`secret` extracted from chaining key, and expanded `t*_plus` buffers) in `src/wireguard/crypto.zig`'s HKDF implementation (`kdf1`, `kdf2`, `kdf3`) was left on the stack without being zeroed. These buffers contain sensitive pseudorandom keys derived from the handshake chaining key.
**Learning:** Helper functions often create temporary buffers on the stack which persist until overwritten. Unlike the main handshake state struct, these are ephemeral but critical. Zig does not automatically zero stack variables on return.
**Prevention:** Always use `defer std.crypto.secureZero(u8, &var);` immediately after declaring any variable that will hold sensitive key material, especially in crypto primitives.

## 2026-10-24 - [HIGH] Buffer Overflow in Network Encoding due to missing bounds check
**Vulnerability:** In `src/protocol/codec.zig`, functions like `encodePing` and `encodeAck` did not verify if the output buffer (`buf`) was large enough to hold the encoded protocol message, including its dynamic number of gossip entries. The encoder could potentially write out-of-bounds if given a small buffer. Although `encodePing` is often passed a generously sized 1500-byte stack buffer (e.g. in `swim.zig`'s `broadcastLeave`), a large number of gossip entries (e.g., up to the maximum 255) could exceed this limit. 1500 bytes allows roughly 16 gossip entries ($16 \times 89 = 1424$), but the encoder was capping the number at 255, permitting up to ~22,000 bytes.
**Learning:** Even internal encoding functions that serialize structurally sound structs must perform bounds checking on the output slice, especially when elements are variable in length or quantity. The max count limit applied was logically sound for the protocol struct, but completely unbounded relative to the target byte slice length.
**Prevention:** Always use explicit bounds checking (`if (buf.len < required) return error.BufferTooShort;`) at the beginning of any serialization logic. Never assume the caller has provided an adequately sized buffer for unbounded or large slices.
9 changes: 9 additions & 0 deletions src/protocol/codec.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ const GOSSIP_ENTRY_SIZE = 32 + 1 + 8 + 1 + 4 + 2 + 1 + 32 + 1 + 4 + 2 + 1; // 89

/// Encode a Ping message into a buffer. Returns bytes written.
pub fn encodePing(buf: []u8, ping: messages.Ping) !usize {
const required = 1 + 32 + 8 + 1 + ping.gossip.len * GOSSIP_ENTRY_SIZE;
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

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

The required size calculation for encodePing uses ping.gossip.len * GOSSIP_ENTRY_SIZE, but the actual encoding loop at line 38 only writes @min(ping.gossip.len, 255) entries (gossip_count). If ping.gossip.len > 255, the bounds check will compute a required value that is much larger than what will actually be written, causing a spurious error.BufferTooShort even when the buffer is sufficient to hold all 255 gossip entries (up to ~22,755 bytes). In swim.zig, this error is silently swallowed with catch return, meaning pings would be silently dropped when ping.gossip.len > 255.

The required calculation should use @min(ping.gossip.len, 255) * GOSSIP_ENTRY_SIZE to correctly reflect how many bytes will actually be written.

Copilot uses AI. Check for mistakes.
if (buf.len < required) return error.BufferTooShort;

var pos: usize = 0;

// Type tag
Expand Down Expand Up @@ -45,6 +48,9 @@ pub fn encodePing(buf: []u8, ping: messages.Ping) !usize {

/// Encode an Ack message into a buffer. Returns bytes written.
pub fn encodeAck(buf: []u8, ack: messages.Ack) !usize {
const required = 1 + 32 + 8 + 1 + ack.gossip.len * GOSSIP_ENTRY_SIZE;
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

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

Same issue as in encodePing: the required size uses ack.gossip.len * GOSSIP_ENTRY_SIZE, but the encoder only writes @min(ack.gossip.len, 255) entries. If ack.gossip.len > 255, the check returns error.BufferTooShort even though the buffer may be large enough for all 255 entries. The required calculation should use @min(ack.gossip.len, 255) * GOSSIP_ENTRY_SIZE to match the actual bytes written.

Copilot uses AI. Check for mistakes.
if (buf.len < required) return error.BufferTooShort;

var pos: usize = 0;

buf[pos] = @intFromEnum(messages.MessageType.ack);
Expand All @@ -69,6 +75,9 @@ pub fn encodeAck(buf: []u8, ack: messages.Ack) !usize {

/// Encode a PingReq message.
pub fn encodePingReq(buf: []u8, req: messages.PingReq) !usize {
const required = 1 + 32 + 32 + 8;
if (buf.len < required) return error.BufferTooShort;

var pos: usize = 0;

buf[pos] = @intFromEnum(messages.MessageType.ping_req);
Expand Down