From f69aeb7696461fc333dc8b12a55f8160a80db1fc Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Fri, 22 Nov 2019 02:49:45 -0500 Subject: [PATCH 1/2] bgpd: fix missing bounds checks for psid attr Guess what - for a bounds check to work, it has to happen *before* you read the data. We were trusting the attribute field received in a prefix SID attribute and then checking if it was correct afterwards, but if was wrong we'd crash before that. This fixes the problem, and adds additional paranoid bounds checks. Signed-off-by: Quentin Young --- bgpd/bgp_attr.c | 98 ++++++++++++++++++++++++++------------- bgpd/bgp_attr.h | 2 +- tests/bgpd/test_mp_attr.c | 2 +- 3 files changed, 67 insertions(+), 35 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index fe7a80ccf2..bee9e40ed7 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -2135,8 +2135,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) { @@ -2149,11 +2148,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); @@ -2185,9 +2185,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, @@ -2203,6 +2205,17 @@ 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) { + if (STREAM_READABLE(peer->curr) < length + || length != BGP_PREFIX_SID_ORIGINATOR_SRGB_LENGTH) { + flog_err(EC_BGP_ATTR_LEN, + "Prefix SID Originator SRGB length is %" PRIu16 + " instead of %u", + length, BGP_PREFIX_SID_ORIGINATOR_SRGB_LENGTH); + return bgp_attr_malformed( + args, BGP_NOTIFY_UPDATE_ATTR_LENG_ERR, + args->total); + } + /* Ignore flags */ stream_getw(peer->curr); @@ -2233,12 +2246,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; @@ -2247,9 +2272,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; @@ -2257,31 +2281,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; @@ -2677,8 +2710,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"); From 473046ee50f06e3f84b4e1468e22abb9182175ba Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Tue, 10 Dec 2019 15:57:28 -0500 Subject: [PATCH 2/2] bgpd: slight correction to sanity checks for SRGB Also improves the log messages for invalid SRGB length fields, truncated attribute data etc Signed-off-by: Quentin Young --- bgpd/bgp_attr.c | 48 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index bee9e40ed7..c8fdaf404d 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -2205,26 +2205,54 @@ static bgp_attr_parse_ret_t bgp_attr_psid_sub(uint8_t type, uint16_t length, /* Placeholder code for the Originator SRGB type */ else if (type == BGP_PREFIX_SID_ORIGINATOR_SRGB) { - if (STREAM_READABLE(peer->curr) < length - || length != BGP_PREFIX_SID_ORIGINATOR_SRGB_LENGTH) { - flog_err(EC_BGP_ATTR_LEN, - "Prefix SID Originator SRGB length is %" PRIu16 - " instead of %u", - length, BGP_PREFIX_SID_ORIGINATOR_SRGB_LENGTH); + /* + * 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); } - /* Ignore flags */ + /* + * 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,