Bug fix: IPv6 addresses in negated range and IPv6 string into radix tree.

I found three somewhat serious IPv6 address bugs within the Suricata 2.0.x source code. Two are in the source module "detect-engine-address.c", and the third is in "util-radix-tree.c".

The first bug occurs within the function DetectAddressParse2(). When parsing an address string and a negated block is encountered (such as when parsing !$HOME_NET, for example), any corresponding IPv6 addresses were not getting added to the Group Heads in the DetectAddressList. Only IPv4 addresses were being added.

I discovered another bug related to IPv6 address ranges in the Signature Match Address Array comparison code for IPv6 addresses. The function DetectAddressMatchIPv6() walks a signature's source or destination match address list comparing each to the current packet's corresponding address value. The match address list consists of value pairs representing a lower and upper IP address range. If the packet's address is within that range (including equal to either the lower or upper bound), then a signature match flag is returned.

The original test of each signature match address to the packet was performed using a set of four compounded AND comparisons looking at each of the four 32-bit blocks that comprise an IPv6 address. The problem with the old comparison is that if ANY of the four 32-bit blocks failed the test, then a "no-match" was returned. This is incorrect. If one or more of the more significant 32-bit blocks met the condition, then it is a match no matter if some of the less significant 32-bit blocks did not meet the condition. Consider this example where Packet represents the packet address being checked, and Target represents the upper bound of a match address pair. We are testing if Packet is less than Target.

Packet -- 2001:0470 : 1f07:00e2 : 1988:01f1 : d468:27ab
Target -- 2001:0470 : 1f07:00e2 : a48c:2e52 : d121:101e

In this example the Packet's address is less than the target and it should give a match. However, the old code would compare each 32-bit block (shown spaced out above for clarity) and logically AND the result with the next least significant block comparison. If any of the four blocks failed the comparison, that kicked out the whole address. The flaw is illustrated above. The first two blocks are 2001:0470 and 1f07:00e2 and yield TRUE; the next less significant block is 1988:01f1 and a48c:2e52, and also yields TRUE (that is, Packet is less than Target); but the last block compare is FALSE (d468:27ab is not less than d121:101e). That last block is the least significant block, though, so its FALSE determination should not invalidate a TRUE from any of the more significant blocks. However, in the previous code using the compound logical AND block, that last least significant block would invalidate the tests done with the more significant blocks.

The other bug I found for IPv6 occurs when trying to parse and insert an IPv6 address into a Radix Tree using the function SCRadixAddKeyIPV6String(). The test for min and max values for an IPv6 CIDR mask incorrectly tests the upper limit as 32 when it should be 128 for an IPv6 address. I think this perhaps is an old copy-paste error if the IPv6 version of this function was initially copied from the corresponding IPv4 version directly above it in the code. Without this patch, the function will return null when you attempt to add an IPv6 network whose CIDR mask is larger than 32 (for example, the popular /64 mask will cause the function to return the NULL error condition).

(amended by Victor Julien)

diff --git a/src/detect-engine-address.c b/src/detect-engine-address.c
index 44a96a8e2a63ea4dcf07bfc3cc7632e3e59fc3b9..51385bba5aca714ba4695cad7a96f917dfdeb809 100644 (file)
--- a/src/detect-engine-address.c
+++ b/src/detect-engine-address.c
@@ -968,27 +968,55 @@ int DetectAddressParse2(DetectAddressHead *gh, DetectAddressHead *ghn, char *s,
                         goto error;
 
                     DetectAddress *tmp_ad;
+                    DetectAddress *tmp_ad2;
 #ifdef DEBUG
-                    SCLogDebug("tmp_gh");
+                    SCLogDebug("tmp_gh: IPv4");
                     for (tmp_ad = tmp_gh.ipv4_head; tmp_ad; tmp_ad = tmp_ad->next) {
                         DetectAddressPrint(tmp_ad);
                     }
-                    SCLogDebug("tmp_ghn");
+                    SCLogDebug("tmp_ghn: IPv4");
                     for (tmp_ad = tmp_ghn.ipv4_head; tmp_ad; tmp_ad = tmp_ad->next) {
                         DetectAddressPrint(tmp_ad);
                     }
+                    SCLogDebug("tmp_gh: IPv6");
+                    for (tmp_ad = tmp_gh.ipv6_head; tmp_ad; tmp_ad = tmp_ad->next) {
+                        DetectAddressPrint(tmp_ad);
+                    }
+                    SCLogDebug("tmp_ghn: IPv6");
+                    for (tmp_ad = tmp_ghn.ipv6_head; tmp_ad; tmp_ad = tmp_ad->next) {
+                        DetectAddressPrint(tmp_ad);
+                    }
 #endif
                     if (DetectAddressMergeNot(&tmp_gh, &tmp_ghn) < 0)
                         goto error;
 
                     SCLogDebug("merged succesfully");
 
-                    /* insert the addresses into the negated list */
+                    /* insert the IPv4 addresses into the negated list */
                     for (tmp_ad = tmp_gh.ipv4_head; tmp_ad; tmp_ad = tmp_ad->next) {
-                        DetectAddressPrint(tmp_ad);
-                        DetectAddressInsert(NULL, ghn, tmp_ad);
+                        /* work with a copy of the address group */
+                        tmp_ad2 = DetectAddressCopy(tmp_ad);
+                        if (tmp_ad2 == NULL) {
+                            SCLogDebug("DetectAddressCopy failed");
+                            goto error;
+                        }
+                        DetectAddressPrint(tmp_ad2);
+                        DetectAddressInsert(NULL, ghn, tmp_ad2);
+                    }
+
+                    /* insert the IPv6 addresses into the negated list */
+                    for (tmp_ad = tmp_gh.ipv6_head; tmp_ad; tmp_ad = tmp_ad->next) {
+                        /* work with a copy of the address group */
+                        tmp_ad2 = DetectAddressCopy(tmp_ad);
+                        if (tmp_ad2 == NULL) {
+                            SCLogDebug("DetectAddressCopy failed");
+                            goto error;
+                        }
+                        DetectAddressPrint(tmp_ad2);
+                        DetectAddressInsert(NULL, ghn, tmp_ad2);
                     }
 
+                    DetectAddressHeadCleanup(&tmp_gh);
                     DetectAddressHeadCleanup(&tmp_ghn);
                 }
                 n_set = 0;
