Skip to content

BFD doesn't guarantee local discriminators are unique or non-zero #865

Description

@taspelund

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

PeerInfo::with_random_discriminator() currently uses an unrestricted
random u32.

This permits:

  • discriminator zero, which RFC 5880 forbids; and
  • collisions with another active local BFD session.

The chance of zero is small for an individual session, but collisions
grow according to the birthday bound as the number of active sessions
increases. More importantly, the required invariant is not represented
or enforced anywhere.

An outgoing session with discriminator zero will send packets that a
compliant peer must discard.

Required behavior

RFC 5880 requires bfd.LocalDiscr to be:

  • nonzero; and
  • unique across all BFD sessions on the system.

It recommends selecting a random value subject to those constraints.

Proposed direction

  • Represent a local discriminator as NonZeroU32.
  • Allocate discriminators at the daemon level, where all active sessions
    are visible.
  • Generate random candidates until one is both nonzero and unused.
  • Release the discriminator when the session has completely shut down.

The packet wire representation can remain u32, but outgoing packet
construction should make an invalid value unrepresentable.

Testing

  • Generated discriminators are never zero.
  • Discriminators are unique across a large collection of concurrently
    active test sessions.
  • A forced RNG collision causes retry rather than duplicate allocation.
  • Removing a session permits safe eventual reuse after shutdown.

References

  • RFC 5880 §6.3
  • RFC 5880 §6.8.1

Context

This behavior predated the async implementation and was retained by #795
and #796.

Metadata

Metadata

Assignees

No one assigned

    Labels

    BugbfdBidirectional Forwarding DetectionmgdMaghemite daemonneeds testingrustPull requests that update rust code

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions