From: Victor Julien Date: Fri, 11 Feb 2022 14:50:01 +0000 (+0100) Subject: radix: fix FP/FN issue in IP-only X-Git-Tag: suricata-5.0.9~81 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1584ce304c001bb5d420818d20c43bba3b4054cc;p=thirdparty%2Fsuricata.git radix: fix FP/FN issue in IP-only A bug was reported about the IP-only rules not correctly matching. This was traced to the rules in question not getting recorded into the IP-only radix tree correctly. Sequence: - 100.117.241.0/25 inserted into the tree - 100.117.241.0/26 inserted into the tree Both are part of the same radix node, but recorded by their different netmasks in the user data portion. Then faulty insert happens: - 100.117.241.64/26 For reference, these net blocks compute to: - 100.117.241.0/25: 100.117.241.0 - 100.117.241.127 - 100.117.241.0/26: 100.117.241.0 - 100.117.241.63 - 100.117.241.64/26: 100.117.241.64 - 100.117.241.127 The IP-only engine first does a search to get to the user data it may need to include. It does so for with `SCRadixFindKeyIPV4ExactMatch` for single IPs, or using `SCRadixFindKeyIPV4Netblock` in case of a netblock. Any "match" from either of these is considered an "exact match" by the IP-only setup code. This exact match expectation turned out to be wrong and `SCRadixFindKeyIPV4Netblock` behaved more like "best match" instead, which is a non-exact match, but its the next best match if no exact match is found. The way the look up for 100.117.241.64/26 went wrong, is that it returned the user data for 100.117.241.0/26. This happens as follows: - first it would do an exact find, which didn't give a result - then it removed bits from the keystream until it found a matching node and explore if any of the netmasks it contained matched. Here the first step of the bug started: it considered the netmask (with user data) a match that matched the number of bits of the matching key, but not of the actual range netmask cidr value. So in this case the number of shared bits between `100.117.241.0/25` and `100.117.241.64/26` was 25, so it assumed that the user data for the netmask 25 was the match. To summarize this step, there are 2 problems with this: 1. it returns a match on something that isn't an exact match 2. it considered the wrong netmask value - the radix code then took the returned node, and did the netmask check again. This time it did use its own netmask value, so this time it did find the netmask 26 (+ user data). However because of the node that was returned, this netmask (+user data) belongs to `100.117.241.0`, not to `100.117.241.64`. - the IP-only detection code was satisfied with what it assumed to be "exact match" and just updated the user data to include the user data that should have been associated with `100.117.241.64/26` to `100.117.241.0/26`. This patch addresses the issue as follows: It makes `SCRadixFindKeyIPV4Netblock` also return an exact match by propagating the netmask in the search and in the evaluation of the stored netmasks. It does away with the secondary netmask (+user data) evaluation. `SCRadixFindKeyIPV4Netblock` is expected to handle this correctly. The IP-only engine will fall back to the "not found" path, which does an explicit "best match" lookup and then insert a new entry into the radix tree based on the user data of the "best match". Issue was present for IPv6 as well. Bug: #5066. (cherry picked from commit 6aa6e3f95372e256006b78e921391d88c1010f15) --- diff --git a/src/util-radix-tree.c b/src/util-radix-tree.c index 32e4886ac6..8b2941d30f 100644 --- a/src/util-radix-tree.c +++ b/src/util-radix-tree.c @@ -1331,8 +1331,8 @@ void SCRadixRemoveKeyIPV6(uint8_t *key_stream, SCRadixTree *tree) * \param prefix Pointer to the prefix that contains the ip address * \param node Pointer to the node from where we have to climb the tree */ -static inline SCRadixNode *SCRadixFindKeyIPNetblock(uint8_t *key_stream, uint8_t key_bitlen, - SCRadixNode *node, void **user_data_result) +static inline SCRadixNode *SCRadixFindKeyIPNetblock( + uint8_t *key_stream, uint8_t key_bitlen, SCRadixNode *node, void **user_data_result) { SCRadixNode *netmask_node = NULL; uint32_t mask = 0; @@ -1399,9 +1399,10 @@ static inline SCRadixNode *SCRadixFindKeyIPNetblock(uint8_t *key_stream, uint8_t * \param key_bitlen The bitlen of the above stream. * \param tree Pointer to the Radix tree * \param exact_match The key to be searched is an ip address + * \param netmask Netmask used during exact match */ -static SCRadixNode *SCRadixFindKey(uint8_t *key_stream, uint16_t key_bitlen, - SCRadixTree *tree, int exact_match, void **user_data_result) +static SCRadixNode *SCRadixFindKey(uint8_t *key_stream, uint16_t key_bitlen, uint8_t netmask, + SCRadixTree *tree, int exact_match, void **user_data_result) { if (tree == NULL || tree->head == NULL) return NULL; @@ -1440,7 +1441,8 @@ static SCRadixNode *SCRadixFindKey(uint8_t *key_stream, uint16_t key_bitlen, if (key_bitlen % 8 == 0 || (node->prefix->stream[bytes] & mask) == (tmp_stream[bytes] & mask)) { - if (SCRadixPrefixContainNetmaskAndSetUserData(node->prefix, key_bitlen, 1, user_data_result)) { + if (SCRadixPrefixContainNetmaskAndSetUserData( + node->prefix, netmask, 1, user_data_result)) { return node; } } @@ -1465,7 +1467,7 @@ static SCRadixNode *SCRadixFindKey(uint8_t *key_stream, uint16_t key_bitlen, SCRadixNode *SCRadixFindKeyGeneric(uint8_t *key_stream, uint16_t key_bitlen, SCRadixTree *tree, void **user_data_result) { - return SCRadixFindKey(key_stream, key_bitlen, tree, 1, user_data_result); + return SCRadixFindKey(key_stream, key_bitlen, 0, tree, 1, user_data_result); /* TODO netmask? */ } /** @@ -1477,7 +1479,7 @@ SCRadixNode *SCRadixFindKeyGeneric(uint8_t *key_stream, uint16_t key_bitlen, */ SCRadixNode *SCRadixFindKeyIPV4ExactMatch(uint8_t *key_stream, SCRadixTree *tree, void **user_data_result) { - return SCRadixFindKey(key_stream, 32, tree, 1, user_data_result); + return SCRadixFindKey(key_stream, 32, 32, tree, 1, user_data_result); } /** @@ -1489,7 +1491,7 @@ SCRadixNode *SCRadixFindKeyIPV4ExactMatch(uint8_t *key_stream, SCRadixTree *tree */ SCRadixNode *SCRadixFindKeyIPV4BestMatch(uint8_t *key_stream, SCRadixTree *tree, void **user_data_result) { - return SCRadixFindKey(key_stream, 32, tree, 0, user_data_result); + return SCRadixFindKey(key_stream, 32, 32, tree, 0, user_data_result); } /** @@ -1502,15 +1504,8 @@ SCRadixNode *SCRadixFindKeyIPV4BestMatch(uint8_t *key_stream, SCRadixTree *tree, SCRadixNode *SCRadixFindKeyIPV4Netblock(uint8_t *key_stream, SCRadixTree *tree, uint8_t netmask, void **user_data_result) { - SCRadixNode *node = NULL; - node = SCRadixFindKey(key_stream, 32, tree, 0, user_data_result); - if (node == NULL) - return node; - - if (SCRadixPrefixContainNetmaskAndSetUserData(node->prefix, netmask, 1, user_data_result)) - return node; - else - return NULL; + SCRadixNode *node = SCRadixFindKey(key_stream, 32, netmask, tree, 1, user_data_result); + return node; } /** @@ -1523,15 +1518,8 @@ SCRadixNode *SCRadixFindKeyIPV4Netblock(uint8_t *key_stream, SCRadixTree *tree, SCRadixNode *SCRadixFindKeyIPV6Netblock(uint8_t *key_stream, SCRadixTree *tree, uint8_t netmask, void **user_data_result) { - SCRadixNode *node = NULL; - node = SCRadixFindKey(key_stream, 128, tree, 0, user_data_result); - if (node == NULL) - return node; - - if (SCRadixPrefixContainNetmaskAndSetUserData(node->prefix, (uint16_t)netmask, 1, user_data_result)) - return node; - else - return NULL; + SCRadixNode *node = SCRadixFindKey(key_stream, 128, netmask, tree, 1, user_data_result); + return node; } /** @@ -1543,7 +1531,7 @@ SCRadixNode *SCRadixFindKeyIPV6Netblock(uint8_t *key_stream, SCRadixTree *tree, */ SCRadixNode *SCRadixFindKeyIPV6ExactMatch(uint8_t *key_stream, SCRadixTree *tree, void **user_data_result) { - return SCRadixFindKey(key_stream, 128, tree, 1, user_data_result); + return SCRadixFindKey(key_stream, 128, 128, tree, 1, user_data_result); } /** @@ -1555,7 +1543,7 @@ SCRadixNode *SCRadixFindKeyIPV6ExactMatch(uint8_t *key_stream, SCRadixTree *tree */ SCRadixNode *SCRadixFindKeyIPV6BestMatch(uint8_t *key_stream, SCRadixTree *tree, void **user_data_result) { - return SCRadixFindKey(key_stream, 128, tree, 0, user_data_result); + return SCRadixFindKey(key_stream, 128, 128, tree, 0, user_data_result); } /**