bgpd: Recent bugs for 10.6

[ton31337]

No description provided.

[Jafaral]

@greptile review

[greptile-apps]

Greptile Summary

This PR backports several targeted bug fixes to stable/10.6, covering AS_PATH reconciliation correctness, martian next-hop filtering, BGP hostname capability parsing robustness, an error-buffer overflow guard, and a route-refresh AFI/SAFI activity check.

• bgp_aspath.c — AS4_PATH reconciliation now correctly subtracts confederation hop counts from both paths instead of only the 2-octet AS_PATH, and the hops < 0 case returns aspath_dup(aspath) immediately per RFC 6793 §4.1.3 instead of continuing with a fabricated hop count.
• bgp_attr.c — IPv4 next-hops received in MP_REACH_NLRI are now rejected with BGP_ATTR_PARSE_WITHDRAW when ipv4_martian() returns true (and allow_martian is not set), consistent with the existing NEXT_HOP attribute check. Note this check also fires for BGP_ATTR_NHLEN_VPNV4 via the existing fallthrough — martian addresses are equally invalid for VPN next-hops, so this is intentional and correct.
• bgp_open.c — Two fixes in bgp_capability_hostname: a pre-read bounds check guards against reading the length byte when the capability body is already exhausted, and an explicit !len check rejects a zero-length hostname. XFREE(peer->hostname) is now called on both domain-name error paths so no partially-parsed hostname leaks. In bgp_capability_parse, a new error_end parameter prevents memcpy from writing past the end of error_data when accumulating unsupported capability data.
• bgp_packet.c — A new early-exit guard returns BGP_PACKET_NOOP when peer->afc_nego[afi][safi] is false for the requested AFI/SAFI, making the subsequent peer_active_nego() wrapper redundant; that wrapper is removed, simplifying the enhanced-refresh stale-path logic.
• tests/bgpd/test_aspath.c — Expected reconciliation results updated to reflect the corrected RFC 6793 behavior (AS_PATH returned as-is when shorter than AS4_PATH).

Confidence Score: 4/5

• This PR is safe to merge; it fixes real bugs with tight, well-scoped changes and updated tests.
• All five changes are targeted fixes to pre-existing bugs: memory-safety issues, an RFC compliance defect, a security improvement, and a redundant check removal. The test suite is updated to match the corrected behavior. No new logic paths are introduced that could regress unrelated functionality. Minor caution: the martian check now also fires for VPNv4 next-hops via fallthrough (acceptable, but worth verifying in environments with unusual VPN topologies).
• bgpd/bgp_open.c — the hostname capability parsing has several layered early-returns; reviewers should trace the XFREE/XSTRDUP lifecycle carefully to confirm no double-free or use-after-free exists in all paths.

Important Files Changed

Filename	Overview
bgpd/bgp_aspath.c	RFC 6793–compliant fix for AS_PATH/AS4_PATH reconciliation: confed hops are now counted in both sides of the subtraction, and a shorter AS_PATH now correctly returns as-is instead of continuing with corrupted hop arithmetic.
bgpd/bgp_attr.c	Adds martian IPv4 next-hop rejection for MP_REACH_NLRI (consistent with existing NEXT_HOP attribute validation); check triggers for both BGP_ATTR_NHLEN_IPV4 and BGP_ATTR_NHLEN_VPNV4 via fallthrough — both are valid targets for the check.
bgpd/bgp_open.c	Multiple robustness fixes: bounds-check before reading the hostname length byte, explicit rejection of zero-length hostname, proper memory cleanup (XFREE peer->hostname) on error paths, and an error-buffer overflow guard in bgp_capability_parse via the new error_end parameter.
bgpd/bgp_packet.c	Adds an early-return guard when the peer has not negotiated the requested AFI/SAFI, then removes the now-redundant peer_active_nego() wrapper around PEER_STATUS_ENHANCED_REFRESH / bgp_set_stale_route since afc_nego[afi][safi] already guarantees activity.
tests/bgpd/test_aspath.c	Test expectations updated to match RFC 6793 behavior: when AS_PATH has fewer hops than AS4_PATH the AS_PATH is returned as-is, so the reconciled path is now the shorter AS_PATH rather than the previously expected stitched-together path.

Sequence Diagram

sequenceDiagram
    participant Peer as BGP Peer
    participant Open as bgp_open_option_parse
    participant Cap as bgp_capability_parse(+error_end)
    participant Hostname as bgp_capability_hostname
    participant Attr as bgp_mp_reach_parse
    participant AS4 as aspath_reconcile_as4

    Peer->>Open: OPEN message
    Open->>Cap: parse capabilities (error_data, error_end=error_data+MAX_PKT_SIZE)
    Cap->>Hostname: CAPABILITY_CODE_FQDN
    Note over Hostname: 1) check len byte readable<br/>2) reject empty (len==0)<br/>3) bounds-check hostname body<br/>4) XFREE old hostname/domainname<br/>5) XFREE hostname on domainname error
    Hostname-->>Cap: 0 or -1
    Note over Cap: bounds-guard before memcpy(*error)<br/>if (*error + cap_len+2 <= error_end)
    Cap-->>Open: ret

    Peer->>Attr: UPDATE w/ MP_REACH_NLRI (IPv4 NH)
    Note over Attr: After reading NH bytes:<br/>ipv4_martian() && !allow_martian<br/>→ BGP_ATTR_PARSE_WITHDRAW

    Peer->>AS4: AS_PATH + AS4_PATH
    Note over AS4: hops = (AS_PATH hops+confeds)<br/>       - (AS4_PATH hops+confeds)<br/>if hops < 0 → return dup(AS_PATH)<br/>(RFC 6793 §4.1.3)
    AS4-->>Peer: reconciled path

    Peer->>Attr: ROUTE-REFRESH (BoRR) for AFI/SAFI
    Note over Attr: if !afc_nego[afi][safi] → NOOP<br/>else: SET ENHANCED_REFRESH<br/>     bgp_set_stale_route() (no peer_active_nego guard)

Loading

_{Last reviewed commit: "bgpd: Free hostname ..."}