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}

[ton31337]

@Mergifyio backport stable/10.6 stable/10.5 stable/10.4

[mergify]

backport stable/10.6 stable/10.5 stable/10.4

✅ Backports have been created

Details

• #21223 bgpd: harden attribute parsing and packet handling in a few places (backport #21095) has been created for branch stable/10.6
• #21224 bgpd: harden attribute parsing and packet handling in a few places (backport #21095) has been created for branch stable/10.5 but encountered conflicts

Cherry-pick of f31897c has failed:

On branch mergify/bp/stable/10.5/pr-21095
Your branch is up to date with 'origin/stable/10.5'.

You are currently cherry-picking commit f31897c54.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   bgpd/bgp_packet.c

no changes added to commit (use "git add" and/or "git commit -a")

Cherry-pick of 5e55e12 has failed:

On branch mergify/bp/stable/10.5/pr-21095
Your branch is ahead of 'origin/stable/10.5' by 2 commits.
  (use "git push" to publish your local commits)

You are currently cherry-picking commit 5e55e12e4.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   bgpd/bgp_attr.c

no changes added to commit (use "git add" and/or "git commit -a")

Cherry-pick of 3e34a1a has failed:

On branch mergify/bp/stable/10.5/pr-21095
Your branch is ahead of 'origin/stable/10.5' by 3 commits.
  (use "git push" to publish your local commits)

You are currently cherry-picking commit 3e34a1a61.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   bgpd/bgp_packet.c

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

• #21225 bgpd: harden attribute parsing and packet handling in a few places (backport #21095) has been created for branch stable/10.4 but encountered conflicts

Cherry-pick of f31897c has failed:

On branch mergify/bp/stable/10.4/pr-21095
Your branch is up to date with 'origin/stable/10.4'.

You are currently cherry-picking commit f31897c54.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   bgpd/bgp_packet.c

no changes added to commit (use "git add" and/or "git commit -a")

Cherry-pick of 5e55e12 has failed:

On branch mergify/bp/stable/10.4/pr-21095
Your branch is ahead of 'origin/stable/10.4' by 2 commits.
  (use "git push" to publish your local commits)

You are currently cherry-picking commit 5e55e12e4.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   bgpd/bgp_attr.c

no changes added to commit (use "git add" and/or "git commit -a")

Cherry-pick of 3e34a1a has failed:

On branch mergify/bp/stable/10.4/pr-21095
Your branch is ahead of 'origin/stable/10.4' by 3 commits.
  (use "git push" to publish your local commits)

You are currently cherry-picking commit 3e34a1a61.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   bgpd/bgp_packet.c

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

[riw777]

looks good