msc_xxxx: off by one heap overflow

[tinselcity]

edited by zimmerle: content will be placed back as soon as we fix it.

[zimmerle]

Hi Reed,

Even having a very small number of users affected (if any) we prefer to receive anything related to security, reported over security@modsecurity.org.

But thanks for let as know. I am editing the issue to remove the content till we got it fixed.

[victorhora]

I think we can safely close this one as the fix has already been applied to master here f66cd41 and here 95048d5

Thanks for the report @tinselcity

[michaelgranzow-avi]

@zimmerle : Could you please re-add the details of the bug now that it has been fixed and a lot of time has passed?

Looking at the diff it's not immediately obvious what the problem was. A check was added to make sure we only enter a branch if a pointer is null. So it's the opposite of a normal "guard against nullptr" check. However, the pointer isn't used directly inside the 'if' branch.

It would be good to have a test case that triggers the issue without the fix.

[tinselcity]

@michaelgranzow-avi I saved a local copy. Since they've fixed quite a while ago, I'd have to assume it's relatively safe to give the details. It's a bit strange though, and maybe hard to exploit (off by one on the heaps are I think? -I'm no expert).

Here's link: ip tree off by one description

[tinselcity]

We have a slightly different data structure for our IP indexing/matching, but we've added a similar test. Just FYI -not sure if modsecurity has added a specific one for this, but it might be easy to add:
test for ipv4 addresses ending with 0. Our CI/CD has asan flipped on to catch potential overflow issues.

[michaelgranzow-avi]

Thanks very much @tinselcity for the detailed background info.

Three observations:

1. I cannot reproduce the issue from your test with current modsec. It looks like you branched from modsec before commit 21777ae, which "fixes" (hides, really) the issue by allocating as many bytes as there are bits in the netmask. I think that change is wrong, but it means the heap overflow doesn't happen. Behaviour may well be incorrect, but memory-wise it's ok.

2. Even with the fix proposed here (and without the upstream "fix"), I can still trigger the heap overflow with a slightly modified test (whether that's a valid config is a different question; it's accepted and crashes during the query).

int main(void)
{
         printf("Hello world\n");
         char *l_err_msg = NULL;
         TreeRoot *l_tree;
         int l_s;
         l_s = create_radix_tree(&l_tree, &l_err_msg);
         printf("create_radix_tree: %d\n", l_s);
         if (l_err_msg) {
             free(l_err_msg);
             l_err_msg = NULL;
         }
         // add "192.168.100.0/24"
         TreeNode *tnode = TreeAddIP("192.168.100.0/24", l_tree->ipv4_tree, IPV4_TREE);
         printf("tnode/24:             %p\n", tnode);

         tnode = TreeAddIP("192.168.100.1", l_tree->ipv4_tree, IPV4_TREE);
         printf("tnode/32:             %p\n", tnode);

         l_s = tree_contains_ip(l_tree, "192.168.100.0", &l_err_msg);
         if (l_err_msg) {
             free(l_err_msg);
         }
         printf("tree_contains_ip:  %d\n", l_s);
         //char *l_buf;
         //*l_buf = 'c';
         destroy_radix_tree(l_tree);
         return 0;
}

The root cause is that for /32 entries (IPv4) and /128 entries (IPv6), we've exhausted the memory allocated for the IP, so we must not go further. This cannot directly be checked because the code tries to be generic with respect to IPv4/IPv6, but doesn't store how many bytes have actually been allocated.

3. There are several potential memory leaks in the msc_tree code, one of them can definitely be triggered. In CPTAddElement, we reset a pointer without freeing what has been allocated to it:

            if (node->count == 0) {
                node->netmasks = NULL;
            }

[michaelgranzow-avi]

To verify the memleak, I added cleanup code for the tree, you'll need it to compile the above poc.

void
destroy_radix_tree(TreeRoot* rtree);
void
destroy_ip_tree(CPTTree* t);
void
destroy_tree_node(TreeNode* n);
void
destroy_cptdata(CPTData* data);


void
destroy_radix_tree(TreeRoot* rtree)
{
    if(rtree) {
        destroy_ip_tree(rtree->ipv4_tree);
        destroy_ip_tree(rtree->ipv6_tree);
        free(rtree);
    }
}

void
destroy_ip_tree(CPTTree* t)
{
    if (t) {
        destroy_tree_node(t->head);
        free(t);
    }
}

void
destroy_tree_node(TreeNode* n)
{
    if (n) {
        destroy_tree_node(n->left);
        destroy_tree_node(n->right);
        if (n->netmasks) {
            free(n->netmasks);
        }
        if (n->prefix) {
            if (n->prefix->buffer) {
                free(n->prefix->buffer);
            }
            if (n->prefix->prefix_data) {
                destroy_cptdata(n->prefix->prefix_data);
            }
            free(n->prefix);
        }
        free(n);
    }
}

void
destroy_cptdata(CPTData* data)
{
    if (data) {
        destroy_cptdata(data->next);
        free(data);
    }
}