Skip to content

ripd: add full RTE bounds check to response/request processing loops#21889

Open
DeadPackets wants to merge 1 commit into
FRRouting:masterfrom
DeadPackets:fix/ripd-rte-bounds-check
Open

ripd: add full RTE bounds check to response/request processing loops#21889
DeadPackets wants to merge 1 commit into
FRRouting:masterfrom
DeadPackets:fix/ripd-rte-bounds-check

Conversation

@DeadPackets
Copy link
Copy Markdown

Summary

Adds a full RTE bounds check to the RIP response/request processing loops in ripd/ripd.c.

The RTE iteration loops in rip_response_process(), rip_request_process(), and rip_packet_dump() check only that the start of each struct rte (20 bytes) is within bounds, not that the full struct fits. While currently protected by an upstream alignment check in rip_read(), adding an explicit bounds check provides defense-in-depth and guards against future refactoring that might remove or alter the caller's validation.

Changes

  • rip_response_process() (line 1188): (caddr_t)rte < lim(caddr_t)(rte + 1) <= lim
  • rip_request_process() (line 1721): ((caddr_t)rte) < lim((caddr_t)(rte + 1)) <= lim
  • rip_packet_dump() (line 740): (caddr_t)rte < lim(caddr_t)(rte + 1) <= lim

Testing

  • Compiled successfully with --enable-ripd on Ubuntu 24.04 / gcc 13.3.0

Related

No existing issues or CVEs for this specific pattern. This is a proactive hardening change.

The RTE iteration loops in rip_response_process(), rip_request_process(),
and rip_packet_dump() check only that the start of each struct rte (20
bytes) is within bounds, not that the full struct fits. While currently
protected by an alignment check in rip_read(), adding an explicit
bounds check provides defense-in-depth and guards against future
refactoring that might remove or alter the caller's validation.

Change the loop condition from (caddr_t)rte < lim to
(caddr_t)(rte + 1) <= lim in all three functions.

Signed-off-by: vulnagent3 <vulnagent3@oss-vuln-research.local>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 8, 2026

Greptile Summary

This PR tightens the RTE iteration loop bounds in rip_packet_dump(), rip_response_process(), and rip_request_process() by changing the guard from (caddr_t)rte < lim to (caddr_t)(rte + 1) <= lim, ensuring each iteration verifies a full 20-byte struct rte fits before accessing it. For correctly aligned packets (guaranteed by rip_read()'s % 20 check) the behaviour is identical; the change adds safety for any future code path that bypasses that upstream guard.

  • Loop bounds (rip_packet_dump, rip_response_process, rip_request_process): all three iteration loops now check that rte + 1 <= lim (full struct in bounds) rather than just rte < lim (start of struct in bounds).
  • Pre-loop accesses in rip_request_process (lines 1696–1708): the early-exit guard lim == (caddr_t)rte is not updated, leaving the rte->family/rte->metric reads outside the loop unguarded by a full-struct check — a minor inconsistency with the PR's own defense-in-depth rationale.

Confidence Score: 4/5

Safe to merge; the loop condition changes are semantically correct for all packets that reach these functions, and the upstream alignment gate remains unchanged.

The three loop fixes are correct and produce identical behaviour for well-formed packets while closing a theoretical over-read for misaligned ones. The one gap is that rip_request_process still reads rte->family and rte->metric before the loop without a full-struct check, leaving a small inconsistency with the PR's own hardening intent.

ripd/ripd.c — specifically the pre-loop RTE field reads in rip_request_process around line 1703.

Important Files Changed

Filename Overview
ripd/ripd.c Three loop bounds conditions tightened from rte < lim to (rte+1) <= lim; pre-loop RTE field access in rip_request_process still guarded only by an exact-equality empty check rather than a full-struct check

Comments Outside Diff (1)

  1. ripd/ripd.c, line 1696-1708 (link)

    P2 Pre-loop RTE access lacks full-struct bounds guard

    The PR closes the loop-iteration gap, but the pre-loop accesses of rte->family (line 1703, 1708) and rte->metric (line 1703) are protected only by lim == (caddr_t)rte, which returns only on an exactly-empty packet. A crafted packet where lim > (caddr_t)rte but lim < (caddr_t)(rte + 1) (i.e., 1–19 trailing bytes) would pass this check and read from a partial struct before either loop body is reached. Today this is blocked by rip_read()'s 20-byte alignment check, but by the same defense-in-depth logic motivating this PR the early-exit guard should be (caddr_t)(rte + 1) > lim rather than lim == (caddr_t)rte.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: ripd/ripd.c
    Line: 1696-1708
    
    Comment:
    **Pre-loop RTE access lacks full-struct bounds guard**
    
    The PR closes the loop-iteration gap, but the pre-loop accesses of `rte->family` (line 1703, 1708) and `rte->metric` (line 1703) are protected only by `lim == (caddr_t)rte`, which returns only on an exactly-empty packet. A crafted packet where `lim > (caddr_t)rte` but `lim < (caddr_t)(rte + 1)` (i.e., 1–19 trailing bytes) would pass this check and read from a partial struct before either loop body is reached. Today this is blocked by `rip_read()`'s 20-byte alignment check, but by the same defense-in-depth logic motivating this PR the early-exit guard should be `(caddr_t)(rte + 1) > lim` rather than `lim == (caddr_t)rte`.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
ripd/ripd.c:1696-1708
**Pre-loop RTE access lacks full-struct bounds guard**

The PR closes the loop-iteration gap, but the pre-loop accesses of `rte->family` (line 1703, 1708) and `rte->metric` (line 1703) are protected only by `lim == (caddr_t)rte`, which returns only on an exactly-empty packet. A crafted packet where `lim > (caddr_t)rte` but `lim < (caddr_t)(rte + 1)` (i.e., 1–19 trailing bytes) would pass this check and read from a partial struct before either loop body is reached. Today this is blocked by `rip_read()`'s 20-byte alignment check, but by the same defense-in-depth logic motivating this PR the early-exit guard should be `(caddr_t)(rte + 1) > lim` rather than `lim == (caddr_t)rte`.

Reviews (1): Last reviewed commit: "ripd: add full RTE bounds check to proce..." | Re-trigger Greptile

@mjstapp
Copy link
Copy Markdown
Contributor

mjstapp commented May 8, 2026

so I have to ask: are you an actual human being? the sign-off in the commit here sounds like a program, not a person.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants