Using RadixTree instead of list to storage IPs

Used by the operator @ipMatch and variants, this structure storage all the IPs
addresses for later comparison. Last version was using RadixTree only if the
set of IPs was specified from files. IPs specified as parameters, was using
a chained list. Chained lists may affect the performance, since lookups in
worst case will be O(n). RadixTrees could provide better results depending
on the amount of elements and its contents.

apache2/apache2_config.c

@@ -1671,8 +1671,7 @@ static const char *cmd_rule_perf_time(cmd_parms *cmd, void *_dcfg,
 }
 
 char *parser_conn_limits_operator(apr_pool_t *mp, const char *p2,
-    TreeRoot **whitelist, msre_ipmatch **whitelist_param,
+    TreeRoot **whitelist, TreeRoot **suspicious_list, 
-    TreeRoot **suspicious_list, msre_ipmatch **suspicious_list_param,
     const char *filename)
 {
     int res = 0;
@@ -1691,22 +1690,18 @@ char *parser_conn_limits_operator(apr_pool_t *mp, const char *p2,
     if ((strncasecmp(p2, "!@ipMatchFromFile", strlen("!@ipMatchFromFile")) == 0) ||
         (strncasecmp(p2, "!@ipMatchF", strlen("!@ipMatchF")) == 0)) {
 
-        res = ip_tree_from_file(whitelist,
+        res = ip_tree_from_file(whitelist, file, mp, &error_msg);
-            file, mp, NULL);
     }
     else if (strncasecmp(p2, "!@ipMatch", strlen("!@ipMatch")) == 0) {
-        res = ip_list_from_param(mp, param,
+        res = ip_tree_from_param(mp, param, whitelist, &error_msg);
-            whitelist_param, &error_msg);
     }
     else if ((strncasecmp(p2, "@ipMatchFromFile", strlen("@ipMatchFromFile")) == 0) ||
         (strncasecmp(p2, "@ipMatchF", strlen("@ipMatchF")) == 0)) {
 
-        res = ip_tree_from_file(suspicious_list,
+        res = ip_tree_from_file(suspicious_list, file, mp, &error_msg);
-            file, mp, NULL);
     }
     else if (strncasecmp(p2, "@ipMatch", strlen("@ipMatch")) == 0) {
-        res = ip_list_from_param(mp, param,
+        res = ip_tree_from_param(mp, param, suspicious_list, &error_msg);
-            suspicious_list_param, &error_msg);
     }
     else {
         return apr_psprintf(mp, "ModSecurity: Invalid operator for " \
@@ -1757,9 +1752,8 @@ static const char *cmd_conn_read_state_limit(cmd_parms *cmd, void *_dcfg,
 
     if (p2 != NULL) {
         char *param = parser_conn_limits_operator(cmd->pool, p2,
-            &conn_read_state_whitelist, &conn_read_state_whitelist_param,
+            &conn_read_state_whitelist, &conn_read_state_suspicious_list,
-            &conn_read_state_suspicious_list,
+            cmd->directive->filename);
-            &conn_read_state_suspicious_list_param, cmd->directive->filename);
 
         if (param)
             return param;
@@ -1797,9 +1791,8 @@ static const char *cmd_conn_write_state_limit(cmd_parms *cmd, void *_dcfg,
 
     if (p2 != NULL) {
         char *param = parser_conn_limits_operator(cmd->pool, p2,
-            &conn_write_state_whitelist, &conn_write_state_whitelist_param,
+            &conn_write_state_whitelist, &conn_write_state_suspicious_list,
-            &conn_write_state_suspicious_list,
+            cmd->directive->filename);
-            &conn_write_state_suspicious_list_param, cmd->directive->filename);
 
         if (param)
             return param;

apache2/mod_security2.c

@@ -66,15 +66,10 @@ int DSOLOCAL status_engine_state = STATUS_ENGINE_DISABLED;
 unsigned long int DSOLOCAL conn_read_state_limit = 0;
 TreeRoot DSOLOCAL *conn_read_state_whitelist = 0;
 TreeRoot DSOLOCAL *conn_read_state_suspicious_list = 0;
-msre_ipmatch DSOLOCAL *conn_read_state_whitelist_param = 0;
-msre_ipmatch DSOLOCAL *conn_read_state_suspicious_list_param = 0;
-
-TreeRoot DSOLOCAL *conn_write_state_whitelist = 0;
-TreeRoot DSOLOCAL *conn_write_state_suspicious_list = 0;
-msre_ipmatch DSOLOCAL *conn_write_state_whitelist_param = 0;
-msre_ipmatch DSOLOCAL *conn_write_state_suspicious_list_param = 0;
 
 unsigned long int DSOLOCAL conn_write_state_limit = 0;
+TreeRoot DSOLOCAL *conn_write_state_whitelist = 0;
+TreeRoot DSOLOCAL *conn_write_state_suspicious_list = 0;
 
 #if defined(WIN32) || defined(VERSION_NGINX)
 int (*modsecDropAction)(request_rec *r) = NULL;
@@ -1428,10 +1423,8 @@ static int hook_connection_early(conn_rec *conn)
         if (conn_read_state_limit > 0 && ip_count_r > conn_read_state_limit)
         {
             if (conn_read_state_suspicious_list &&
-                (!((tree_contains_ip(conn->pool,
+                (tree_contains_ip(conn->pool,
-                   conn_read_state_suspicious_list, client_ip, NULL, &error_msg) <= 0) ||
+                   conn_read_state_suspicious_list, client_ip, NULL, &error_msg) <= 0))
-                (list_contains_ip(conn->pool,
-                   conn_read_state_suspicious_list_param, client_ip, &error_msg) <= 0))))
             {
                 ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL, 
                     "ModSecurity: Too many threads [%ld] of %ld allowed in " \
@@ -1440,10 +1433,8 @@ static int hook_connection_early(conn_rec *conn)
                     conn_read_state_limit, client_ip);
             }
 
-            else if ((tree_contains_ip(conn->pool,
+            else if (tree_contains_ip(conn->pool,
-                conn_read_state_whitelist, client_ip, NULL, &error_msg) > 0) ||
+                conn_read_state_whitelist, client_ip, NULL, &error_msg) > 0)
-                (list_contains_ip(conn->pool,
-                    conn_read_state_whitelist_param, client_ip, &error_msg) > 0))
             {
                 ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
                     "ModSecurity: Too many threads [%ld] of %ld allowed in " \
@@ -1464,10 +1455,8 @@ static int hook_connection_early(conn_rec *conn)
         if (conn_write_state_limit > 0 && ip_count_w > conn_write_state_limit)
         {
             if (conn_write_state_suspicious_list &&
-                (!((tree_contains_ip(conn->pool,
+                (tree_contains_ip(conn->pool,
-                    conn_write_state_suspicious_list, client_ip, NULL, &error_msg) <= 0) ||
+                    conn_write_state_suspicious_list, client_ip, NULL, &error_msg) <= 0))
-                (list_contains_ip(conn->pool,
-                    conn_write_state_suspicious_list_param, client_ip, &error_msg) <= 0))))
             {
                 ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
                     "ModSecurity: Too many threads [%ld] of %ld allowed in " \
@@ -1475,10 +1464,8 @@ static int hook_connection_early(conn_rec *conn)
                     "that IP is not part of it, access granted", ip_count_w,
                     conn_read_state_limit, client_ip);
             }
-            else if ((tree_contains_ip(conn->pool,
+            else if (tree_contains_ip(conn->pool,
-                conn_write_state_whitelist, client_ip, NULL, &error_msg) > 0) ||
+                conn_write_state_whitelist, client_ip, NULL, &error_msg) > 0)
-                (list_contains_ip(conn->pool,
-                    conn_write_state_whitelist_param, client_ip, &error_msg) > 0))
             {
                 ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
                     "ModSecurity: Too many threads [%ld] of %ld allowed in " \

apache2/modsecurity.h

@@ -148,14 +148,10 @@ extern DSOLOCAL int status_engine_state;
 extern DSOLOCAL unsigned long int conn_read_state_limit;
 extern DSOLOCAL TreeRoot *conn_read_state_whitelist;
 extern DSOLOCAL TreeRoot *conn_read_state_suspicious_list;
-extern DSOLOCAL msre_ipmatch *conn_read_state_whitelist_param;
-extern DSOLOCAL msre_ipmatch *conn_read_state_suspicious_list_param;
 
 extern DSOLOCAL unsigned long int conn_write_state_limit;
 extern DSOLOCAL TreeRoot *conn_write_state_whitelist;
 extern DSOLOCAL TreeRoot *conn_write_state_suspicious_list;
-extern DSOLOCAL msre_ipmatch *conn_write_state_whitelist_param;
-extern DSOLOCAL msre_ipmatch *conn_write_state_suspicious_list_param;
 
 extern DSOLOCAL unsigned long int unicode_codepage;
 

apache2/msc_util.c

@@ -2389,8 +2389,6 @@ char *construct_single_var(modsec_rec *msr, char *name) {
     return (char *)vx->value;
 }
 
-<<<<<<< HEAD
-
 /**
  * @brief Transforms an apr_array_header_t to a text buffer
  *
@@ -2455,7 +2453,44 @@ not_enough_memory:
     return headers_length;
 }
 
-int ip_tree_from_file(TreeRoot **rtree_, char *uri,
+
+int create_radix_tree(apr_pool_t *mp, TreeRoot **rtree, char **error_msg)
+{
+    *rtree = apr_palloc(mp, sizeof(TreeRoot));
+    if (*rtree == NULL)
+    {
+        *error_msg = apr_psprintf(mp, "Failed allocating " \
+            "memory to TreeRoot.");
+        goto root_node_failed;
+    }
+    memset(*rtree, 0, sizeof(TreeRoot));
+
+    (*rtree)->ipv4_tree = CPTCreateRadixTree(mp);
+    if ((*rtree)->ipv4_tree == NULL)
+    {
+        *error_msg = apr_psprintf(mp, "IPmatch: Tree initialization " \
+            "failed.");
+        goto ipv4_tree_failed;
+    }
+
+    (*rtree)->ipv6_tree = CPTCreateRadixTree(mp);
+    if ((*rtree)->ipv6_tree == NULL)
+    {
+        *error_msg = apr_psprintf(mp, "IPmatch: Tree initialization " \
+            "failed.");
+        goto ipv6_tree_failed;
+    }
+
+    return 0;
+
+ipv6_tree_failed:
+ipv4_tree_failed:
+root_node_failed:
+    return -1;
+}
+
+
+int ip_tree_from_file(TreeRoot **rtree, char *uri,
     apr_pool_t *mp, char **error_msg)
 {
     TreeNode *tnode = NULL;
@@ -2466,30 +2501,9 @@ int ip_tree_from_file(TreeRoot **rtree_, char *uri,
     char *end;
     char buf[HUGE_STRING_LEN + 1]; // FIXME: 2013-10-29 zimmerle: dynamic?
     char errstr[1024];             //
-    TreeRoot *rtree;
 
-    rtree = apr_palloc(mp, sizeof(TreeRoot));
+    if (create_radix_tree(mp, rtree, error_msg))
-    if (rtree == NULL)
     {
-        *error_msg = apr_psprintf(mp, "Failed allocating " \
-            "memory to TreeRoot.");
-        return -1;
-    }
-    memset(rtree, 0, sizeof(TreeRoot));
-
-    rtree->ipv4_tree = CPTCreateRadixTree(mp);
-    if (rtree->ipv4_tree == NULL)
-    {
-        *error_msg = apr_psprintf(mp, "ipmatchFromFile: Tree initialization " \
-            "failed.");
-        return -1;
-    }
-
-    rtree->ipv6_tree = CPTCreateRadixTree(mp);
-    if (rtree->ipv6_tree == NULL)
-    {
-        *error_msg = apr_psprintf(mp, "ipmatchFromFile: Tree initialization " \
-            "failed.");
         return -1;
     }
 
@@ -2545,14 +2559,15 @@ int ip_tree_from_file(TreeRoot **rtree_, char *uri,
 
         if (strchr(start, ':') == NULL)
         {
-            tnode = TreeAddIP(start, rtree->ipv4_tree, IPV4_TREE);
+            tnode = TreeAddIP(start, (*rtree)->ipv4_tree, IPV4_TREE);
         }
 #if APR_HAVE_IPV6
         else
         {
-            tnode = TreeAddIP(start, rtree->ipv6_tree, IPV6_TREE);
+            tnode = TreeAddIP(start, (*rtree)->ipv6_tree, IPV6_TREE);
         }
 #endif
+ 
         if (tnode == NULL)
         {
             *error_msg = apr_psprintf(mp, "Could not add entry " \
@@ -2566,8 +2581,6 @@ int ip_tree_from_file(TreeRoot **rtree_, char *uri,
         apr_file_close(fd);
     }
 
-    *rtree_ = rtree;
-
     return 0;
 }
 
@@ -2586,7 +2599,7 @@ int tree_contains_ip(apr_pool_t *mp, TreeRoot *rtree,
 
     if (strchr(value, ':') == NULL) {
         if (inet_pton(AF_INET, value, &in) <= 0) {
-            *error_msg = apr_psprintf(mp, "IPmatchFromFile: bad IPv4 " \
+            *error_msg = apr_psprintf(mp, "IPmatch: bad IPv4 " \
                 "specification \"%s\".", value);
             return -1;
         }
@@ -2599,7 +2612,7 @@ int tree_contains_ip(apr_pool_t *mp, TreeRoot *rtree,
 #if APR_HAVE_IPV6
     else {
         if (inet_pton(AF_INET6, value, &in6) <= 0) {
-            *error_msg = apr_psprintf(mp, "IPmatchFromFile: bad IPv6 " \
+            *error_msg = apr_psprintf(mp, "IPmatch: bad IPv6 " \
                 "specification \"%s\".", value);
             return -1;
         }
@@ -2614,74 +2627,39 @@ int tree_contains_ip(apr_pool_t *mp, TreeRoot *rtree,
     return 0;
 }
 
-int ip_list_from_param(apr_pool_t *pool,
+int ip_tree_from_param(apr_pool_t *mp,
-    char *param, msre_ipmatch **last, char **error_msg)
+    char *param, TreeRoot **rtree, char **error_msg)
 {
     char *saved = NULL;
     char *str = NULL;
-    apr_status_t rv;
+    TreeNode *tnode = NULL;
-    msre_ipmatch *current;
 
-    str = apr_strtok(param, ",", &saved);
+    if (create_radix_tree(mp, rtree, error_msg))
-    while( str != NULL)  {
-        const char *ipstr, *mask, *sep;
-
-        /* get the IP address and mask strings */
-        sep = strchr(str, '/');
-        if (sep) {
-            ipstr = apr_pstrndup(pool, str, (sep - str) );
-            mask  = apr_pstrdup(pool, (sep + 1) );
-        }
-        else {
-            ipstr = apr_pstrdup(pool, str);
-            mask = NULL;
-        }
-        /* create a new msre_ipmatch containing a new apr_ipsubnet_t*, and add it to the linked list */
-        current = apr_pcalloc(pool, sizeof(msre_ipmatch));
-        rv = apr_ipsubnet_create(&current->ipsubnet, ipstr, mask, pool);
-        if ( rv != APR_SUCCESS ) {
-            char msgbuf[120];
-            apr_strerror(rv, msgbuf, sizeof msgbuf);
-            *error_msg = apr_pstrcat(pool, "Error: ", msgbuf, NULL);
-            return -1;
-        }
-        current->address = str;
-        current->next = NULL;
-        *last = current;
-        last = &current->next;
-
-        str = apr_strtok(NULL, ",",&saved);
-    }
-
-    return 0;
-}
-
-int list_contains_ip(apr_pool_t *mp, msre_ipmatch *current,
-    const char *value, char **error_msg)
-{
-    apr_sockaddr_t *sa;
-
-    if (current == NULL)
     {
-        return 0;
-    }
-
-    /* create an apr_sockaddr_t for the value string */
-    if (apr_sockaddr_info_get(&sa, value,
-       APR_UNSPEC, 0, 0, mp) != APR_SUCCESS ) {
-        *error_msg = apr_psprintf(mp, "ipMatch Internal Error: Invalid " \
-            "ip address.");
         return -1;
     }
 
-    /* look through the linked list for a match */
+    str = apr_strtok(param, ",", &saved);
-    while (current) {
+    while (str != NULL)
-        if (apr_ipsubnet_test(current->ipsubnet, sa)) {
+    {
-            *error_msg = apr_psprintf(mp, "IPmatch \"%s\" matched \"%s\"",
+        if (strchr(str, ':') == NULL)
-                value, current->address);
+        {
-            return 1;
+            tnode = TreeAddIP(str, (*rtree)->ipv4_tree, IPV4_TREE);
         }
-        current = current->next;
+#if APR_HAVE_IPV6
+        else
+        {
+            tnode = TreeAddIP(str, (*rtree)->ipv6_tree, IPV6_TREE);
+        }
+#endif
+        if (tnode == NULL)
+        {
+            *error_msg = apr_psprintf(mp, "Could not add entry " \
+                "\"%s\" from: %s.", str, param);
+            return -1;
+        }
+
+        str = apr_strtok(NULL, ",", &saved);
     }
 
     return 0;

apache2/msc_util.h

@@ -156,10 +156,7 @@ int DSOLOCAL ip_tree_from_file(TreeRoot **rtree, char *uri,
 int DSOLOCAL tree_contains_ip(apr_pool_t *mp, TreeRoot *rtree,
     const char *value, modsec_rec *msr, char **error_msg);
 
-int DSOLOCAL ip_list_from_param(apr_pool_t *pool,
+int DSOLOCAL ip_tree_from_param(apr_pool_t *pool,
-    char *param, msre_ipmatch **last, char **error_msg);
+    char *param, TreeRoot **rtree, char **error_msg);
-
-int list_contains_ip(apr_pool_t *mp, msre_ipmatch *current,
-    const char *value, char **error_msg);
 
 #endif

apache2/re.h

@@ -19,7 +19,6 @@
 #define POSITIVE_VALUE 1
 #define NEGATIVE_VALUE 2
 
-typedef struct msre_ipmatch msre_ipmatch;
 typedef struct msre_engine msre_engine;
 typedef struct msre_ruleset msre_ruleset;
 typedef struct msre_ruleset_internal msre_ruleset_internal;
@@ -38,6 +37,7 @@ typedef struct msre_cache_rec msre_cache_rec;
 #include "apr_tables.h"
 #include "modsecurity.h"
 #include "msc_pcre.h"
+#include "msc_tree.h"
 #include "persist_dbm.h"
 #include "apache2.h"
 #include "http_config.h"
@@ -139,12 +139,6 @@ int DSOLOCAL msre_ruleset_phase_rule_remove_with_exception(msre_ruleset *ruleset
 #define RULE_TYPE_LUA           3  /* SecRuleScript */
 #endif
 
-struct msre_ipmatch  {
-    apr_ipsubnet_t *ipsubnet;
-    const char * address;
-    struct  msre_ipmatch *next;
-};
-
 struct msre_rule {
     apr_array_header_t      *targets;
     const char              *op_name;
@@ -183,7 +177,7 @@ struct msre_rule {
     int                     re_precomp;
     int                     escape_re;
 
-    msre_ipmatch            *ip_op;
+    TreeRoot                *ip_op;
 };
 
 char DSOLOCAL *msre_rule_generate_unparsed(apr_pool_t *pool, const msre_rule *rule, const char *targets, const char *args, const char *actions);

apache2/re_operators.c

@@ -98,9 +98,9 @@ static int msre_op_ipmatch_param_init(msre_rule *rule, char **error_msg) {
     else
         *error_msg = NULL;
 
-    param = apr_pstrdup(rule->ruleset->mp, rule->op_param);//,  &rule->ip_op);
+    param = apr_pstrdup(rule->ruleset->mp, rule->op_param);
 
-    res = ip_list_from_param(rule->ruleset->mp, param, &rule->ip_op,
+    res = ip_tree_from_param(rule->ruleset->mp, param, &rule->ip_op,
         error_msg);
 
     if (res)
@@ -122,7 +122,7 @@ static int msre_op_ipmatch_param_init(msre_rule *rule, char **error_msg) {
 * \retval 0 On No Match
 */
 static int msre_op_ipmatch_execute(modsec_rec *msr, msre_rule *rule, msre_var *var, char **error_msg) {
-    msre_ipmatch *current = rule->ip_op;
+    TreeRoot *rtree = rule->ip_op;
     int res = 0;
 
     if (error_msg == NULL)
@@ -130,12 +130,12 @@ static int msre_op_ipmatch_execute(modsec_rec *msr, msre_rule *rule, msre_var *v
     else
         *error_msg = NULL;
 
-    if(current == NULL) {
+    if (rtree == NULL) {
         msr_log(msr, 1, "ipMatch Internal Error: ipmatch value is null.");
         return 0;
     }
 
-    res = list_contains_ip(msr->mp, current, var->value, error_msg);
+    res = tree_contains_ip(msr->mp, rtree, var->value, NULL, error_msg);
 
     if (res < 0) {
         msr_log(msr, 1, "%s", *error_msg);