bgpd: add STREAM_READABLE pre-check in bgp_open_receive by guoguojia2021 · Pull Request #22277 · FRRouting/frr

guoguojia2021 · 2026-06-09T11:40:49Z

bgp_open_receive() immediately calls stream_getc(), stream_getw(), and stream_get_ipv4() to parse the OPEN message fixed fields (version, AS number, hold time, BGP Identifier, opt parm len) without first verifying that the stream contains at least 10 bytes of readable data.

While bgp_io.c validates BGP_MSG_OPEN_MIN_SIZE at the I/O layer, that check uses the total packet size including the 19-byte header. If a bug or future refactor changes how the stream getp is positioned before calling bgp_open_receive(), the stream_get*() calls could read past the end of valid data. The stream functions have internal assertions that abort the entire daemon rather than sending a proper NOTIFICATION.

Add a STREAM_READABLE(connection->curr) < 10 guard at the top of bgp_open_receive(). If the stream has fewer than 10 bytes remaining, log an error, send a NOTIFICATION with OPEN Message Error / Malformed Attribute, and return BGP_Stop. This is defense-in-depth: it ensures the function is self-contained in its safety guarantees rather than relying on the caller having validated the size correctly.

RFC 4271 Section 4.2 specifies the OPEN message format with exactly 10 bytes of fixed fields after the header:

Version (1 byte)
My Autonomous System (2 bytes)
Hold Time (2 bytes)
BGP Identifier (4 bytes)
Optional Parameters Length (1 byte)

greptile-apps · 2026-06-09T11:43:32Z

Greptile Summary

This PR adds a defensive stream-length guard at the top of bgp_open_receive() in bgpd/bgp_packet.c, checking that at least 10 bytes remain in connection->curr before the fixed-field reads begin. This is a defense-in-depth change: it ensures the function is self-contained even if the I/O layer's BGP_MSG_OPEN_MIN_SIZE guard is ever bypassed.

Adds a STREAM_READABLE(connection->curr) < 10 pre-check matching the 10-byte fixed field layout defined by RFC 4271 §4.2 (version 1 + AS 2 + holdtime 2 + BGP-ID 4 + optlen 1).
On failure, logs an error with EC_BGP_PKT_OPEN, sends a BGP_NOTIFY_OPEN_ERR / BGP_NOTIFY_OPEN_MALFORMED_ATTR NOTIFICATION, and returns BGP_Stop.
The new guard is structurally consistent with the existing stream-length guards already present later in bgp_open_receive() (lines ~1841–1886).

Confidence Score: 4/5

Safe to merge; the guard is correct and unreachable under normal operation, making it a low-risk addition.

The byte count of 10 is arithmetically correct for RFC 4271's fixed OPEN fields, the %zu format specifier matches the size_t return type of STREAM_READABLE, and the early-return path mirrors the existing guards later in the same function. The only note is that BGP_NOTIFY_OPEN_MALFORMED_ATTR (subcode 0) isn't an RFC-defined OPEN error subcode for a short-message condition — a header-error subcode would be more precise — though this matches FRR's existing convention in bgp_open_receive.

No files require special attention; the single-file change is straightforward and self-contained.

Important Files Changed

Filename	Overview
bgpd/bgp_packet.c	Adds a 10-byte STREAM_READABLE pre-check in bgp_open_receive(); byte count is correct, format specifier is correct, and the check is consistent with existing guards in the same function. One minor concern: BGP_NOTIFY_OPEN_MALFORMED_ATTR (subcode 0) is not a formally defined RFC 4271 OPEN error subcode for a short message, though this matches the pattern used elsewhere in the function.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[bgp_open_receive called] --> B{STREAM_READABLE < 10?}
    B -- Yes --> C[flog_err EC_BGP_PKT_OPEN]
    C --> D[bgp_notify_send\nOPEN_ERR / MALFORMED_ATTR]
    D --> E[return BGP_Stop]
    B -- No --> F[stream_getc: version]
    F --> G[stream_getw: remote_as]
    G --> H[stream_getw: holdtime]
    H --> I[stream_get_ipv4: remote_id]
    I --> J[stream_getc: optlen]
    J --> K{Extended opt params?}
    K -- Yes --> L{STREAM_READABLE < 1?}
    L -- Yes --> M[bgp_notify_send\nOPEN_ERR / MALFORMED_ATTR\nreturn BGP_Stop]
    L -- No --> N[Continue parsing capabilities...]
    K -- No --> N

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
bgpd/bgp_packet.c:1802-1803
**NOTIFICATION subcode doesn't match the error condition**

