Merge pull request #5418 from qlyoung/fix-bgp-prefix-sid-missing-boundscheck

bgpd: fix missing bounds checks for psid attr

3 files changed, 97 insertions, 37 deletions

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index bdac2a8dcc..16de59b72c 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -2136,8 +2136,7 @@ static int bgp_attr_encap(uint8_t type, struct peer *peer, /* IN */
  * Read an individual SID value returning how much data we have read
  * Returns 0 if there was an error that needs to be passed up the stack
  */
-static bgp_attr_parse_ret_t bgp_attr_psid_sub(int32_t type,
-					      int32_t length,
+static bgp_attr_parse_ret_t bgp_attr_psid_sub(uint8_t type, uint16_t length,
 					      struct bgp_attr_parser_args *args,
 					      struct bgp_nlri *mp_update)
 {
@@ -2150,11 +2149,12 @@ static bgp_attr_parse_ret_t bgp_attr_psid_sub(int32_t type,
 	int srgb_count;
 
 	if (type == BGP_PREFIX_SID_LABEL_INDEX) {
-		if (length != BGP_PREFIX_SID_LABEL_INDEX_LENGTH) {
-			flog_err(
-				EC_BGP_ATTR_LEN,
-				"Prefix SID label index length is %d instead of %d",
-				length, BGP_PREFIX_SID_LABEL_INDEX_LENGTH);
+		if (STREAM_READABLE(peer->curr) < length
+		    || length != BGP_PREFIX_SID_LABEL_INDEX_LENGTH) {
+			flog_err(EC_BGP_ATTR_LEN,
+				 "Prefix SID label index length is %" PRIu16
+				 " instead of %u",
+				 length, BGP_PREFIX_SID_LABEL_INDEX_LENGTH);
 			return bgp_attr_malformed(args,
 						  BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
 						  args->total);
@@ -2186,9 +2186,11 @@ static bgp_attr_parse_ret_t bgp_attr_psid_sub(int32_t type,
 
 	/* Placeholder code for the IPv6 SID type */
 	else if (type == BGP_PREFIX_SID_IPV6) {
-		if (length != BGP_PREFIX_SID_IPV6_LENGTH) {
+		if (STREAM_READABLE(peer->curr) < length
+		    || length != BGP_PREFIX_SID_IPV6_LENGTH) {
 			flog_err(EC_BGP_ATTR_LEN,
-				 "Prefix SID IPv6 length is %d instead of %d",
+				 "Prefix SID IPv6 length is %" PRIu16
+				 " instead of %u",
 				 length, BGP_PREFIX_SID_IPV6_LENGTH);
 			return bgp_attr_malformed(args,
 						  BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
@@ -2204,15 +2206,54 @@ static bgp_attr_parse_ret_t bgp_attr_psid_sub(int32_t type,
 
 	/* Placeholder code for the Originator SRGB type */
 	else if (type == BGP_PREFIX_SID_ORIGINATOR_SRGB) {
-		/* Ignore flags */
-		stream_getw(peer->curr);
+		/*
+		 * ietf-idr-bgp-prefix-sid-05:
+		 *     Length is the total length of the value portion of the
+		 *     TLV: 2 + multiple of 6.
+		 *
+		 * peer->curr stream readp should be at the beginning of the 16
+		 * bit flag field at this point in the code.
+		 */
 
-		length -= 2;
+		/*
+		 * Check that the TLV length field is sane: at least 2 bytes of
+		 * flag, and at least 1 SRGB (these are 6 bytes each)
+		 */
+		if (length < (2 + BGP_PREFIX_SID_ORIGINATOR_SRGB_LENGTH)) {
+			flog_err(
+				EC_BGP_ATTR_LEN,
+				"Prefix SID Originator SRGB length field claims length of %" PRIu16 " bytes, but the minimum for this TLV type is %u",
+				length,
+				2 + BGP_PREFIX_SID_ORIGINATOR_SRGB_LENGTH);
+			return bgp_attr_malformed(
+				args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
+				args->total);
+		}
 
+		/*
+		 * Check that we actually have at least as much data as
+		 * specified by the length field
+		 */
+		if (STREAM_READABLE(peer->curr) < length) {
+			flog_err(EC_BGP_ATTR_LEN,
+				 "Prefix SID Originator SRGB specifies length %" PRIu16 ", but only %zu bytes remain",
+				 length, STREAM_READABLE(peer->curr));
+			return bgp_attr_malformed(
+				args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
+				args->total);
+		}
+
+		/*
+		 * Check that the portion of the TLV containing the sequence of
+		 * SRGBs corresponds to a multiple of the SRGB size; to get
+		 * that length, we skip the 16 bit flags field
+		 */
+		stream_getw(peer->curr);
+		length -= 2;
 		if (length % BGP_PREFIX_SID_ORIGINATOR_SRGB_LENGTH) {
 			flog_err(
 				EC_BGP_ATTR_LEN,
-				"Prefix SID Originator SRGB length is %d, it must be a multiple of %d ",
+				"Prefix SID Originator SRGB length field claims attribute SRGB sequence section is %" PRIu16 "bytes, but it must be a multiple of %u",
 				length, BGP_PREFIX_SID_ORIGINATOR_SRGB_LENGTH);
 			return bgp_attr_malformed(
 				args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
@@ -2234,12 +2275,24 @@ static bgp_attr_parse_ret_t bgp_attr_psid_sub(int32_t type,
 	 */
 	else if (type == BGP_PREFIX_SID_SRV6_L3_SERVICE
 	    || type == BGP_PREFIX_SID_SRV6_L2_SERVICE) {
+
+		if (STREAM_READABLE(peer->curr) < length) {
+			flog_err(
+				EC_BGP_ATTR_LEN,
+				"Prefix SID SRv6 length is %" PRIu16
+				" - too long, only %zu remaining in this UPDATE",
+				length, STREAM_READABLE(peer->curr));
+			return bgp_attr_malformed(
+				args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
+				args->total);
+		}
+
 		if (bgp_debug_update(peer, NULL, NULL, 1))
 			zlog_debug(
 				"%s attr Prefix-SID sub-type=%u is not supported, skipped",
 				peer->host, type);
-		for (int i = 0; i < length; i++)
-			stream_getc(peer->curr);
+
+		stream_forward_getp(peer->curr, length);
 	}
 
 	return BGP_ATTR_PARSE_PROCEED;
@@ -2248,9 +2301,8 @@ static bgp_attr_parse_ret_t bgp_attr_psid_sub(int32_t type,
 /* Prefix SID attribute
  * draft-ietf-idr-bgp-prefix-sid-05
  */
-bgp_attr_parse_ret_t
-bgp_attr_prefix_sid(int32_t tlength, struct bgp_attr_parser_args *args,
-		    struct bgp_nlri *mp_update)
+bgp_attr_parse_ret_t bgp_attr_prefix_sid(struct bgp_attr_parser_args *args,
+					 struct bgp_nlri *mp_update)
 {
 	struct peer *const peer = args->peer;
 	struct attr *const attr = args->attr;
@@ -2258,31 +2310,40 @@ bgp_attr_prefix_sid(int32_t tlength, struct bgp_attr_parser_args *args,
 
 	attr->flag |= ATTR_FLAG_BIT(BGP_ATTR_PREFIX_SID);
 
-	while (tlength) {
-		int32_t type, length;
+	uint8_t type;
+	uint16_t length;
+	size_t headersz = sizeof(type) + sizeof(length);
 
-		type = stream_getc(peer->curr);
-		length = stream_getw(peer->curr);
+	while (STREAM_READABLE(peer->curr) > 0) {
 
-		ret = bgp_attr_psid_sub(type, length, args, mp_update);
+		if (STREAM_READABLE(peer->curr) < headersz) {
+			flog_err(
+				EC_BGP_ATTR_LEN,
+				"Malformed Prefix SID attribute - insufficent data (need %zu for attribute header, have %zu remaining in UPDATE)",
+				headersz, STREAM_READABLE(peer->curr));
+			return bgp_attr_malformed(
+				args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
+				args->total);
+		}
 
-		if (ret != BGP_ATTR_PARSE_PROCEED)
-			return ret;
-		/*
-		 * Subtract length + the T and the L
-		 * since length is the Vector portion
-		 */
-		tlength -= length + 3;
+		type = stream_getc(peer->curr);
+		length = stream_getw(peer->curr);
 
-		if (tlength < 0) {
+		if (STREAM_READABLE(peer->curr) < length) {
 			flog_err(
 				EC_BGP_ATTR_LEN,
-				"Prefix SID internal length %d causes us to read beyond the total Prefix SID length",
-				length);
+				"Malformed Prefix SID attribute - insufficient data (need %" PRIu8
+				" for attribute body, have %zu remaining in UPDATE)",
+				length, STREAM_READABLE(peer->curr));
 			return bgp_attr_malformed(args,
 						  BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
 						  args->total);
 		}
+
+		ret = bgp_attr_psid_sub(type, length, args, mp_update);
+
+		if (ret != BGP_ATTR_PARSE_PROCEED)
+			return ret;
 	}
 
 	return BGP_ATTR_PARSE_PROCEED;
@@ -2678,8 +2739,7 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr,
 					     startp);
 			break;
 		case BGP_ATTR_PREFIX_SID:
-			ret = bgp_attr_prefix_sid(length,
-						  &attr_args, mp_update);
+			ret = bgp_attr_prefix_sid(&attr_args, mp_update);
 			break;
 		case BGP_ATTR_PMSI_TUNNEL:
 			ret = bgp_attr_pmsi_tunnel(&attr_args);
diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h
index 40e87e190a..8bd527c0ff 100644
--- a/bgpd/bgp_attr.h
+++ b/bgpd/bgp_attr.h
@@ -319,7 +319,7 @@ extern int bgp_mp_reach_parse(struct bgp_attr_parser_args *args,
 extern int bgp_mp_unreach_parse(struct bgp_attr_parser_args *args,
 				struct bgp_nlri *);
 extern bgp_attr_parse_ret_t
-bgp_attr_prefix_sid(int32_t tlength, struct bgp_attr_parser_args *args,
+bgp_attr_prefix_sid(struct bgp_attr_parser_args *args,
 		    struct bgp_nlri *mp_update);
 
 extern struct bgp_attr_encap_subtlv *
diff --git a/tests/bgpd/test_mp_attr.c b/tests/bgpd/test_mp_attr.c
index af9e5791b7..c97ea57150 100644
--- a/tests/bgpd/test_mp_attr.c
+++ b/tests/bgpd/test_mp_attr.c
@@ -1027,7 +1027,7 @@ static void parse_test(struct peer *peer, struct test_segment *t, int type)
 		parse_ret = bgp_mp_unreach_parse(&attr_args, &nlri);
 		break;
 	case BGP_ATTR_PREFIX_SID:
-		parse_ret = bgp_attr_prefix_sid(t->len, &attr_args, &nlri);
+		parse_ret = bgp_attr_prefix_sid(&attr_args, &nlri);
 		break;
 	default:
 		printf("unknown type");