From a86f484f747fcd2221c72c28fbc20f04b2083447 Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Tue, 13 Oct 2026 09:20:12 -0300 Subject: [PATCH] fix: keep every netmask of an @ipMatch prefix in the radix tree Port of owasp-modsecurity/ModSecurity#3660 to the 2.x tree code in apache2/msc_tree.c, which is the same implementation: - CPTAddElement only handled the "same key" case when the netmask was an exact duplicate; a different netmask on an existing key (10.0.0.0/16 followed by 10.0.0.0/8) was silently dropped. - InsertNetmask and the duplicate branch allocated a fresh, empty netmasks array instead of growing it, losing the earlier netmasks on the node. - CPTFindElementIPNetblock never reset the byte index between the netmasks of a node, so only the most specific one was applied. Netmask insertion is now one helper that copies the existing entries into a larger pool allocation and keeps them sorted; the same key branch appends the new netmask to the existing prefix; the lookup masks the address again for every netmask. Fixes #3588 Related to #2192, #2893 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0181jBv5JdNQviwDzfnkf1Gy --- apache2/msc_tree.c | 133 +++++++++----------- tests/regression/misc/60-ipmatch-overlap.t | 138 +++++++++++++++++++++ 2 files changed, 195 insertions(+), 76 deletions(-) create mode 100644 tests/regression/misc/60-ipmatch-overlap.t diff --git a/apache2/msc_tree.c b/apache2/msc_tree.c index feabdd0e03..0d8dff6574 100644 --- a/apache2/msc_tree.c +++ b/apache2/msc_tree.c @@ -78,6 +78,8 @@ TreeNode *CPTCreateNode(apr_pool_t *pool) { return node; } +int TreeCheckData(TreePrefix *prefix, CPTData *prefix_data, unsigned int netmask); + CPTData *CPTCreateCPTData(unsigned char netmask, apr_pool_t *pool) { CPTData *prefix_data = apr_palloc(pool, sizeof(CPTData)); @@ -249,48 +251,54 @@ TreeNode *SetParentNode(TreeNode *node, TreeNode *new_node, CPTTree *tree) { return new_node; } -int InsertNetmask(TreeNode *node, TreeNode *parent, TreeNode *new_node, - CPTTree *tree, unsigned char netmask, unsigned char bitlen) { +/* + * Adds a netmask to a node, keeping the array sorted from the most to the + * least specific netmask. The array is grown (copied), not replaced, so the + * netmasks already on the node survive (#3588). + */ +int CPTAddNetmask(TreeNode *node, unsigned char netmask, apr_pool_t *pool) { + unsigned char *netmasks; int i; - if (netmask != NETMASK_256-1 && netmask != NETMASK_128) { - if ((netmask != NETMASK_32 || (netmask == NETMASK_32 && bitlen != NETMASK_32))) { - - node = new_node; - parent = new_node->parent; + netmasks = apr_palloc(pool, (node->count + 1) * sizeof(unsigned char)); + if (netmasks == NULL) + return 0; + if (node->count > 0) + memcpy(netmasks, node->netmasks, node->count * sizeof(unsigned char)); - while (parent != NULL && netmask < (parent->bit + 1)) { - node = parent; - parent = parent->parent; - } + i = node->count - 1; + while (i >= 0 && netmasks[i] < netmask) { + netmasks[i + 1] = netmasks[i]; + i--; + } + netmasks[i + 1] = netmask; - node->count++; - node->netmasks = apr_palloc(tree->pool, (node->count * sizeof(unsigned char))); + node->netmasks = netmasks; + node->count++; - if(node->netmasks == NULL) - return 0; - if ((node->count-1) == 0) { - node->netmasks[0] = netmask; - return 1; - } + return 1; +} - node->netmasks[node->count - 1] = netmask; +/* + * Registers a netmask on the node that covers it: starting from new_node, + * walk up while the netmask is wider than the parent's branching bit. + */ +int InsertNetmask(TreeNode *node, TreeNode *parent, TreeNode *new_node, + CPTTree *tree, unsigned char netmask, unsigned char bitlen) { + if (netmask == NETMASK_256-1 || netmask == NETMASK_128) + return 0; + if (netmask == NETMASK_32 && bitlen == NETMASK_32) + return 0; - i = node->count - 2; - while (i >= 0) { - if (netmask < node->netmasks[i]) { - node->netmasks[i + 1] = netmask; - break; - } + node = new_node; + parent = new_node->parent; - node->netmasks[i + 1] = node->netmasks[i]; - node->netmasks[i] = netmask; - i--; - } - } + while (parent != NULL && netmask < (parent->bit + 1)) { + node = parent; + parent = parent->parent; } - return 0; + return CPTAddNetmask(node, netmask, tree->pool); } TreeNode *CPTAddElement(unsigned char *ipdata, unsigned int ip_bitmask, CPTTree *tree, unsigned char netmask) { @@ -388,52 +396,21 @@ TreeNode *CPTAddElement(unsigned char *ipdata, unsigned int ip_bitmask, CPTTree if (test_bit == bitlen && node->bit == bitlen) { if (node->prefix != NULL) { - int found = 0; - CPTData *prefix_data = NULL; - - prefix_data = node->prefix->prefix_data; - - while(prefix_data != NULL) { - if (prefix_data->netmask == netmask) - ++found; - prefix_data = prefix_data->next; - } - - if (found != 0) { - + /* + * Same key already in the tree (e.g. 10.0.0.0/16 and then + * 10.0.0.0/8). Record the netmask unless it is a duplicate; + * previously anything but an exact duplicate was dropped (#3588). + */ + if (TreeCheckData(node->prefix, node->prefix->prefix_data, + netmask) == 0) { CPTData *prefix_data = CPTCreateCPTData(netmask, tree->pool); - CPTAppendToCPTDataList(prefix_data, &prefix->prefix_data); - - if(CheckBitmask(netmask, ip_bitmask)) - return node; - - parent = node->parent; - while (parent != NULL && netmask < (parent->bit + 1)) { - node = parent; - parent = parent->parent; - } - - node->count++; - new_node = node; - node->netmasks = apr_palloc(tree->pool, (node->count * sizeof(unsigned char))); - - if ((node->count -1) == 0) { - node->netmasks[0] = netmask; - return new_node; - } - - node->netmasks[node->count - 1] = netmask; - - i = node->count - 2; - while (i >= 0) { - if (netmask < node->netmasks[i]) { - node->netmasks[i + 1] = netmask; - break; - } + if (prefix_data == NULL) + return NULL; + CPTAppendToCPTDataList(prefix_data, &node->prefix->prefix_data); - node->netmasks[i + 1] = node->netmasks[i]; - node->netmasks[i] = netmask; - i--; + if (CheckBitmask(netmask, ip_bitmask) == 0) { + InsertNetmask(node, node->parent, node, tree, netmask, + bitlen); } } } else { @@ -645,6 +622,10 @@ TreeNode *CPTFindElementIPNetblock(modsec_rec *msr, unsigned char *ipdata, unsig while(j < netmask_node->count) { bytes = ip_bitmask / 8; + /* Netmasks are sorted from most to least specific, so masking the + * already masked address again is the same as masking the original. + * Previously only the first netmask was applied (#3588). */ + i = 0; while( i < bytes ) { mask = -1; diff --git a/tests/regression/misc/60-ipmatch-overlap.t b/tests/regression/misc/60-ipmatch-overlap.t new file mode 100644 index 0000000000..631fb21486 --- /dev/null +++ b/tests/regression/misc/60-ipmatch-overlap.t @@ -0,0 +1,138 @@ +### @ipMatch with overlapping prefixes on the same key (#3588) + +{ + type => "misc", + comment => '@ipMatch narrow then wide on the same key, IP in the wide range', + conf => q( + SecRuleEngine On + SecRule ARGS:ip "@ipMatch 10.0.0.0/16,10.0.0.0/8" "id:500588,phase:1,deny,status:403" + ), + match_log => { + error => [ qr/Access denied with code 403 \(phase 1\)/, 1 ], + }, + match_response => { + status => qr/^403$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?ip=10.5.0.1", + ), +}, +{ + type => "misc", + comment => '@ipMatch narrow then wide on the same key, IP in the narrow range', + conf => q( + SecRuleEngine On + SecRule ARGS:ip "@ipMatch 10.0.0.0/16,10.0.0.0/8" "id:500588,phase:1,deny,status:403" + ), + match_log => { + error => [ qr/Access denied with code 403 \(phase 1\)/, 1 ], + }, + match_response => { + status => qr/^403$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?ip=10.0.5.1", + ), +}, +{ + type => "misc", + comment => '@ipMatch wide then narrow on the same key', + conf => q( + SecRuleEngine On + SecRule ARGS:ip "@ipMatch 10.0.0.0/8,10.0.0.0/16" "id:500588,phase:1,deny,status:403" + ), + match_log => { + error => [ qr/Access denied with code 403 \(phase 1\)/, 1 ], + }, + match_response => { + status => qr/^403$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?ip=10.5.0.1", + ), +}, +{ + type => "misc", + comment => '@ipMatch host then network on the same key', + conf => q( + SecRuleEngine On + SecRule ARGS:ip "@ipMatch 172.16.0.0,172.16.0.0/12" "id:500588,phase:1,deny,status:403" + ), + match_log => { + error => [ qr/Access denied with code 403 \(phase 1\)/, 1 ], + }, + match_response => { + status => qr/^403$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?ip=172.20.0.1", + ), +}, +{ + type => "misc", + comment => '@ipMatch duplicate prefixes still match', + conf => q( + SecRuleEngine On + SecRule ARGS:ip "@ipMatch 10.0.0.0/8,127.0.0.1,10.0.0.0/8" "id:500588,phase:1,deny,status:403" + ), + match_log => { + error => [ qr/Access denied with code 403 \(phase 1\)/, 1 ], + }, + match_response => { + status => qr/^403$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?ip=10.5.0.1", + ), +}, +{ + type => "misc", + comment => '@ipMatch address outside every prefix', + conf => q( + SecRuleEngine On + SecRule ARGS:ip "@ipMatch 10.0.0.0/16,10.0.0.0/8,192.168.0.0/16" "id:500588,phase:1,deny,status:403" + ), + match_log => { + -error => [ qr/Access denied/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?ip=11.0.0.1", + ), +}, +{ + type => "misc", + comment => '@ipMatch IPv6 narrow then wide on the same key', + conf => q( + SecRuleEngine On + SecRule ARGS:ip "@ipMatch 2001:db8::/48,2001:db8::/32" "id:500588,phase:1,deny,status:403" + ), + match_log => { + error => [ qr/Access denied with code 403 \(phase 1\)/, 1 ], + }, + match_response => { + status => qr/^403$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?ip=2001:db8:1::1", + ), +}, +{ + type => "misc", + comment => '@ipMatch IPv6 address outside every prefix', + conf => q( + SecRuleEngine On + SecRule ARGS:ip "@ipMatch 2001:db8::/48,2001:db8::/32" "id:500588,phase:1,deny,status:403" + ), + match_log => { + -error => [ qr/Access denied/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?ip=2001:db9::1", + ), +},