`BGP_NOTIFY_OPEN_MALFORMED_ATTR` has value 0, which is not a defined subcode for OPEN Message Errors (error code 2) in RFC 4271 §6.2. A stream that is too short is closer to a bad message length, for which `BGP_NOTIFY_HEADER_ERR` / `BGP_NOTIFY_HEADER_BAD_MESLEN` (error code 1, subcode 2) would be a more precise choice. That said, this subcode value is already used by the other existing guards in this same function (lines ~1846, ~1858, ~1886), so the new check is at least internally consistent within FRR's current convention.

_{Reviews (1): Last reviewed commit: "bgpd: add STREAM_READABLE pre-check in b..." | Re-trigger Greptile}

greptile-apps · 2026-06-09T11:43:36Z

+		bgp_notify_send(connection, BGP_NOTIFY_OPEN_ERR,
+				BGP_NOTIFY_OPEN_MALFORMED_ATTR);


NOTIFICATION subcode doesn't match the error condition

BGP_NOTIFY_OPEN_MALFORMED_ATTR has value 0, which is not a defined subcode for OPEN Message Errors (error code 2) in RFC 4271 §6.2. A stream that is too short is closer to a bad message length, for which BGP_NOTIFY_HEADER_ERR / BGP_NOTIFY_HEADER_BAD_MESLEN (error code 1, subcode 2) would be a more precise choice. That said, this subcode value is already used by the other existing guards in this same function (lines ~1846, ~1858, ~1886), so the new check is at least internally consistent within FRR's current convention.

Prompt To Fix With AI

This is a comment left during a code review. Path: bgpd/bgp_packet.c Line: 1802-1803 Comment: **NOTIFICATION subcode doesn't match the error condition** `BGP_NOTIFY_OPEN_MALFORMED_ATTR` has value 0, which is not a defined subcode for OPEN Message Errors (error code 2) in RFC 4271 §6.2. A stream that is too short is closer to a bad message length, for which `BGP_NOTIFY_HEADER_ERR` / `BGP_NOTIFY_HEADER_BAD_MESLEN` (error code 1, subcode 2) would be a more precise choice. That said, this subcode value is already used by the other existing guards in this same function (lines ~1846, ~1858, ~1886), so the new check is at least internally consistent within FRR's current convention. How can I resolve this? If you propose a fix, please make it concise.

ton31337 · 2026-06-09T12:04:30Z

-	/* Parse open packet. */
+	/* Parse open packet.
+	 * Minimum OPEN body: version(1) + AS(2) + holdtime(2) + BGP-ID(4) + optlen(1) = 10 */
+	if (STREAM_READABLE(connection->curr) < 10) {


Define 10 as a "constant".

Good suggestion, done. Defined BGP_OPEN_BODY_MIN_SIZE (10) in bgpd.h with a comment documenting the field breakdown, and reused it in both the new guard and the existing BGP_MSG_OPEN_MIN_SIZE macro to avoid duplication.

ton31337 · 2026-06-10T05:50:23Z

+			 "%s: OPEN message too short (%zu bytes, need %u)",
+			 peer->host, STREAM_READABLE(connection->curr),
+			 BGP_OPEN_BODY_MIN_SIZE);
+		bgp_notify_send(connection, BGP_NOTIFY_OPEN_ERR,


BGP_NOTIFY_HEADER_ERR

Thanks for the review, Changed to BGP_NOTIFY_HEADER_ERR / BGP_NOTIFY_HEADER_BAD_MESLEN.

ton31337 · 2026-06-10T05:50:27Z

+			 peer->host, STREAM_READABLE(connection->curr),
+			 BGP_OPEN_BODY_MIN_SIZE);
+		bgp_notify_send(connection, BGP_NOTIFY_OPEN_ERR,
+				BGP_NOTIFY_OPEN_MALFORMED_ATTR);


