Re-apply lost fix 4f33f5b: collection_unpack value_len underflow regression

[g4mm4-VCF]

Summary

Re-applies commit 4f33f5b ("Fix possible segfault in collection_unpack",
@twouters, 2024-03-01) which was silently dropped during merge 649aea7
on 2024-04-04, and adds one further bound check that the review
process surfaced. The drop has been confirmed by:

• git show 4f33f5b:apache2/persist_dbm.c | sed -n '60,75p' — has the guards
• git show 649aea7:apache2/persist_dbm.c | sed -n '60,75p' — guards gone
• git show v2.9.13:apache2/persist_dbm.c | sed -n '60,75p' — still gone

Affected releases: v2.9.10, v2.9.11, v2.9.12, v2.9.13.

What this PR does

collection_unpack() in apache2/persist_dbm.c had two missing
bound checks that this PR restores / adds:

1. Re-introduce the var->name_len < 1 || and
   var->value_len < 1 || guards from 4f33f5b. Without them, a
   blob containing 0x00 0x00 in the value_len field underflows
   value_len - 1 to 0xFFFFFFFF and apr_pstrmemdup() is invoked
   with size = 4 GiB.

2. Strengthen the name-bound check from
   blob_offset + name_len > blob_size to
   blob_offset + name_len + 2 > blob_size to also cover the
   2-byte value_len header read site (caught during Copilot
   review on this PR). The previous shape allowed an OOB read on a
   truncated blob whose tail landed exactly at the end of the name
   body.

Both changes apply to the same function and are exercised by the same
crash primitive (a malformed SDBM entry under SecDataDir), so they
are landed together.

The final diff against v2/master is four lines in
apache2/persist_dbm.c plus one explanatory comment — no additional
files, no new dependencies, no behaviour change for well-formed
blobs.

Test plan

• standalone harness (replicating the collection_unpack
  arithmetic byte-for-byte) confirms copy_len = UINT_MAX on
  the buggy code path and rc = 0 (NULL return) on the patched
  code path for a value_len = 0 blob
• manual: feed an 8-byte SDBM blob (header + name_len = 1
  + name = "" + value_len = 0) into a SecDataDir-backed
  collection; observe the previously-crashing worker now ignore
  the malformed entry
• manual (truncated-blob path, Copilot TODO  #1): feed a blob
  whose tail lands exactly at blob_offset + name_len;
  observe the patched code returns NULL instead of reading
  value_len past the buffer

The standalone unit-test harness is not included in-tree — the
existing tests/regression/ is a Perl HTTP integration harness that
doesn't fit a unit test of a static function, and wiring a new
check_PROGRAMS into tests/Makefile.am (where msc_test lives)
would require restructuring out of scope for this fix. The harness
is preserved in the disclosure PoC archive for anyone who wants to
verify offline.

References

• Original issue: Segmentation fault during collection cleanup on possibly corrupted dbm data #3082
• Original fix: 4f33f5b
• Reverting merge: 649aea7
• Affected tag: https://github.com/owasp-modsecurity/ModSecurity/releases/tag/v2.9.13

Coordinated with @airween via private security channel.

[airween]

Hi @g4mm4-VCF,

thanks for this patch.

Please take a look at the SonarCLoud report and fix those issues.

[g4mm4-VCF]

Hi there, I have fixed all the issues, and everything is now working properly. ~g4mm4

…

On Sun, May 10, 2026 at 5:43 PM Ervin Hegedus ***@***.***> wrote: *airween* left a comment (owasp-modsecurity/ModSecurity#3560) <#3560 (comment)> Hi @g4mm4-VCF <https://github.com/g4mm4-VCF>, thanks for this patch. Please take a look at the SonarCLoud report and fix those issues. — Reply to this email directly, view it on GitHub <#3560 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/CC66HTG37WTJBERASUVT43L42BMNXAVCNFSM6AAAAACYX36OD6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DIMJVGA4DIOJRG4> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[Copilot]

Pull request overview

Re-applies a previously lost defensive check in collection_unpack() to prevent value_len - 1 underflow (and resulting huge memcpy/apr_pstrmemdup), and adds a standalone regression test intended to demonstrate the buggy vs fixed arithmetic.

Changes:

• Reintroduce name_len < 1 / value_len < 1 guards in apache2/persist_dbm.c before subtracting 1 from those lengths.
• Add a new standalone C regression test for the value_len == 0 underflow scenario.
• Add an Automake snippet intended to build/run the new regression test.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
apache2/persist_dbm.c	Restores the missing < 1 guards to prevent *_len - 1 unsigned underflow leading to oversized copies.
tests/regression/persist_dbm/test_persist_dbm_value_len.c	Adds a standalone test program modeling the buggy vs patched arithmetic around value_len - 1.
tests/regression/persist_dbm/Makefile.am	Attempts to hook the new test into an Automake-based make check flow for that subdirectory.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[airween]

Hi there, I have fixed all the issues, and everything is now working properly.

Thanks - now please take a look at Copilot suggestions. I think all of them make sense.

[g4mm4-VCF]

Thanks for running Copilot. Pushed f239b02 to address all 5 suggestions. Summary:

#1 (apache2/persist_dbm.c:72) — fixed. Copilot is right, this is a real adjacent bug. After consuming the name field, blob_offset can land exactly at blob_size, and the 16-bit read of value_len from blob[blob_offset] / blob[blob_offset+1] would OOB-read on a truncated blob. Added if (blob_offset + 1 >= blob_size) return NULL; before the read. Same defensive pattern as the existing checks for name_len.

#2–#5 (tests/regression/persist_dbm/) — dropped. Copilot was right that the directory wasn't wired into Autotools (no AC_CONFIG_FILES, no parent SUBDIRS), and the sanitizer flags weren't portable. Looking at the existing test layout (tests/regression/ is the Perl HTTP regression harness, tests/ is the msc_test binary that links all the apache2/ sources), a unit test of a static function inside persist_dbm.c doesn't slot cleanly into either: it's not an HTTP-level test for the Perl harness, and adding a separate check_PROGRAMS to tests/Makefile.am would require non-trivial restructuring of the existing pattern.

The cleanest move was to drop the in-tree test directory entirely. The standalone test harness is preserved outside the upstream tree (in the disclosure PoC archive) for anyone who wants to verify the fix offline:

// snippet from the standalone test
unsigned char blob[] = {
    0x49, 0x52, 0x01,   // SDBM header
    0x00, 0x01,         // name_len = 1
    0x00,               // name = ""
    0x00, 0x00,         // value_len = 0  ← BUG TRIGGER
};
// vulnerable build → apr_pstrmemdup(..., 0xFFFFFFFF) → SIGSEGV
// patched build    → second bound check returns NULL early

Happy to revisit the in-tree test if you'd prefer a Perl-level integration test that drops a malformed SDBM file into SecDataDir and asserts no segfault on retrieve. That'd be more work but fits your existing harness cleanly. Let me know.

Diff vs upstream/v2/master is now just the three checks in apache2/persist_dbm.c:

• the original name_len < 1 and value_len < 1 guards from 4f33f5b, plus
• the new blob_offset + 1 >= blob_size check that Copilot caught.

CI / SonarCloud should pass on the new commit. Ping me if anything else surfaces.

[g4mm4-VCF]

SonarCloud caught a follow-up: cognitive complexity tipped to 26/25 because of the new bound check. Force-pushed fbdd35a4 that folds Copilot's catch into the existing name-bound check instead of adding a new line:

- if (blob_offset + var->name_len > blob_size) return NULL;
+ /* Need name_len bytes for the name body plus 2 more for the value_len header. */
+ if (var->name_len < 1 || blob_offset + var->name_len + 2 > blob_size) return NULL;

The strengthened predicate now requires room for both the name body and the 2-byte value_len header. That's a strict superset of the original check, covers the truncated-blob OOB read Copilot flagged, and adds zero new branches → cognitive complexity stays at the previous baseline.

Final diff vs upstream/v2/master is now exactly three semantic changes + one comment line in apache2/persist_dbm.c:

1. name_len < 1 || — the original 4f33f5b guard that 649aea7 dropped
2. + 2 on the upper bound — covers the new value_len-header read site
3. value_len < 1 || — the matching 4f33f5b guard for the value path

CI/SonarCloud should be green again on fbdd35a4.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[g4mm4-VCF]

Good catch — the PR description was stale after I dropped tests/regression/persist_dbm/ in fbdd35a4. Just updated the body to match the actual contents of the branch:

• removed the "adds a standalone regression test" claim
• removed the cd tests/regression/persist_dbm && make check checkbox from the test plan
• documented why the in-tree test was dropped (Perl harness doesn't fit a unit test of a static function; wiring a new check_PROGRAMS is out of scope for a security fix) and pointed at the standalone harness preserved in the disclosure PoC archive
• added the new Copilot-flagged manual test step (truncated blob whose tail lands at blob_offset + name_len)
• expanded the "what this PR does" to call out the strengthened bound check explicitly

The branch itself is unchanged; only the description was out of sync.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

apache2/persist_dbm.c:73

• The restored value_len == 0 guard is not covered by an in-tree regression test. Because this PR is re-applying a security fix that was previously dropped, an automated test for the malformed collection blob path would materially reduce the chance of losing or breaking this bound check again; the existing regression suite already has SecDataDir coverage for persistent collections.

        if (var->value_len < 1 || blob_offset + var->value_len > blob_size) return NULL;

[g4mm4-VCF]

Two follow-ups from the latest Copilot pass, both on apache2/persist_dbm.c:

Copilot — "var->name_len < 1 is unreachable" — agreed, fixed in 2d2c0b0a.

Verified by walking the control flow:

line	condition	exit
38	reads var->name_len = (blob<<8)+blob — value range 0..65535
39	if (var->name_len == 0)	break (exits while)
52	else if (var->name_len > 65536)	break (mathematically unreachable, but harmless)
65	reachable iff var->name_len ∈ [1, 65536]	—

So var->name_len < 1 at line 65 is provably unreachable. Removed and replaced with a comment documenting that name_len == 0 is handled upstream by the early break:

blob_offset += 2;
/* Need name_len bytes for the name body plus 2 more for the value_len header.
 * name_len == 0 is already handled by the early break above, so no zero-check
 * is required at this point. */
if (blob_offset + var->name_len + 2 > blob_size) return NULL;

The value_len < 1 clause on the value-side check is left in — there is no earlier zero-check for value_len, so that one is the live guard.

The original 4f33f5b patch by @twouters added the name_len < 1 clause for symmetry with the value side; my removal here deviates from that, but the dead-code observation is correct and the asymmetry is now documented in the comment. Happy to revert to the symmetric form if you'd prefer to mirror the original commit exactly.

---

Copilot — "the new bound check is not covered by an in-tree regression test" — push-back, requesting maintainer guidance.

I want to flag the framework limitation explicitly so we can decide together rather than play whack-a-mole with Copilot.

The fix lives inside collection_unpack(), which is static in apache2/persist_dbm.c. The existing v2 test surfaces are:

1. tests/regression/ — Perl-driven HTTP integration harness. Tests rule execution against a running Apache; it doesn't have a slot for invoking internal static functions directly. A test in this format would have to: (a) drop a malformed SDBM file into SecDataDir from a fixture, (b) issue an HTTP request that triggers collection_retrieve_ex, (c) assert the worker doesn't crash. Possible but requires new fixture machinery that doesn't exist today.

2. tests/msc_test.c — JSON-driven harness for tfn / op / action plugin types only. There's no slot for invoking persistent-collection deserialisation, and collection_unpack would need to be exposed (made non-static or wrapped) before it could be reached from this harness.

Three options I see, in increasing scope:

• (A) Land the security fix without an in-tree test. Standalone harness is preserved in the disclosure PoC archive; CI keeps relying on the existing Perl regression suite for end-to-end coverage. Smallest patch.
• (B) Add a tests/regression/persist_dbm/*.t Perl test that drops a fixture SDBM file containing the 8-byte malicious blob into a temporary SecDataDir, then issues a request that resolves a persistent collection and asserts no segfault. This is the most idiomatic fit with the existing v2 framework. ~50 LoC, no API changes. Recommended if you want in-tree coverage.
• (C) Expose collection_unpack (drop static or add a __attribute__((visibility)) wrapper) and add unit-test cases inside tests/msc_test.c. Strictest coverage but adds a public-ish symbol and requires extending the JSON harness. Largest patch.

Default position is (A), which is what the current branch reflects. If you'd prefer (B), I can put together the Perl fixture in a follow-up commit on this branch — happy to do the work, just want to confirm scope before adding test machinery that the harness doesn't currently have. (C) feels disproportionate for a 2-line bound-check fix.

Let me know which way you'd like to go.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud