bgpd: harden attribute parsing and packet handling in a few places

[ton31337]

No description provided.

[greptile-apps]

Greptile Summary

This PR applies five security fixes across BGP attribute parsing and packet handling to prevent buffer over-reads and incorrect continued processing after error conditions.

Key changes:

• bgp_packet.c — Off-by-3 pointer arithmetic fix (critical): bgp_dynamic_capability_graceful_restart computed end = pnt + hdr->length, but the data pointer is data = pnt + 3. The correct expression is end = data + hdr->length. With the old code, if hdr->length < 3, size_t len = end - data would underflow to a huge value, potentially enabling a remote attacker to trigger an unbounded buffer over-read via a crafted CAPABILITY message.

• bgp_packet.c — Missing return BGP_Stop after NOTIFY (Enhanced Route Refresh): After calling bgp_notify_send for an invalid msg_length, execution fell through into ORF parsing. The added return BGP_Stop halts processing as expected.

• bgp_packet.c — Missing return BGP_PACKET_NOOP for invalid subtype: A ROUTE-REFRESH with subtype > 2 was logged but then continued into ORF processing. Per RFC 7313, such messages must be silently ignored; the added return BGP_PACKET_NOOP enforces this.

• bgp_attr.c — Tunnel Encap sub-TLV pre-read length guards: Before reading the sub-TLV type and length bytes from the stream (1+1, 1+2, or 2+2 bytes depending on the branch), bounds checks against the declared attribute length are now performed, preventing reads beyond the attribute boundary.

• bgp_attr.c — AIGP TLV length validation: A check that tlv_length <= length is added before advancing the data pointer, preventing an over-advance that would cause subsequent loop iterations to read past the buffer. Note: the minimum 3-byte TLV header (type + 2-byte length) is still read before this guard, leaving a narrow pre-existing gap for inputs with fewer than 3 bytes remaining (see inline comment).

Confidence Score: 4/5

• PR is safe to merge; all five fixes correctly address real security vulnerabilities with no functional regressions introduced.
• All fixes are demonstrably correct: the pointer arithmetic correction in graceful restart eliminates a potential size_t underflow, the missing return statements prevent use-after-notification processing, and the pre-read length guards in encap/AIGP parsing close clear buffer over-read paths. The only minor gap is in the AIGP TLV path where the 3-byte header itself is not validated before the reads, but this is a pre-existing issue not introduced by this PR.
• No files require special attention beyond the minor pre-existing AIGP TLV header read gap noted in bgpd/bgp_attr.c.

Important Files Changed

Filename	Overview
bgpd/bgp_attr.c	Adds pre-read bounds checks for Tunnel Encap sub-TLV parsing (3 branches: subtype<128, subtype>=128, VNC) and AIGP TLV length validation. The Tunnel Encap guards are correct and complete. The AIGP check guards against tlv_length > length, but does not first verify the 3-byte TLV header is available before calling ptr_get_be16(data+1, ...), leaving a narrow pre-existing gap.
bgpd/bgp_packet.c	Three distinct fixes: (1) adds missing return BGP_Stop after bgp_notify_send for invalid Enhanced Route Refresh message length, preventing continued processing of a malformed message; (2) adds missing return BGP_PACKET_NOOP after logging an invalid subtype (>2), correctly ignoring the message per RFC 7313; (3) fixes a critical off-by-3 pointer arithmetic bug in bgp_dynamic_capability_graceful_restart where end was pnt + hdr->length instead of the correct data + hdr->length (data = pnt + 3), which could cause a size_t underflow and subsequent buffer over-read when hdr->length < 3.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Receive BGP UPDATE / CAPABILITY / ROUTE-REFRESH] --> B{Message type?}

    B -->|ROUTE-REFRESH| C{subtype != 0?}
    C -->|Yes| D{msg_length == 4?}
    D -->|No| E[bgp_notify_send INVALID_MSG_LEN\n+ return BGP_Stop ✅ FIXED]
    D -->|Yes| F{subtype > 2?}
    F -->|Yes| G[flog_err + return BGP_PACKET_NOOP ✅ FIXED]
    F -->|No| H[Process ORF data]

    B -->|CAPABILITY: GRACEFUL_RESTART| I[data = pnt + 3\nend = data + hdr->length ✅ FIXED\nlen = end - data]
    I --> J{len < 2?}
    J -->|Yes| K[Error: invalid length, return]
    J -->|No| L[Parse gr_restart_flag_time\nIterate AFI/SAFI entries]

    B -->|UPDATE: AIGP attr| M[bgp_attr_aigp_get_tlv_metric]
    M --> N{length >= tlv_length? ✅ FIXED}
    N -->|No| O[flog_err + return false]
    N -->|Yes| P[Read metric value]

    B -->|UPDATE: Tunnel Encap attr| Q{subtype < 128?}
    Q -->|Yes| R{length >= 2? ✅ FIXED}
    R -->|No| S[bgp_attr_malformed]
    R -->|Yes| T[Read 1-byte sublength]
    Q -->|No| U{length >= 3? ✅ FIXED}
    U -->|No| S
    U -->|Yes| V[Read 2-byte sublength]

Loading

Comments Outside Diff (1)

1. bgpd/bgp_attr.c, line 459-468 (link)

   Minimum TLV header size not checked before reads

   The new bounds check (length < tlv_length) is added after *data (1 byte) and ptr_get_be16(data + 1, ...) (2 bytes) are already read. If the remaining length is less than 3 bytes, the reads at data + 1 and data + 2 may access out-of-bounds memory before the new guard triggers.

   A pre-read check for the minimum 3-byte TLV header would fully close the gap:

_{Last reviewed commit: 841d407}

[donaldsharp]

Medium: 62da4d0 still leaves a remotely triggerable session-reset path for unknown Enhanced Route Refresh subtypes. The new early returns stop parsing after bgp_notify_send(), which is good, but the checks are still ordered so that msg_length != 4 is evaluated before subtype > 2. That means an attacker can send subtype 3+ with a non-4-byte payload and still force BGP_Stop, even though the code comment says unknown subtypes "MUST ignore" the message.

[ton31337]

How about now?

[donaldsharp]

Low: e0c7d40 fixes the length -= 2/3/4 underflow, but it still reads the sub-TLV subtype before proving there is even 1 byte of attribute payload left. If length == 0 and there are more bytes later in the packet, stream_getc() will still consume one byte from the following attribute before the malformed path runs. bgp_attr_malformed() later restores getp to the declared end of the attribute, so this is probably not exploitable memory corruption, but the commit does not fully eliminate cross-attribute reads on malformed input.

[ton31337]

How about now?