From e37c5a20f7d956ac21c01308d89454c07583bb43 Mon Sep 17 00:00:00 2001
From: Sonike <1700162+Sonike@users.noreply.github.com>
Date: Sun, 30 Aug 2026 17:04:27 +0800
Subject: [PATCH] packet/bgp: reject KEEPALIVE messages that are not 19 octets

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 #3449. OPEN validation remains in ValidateOpenMsg, while the general length ceiling is session-dependent when RFC 8654 is negotiated.

Refs #3449

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Sonike <1700162+Sonike@users.noreply.github.com>
---
 pkg/packet/bgp/bgp.go                   |  7 ++++
 pkg/packet/bgp/extended_message_test.go | 43 +++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/pkg/packet/bgp/bgp.go b/pkg/packet/bgp/bgp.go
index b4a18581c..d3eaf7f1d 100644
--- a/pkg/packet/bgp/bgp.go
+++ b/pkg/packet/bgp/bgp.go
@@ -17025,6 +17025,13 @@ func ShouldHardReset(subcode uint8, hardResetOnAdminReset bool) bool {
 type BGPKeepAlive struct{}
 
 func (msg *BGPKeepAlive) DecodeFromBytes(data []byte, options ...*MarshallingOption) error {
+	// RFC 4271 Section 4.4: a KEEPALIVE consists of only the message
+	// header and has a length of exactly 19 octets, so the body that
+	// parseBody hands us must be empty.
+	if len(data) != 0 {
+		length := uint16(BGP_HEADER_LENGTH + len(data))
+		return NewMessageError(BGP_ERROR_MESSAGE_HEADER_ERROR, BGP_ERROR_SUB_BAD_MESSAGE_LENGTH, binary.BigEndian.AppendUint16(nil, length), fmt.Sprintf("KEEPALIVE length must be %d", BGP_HEADER_LENGTH))
+	}
 	return nil
 }
 
diff --git a/pkg/packet/bgp/extended_message_test.go b/pkg/packet/bgp/extended_message_test.go
index ef73830ae..3f20e4abf 100644
--- a/pkg/packet/bgp/extended_message_test.go
+++ b/pkg/packet/bgp/extended_message_test.go
@@ -2,6 +2,8 @@ package bgp
 
 import (
 	"bytes"
+	"encoding/binary"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -140,3 +142,44 @@ func TestIsExtendedMessageSerialization(t *testing.T) {
 	require.True(t, IsExtendedMessageSerialization([]*MarshallingOption{nil, {ExtendedMessage: true}}))
 	require.True(t, IsExtendedMessageSerialization([]*MarshallingOption{{ExtendedMessage: true}, {}}))
 }
+
+// bgpHeaderBytes builds the 19-octet on-the-wire BGP header: an
+// all-ones marker, the declared total message length, then the type.
+// BGPHeader.DecodeFromBytes reads nothing beyond these 19 octets, so
+// the body is deliberately absent.
+func bgpHeaderBytes(msgType uint8, declaredLen uint16) []byte {
+	buf := make([]byte, BGP_HEADER_LENGTH)
+	for i := range 16 {
+		buf[i] = 0xFF
+	}
+	binary.BigEndian.PutUint16(buf[16:18], declaredLen)
+	buf[18] = msgType
+	return buf
+}
+
+// TestParseBGPMessageKeepAliveLength pins RFC 4271 Section 4.4: a
+// KEEPALIVE is header-only, so anything other than 19 octets is
+// malformed. The decoder used to accept any length and return a
+// non-nil message with a nil error.
+func TestParseBGPMessageKeepAliveLength(t *testing.T) {
+	t.Run("exactly_19_accepted", func(t *testing.T) {
+		m, err := ParseBGPMessage(bgpHeaderBytes(BGP_MSG_KEEPALIVE, BGP_HEADER_LENGTH))
+		require.NoError(t, err)
+		require.Equal(t, uint8(BGP_MSG_KEEPALIVE), m.Header.Type)
+	})
+
+	for _, declaredLen := range []uint16{20, 100, BGP_MAX_MESSAGE_LENGTH} {
+		t.Run(fmt.Sprintf("length_%d_rejected", declaredLen), func(t *testing.T) {
+			buf := append(bgpHeaderBytes(BGP_MSG_KEEPALIVE, declaredLen),
+				make([]byte, int(declaredLen)-BGP_HEADER_LENGTH)...)
+			_, err := ParseBGPMessage(buf)
+			require.Error(t, err, "KEEPALIVE with length %d must be rejected", declaredLen)
+
+			var msgErr *MessageError
+			require.ErrorAs(t, err, &msgErr)
+			require.Equal(t, uint8(BGP_ERROR_MESSAGE_HEADER_ERROR), msgErr.TypeCode)
+			require.Equal(t, uint8(BGP_ERROR_SUB_BAD_MESSAGE_LENGTH), msgErr.SubTypeCode)
+			require.Equal(t, binary.BigEndian.AppendUint16(nil, declaredLen), msgErr.Data)
+		})
+	}
+}