detect/iponly: fix netmask handling

If the ipaddress was not the address range start, it was not masked to turn
it into that. So 1.2.3.4/24 was not stored as address 1.2.3.0 with netmask 24,
but as 1.2.3.4 with netmask 24. This was then propagated into the radix tree,
where it was used as an exact key in exact lookups, giving unexpected results.

This patch implements the netmask handling for IPv4 and IPv6, and adds a set
of tests for it.

Bug: #5081.
Bug: #5066.
(cherry picked from commit 51d4e0dced9cac7463ee924b00bc8666c68b20c3)

diff --git a/src/detect-engine-address.c b/src/detect-engine-address.c
index 51445852cf7d0344b936de8edac0c36ffbd9f59e..fb8c045953542f5998fa96353a129627abaefd03 100644 (file)
--- a/src/detect-engine-address.c
+++ b/src/detect-engine-address.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 2007-2021 Open Information Security Foundation
+/* Copyright (C) 2007-2022 Open Information Security Foundation
  *
  * You can copy, redistribute or modify this Program under the terms of
  * the GNU General Public License version 2 as published by the Free
@@ -1968,20 +1968,37 @@ static int AddressTestParse03(void)
 
 static int AddressTestParse04(void)
 {
-    int result = 1;
     DetectAddress *dd = DetectAddressParseSingle("1.2.3.4/255.255.255.0");
+    FAIL_IF_NULL(dd);
+
+    char left[16], right[16];
+    PrintInet(AF_INET, (const void *)&dd->ip.addr_data32[0], left, sizeof(left));
+    PrintInet(AF_INET, (const void *)&dd->ip2.addr_data32[0], right, sizeof(right));
+    SCLogDebug("left %s right %s", left, right);
+    FAIL_IF_NOT(dd->ip.addr_data32[0] == SCNtohl(16909056));
+    FAIL_IF_NOT(dd->ip2.addr_data32[0] == SCNtohl(16909311));
+    FAIL_IF_NOT(strcmp(left, "1.2.3.0") == 0);
+    FAIL_IF_NOT(strcmp(right, "1.2.3.255") == 0);
+
+    DetectAddressFree(dd);
+    PASS;
+}
 
-    if (dd) {
-        if (dd->ip.addr_data32[0] != SCNtohl(16909056)||
-            dd->ip2.addr_data32[0] != SCNtohl(16909311)) {
-            result = 0;
-        }
+/** \test that address range sets proper start address */
+static int AddressTestParse04bug5081(void)
+{
+    DetectAddress *dd = DetectAddressParseSingle("1.2.3.64/26");
+    FAIL_IF_NULL(dd);
 
-        DetectAddressFree(dd);
-        return result;
-    }
+    char left[16], right[16];
+    PrintInet(AF_INET, (const void *)&dd->ip.addr_data32[0], left, sizeof(left));
+    PrintInet(AF_INET, (const void *)&dd->ip2.addr_data32[0], right, sizeof(right));
+    SCLogDebug("left %s right %s", left, right);
+    FAIL_IF_NOT(strcmp(left, "1.2.3.64") == 0);
+    FAIL_IF_NOT(strcmp(right, "1.2.3.127") == 0);
 
-    return 0;
+    DetectAddressFree(dd);
+    PASS;
 }
 
 static int AddressTestParse05(void)
@@ -4839,6 +4856,7 @@ void DetectAddressTests(void)
     UtRegisterTest("AddressTestParse02", AddressTestParse02);
     UtRegisterTest("AddressTestParse03", AddressTestParse03);
     UtRegisterTest("AddressTestParse04", AddressTestParse04);
+    UtRegisterTest("AddressTestParse04bug5081", AddressTestParse04bug5081);
     UtRegisterTest("AddressTestParse05", AddressTestParse05);
     UtRegisterTest("AddressTestParse06", AddressTestParse06);
     UtRegisterTest("AddressTestParse07", AddressTestParse07);

