packet/bgp: reject KEEPALIVE messages that are not 19 octets - #3567
Conversation
|
I think that bird does not reject an over-long KEEPALIVE. |
| // header and has a length of exactly 19 octets, so the body that | ||
| // parseBody hands us must be empty. | ||
| if len(data) != 0 { | ||
| return NewMessageError(BGP_ERROR_MESSAGE_HEADER_ERROR, BGP_ERROR_SUB_BAD_MESSAGE_LENGTH, nil, fmt.Sprintf("KEEPALIVE length must be %d", BGP_HEADER_LENGTH)) |
There was a problem hiding this comment.
You can't use nil here. RFC 4271 Section 6.1 says that The Data field MUST contain the erroneous Type field..
There was a problem hiding this comment.
Good catch on the empty Data field. I added a regression assertion and now populate it in e37c5a2. Because this is Bad Message Length, RFC 4271 Section 6.1 requires the Data field to carry the erroneous two-octet Length field; the following sentence about Type applies to Bad Message Type. The test now checks the code, subcode, and big-endian length for 20, 100, and 4096.
RFC 4271 Sections 4.4 and 6.1 require a KEEPALIVE to be exactly 19 octets. A Bad Message Length notification must also carry the erroneous two-octet Length field. BGPKeepAlive.DecodeFromBytes did not inspect its input, so ParseBGPMessage accepted KEEPALIVEs with a body. Reject non-empty bodies and populate MessageError.Data with the declared length. Add table-driven coverage for valid length 19 and invalid lengths 20, 100, and 4096, including the error code, subcode, and Data. This deliberately covers only the KEEPALIVE part of osrg#3449. OPEN validation remains in ValidateOpenMsg, while the general length ceiling is session-dependent when RFC 8654 is negotiated. Refs osrg#3449 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sonike <1700162+Sonike@users.noreply.github.com>
d171ff6 to
e37c5a2
Compare
|
You are right about BIRD: its current receive path checks only the general 19-to-session-maximum range and dispatches KEEPALIVE without a type-specific length check. I removed that comparison from the PR body and amended commit message. The change is now justified directly by RFC 4271 Sections 4.4 and 6.1. |
|
Thanks. |
2 participants
What
BGPKeepAlive.DecodeFromBytesreturnednilwithout inspecting its input, soboth
ParseBGPMessagecallers and the FSM accepted a KEEPALIVE whose declaredlength was not 19 octets. RFC 4271 Sections 4.4 and 6.1 require the length to be
exactly 19.
How
parseBodypasses the bytes declared after the 19-octet header to the bodydecoder. A conforming KEEPALIVE therefore arrives as an empty slice; any
non-empty slice is rejected with Bad Message Length.
RFC 4271 Section 6.1 also requires the NOTIFICATION Data field for this error
to contain the erroneous two-octet Length field. The decoder now records that
big-endian length in
MessageError.Data.Scope
This deliberately covers only the KEEPALIVE item from #3449:
ValidateOpenMsg. Moving them into the decoder would change validationboundaries and should be a separate maintainer decision.
raises it for UPDATE, NOTIFICATION and ROUTE-REFRESH after Extended Message
Capability negotiation. The session layer has that state; the standalone
parser does not.
Testing
go test ./pkg/packet/bgp -run '^TestParseBGPMessageKeepAliveLength$' -count=1go test -race -timeout 240s ./pkg/packet/bgpgo build ./...go vet ./...golangci-lint run ./pkg/packet/bgp/...golangci-lint run --new-from-rev=upstream/masterThe full
go test -race -timeout 240s ./...run passed outsidepkg/server.TestEBGPRouteStuckandTestRTCDeferralTimerRaceConditioncannot bind127.0.0.100 and 127.0.0.201 on this macOS host; these are unchanged
environment limitations and unrelated to the packet decoder.
Refs #3449