bgpd: separate lcommunity validation from tokenizer

`lcommunity_gettoken` expects a space-delimeted list of 0 or more large
communities. `lcommunity_list_valid` can perform this check.
`lcommunity_list_valid` now validates large community lists more
accurately based on the following condition: Each quantity in a standard bgp
large community must:

1. Contain at least one digit
2. Fit within 4 octets
3. Contain only digits unless the lcommunity is "expanded"
4. Contain a valid regex if the lcommunity is "expanded"

Moreover we validate that each large community list contains exactly 3
such values separated by a single colon each.

One quirk of our validation which is worth documenting is:

```
bgp large-community-list standard test2 permit 1:c:3
bgp large-community-list expanded test1 permit 1:c:3
```

The first line will throw an error complaining about a "malformed community-list
value". The second line will be accepted because the each value is each treated as
a regex when matching large communities, it simply will never match anything so
it's rather useless.

Signed-off-by: Wesley Coakley <wcoakley@nvidia.com>

diff --git a/bgpd/bgp_clist.c b/bgpd/bgp_clist.c
index 247d758f8cd6b7232946b81c807c5817d3aa3d46..6ac6cf56ddcfa1bc259aa4caf1a830f68db98cad 100644 (file)
--- a/bgpd/bgp_clist.c
+++ b/bgpd/bgp_clist.c
@@ -1110,11 +1110,14 @@ struct lcommunity *lcommunity_list_match_delete(struct lcommunity *lcom,
 }
 
 /* Helper to check if every octet do not exceed UINT_MAX */
-static bool lcommunity_list_valid(const char *community)
+bool lcommunity_list_valid(const char *community, int style)
 {
        int octets;
        char **splits, **communities;
+       char *endptr;
        int num, num_communities;
+       regex_t *regres;
+       int invalid = 0;
 
        frrstr_split(community, " ", &communities, &num_communities);
 
@@ -1123,25 +1126,43 @@ static bool lcommunity_list_valid(const char *community)
                frrstr_split(communities[j], ":", &splits, &num);
 
                for (int i = 0; i < num; i++) {
-                       if (strtoul(splits[i], NULL, 10) > UINT_MAX)
-                               return false;
-
                        if (strlen(splits[i]) == 0)
-                               return false;
+                               /* There is no digit to check */
+                               invalid++;
+
+                       if (style == LARGE_COMMUNITY_LIST_STANDARD) {
+                               if (*splits[i] == '-')
+                                       /* Must not be negative */
+                                       invalid++;
+                               else if (strtoul(splits[i], &endptr, 10)
+                                        > UINT_MAX)
+                                       /* Larger than 4 octets */
+                                       invalid++;
+                               else if (*endptr)
+                                       /* Not all characters were digits */
+                                       invalid++;
+                       } else {
+                               regres = bgp_regcomp(communities[j]);
+                               if (!regres)
+                                       /* malformed regex */
+                                       invalid++;
+                               else
+                                       bgp_regex_free(regres);
+                       }
 
                        octets++;
                        XFREE(MTYPE_TMP, splits[i]);
                }
                XFREE(MTYPE_TMP, splits);
 
-               if (octets < 3)
-                       return false;
+               if (octets != 3)
+                       invalid++;
 
                XFREE(MTYPE_TMP, communities[j]);
        }
        XFREE(MTYPE_TMP, communities);
 
-       return true;
+       return (invalid > 0) ? false : true;
 }
 
 /* Set lcommunity-list.  */