This must be changed to BGP_NOTIFY_HEADER_BAD_MESLEN.

Thanks for the review, Changed to BGP_NOTIFY_HEADER_ERR / BGP_NOTIFY_HEADER_BAD_MESLEN.

bgp_open_receive() immediately calls stream_getc(), stream_getw(), and stream_get_ipv4() to parse the OPEN message fixed fields (version, AS number, hold time, BGP Identifier, opt parm len) without first verifying that the stream contains at least 10 bytes of readable data. While bgp_io.c validates BGP_MSG_OPEN_MIN_SIZE at the I/O layer, that check uses the total packet size including the 19-byte header. If a bug or future refactor changes how the stream getp is positioned before calling bgp_open_receive(), the stream_get*() calls could read past the end of valid data. The stream functions have internal assertions that abort the entire daemon rather than sending a proper NOTIFICATION. Add a STREAM_READABLE(connection->curr) < BGP_OPEN_BODY_MIN_SIZE guard at the top of bgp_open_receive(). If the stream has fewer than BGP_OPEN_BODY_MIN_SIZE bytes remaining, log an error, send a NOTIFICATION with Header Error / Bad Message Length, and return BGP_Stop. Define BGP_OPEN_BODY_MIN_SIZE (10) as a constant in bgpd.h to avoid hardcoding the magic number. This value matches RFC 4271 Section 4.2 which specifies the OPEN message format with exactly 10 bytes of fixed fields after the header: - Version (1 byte) - My Autonomous System (2 bytes) - Hold Time (2 bytes) - BGP Identifier (4 bytes) - Optional Parameters Length (1 byte) Use BGP_NOTIFY_HEADER_ERR / BGP_NOTIFY_HEADER_BAD_MESLEN instead of BGP_NOTIFY_OPEN_ERR / BGP_NOTIFY_OPEN_MALFORMED_ATTR, since a message that is too short to contain all required fields is a header-level error (bad message length), not an OPEN-specific malformed attribute error. This is defense-in-depth: it ensures the function is self-contained in its safety guarantees rather than relying on the caller having validated the size correctly. Signed-off-by: guozhongfeng.gzf <guozhongfeng.gzf@alibaba-inc.com>

frrbot Bot added the bgp label Jun 9, 2026

github-actions Bot added master size/S labels Jun 9, 2026

greptile-apps Bot reviewed Jun 9, 2026

View reviewed changes

ton31337 reviewed Jun 9, 2026

View reviewed changes

guoguojia2021 force-pushed the fix/bgpd-open-stream-readable-check branch from eb00b9f to 4ac99a9 Compare June 10, 2026 04:16

github-actions Bot added the rebase PR needs rebase label Jun 10, 2026

ton31337 reviewed Jun 10, 2026

View reviewed changes

guoguojia2021 force-pushed the fix/bgpd-open-stream-readable-check branch from 4ac99a9 to 5402972 Compare June 11, 2026 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bgpd: add STREAM_READABLE pre-check in bgp_open_receive#22277

bgpd: add STREAM_READABLE pre-check in bgp_open_receive#22277
guoguojia2021 wants to merge 1 commit into
FRRouting:masterfrom
guoguojia2021:fix/bgpd-open-stream-readable-check

guoguojia2021 commented Jun 9, 2026

Uh oh!

greptile-apps Bot commented Jun 9, 2026

Uh oh!

greptile-apps Bot Jun 9, 2026

Uh oh!

ton31337 Jun 9, 2026

Uh oh!

guoguojia2021 Jun 10, 2026

Uh oh!

ton31337 Jun 10, 2026

Uh oh!

guoguojia2021 Jun 11, 2026

Uh oh!

ton31337 Jun 10, 2026

Uh oh!

guoguojia2021 Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		bgp_notify_send(connection, BGP_NOTIFY_OPEN_ERR,
		BGP_NOTIFY_OPEN_MALFORMED_ATTR);

Conversation

guoguojia2021 commented Jun 9, 2026

Uh oh!

greptile-apps Bot commented Jun 9, 2026

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Flowchart

Uh oh!

greptile-apps Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

ton31337 Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

guoguojia2021 Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

ton31337 Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

guoguojia2021 Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

ton31337 Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

guoguojia2021 Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants