Repository navigation
Conversation
resolve_tfns() pushed each entry of the transformation table given to m.getvar() or m.getvars() with lua_rawgeti() and never popped it. A C function only has LUA_MINSTACK free stack slots unless it calls lua_checkstack(), so a longer table wrote past the Lua stack and crashed the Apache child. Pop the entry after its name has been used, on the "none" path too. Add a regression test with a 1000-entry table. Refs owasp-modsecurity#3737
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the Lua transformation-table stack-growth fix and its added regression case.
No open issues 🎉
What changed in this PR
Fixes Lua stack growth in ModSecurity v2’s transformation-table handling, addressing the v2 portion of #3737.
Changes:
- Pops each transformation entry after use, including the
"none"reset path. - Adds a regression case using 1,000
"lowercase"transformations, checking forabcoutput and an HTTP 200 response.
| File | Description |
|---|---|
tests/regression/server_root/conf/tfn-long-table.lua |
Adds a Lua script exercising a 1,000-entry transformation table. |
tests/regression/rule/00-script.t |
Checks the script’s transformed output and successful response. |
apache2/msc_lua.c |
Balances Lua stack pushes and pops in resolve_tfns(). |
Review effort: Balanced
resolve_tfns() read each table entry with luaL_checkstring(), so an entry that isn't a string or a number, such as a table, raised a Lua error and failed the whole script with "bad argument", and a number was converted to a name that never resolves. Check the type with lua_type() instead, and skip an entry that isn't a string with a debug message, as v3 does since owasp-modsecurity#3738. Check a single transformation name the same way; a number there now gets the "Transformation parameter must be ..." message. Add a regression test with a table and a number entry. Refs owasp-modsecurity#3737
resolve_tfns() passed the stack index to lua_typename(), which takes a type, so the message for a transformation parameter that is neither a string nor a table named the wrong type: "userdata" for a boolean, for example. v3 had the same bug, fixed in owasp-modsecurity#3738. Add a regression test with a boolean parameter. Refs owasp-modsecurity#3737
|
what
resolve_tfns()(apache2/msc_lua.c) pops each entry of a transformation table after it has used the name, on the"none"path too.Add a case to
tests/regression/rule/00-script.t, with the scripttests/regression/server_root/conf/tfn-long-table.lua(force-added, like the other scripts there, because the directory is in.gitignore):m.getvar("ARGS.a", t)with a 1000-entry"lowercase"table must returnabcfor?a=ABC.On the current
v2/master, without the fix, this case fails: the Apache child serving the request exits with a segmentation fault (AH00051: child pid ... exit signal Segmentation fault (11)in the error log), so no 200 comes back. With the fix, all 7 cases in00-script.tpass.why
resolve_tfns()is called bym.getvar()andm.getvars()for their transformation argument. It pushed each table entry withlua_rawgeti()and never popped it. A C function only hasLUA_MINSTACK(20) free stack slots unless it callslua_checkstack(), so a longer table wrote past the Lua stack. In v3, the same loop crashes the process with 100 entries (Lua m.getvar()/m.getvars(): long transformation tables crash, and "none" doesn't reset in v3 #3737).strcmp(),msre_engine_tfn_resolve()and the log message. The resolved transformations are stored asmsre_tfn_metadatapointers, not names, so nothing points into the popped string afterwards."none"already empties the list, and there are no C++ destructors forluaL_checkstring()to skip.references
AI disclosure
Made with help of Claude Code, Opus 5.5