From 0037a0732a1514fa6b7bb526b58c76f5f3ceaea8 Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Thu, 31 Oct 2013 09:53:24 -0700 Subject: [PATCH] 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 | 25 +++---- apache2/mod_security2.c | 33 +++----- apache2/modsecurity.h | 4 - apache2/msc_util.c | 158 +++++++++++++++++---------------------- apache2/msc_util.h | 7 +- apache2/re.h | 10 +-- apache2/re_operators.c | 10 +-- 7 files changed, 96 insertions(+), 151 deletions(-) diff --git a/apache2/apache2_config.c b/apache2/apache2_config.c index 9d2d9ffa..678fb96c 100644 --- 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 index 69d84cfa..dfa3e40c 100644 --- 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; - -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, - 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,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, - 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 " \ @@ -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, - 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 index 6386781d..a9460e78 100644 --- 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 index 73b3f469..3aa619fb 100644 --- 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,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 (rtree == NULL) + if (create_radix_tree(mp, rtree, error_msg)) { - *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, - 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; + TreeNode *tnode = NULL; - 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(¤t->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 = ¤t->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) + 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 index 7d6588ba..dc0f8c85 100644 --- 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 index 68c465c5..df659a4b 100644 --- a/apache2/re.h +++ b/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); diff --git a/apache2/re_operators.c b/apache2/re_operators.c index 41a4f8e3..b1834c67 100644 --- a/apache2/re_operators.c +++ b/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);