Using RadixTree instead of list to storage IPs

Felipe Zimmerle · Felipe Zimmerle · commit 0037a0732a15 · 2014-03-31T16:22:10.000-07:00
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.
diff --git a/apache2/apache2_config.c b/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 **suspicious_list, msre_ipmatch **suspicious_list_param,
+    TreeRoot **whitelist, TreeRoot **suspicious_list, 
     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,
-            file, mp, NULL);
+        res = ip_tree_from_file(whitelist, file, mp, &error_msg);
     }
     else if (strncasecmp(p2, "!@ipMatch", strlen("!@ipMatch")) == 0) {
-        res = ip_list_from_param(mp, param,
-            whitelist_param, &error_msg);
+        res = ip_tree_from_param(mp, param, whitelist, &error_msg);
     }
     else if ((strncasecmp(p2, "@ipMatchFromFile", strlen("@ipMatchFromFile")) == 0) ||
         (strncasecmp(p2, "@ipMatchF", strlen("@ipMatchF")) == 0)) {
 
-        res = ip_tree_from_file(suspicious_list,
-            file, mp, NULL);
+        res = ip_tree_from_file(suspicious_list, file, mp, &error_msg);
     }
     else if (strncasecmp(p2, "@ipMatch", strlen("@ipMatch")) == 0) {
-        res = ip_list_from_param(mp, param,
-            suspicious_list_param, &error_msg);
+        res = ip_tree_from_param(mp, param, suspicious_list, &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_suspicious_list,
-            &conn_read_state_suspicious_list_param, cmd->directive->filename);
+            &conn_read_state_whitelist, &conn_read_state_suspicious_list,
+            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_suspicious_list,
-            &conn_write_state_suspicious_list_param, cmd->directive->filename);
+            &conn_write_state_whitelist, &conn_write_state_suspicious_list,
+            cmd->directive->filename);
 
         if (param)
             return param;
diff --git a/apache2/mod_security2.c b/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;
 
+unsigned long int DSOLOCAL conn_write_state_limit = 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;
 
 #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,
-                   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))))
+                (tree_contains_ip(conn->pool,
+                   conn_read_state_suspicious_list, client_ip, NULL, &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,
-                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))
+            else if (tree_contains_ip(conn->pool,
+                conn_read_state_whitelist, client_ip, NULL, &error_msg) > 0)
             {
                 ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
                     "ModSecurity: Too many threads [%ld] of %ld allowed in " \
@@ -1464,21 +1455,17 @@ 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,
-                    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))))
+                (tree_contains_ip(conn->pool,
+                    conn_write_state_suspicious_list, client_ip, NULL, &error_msg) <= 0))
             {
                 ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
                     "ModSecurity: Too many threads [%ld] of %ld allowed in " \
                     "WRITE state from %s - There is a suspission list but " \
                     "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,
-                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))
+            else if (tree_contains_ip(conn->pool,
+                conn_write_state_whitelist, client_ip, NULL, &error_msg) > 0)
             {
                 ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
                     "ModSecurity: Too many threads [%ld] of %ld allowed in " \
diff --git a/apache2/modsecurity.h b/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;
 
diff --git a/apache2/msc_util.c b/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,41 +2453,57 @@ int msc_headers_to_buffer(const apr_array_header_t *arr, char *buffer,
     return headers_length;
 }
 
-int ip_tree_from_file(TreeRoot **rtree_, char *uri,
-    apr_pool_t *mp, char **error_msg)
-{
-    TreeNode *tnode = NULL;
-    apr_status_t rc;
-    int line = 0;
-    apr_file_t *fd;
-    char *start;
-    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 (rtree == NULL)
+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.");
-        return -1;
+        goto root_node_failed;
     }
-    memset(rtree, 0, sizeof(TreeRoot));
+    memset(*rtree, 0, sizeof(TreeRoot));
 
-    rtree->ipv4_tree = CPTCreateRadixTree(mp);
-    if (rtree->ipv4_tree == NULL)
+    (*rtree)->ipv4_tree = CPTCreateRadixTree(mp);
+    if ((*rtree)->ipv4_tree == NULL)
     {
-        *error_msg = apr_psprintf(mp, "ipmatchFromFile: Tree initialization " \
+        *error_msg = apr_psprintf(mp, "IPmatch: Tree initialization " \
             "failed.");
-        return -1;
+        goto ipv4_tree_failed;
     }
 
-    rtree->ipv6_tree = CPTCreateRadixTree(mp);
-    if (rtree->ipv6_tree == NULL)
+    (*rtree)->ipv6_tree = CPTCreateRadixTree(mp);
+    if ((*rtree)->ipv6_tree == NULL)
     {
-        *error_msg = apr_psprintf(mp, "ipmatchFromFile: Tree initialization " \
+        *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;
+    apr_status_t rc;
+    int line = 0;
+    apr_file_t *fd;
+    char *start;
+    char *end;
+    char buf[HUGE_STRING_LEN + 1]; // FIXME: 2013-10-29 zimmerle: dynamic?
+    char errstr[1024];             //
+
+    if (create_radix_tree(mp, rtree, error_msg))
+    {
         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,
-    char *param, msre_ipmatch **last, char **error_msg)
+int ip_tree_from_param(apr_pool_t *mp,
+    char *param, TreeRoot **rtree, char **error_msg)
 {
     char *saved = NULL;
     char *str = NULL;
-    apr_status_t rv;
-    msre_ipmatch *current;
-
-    str = apr_strtok(param, ",", &saved);
-    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;
+    TreeNode *tnode = NULL;
 
-    if (current == NULL)
+    if (create_radix_tree(mp, rtree, error_msg))
     {
-        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 */
-    while (current) {
-        if (apr_ipsubnet_test(current->ipsubnet, sa)) {
-            *error_msg = apr_psprintf(mp, "IPmatch \"%s\" matched \"%s\"",
-                value, current->address);
-            return 1;
+    str = apr_strtok(param, ",", &saved);
+    while (str != NULL)
+    {
+        if (strchr(str, ':') == NULL)
+        {
+            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;
diff --git a/apache2/msc_util.h b/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,
-    char *param, msre_ipmatch **last, char **error_msg);
-
-int list_contains_ip(apr_pool_t *mp, msre_ipmatch *current,
-    const char *value, char **error_msg);
+int DSOLOCAL ip_tree_from_param(apr_pool_t *pool,
+    char *param, TreeRoot **rtree, char **error_msg);
 
 #endif
diff --git a/apache2/re.h b/apache2/re.h
diff --git a/apache2/re_operators.c b/apache2/re_operators.c
