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 */ + /* + * 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. + */ + + /* + * 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); + + while (STREAM_READABLE(peer->curr) > 0) { + + 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); + } type = stream_getc(peer->curr); length = stream_getw(peer->curr); + if (STREAM_READABLE(peer->curr) < length) { + flog_err( + EC_BGP_ATTR_LEN, + "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; - /* - * Subtract length + the T and the L - * since length is the Vector portion - */ - tlength -= length + 3; - - if (tlength < 0) { - flog_err( - EC_BGP_ATTR_LEN, - "Prefix SID internal length %d causes us to read beyond the total Prefix SID length", - length); - return bgp_attr_malformed(args, - BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, - args->total); - } } 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");