@@ -1176,7 +1197,7 @@ int lcommunity_list_set(struct community_list_handler *ch, const char *name,
        }
 
        if (str) {
-               if (!lcommunity_list_valid(str))
+               if (!lcommunity_list_valid(str, style))
                        return COMMUNITY_LIST_ERR_MALFORMED_VAL;
 
                if (style == LARGE_COMMUNITY_LIST_STANDARD)

diff --git a/bgpd/bgp_clist.h b/bgpd/bgp_clist.h
index f7d46525a0d2ebc26380d739d7e0846629095e9b..632e1730cc6c3739e21878253190c0322e0a23e6 100644 (file)
--- a/bgpd/bgp_clist.h
+++ b/bgpd/bgp_clist.h
@@ -154,6 +154,7 @@ extern int extcommunity_list_unset(struct community_list_handler *ch,
 extern int lcommunity_list_set(struct community_list_handler *ch,
                               const char *name, const char *str,
                               const char *seq, int direct, int style);
+extern bool lcommunity_list_valid(const char *community, int style);
 extern int lcommunity_list_unset(struct community_list_handler *ch,
                                 const char *name, const char *str,
                                 const char *seq, int direct, int style);

diff --git a/bgpd/bgp_lcommunity.c b/bgpd/bgp_lcommunity.c
index 5900fcf862b91c2ab6c4d710be4ea55ee06ac17d..88a85c979e74c06c330a51c51bd139718df11cc3 100644 (file)
--- a/bgpd/bgp_lcommunity.c
+++ b/bgpd/bgp_lcommunity.c
@@ -348,16 +348,12 @@ void lcommunity_finish(void)
        lcomhash = NULL;
 }
 
-/* Large Communities token enum. */
-enum lcommunity_token {
-       lcommunity_token_unknown = 0,
-       lcommunity_token_val,
-};
-
-/* Get next Large Communities token from the string. */
+/* Get next Large Communities token from the string.
+ * Assumes str is space-delimeted and describes 0 or more
+ * valid large communities
+ */
 static const char *lcommunity_gettoken(const char *str,
-                                      struct lcommunity_val *lval,
-                                      enum lcommunity_token *token)
+                                      struct lcommunity_val *lval)
 {
        const char *p = str;
 
@@ -372,60 +368,55 @@ static const char *lcommunity_gettoken(const char *str,
                return NULL;
 
        /* Community value. */
-       if (isdigit((unsigned char)*p)) {
-               int separator = 0;
-               int digit = 0;
-               uint32_t globaladmin = 0;
-               uint32_t localdata1 = 0;
-               uint32_t localdata2 = 0;
-
-               while (isdigit((unsigned char)*p) || *p == ':') {
-                       if (*p == ':') {
-                               if (separator == 2) {
-                                       *token = lcommunity_token_unknown;
-                                       return NULL;
-                               } else {
-                                       separator++;
-                                       digit = 0;
-                                       if (separator == 1) {
-                                               globaladmin = localdata2;
-                                       } else {
-                                               localdata1 = localdata2;
-                                       }
-                                       localdata2 = 0;
-                               }
+       int separator = 0;
+       int digit = 0;
+       uint32_t globaladmin = 0;
+       uint32_t localdata1 = 0;
+       uint32_t localdata2 = 0;
+
+       while (*p && *p != ' ') {
+               /* large community valid chars */
+               assert(isdigit((unsigned char)*p) || *p == ':');
+
+               if (*p == ':') {
+                       separator++;
+                       digit = 0;
+                       if (separator == 1) {
+                               globaladmin = localdata2;
                        } else {
-                               digit = 1;
-                               localdata2 *= 10;
-                               localdata2 += (*p - '0');
+                               localdata1 = localdata2;
                        }
-                       p++;
+                       localdata2 = 0;
+               } else {
+                       digit = 1;
+                       /* left shift the accumulated value and add current
+                        * digit
+                        */
+                       localdata2 *= 10;
+                       localdata2 += (*p - '0');
                }
-               if (!digit) {
-                       *token = lcommunity_token_unknown;
-                       return NULL;
-               }
-
-               /*
-                * Copy the large comm.
-                */
-               lval->val[0] = (globaladmin >> 24) & 0xff;
-               lval->val[1] = (globaladmin >> 16) & 0xff;
-               lval->val[2] = (globaladmin >> 8) & 0xff;
-               lval->val[3] = globaladmin & 0xff;
-               lval->val[4] = (localdata1 >> 24) & 0xff;
-               lval->val[5] = (localdata1 >> 16) & 0xff;
-               lval->val[6] = (localdata1 >> 8) & 0xff;
-               lval->val[7] = localdata1 & 0xff;
-               lval->val[8] = (localdata2 >> 24) & 0xff;
-               lval->val[9] = (localdata2 >> 16) & 0xff;
-               lval->val[10] = (localdata2 >> 8) & 0xff;
-               lval->val[11] = localdata2 & 0xff;
-
-               *token = lcommunity_token_val;
-               return p;
+               p++;
        }
-       *token = lcommunity_token_unknown;
+
+       /* Assert str was a valid large community */
+       assert(separator == 2 && digit == 1);
+
+       /*
+        * Copy the large comm.
+        */
+       lval->val[0] = (globaladmin >> 24) & 0xff;
+       lval->val[1] = (globaladmin >> 16) & 0xff;
+       lval->val[2] = (globaladmin >> 8) & 0xff;
+       lval->val[3] = globaladmin & 0xff;
+       lval->val[4] = (localdata1 >> 24) & 0xff;
+       lval->val[5] = (localdata1 >> 16) & 0xff;
+       lval->val[6] = (localdata1 >> 8) & 0xff;
+       lval->val[7] = localdata1 & 0xff;
+       lval->val[8] = (localdata2 >> 24) & 0xff;
+       lval->val[9] = (localdata2 >> 16) & 0xff;
+       lval->val[10] = (localdata2 >> 8) & 0xff;
+       lval->val[11] = localdata2 & 0xff;
+
        return p;
 }
 
@@ -439,23 +430,16 @@ static const char *lcommunity_gettoken(const char *str,
 struct lcommunity *lcommunity_str2com(const char *str)
 {
        struct lcommunity *lcom = NULL;
-       enum lcommunity_token token = lcommunity_token_unknown;
        struct lcommunity_val lval;
 
+       if (!lcommunity_list_valid(str, LARGE_COMMUNITY_LIST_STANDARD))
+               return NULL;
+
        do {
-               str = lcommunity_gettoken(str, &lval, &token);
-               switch (token) {
-               case lcommunity_token_val:
-                       if (lcom == NULL)
-                               lcom = lcommunity_new();
-                       lcommunity_add_val(lcom, &lval);
-                       break;
-               case lcommunity_token_unknown:
-               default:
-                       if (lcom)
-                               lcommunity_free(&lcom);
-                       return NULL;
-               }
+               str = lcommunity_gettoken(str, &lval);
+               if (lcom == NULL)
+                       lcom = lcommunity_new();
+               lcommunity_add_val(lcom, &lval);
        } while (str);
 
        return lcom;

diff --git a/bgpd/bgp_lcommunity.h b/bgpd/bgp_lcommunity.h
index c96df8482ddbe5eeae7a1050b83b1ea02164fd12..6ccb6b7879e4520a747068bdd26f037e8a6aa555 100644 (file)
--- a/bgpd/bgp_lcommunity.h
+++ b/bgpd/bgp_lcommunity.h
@@ -23,6 +23,7 @@
 
 #include "lib/json.h"
 #include "bgpd/bgp_route.h"
+#include "bgpd/bgp_clist.h"
 
 /* Large Communities value is twelve octets long.  */
 #define LCOMMUNITY_SIZE                        12

diff --git a/tests/topotests/bgp_large_community/test_bgp_large_community_topo_1.py b/tests/topotests/bgp_large_community/test_bgp_large_community_topo_1.py
index 40489f438f6892916fc5c850eef6e0b5feed1dc9..31fbdcd4b5a02b4c4fd0556e63825d966dd64f59 100644 (file)
--- a/tests/topotests/bgp_large_community/test_bgp_large_community_topo_1.py
+++ b/tests/topotests/bgp_large_community/test_bgp_large_community_topo_1.py
@@ -1174,6 +1174,39 @@ def test_large_community_boundary_values(request):
     )
 
 
+def test_large_community_invalid_chars(request):
+    """
+    BGP canonical lcommunities must only be digits
+    """
+    tc_name = request.node.name
+    write_test_header(tc_name)
+    tgen = get_topogen()
+
+    # Don"t run this test if we have any failure.
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    input_dict = {
+        "r4": {
+            "bgp_community_lists": [
+                {
+                    "community_type": "standard",
+                    "action": "permit",
+                    "name": "ANY",
+                    "value": "1:a:2",
+                    "large": True,
+                }
+            ]
+        }
+    }
+
+    step("Checking boundary value for community 1:a:2")
+    result = create_bgp_community_lists(tgen, input_dict)
+    assert result is not True, "Test case {} : Failed \n Error: {}".format(
+        tc_name, result
+    )
+
+
 def test_large_community_after_clear_bgp(request):
     """
     Clear BGP neighbor-ship and check if large community and community