]> git.proxmox.com Git - mirror_frr.git/commitdiff
Merge pull request #5418 from qlyoung/fix-bgp-prefix-sid-missing-boundscheck
authorDonatas Abraitis <donatas.abraitis@gmail.com>
Wed, 8 Jan 2020 19:59:07 +0000 (21:59 +0200)
committerGitHub <noreply@github.com>
Wed, 8 Jan 2020 19:59:07 +0000 (21:59 +0200)
bgpd: fix missing bounds checks for psid attr

1  2 
bgpd/bgp_attr.c

diff --combined bgpd/bgp_attr.c
index bdac2a8dcc20669e024bf644c998d9e2afe7601d,c8fdaf404dfb57791eed4d911a19cdb3f50d49f6..16de59b72caca0ee2320ff71164d22ba90bb58bb
@@@ -152,9 -152,8 +152,9 @@@ static bool cluster_hash_cmp(const voi
        const struct cluster_list *cluster2 = p2;
  
        return (cluster1->length == cluster2->length
 -              && memcmp(cluster1->list, cluster2->list, cluster1->length)
 -                         == 0);
 +              && (cluster1->list == cluster2->list
 +                  || memcmp(cluster1->list, cluster2->list, cluster1->length)
 +                             == 0));
  }
  
  static void cluster_free(struct cluster_list *cluster)
@@@ -2136,8 -2135,7 +2136,7 @@@ static int bgp_attr_encap(uint8_t type
   * 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)
  {
        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);
  
        /* 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,
  
        /* 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,
         */
        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;
  /* 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;
  
        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 -2738,7 +2739,7 @@@ bgp_attr_parse_ret_t bgp_attr_parse(str
                                             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);