From: Wesley Coakley Date: Tue, 5 Jan 2021 09:22:57 +0000 (-0500) Subject: bgpd: separate lcommunity validation from tokenizer X-Git-Tag: base_7.6~82^2 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=refs%2Fpull%2F7749%2Fhead;p=mirror%2Ffrr.git 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 --- diff --git a/bgpd/bgp_clist.c b/bgpd/bgp_clist.c index 247d758f8c..6ac6cf56dd 100644 --- 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 f7d46525a0..632e1730cc 100644 --- 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 5900fcf862..88a85c979e 100644 --- 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 c96df8482d..6ccb6b7879 100644 --- 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 40489f438f..31fbdcd4b5 100644 --- 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