Re-apply lost fix 4f33f5b: collection_unpack value_len underflow regression#3560
Open
g4mm4-VCF wants to merge 3 commits intoowasp-modsecurity:v2/masterfrom
Open
Re-apply lost fix 4f33f5b: collection_unpack value_len underflow regression#3560g4mm4-VCF wants to merge 3 commits intoowasp-modsecurity:v2/masterfrom
g4mm4-VCF wants to merge 3 commits intoowasp-modsecurity:v2/masterfrom
Conversation
g4mm4-VCF
force-pushed
the
fix/persist-dbm-value-len-regression
branch
from
cf1f9c4 to
17f3445
Compare
Member
|
Hi @g4mm4-VCF, thanks for this patch. Please take a look at the SonarCLoud report and fix those issues. |
The fix from owasp-modsecurity#3082 was lost in merge 649aea7 (2024-04-04) and has been missing from every release v2.9.10 .. v2.9.13. This re-applies the same one-line guard as the original commit by @twouters. Refs: owasp-modsecurity#3082
Standalone harness that replicates the unpack arithmetic and asserts that the patched bound check rejects a value_len=0 blob while the buggy version would have produced a 4 GiB copy length. Runs without APR / Apache build dependencies so it integrates into `make check` as a self-contained TAP test.
Split combined declarations and remove redundant inner casts as flagged by SonarCloud's static analyser. No functional change — the regression test still produces buggy_copy = UINT_MAX on the buggy path and patched_rc = 0 on the patched path.
g4mm4-VCF
force-pushed
the
fix/persist-dbm-value-len-regression
branch
from
b47c646 to
d7759ab
Compare
|
Author
|
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:
***@***.***>
|
Contributor
There was a problem hiding this comment.
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 < 1guards inapache2/persist_dbm.cbefore subtracting 1 from those lengths. - Add a new standalone C regression test for the
value_len == 0underflow 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.
Comment on lines
+64
to
+72
| if (var->name_len < 1 || blob_offset + var->name_len > blob_size) return NULL; | ||
| var->name = apr_pstrmemdup(msr->mp, (const char *)blob + blob_offset, var->name_len - 1); | ||
| blob_offset += var->name_len; | ||
| var->name_len--; | ||
|
|
||
| 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 (var->value_len < 1 || blob_offset + var->value_len > blob_size) return NULL; |
Comment on lines
+1
to
+3
| TESTS = test_persist_dbm_value_len | ||
| check_PROGRAMS = test_persist_dbm_value_len | ||
| test_persist_dbm_value_len_SOURCES = test_persist_dbm_value_len.c |
Comment on lines
+4
to
+5
| test_persist_dbm_value_len_CFLAGS = -fsanitize=address,undefined -O0 -g -Wall | ||
| test_persist_dbm_value_len_LDFLAGS = -fsanitize=address,undefined |
Comment on lines
+54
to
+60
| * Faithful replica of collection_unpack's deserialisation arithmetic. | ||
| * | ||
| * The two functions below differ ONLY in the `value_len < 1 ||` clause | ||
| * that 4f33f5b added and 649aea7 removed. The blob layout, the read of | ||
| * the 16-bit big-endian length, and the apr_pstrmemdup() argument | ||
| * computation are otherwise byte-for-byte identical. | ||
| * |
| * | ||
| * 0 = both before-fix and after-fix paths behaved as expected | ||
| * (test PASSED — confirms regression and confirms patch fixes it) | ||
| * 1 = test FAILED (either path returned an unexpected result) |
Member
Thanks - now please take a look at Copilot suggestions. I think all of them make sense. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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. The drop has been confirmed by:
git show 4f33f5b:apache2/persist_dbm.c | sed -n '60,75p'— has the guardgit show 649aea7:apache2/persist_dbm.c | sed -n '60,75p'— guard gonegit show v2.9.13:apache2/persist_dbm.c | sed -n '60,75p'— still goneAffected releases: v2.9.10, v2.9.11, v2.9.12, v2.9.13.
What this PR does
var->name_len < 1 ||andvar->value_len < 1 ||guards in
collection_unpack()(one-line change each).tests/regression/persist_dbm/that replicates the unpackarithmetic and asserts:
copy_len = UINT_MAXfor avalue_len=0 blob (smoking gun)
Test plan
cd tests/regression/persist_dbm && make checkpassesvalue_len=0) into a SecDataDir-backed collection; observe the
previously-crashing worker now ignore the malformed entry.
References
Coordinated with @airween via private security channel.