oxidecomputer / oxidecomputer/maghemite

BFD should implement receive validation and discriminator checks

Open
#860 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bfd Bug mgd needs testing rust
Dominant language
Rust
Stars
94
Forks
6
Avg merge
1d 8h
Merged PRs (30d)
12

Description

The following is an issue identified by AI when I asked it to do an adversarial code review of #795. This was pre-existing in the sync/pthread impl, which is why it was not addressed as part of the async swap-out.

Problem

bfd::packet::Control::from_bytes() currently validates only that:

  • the UDP payload contains at least 24 bytes; and
  • Detect Mult is nonzero.

The receive path accepts packets that RFC 5880 §6.8.6 requires us to
discard, including packets with:

  • a version other than 1;
  • a Length field smaller than the mandatory header;
  • a Length field larger than the UDP payload;
  • a zero My Discriminator;
  • an unmatched Your Discriminator;
  • the Multipoint bit set;
  • the Authentication bit set despite authentication not being in use;
  • both Poll and Final set.

After parsing, packets are initially selected by source IP and passed to
the state machine. The state machine updates remote parameters, resets
the receive deadline, and may transition state before any
session-dependent discriminator validation occurs.

Impact

A malformed or stale packet from the configured source IP can:

  • bring a Down session Up;
  • force an Up session Down;
  • indefinitely refresh the receive deadline;
  • replace the learned remote discriminator;
  • change transmission behavior through forged timer or Demand fields.

The unit tests currently reinforce part of this behavior:
Control::default() has my_discriminator = 0, and session tests use
such packets as normal handshake traffic.

Required behavior

Implement the ordered validation from RFC 5880 §6.8.6.

Structural checks should include:

  • Version == 1;
  • valid advertised Length relative to authentication and UDP payload;
  • Detect Mult != 0;
  • Multipoint == 0;
  • Poll and Final are not both set;
  • reject Authentication-present packets while authentication is not
    configured/supported.

Session-dependent checks should include:

  • My Discriminator != 0;
  • nonzero Your Discriminator selects or matches the local session;
  • zero Your Discriminator is accepted only for Down or AdminDown initial
    packets;
  • invalid packets must not update timers, remote parameters, counters
    that represent accepted packets, or session state.

Receive-side TTL/Hop-Limit validation for single-hop sessions is tracked
separately in #656.

Testing

  • Replace zero-discriminator handshake fixtures with valid packets.
  • Add one rejection test for each RFC validation rule.
  • Verify discarded packets do not reset the detection deadline.
  • Verify an incorrect nonzero Your Discriminator cannot alter session
    state.
  • Verify a zero Your Discriminator with Init or Up is discarded.

References

  • RFC 5880 §6.3: discriminator-based demultiplexing
  • RFC 5880 §6.5: Poll Sequence
  • RFC 5880 §6.8.6: reception of BFD Control packets
  • #656: single-hop TTL/Hop-Limit validation

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with bfd::packet::Control::from_bytes() and trace the receive path into the state machine; review the session tests that use Control::default() as handshake traffic. Use RFC 5880 §6.8.6 to map the required validation order. Done means each listed malformed packet is rejected, valid fixtures use nonzero discriminators, and rejected packets do not alter timers, counters, remote parameters, or session state.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.