bgpd: Add sanity check for capability lengths before processing them

This is for CAPABILITY messages, not for OPEN message capabilities.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>

3 files changed, 91 insertions, 61 deletions

diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c
index e163df595..248d478fe 100644
--- a/bgpd/bgp_open.c
+++ b/bgpd/bgp_open.c
@@ -47,22 +47,22 @@ const struct message capcode_str[] = {
 };
 
 /* Minimum sizes for length field of each cap (so not inc. the header) */
-static const size_t cap_minsizes[] = {
-		[CAPABILITY_CODE_MP] = CAPABILITY_CODE_MP_LEN,
-		[CAPABILITY_CODE_REFRESH] = CAPABILITY_CODE_REFRESH_LEN,
-		[CAPABILITY_CODE_ORF] = CAPABILITY_CODE_ORF_LEN,
-		[CAPABILITY_CODE_RESTART] = CAPABILITY_CODE_RESTART_LEN,
-		[CAPABILITY_CODE_AS4] = CAPABILITY_CODE_AS4_LEN,
-		[CAPABILITY_CODE_ADDPATH] = CAPABILITY_CODE_ADDPATH_LEN,
-		[CAPABILITY_CODE_DYNAMIC] = CAPABILITY_CODE_DYNAMIC_LEN,
-		[CAPABILITY_CODE_ENHE] = CAPABILITY_CODE_ENHE_LEN,
-		[CAPABILITY_CODE_FQDN] = CAPABILITY_CODE_MIN_FQDN_LEN,
-		[CAPABILITY_CODE_ENHANCED_RR] = CAPABILITY_CODE_ENHANCED_LEN,
-		[CAPABILITY_CODE_EXT_MESSAGE] = CAPABILITY_CODE_EXT_MESSAGE_LEN,
-		[CAPABILITY_CODE_LLGR] = CAPABILITY_CODE_LLGR_LEN,
-		[CAPABILITY_CODE_ROLE] = CAPABILITY_CODE_ROLE_LEN,
-		[CAPABILITY_CODE_SOFT_VERSION] = CAPABILITY_CODE_SOFT_VERSION_LEN,
-		[CAPABILITY_CODE_PATHS_LIMIT] = CAPABILITY_CODE_PATHS_LIMIT_LEN,
+const size_t cap_minsizes[] = {
+	[CAPABILITY_CODE_MP] = CAPABILITY_CODE_MP_LEN,
+	[CAPABILITY_CODE_REFRESH] = CAPABILITY_CODE_REFRESH_LEN,
+	[CAPABILITY_CODE_ORF] = CAPABILITY_CODE_ORF_LEN,
+	[CAPABILITY_CODE_RESTART] = CAPABILITY_CODE_RESTART_LEN,
+	[CAPABILITY_CODE_AS4] = CAPABILITY_CODE_AS4_LEN,
+	[CAPABILITY_CODE_ADDPATH] = CAPABILITY_CODE_ADDPATH_LEN,
+	[CAPABILITY_CODE_DYNAMIC] = CAPABILITY_CODE_DYNAMIC_LEN,
+	[CAPABILITY_CODE_ENHE] = CAPABILITY_CODE_ENHE_LEN,
+	[CAPABILITY_CODE_FQDN] = CAPABILITY_CODE_MIN_FQDN_LEN,
+	[CAPABILITY_CODE_ENHANCED_RR] = CAPABILITY_CODE_ENHANCED_LEN,
+	[CAPABILITY_CODE_EXT_MESSAGE] = CAPABILITY_CODE_EXT_MESSAGE_LEN,
+	[CAPABILITY_CODE_LLGR] = CAPABILITY_CODE_LLGR_LEN,
+	[CAPABILITY_CODE_ROLE] = CAPABILITY_CODE_ROLE_LEN,
+	[CAPABILITY_CODE_SOFT_VERSION] = CAPABILITY_CODE_SOFT_VERSION_LEN,
+	[CAPABILITY_CODE_PATHS_LIMIT] = CAPABILITY_CODE_PATHS_LIMIT_LEN,
 };
 
 /* value the capability must be a multiple of.
@@ -70,22 +70,22 @@ static const size_t cap_minsizes[] = {
  * Other capabilities whose data doesn't fall on convenient boundaries for this
  * table should be set to 1.
  */
-static const size_t cap_modsizes[] = {
-		[CAPABILITY_CODE_MP] = 4,
-		[CAPABILITY_CODE_REFRESH] = 1,
-		[CAPABILITY_CODE_ORF] = 1,
-		[CAPABILITY_CODE_RESTART] = 1,
-		[CAPABILITY_CODE_AS4] = 4,
-		[CAPABILITY_CODE_ADDPATH] = 4,
-		[CAPABILITY_CODE_DYNAMIC] = 1,
-		[CAPABILITY_CODE_ENHE] = 6,
-		[CAPABILITY_CODE_FQDN] = 1,
-		[CAPABILITY_CODE_ENHANCED_RR] = 1,
-		[CAPABILITY_CODE_EXT_MESSAGE] = 1,
-		[CAPABILITY_CODE_LLGR] = 1,
-		[CAPABILITY_CODE_ROLE] = 1,
-		[CAPABILITY_CODE_SOFT_VERSION] = 1,
-		[CAPABILITY_CODE_PATHS_LIMIT] = 5,
+const size_t cap_modsizes[] = {
+	[CAPABILITY_CODE_MP] = 4,
+	[CAPABILITY_CODE_REFRESH] = 1,
+	[CAPABILITY_CODE_ORF] = 1,
+	[CAPABILITY_CODE_RESTART] = 1,
+	[CAPABILITY_CODE_AS4] = 4,
+	[CAPABILITY_CODE_ADDPATH] = 4,
+	[CAPABILITY_CODE_DYNAMIC] = 1,
+	[CAPABILITY_CODE_ENHE] = 6,
+	[CAPABILITY_CODE_FQDN] = 1,
+	[CAPABILITY_CODE_ENHANCED_RR] = 1,
+	[CAPABILITY_CODE_EXT_MESSAGE] = 1,
+	[CAPABILITY_CODE_LLGR] = 1,
+	[CAPABILITY_CODE_ROLE] = 1,
+	[CAPABILITY_CODE_SOFT_VERSION] = 1,
+	[CAPABILITY_CODE_PATHS_LIMIT] = 5,
 };
 
 /* BGP-4 Multiprotocol Extentions lead us to the complex world. We can
diff --git a/bgpd/bgp_open.h b/bgpd/bgp_open.h
index a01e49ceb..3a8cba9b7 100644
--- a/bgpd/bgp_open.h
+++ b/bgpd/bgp_open.h
@@ -112,5 +112,7 @@ extern as_t peek_for_as4_capability(struct peer *peer, uint16_t length);
 extern const struct message capcode_str[];
 extern const struct message orf_type_str[];
 extern const struct message orf_mode_str[];
+extern const size_t cap_minsizes[];
+extern const size_t cap_modsizes[];
 
 #endif /* _QUAGGA_BGP_OPEN_H */
diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 9c194eac1..47aed53bc 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -3406,6 +3406,22 @@ static void bgp_dynamic_capability_orf(uint8_t *pnt, int action,
 	}
 }
 
+static void bgp_dynamic_capability_role(uint8_t *pnt, int action,
+					struct peer *peer)
+{
+	uint8_t role;
+
+	if (action == CAPABILITY_ACTION_SET) {
+		SET_FLAG(peer->cap, PEER_CAP_ROLE_RCV);
+		memcpy(&role, pnt + 3, sizeof(role));
+
+		peer->remote_role = role;
+	} else {
+		UNSET_FLAG(peer->cap, PEER_CAP_ROLE_RCV);
+		peer->remote_role = ROLE_UNDEFINED;
+	}
+}
+
 static void bgp_dynamic_capability_fqdn(uint8_t *pnt, int action,
 					struct capability_header *hdr,
 					struct peer *peer)
@@ -3775,6 +3791,46 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt,
 
 		capability = lookup_msg(capcode_str, hdr->code, "Unknown");
 
+		/* Length sanity check, type-specific, for known capabilities */
+		switch (hdr->code) {
+		case CAPABILITY_CODE_MP:
+		case CAPABILITY_CODE_REFRESH:
+		case CAPABILITY_CODE_ORF:
+		case CAPABILITY_CODE_RESTART:
+		case CAPABILITY_CODE_AS4:
+		case CAPABILITY_CODE_ADDPATH:
+		case CAPABILITY_CODE_DYNAMIC:
+		case CAPABILITY_CODE_ENHE:
+		case CAPABILITY_CODE_FQDN:
+		case CAPABILITY_CODE_ENHANCED_RR:
+		case CAPABILITY_CODE_EXT_MESSAGE:
+		case CAPABILITY_CODE_ROLE:
+		case CAPABILITY_CODE_SOFT_VERSION:
+		case CAPABILITY_CODE_PATHS_LIMIT:
+			if (hdr->length < cap_minsizes[hdr->code]) {
+				zlog_info("%pBP: %s Capability length error: got %u, expected at least %u",
+					  peer, capability, hdr->length,
+					  (unsigned int)cap_minsizes[hdr->code]);
+				bgp_notify_send(peer->connection,
+						BGP_NOTIFY_OPEN_ERR,
+						BGP_NOTIFY_OPEN_MALFORMED_ATTR);
+				goto done;
+			}
+			if (hdr->length &&
+			    hdr->length % cap_modsizes[hdr->code] != 0) {
+				zlog_info("%pBP %s Capability length error: got %u, expected a multiple of %u",
+					  peer, capability, hdr->length,
+					  (unsigned int)cap_modsizes[hdr->code]);
+				bgp_notify_send(peer->connection,
+						BGP_NOTIFY_OPEN_ERR,
+						BGP_NOTIFY_OPEN_MALFORMED_ATTR);
+				goto done;
+			}
+			break;
+		default:
+			break;
+		}
+
 		switch (hdr->code) {
 		case CAPABILITY_CODE_SOFT_VERSION:
 			bgp_dynamic_capability_software_version(pnt, action,
@@ -3832,15 +3888,6 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt,
 			}
 			break;
 		case CAPABILITY_CODE_RESTART:
-			if ((hdr->length - 2) % 4) {
-				zlog_err("%pBP: Received invalid Graceful-Restart capability length %d",
-					 peer, hdr->length);
-				bgp_notify_send(peer->connection,
-						BGP_NOTIFY_CEASE,
-						BGP_NOTIFY_SUBCODE_UNSPECIFIC);
-				goto done;
-			}
-
 			bgp_dynamic_capability_graceful_restart(pnt, action,
 								hdr, peer);
 			break;
@@ -3868,26 +3915,7 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt,
 		case CAPABILITY_CODE_EXT_MESSAGE:
 			break;
 		case CAPABILITY_CODE_ROLE:
-			if (hdr->length != CAPABILITY_CODE_ROLE_LEN) {
-				zlog_err("%pBP: Capability (%s) length error",
-					 peer, capability);
-				bgp_notify_send(peer->connection,
-						BGP_NOTIFY_CEASE,
-						BGP_NOTIFY_SUBCODE_UNSPECIFIC);
-				goto done;
-			}
-
-			uint8_t role;
-
-			if (action == CAPABILITY_ACTION_SET) {
-				SET_FLAG(peer->cap, PEER_CAP_ROLE_RCV);
-				memcpy(&role, pnt + 3, sizeof(role));
-
-				peer->remote_role = role;
-			} else {
-				UNSET_FLAG(peer->cap, PEER_CAP_ROLE_RCV);
-				peer->remote_role = ROLE_UNDEFINED;
-			}
+			bgp_dynamic_capability_role(pnt, action, peer);
 			break;
 		default:
 			flog_warn(EC_BGP_UNRECOGNIZED_CAPABILITY,