diff --git a/src/detect-engine-iponly.c b/src/detect-engine-iponly.c
index fb687cbfed44731cffb46fdfb3e3d41cc4bf8975..1ee8f634194946a7860125ec96365c2240bef8da 100644 (file)
--- a/src/detect-engine-iponly.c
+++ b/src/detect-engine-iponly.c
@@ -55,6 +55,7 @@
 #include "util-print.h"
 #include "util-profiling.h"
 #include "util-validate.h"
+#include "util-cidr.h"
 
 #ifdef OS_WIN32
 #include <winsock.h>
@@ -175,6 +176,7 @@ static int IPOnlyCIDRItemParseSingle(IPOnlyCIDRItem **pdd, const char *str)
                     goto error;
 
                 dd->netmask = cidr;
+                netmask = CIDRGet(cidr);
             } else {
                 /* 1.2.3.4/255.255.255.0 format */
                 r = inet_pton(AF_INET, mask, &in);
@@ -182,12 +184,12 @@ static int IPOnlyCIDRItemParseSingle(IPOnlyCIDRItem **pdd, const char *str)
                     goto error;
 
                 netmask = in.s_addr;
-
                 if (netmask != 0) {
+                    uint32_t m = netmask;
                     /* Extract cidr netmask */
-                    while ((0x01 & netmask) == 0) {
+                    while ((0x01 & m) == 0) {
                         dd->netmask++;
-                        netmask = netmask >> 1;
+                        m = m >> 1;
                     }
                     dd->netmask = 32 - dd->netmask;
                 }
@@ -197,7 +199,7 @@ static int IPOnlyCIDRItemParseSingle(IPOnlyCIDRItem **pdd, const char *str)
             if (r <= 0)
                 goto error;
 
-            dd->ip[0] = in.s_addr;
+            dd->ip[0] = in.s_addr & netmask;
 
         } else if ((ip2 = strchr(ip, '-')) != NULL)  {
             /* 1.2.3.4-1.2.3.6 range format */
@@ -266,8 +268,8 @@ static int IPOnlyCIDRItemParseSingle(IPOnlyCIDRItem **pdd, const char *str)
         }
     } else {
         /* IPv6 Address */
-        struct in6_addr in6;
-        uint32_t ip6addr[4];
+        struct in6_addr in6, mask6;
+        uint32_t ip6addr[4], netmask[4];
 
         dd->family = AF_INET6;
 
@@ -282,7 +284,14 @@ static int IPOnlyCIDRItemParseSingle(IPOnlyCIDRItem **pdd, const char *str)
             /* Format is cidr val */
             dd->netmask = atoi(mask);
 
-            memcpy(dd->ip, &in6.s6_addr, sizeof(ip6addr));
+            memcpy(&ip6addr, &in6.s6_addr, sizeof(ip6addr));
+            CIDRGetIPv6(dd->netmask, &mask6);
+            memcpy(&netmask, &mask6.s6_addr, sizeof(netmask));
+
+            dd->ip[0] = ip6addr[0] & netmask[0];
+            dd->ip[1] = ip6addr[1] & netmask[1];
+            dd->ip[2] = ip6addr[2] & netmask[2];
+            dd->ip[3] = ip6addr[3] & netmask[3];
         } else {
             r = inet_pton(AF_INET6, ip, &in6);
             if (r <= 0)
@@ -2308,6 +2317,97 @@ static int IPOnlyTestSig18(void)
     PASS;
 }
 
