Segmentation fault during collection cleanup on possibly corrupted dbm data

[twouters]

Describe the bug

Apache segfaults during processing of requests when the persistent collection is being processed to remove stale records.

Logs and dumps

Note: IP addresses have been obfuscated

(gdb) bt full
#0  0x00007236e618c456 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
No symbol table info available.
#1  0x00007236e65bb2f8 in memcpy (__len=4294967295, __src=0x7236bc01deae, __dest=<optimized out>)
    at /usr/include/x86_64-linux-gnu/bits/string_fortified.h:34
No locals.
#2  apr_pstrmemdup (a=<optimized out>, s=s@entry=0x7236bc01deae "\017\061\070\061.123.123.12", 
    n=4294967295) at strings/apr_strings.c:107
        res = <optimized out>
#3  0x00007236e5fb4664 in collection_unpack (msr=msr@entry=0x7236bc020eb0, blob=0x7236bc01de76 "7", 
    blob_size=228, log_vars=log_vars@entry=0) at persist_dbm.c:71
        var = 0x7236bc0b2e60
        col = 0x7236bc0b2a38
        blob_offset = 56
#4  0x00007236e5fb5c96 in collections_remove_stale (msr=msr@entry=0x7236bc020eb0, 
    col_name=0x7236bc009020 "ip") at persist_dbm.c:781
        col = 0x0
        var = 0x0
        dbm_filename = 0x7236bc02d578 "/var/cache/httpd/www-ip"
        key = {dptr = 0x7236bc030ea0 "127.12.12.123", dsize = 14}
        value = {dptr = 0x7236bc01de76 "7", dsize = 228}
        dbm = <optimized out>
        rc = <optimized out>
        keys_arr = 0x7236bc02d700
        keys = 0x7236bc031ae8
        now = <optimized out>
        i = 314
        userinfo = 0x7236bc02d570 "www"
[snip]

To Reproduce

Not sure, I guess: have corrupt persistent collection data files (I made backups, if you're interested) and perform some specific requests to trigger a segfault (don't known exactly when it triggers).

Expected behavior

Webserver doesn't crash

Server (please complete the following information):

• ModSecurity version: 2.9.7
• WebServer: Apache 2.4.57
• OS (and distro): Debian bullseye/bookworm

Additional context

ModSecurity/apache2/persist_dbm.c

Line 71 in 705002b

var->value = apr_pstrmemdup(msr->mp, (const char *)blob + blob_offset, var->value_len - 1);

collection_unpack() checks if blob_offset + var->value_len expands beyond blob_size, but then runs apr_pstrmemdup() with a size of var->value_len - 1. If var->value_len = 0 the size value will wrap around to 4294967295.

The following patch might help:

--- a/apache2/persist_dbm.c
+++ b/apache2/persist_dbm.c
@@ -67,7 +67,7 @@
         var->value_len = (blob[blob_offset] << 8) + blob[blob_offset + 1];
         blob_offset += 2;
 
-        if (blob_offset + var->value_len > blob_size) return NULL;
+        if (blob_offset + (var->value_len - 1) > blob_size) return NULL;
         var->value = apr_pstrmemdup(msr->mp, (const char *)blob + blob_offset, var->value_len - 1);
         blob_offset += var->value_len;
         var->value_len--;

[airween]

Hi @twouters,

thanks for reporting this issue.

I don't remember now, but may be @marcstern already sent a fix against this bug, but there was a strange issue so we had to revert many PR's (see #3074). Now we are working on the CI workflow for mod_security2 too (v3 already has it), then we can start to re-send the PR's.

I will let you know here if the CI will done - after that if you think you can send a PR to fix this issue.

Thanks again.

[twouters]

To be fair: my proposed patch only prevents a crash when this situation is triggered, it doesn't prevent the cause. Nor did I take the time to figure out why value_len became 0, so it's more of a quick-fix.

We use a cronjob to clean up the persistent collections with sdbm-util because parsing the collection tends to become too slow when the files become too big. This happens while apache is running, which might be the cause of the corruption, or it's just because persistent collections aren't entirely thread-safe. Hard to tell.

Maybe I should modify the patch to produce an error log when it happens, so we can keep track and figure out (on the long run) if it still happens if we stop apache before cleaning up the collections. I'm running thousands of servers and only a handful of servers run into this issue after a span of several months, so it'll take time to get to the bottom of this unless someone with knowledge of the sdbm data structure (or handling of the data) can help with reproducing the bug.

If you're OK with accepting my proposed patch (by the time we know it's not already fixed by Marc), I'll be happy to push a PR for it. Just know that I won't be able to produce a proper test case at this time because I don't know how to reliably trigger the proper conditions.

[airween]

@marcstern could you take a look at this issue?

[marcstern]

Hi @twouters,

I never encountered this, so I didn't fix this.

I think your patch is incorrect. Example:

• blob ranges from 0 to 4
• blob_size = 5
• blob_offset = 2
• var->value_len = 4

With your patch, it won't return and we'll copy memory up to offset 5.
On top of that, you rely on var->value_len - 1 being huge if var->value_len == 0, which should probably be always the case, but we're not supposed to do it that way (let's imagine it becomes a signed int one day).

What about a more explicit check:
if (var->value_len < 1 || blob_offset + var->value_len > blob_size) return NULL;

And the same check for var->name_len I imagine (anything could be corrupted).

I would merge such a fix.

[twouters]

we were already running with a modified patch, but checking for < 1 seems more appropriate indeed:

diff --git a/apache2/persist_dbm.c b/apache2/persist_dbm.c
index e4f8036f..a7f1af3b 100644
--- a/apache2/persist_dbm.c
+++ b/apache2/persist_dbm.c
@@ -67,6 +67,12 @@ static apr_table_t *collection_unpack(modsec_rec *msr, const unsigned char *blob
         var->value_len = (blob[blob_offset] << 8) + blob[blob_offset + 1];
         blob_offset += 2;
 
+        if (var->value_len == 0) {
+            msr_log(msr, 4, "collection_unpack: Unable to unpack \"%s\", value len 0.",
+                    log_escape_ex(msr->mp, var->name, var->name_len));
+            return NULL;
+        }
+
         if (blob_offset + var->value_len > blob_size) return NULL;
         var->value = apr_pstrmemdup(msr->mp, (const char *)blob + blob_offset, var->value_len - 1);
         blob_offset += var->value_len;

I'll push a PR