Skip to content

fix(inputs): bound InputMessage end_tick lookahead (DoS)#1525

Merged
cBournhonesque merged 1 commit into
cBournhonesque:mainfrom
mmannerm:fix/input-buffer-end-tick-bound
Jun 24, 2026
Merged

fix(inputs): bound InputMessage end_tick lookahead (DoS)#1525
cBournhonesque merged 1 commit into
cBournhonesque:mainfrom
mmannerm:fix/input-buffer-end-tick-bound

Conversation

@mmannerm

Copy link
Copy Markdown
Contributor

Summary

Splits the DoS half out of #1476 into its own focused PR.

A malicious/modified client can send an InputMessage with a far-future end_tick (e.g. server_tick + 30_000). On the server receive path, InputBuffer::set_raw / extend_to_range extend the internal VecDeque to fit that tick value, filling intermediate entries with Absent / SameAsPrecedentone allocation per intermediate tick. A single forged message allocates ~30k entries; repeated across messages and connections, the server is memory-exhausted.

Fix

Reject input messages whose end_tick is implausibly far from the server's current tick before any buffer write, via a new is_input_within_lookahead helper in lightyear_inputs::server::receive_input_message:

pub(crate) fn is_input_within_lookahead(end_tick: Tick, server_tick: Tick) -> bool {
    let delta = end_tick - server_tick;
    (-MAX_INPUT_PAST_TICKS..=MAX_INPUT_LOOKAHEAD_TICKS).contains(&delta)
}
  • MAX_INPUT_LOOKAHEAD_TICKS = 64 (~1 s at 64 Hz). Legitimate clients run at most a few ticks ahead of the server (typical InputDelayConfig is 0–3 ticks), so this is generous.
  • MAX_INPUT_PAST_TICKS = 256. The symmetric past bound is what makes the check safe under wrap-around: since Tick is u32 and Tick - Tick returns i32, an end_tick = server_tick + 2^31 wraps the delta to i32::MIN, which a forward-only delta <= MAX check would accept. Rejecting both ends eliminates that.

Tests

  • Unit tests for the bound: forward-inclusive, past-inclusive, far-future rejection, and the i32::MIN wrap-around case.
  • End-to-end regression test (test_input_message_with_huge_end_tick_does_not_allocate_unbounded_buffer) that forges a message via the public MessageSender::send::<InputChannel> API — modelling exactly what a modified client binary could do — and asserts the server's InputBuffer stays bounded.

Verified red→green on current main: without the bound the server allocates 29 998 buffer entries from the single forged message; with it the buffer stays < 1000.

Notes

A malicious client can send an `InputMessage` with a far-future
`end_tick` (e.g. `server_tick + 30_000`). `InputBuffer::set_raw` /
`extend_to_range` then extend the internal `VecDeque` to fit that tick,
allocating one entry per intermediate tick — ~30k entries from a single
message, repeatable across messages and connections (memory-exhaustion
DoS).

Reject input messages whose `end_tick` falls outside
`[server_tick - 256, server_tick + 64]` before any buffer write, via a
new `is_input_within_lookahead` helper in the server receive path.
Legitimate clients run at most a few ticks ahead (typical input-delay is
0–3 ticks), so the window is generous. The symmetric past bound also
closes a wrap-around case: `end_tick = server_tick + 2^31` makes
`Tick - Tick` wrap to `i32::MIN`, which a forward-only `delta <= MAX`
check would accept.

Tests:
- unit tests for the bound (forward, past, far-future, i32::MIN wrap);
- an end-to-end regression test sending a forged message via the public
  `MessageSender::send::<InputChannel>` API and asserting the server's
  InputBuffer stays bounded (29998 entries without the fix → <1000 with).

Split out from cBournhonesque#1476 (the input-target authorization half follows in a
separate PR).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@cBournhonesque

Copy link
Copy Markdown
Owner

This works, but rather than just providing specific DoS protections, do you have an idea for providing an API where the user would inspect/validate the inputs?
I.e. a hook that would run on the InputMessages after deserialization but before they are handled.

///
/// Past-direction messages are normally handled harmlessly by
/// [`InputBuffer::set_raw`]'s start-tick guard. The reason we still bound them:
/// `Tick - Tick` returns `i32` via signed wrapping arithmetic over `u32` tick

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's keep this for now, but I don't think there is any point of making Tick wrapping now that it's a u32; I should just make it a normal u32.

@cBournhonesque cBournhonesque merged commit 17c465d into cBournhonesque:main Jun 24, 2026
4 checks passed
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