+/** \test build IP-only tree */
+static int IPOnlyTestBug5066v1(void)
+{
+    DetectEngineCtx *de_ctx = DetectEngineCtxInit();
+    FAIL_IF(de_ctx == NULL);
+    de_ctx->flags |= DE_QUIET;
+
+    Signature *s = DetectEngineAppendSig(
+            de_ctx, "alert ip [1.2.3.4/24,1.2.3.64/27] any -> any any (sid:1;)");
+    FAIL_IF_NULL(s);
+    s = DetectEngineAppendSig(de_ctx, "alert ip [1.2.3.4/24] any -> any any (sid:2;)");
+    FAIL_IF_NULL(s);
+
+    SigGroupBuild(de_ctx);
+
+    DetectEngineCtxFree(de_ctx);
+    PASS;
+}
+
+static int IPOnlyTestBug5066v2(void)
+{
+    IPOnlyCIDRItem *x = IPOnlyCIDRItemNew();
+    FAIL_IF_NULL(x);
+
+    FAIL_IF(IPOnlyCIDRItemParseSingle(&x, "1.2.3.4/24") != 0);
+
+    char ip[16];
+    PrintInet(AF_INET, (const void *)&x->ip[0], ip, sizeof(ip));
+    SCLogDebug("ip %s netmask %d", ip, x->netmask);
+
+    FAIL_IF_NOT(strcmp(ip, "1.2.3.0") == 0);
+    FAIL_IF_NOT(x->netmask == 24);
+
+    IPOnlyCIDRListFree(x);
+    PASS;
+}
+
+static int IPOnlyTestBug5066v3(void)
+{
+    IPOnlyCIDRItem *x = IPOnlyCIDRItemNew();
+    FAIL_IF_NULL(x);
+
+    FAIL_IF(IPOnlyCIDRItemParseSingle(&x, "1.2.3.64/26") != 0);
+
+    char ip[16];
+    PrintInet(AF_INET, (const void *)&x->ip[0], ip, sizeof(ip));
+    SCLogDebug("ip %s netmask %d", ip, x->netmask);
+
+    FAIL_IF_NOT(strcmp(ip, "1.2.3.64") == 0);
+    FAIL_IF_NOT(x->netmask == 26);
+
+    IPOnlyCIDRListFree(x);
+    PASS;
+}
+
+static int IPOnlyTestBug5066v4(void)
+{
+    IPOnlyCIDRItem *x = IPOnlyCIDRItemNew();
+    FAIL_IF_NULL(x);
+
+    FAIL_IF(IPOnlyCIDRItemParseSingle(&x, "2000::1:1/122") != 0);
+
+    char ip[64];
+    PrintInet(AF_INET6, (const void *)&x->ip, ip, sizeof(ip));
+    SCLogDebug("ip %s netmask %d", ip, x->netmask);
+
+    FAIL_IF_NOT(strcmp(ip, "2000:0000:0000:0000:0000:0000:0001:0000") == 0);
+    FAIL_IF_NOT(x->netmask == 122);
+
+    IPOnlyCIDRListFree(x);
+    PASS;
+}
+
+static int IPOnlyTestBug5066v5(void)
+{
+    IPOnlyCIDRItem *x = IPOnlyCIDRItemNew();
+    FAIL_IF_NULL(x);
+
+    FAIL_IF(IPOnlyCIDRItemParseSingle(&x, "2000::1:40/122") != 0);
+
+    char ip[64];
+    PrintInet(AF_INET6, (const void *)&x->ip, ip, sizeof(ip));
+    SCLogDebug("ip %s netmask %d", ip, x->netmask);
+
+    FAIL_IF_NOT(strcmp(ip, "2000:0000:0000:0000:0000:0000:0001:0040") == 0);
+    FAIL_IF_NOT(x->netmask == 122);
+
+    IPOnlyCIDRListFree(x);
+    PASS;
+}
+
 #endif /* UNITTESTS */
 
 void IPOnlyRegisterTests(void)
@@ -2344,6 +2444,12 @@ void IPOnlyRegisterTests(void)
 
     UtRegisterTest("IPOnlyTestSig17", IPOnlyTestSig17);
     UtRegisterTest("IPOnlyTestSig18", IPOnlyTestSig18);
+
+    UtRegisterTest("IPOnlyTestBug5066v1", IPOnlyTestBug5066v1);
+    UtRegisterTest("IPOnlyTestBug5066v2", IPOnlyTestBug5066v2);
+    UtRegisterTest("IPOnlyTestBug5066v3", IPOnlyTestBug5066v3);
+    UtRegisterTest("IPOnlyTestBug5066v4", IPOnlyTestBug5066v4);
+    UtRegisterTest("IPOnlyTestBug5066v5", IPOnlyTestBug5066v5);
 #endif
 
     return;