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.

diff --git a/src/util-radix-tree.c b/src/util-radix-tree.c
index e2552b34c6544701771f718410de369915b43adb..f595c0a340e0bdddd04821aa23b6329f5fe51e3b 100644 (file)
--- a/src/util-radix-tree.c
+++ b/src/util-radix-tree.c
@@ -1329,8 +1329,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;
@@ -1397,9 +1397,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;
@@ -1438,7 +1439,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;
             }
         }
@@ -1463,7 +1465,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? */
 }
 
 /**
@@ -1475,7 +1477,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);
 }
 
 /**
@@ -1487,7 +1489,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);
 }
 
 /**
@@ -1500,15 +1502,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;
 }
 
 /**
@@ -1521,15 +1516,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;
 }
 
 /**
@@ -1541,7 +1529,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);
 }
 
 /**
@@ -1553,7 +1541,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);
 }
 
 /**