@@ -1656,21 +1684,68 @@ int DetectAddressMatchIPv6(DetectMatchAddressIPv6 *addrs, uint16_t addrs_cnt, Ad
     }
 
     uint16_t idx;
-    for (idx = 0; idx < addrs_cnt; idx++) {
-        if ((ntohl(a->addr_data32[0]) >= addrs[idx].ip[0] &&
-             ntohl(a->addr_data32[0]) <= addrs[idx].ip2[0]) &&
-
-            (ntohl(a->addr_data32[1]) >= addrs[idx].ip[1] &&
-             ntohl(a->addr_data32[1]) <= addrs[idx].ip2[1]) &&
+    int i = 0;
+    uint16_t result1, result2;
 
-            (ntohl(a->addr_data32[2]) >= addrs[idx].ip[2] &&
-             ntohl(a->addr_data32[2]) <= addrs[idx].ip2[2]) &&
+    /* See if the packet address is within the range of any entry in the
+     * signature's address match array.
+     */
+    for (idx = 0; idx < addrs_cnt; idx++) {
+        result1 = result2 = 0;
 
-            (ntohl(a->addr_data32[3]) >= addrs[idx].ip[3] &&
-             ntohl(a->addr_data32[3]) <= addrs[idx].ip2[3]))
+        /* See if packet address equals either limit. Return 1 if true. */
+        if (ntohl(a->addr_data32[0]) == addrs[idx].ip[0] &&
+            ntohl(a->addr_data32[1]) == addrs[idx].ip[1] &&
+            ntohl(a->addr_data32[2]) == addrs[idx].ip[2] &&
+            ntohl(a->addr_data32[3]) == addrs[idx].ip[3])
+        {
+            SCReturnInt(1);
+        }
+        if (ntohl(a->addr_data32[0]) == addrs[idx].ip2[0] &&
+            ntohl(a->addr_data32[1]) == addrs[idx].ip2[1] &&
+            ntohl(a->addr_data32[2]) == addrs[idx].ip2[2] &&
+            ntohl(a->addr_data32[3]) == addrs[idx].ip2[3])
         {
             SCReturnInt(1);
         }
+
+        /* See if packet address is greater than lower limit
+         * of the current signature address match pair.
+         */
+        for (i = 0; i < 4; i++) {
+            if (ntohl(a->addr_data32[i]) > addrs[idx].ip[i]) {
+                result1 = 1;
+                break;
+            }
+            if (ntohl(a->addr_data32[i]) < addrs[idx].ip[i]) {
+                result1 = 0;
+                break;
+            }
+        }
+
+        /* If not greater than lower limit, try next address match entry */
+        if (result1 == 0)
+            continue;
+
+        /* See if packet address is less than upper limit
+         * of the current signature address match pair.
+         */
+        for (i = 0; i < 4; i++) {
+            if (ntohl(a->addr_data32[i]) < addrs[idx].ip2[i]) {
+                result2 = 1;
+                break;
+            }
+            if (ntohl(a->addr_data32[i]) > addrs[idx].ip2[i]) {
+                result2 = 0;
+                break;
+            }
+        }
+
+        /* Return a match if packet address is between the two
+         * signature address match limits.
+         */
+        if (result1 == 1 && result2 == 1)
+            SCReturnInt(1);
     }
 
     SCReturnInt(0);

diff --git a/src/util-radix-tree.c b/src/util-radix-tree.c
index 5ae08b472a61c734e826ae12378ac2967011cf8b..39f8304ef907e4fe97d3cc9592ebf18dc1642636 100644 (file)
--- a/src/util-radix-tree.c
+++ b/src/util-radix-tree.c
@@ -1001,7 +1001,7 @@ SCRadixNode *SCRadixAddKeyIPV6String(const char *str, SCRadixTree *tree, void *u
 
         /* Get binary values for cidr mask */
         cidr = atoi(mask_str);
-        if ((cidr < 0) || (cidr > 32)) {
+        if ((cidr < 0) || (cidr > 128)) {
             return NULL;
         }
         netmask = (uint8_t)cidr;