From 54c16a719431631e6b71ade07582ef8f93a69cae Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 2 Nov 2026 14:24:53 +0100 Subject: [PATCH 1/3] fix: pop each entry of a Lua transformation table (v2) 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/ModSecurity#3737 --- apache2/msc_lua.c | 6 ++++++ tests/regression/rule/00-script.t | 19 +++++++++++++++++++ .../server_root/conf/tfn-long-table.lua | 7 +++++++ 3 files changed, 32 insertions(+) create mode 100644 tests/regression/server_root/conf/tfn-long-table.lua diff --git a/apache2/msc_lua.c b/apache2/msc_lua.c index b165381bd..aee4676a0 100644 --- a/apache2/msc_lua.c +++ b/apache2/msc_lua.c @@ -180,6 +180,7 @@ static apr_array_header_t *resolve_tfns(lua_State *L, int idx, modsec_rec *msr, /* A "none" means start over */ if (strcmp("none", name) == 0) { tfn_arr->nelts = 0; + lua_pop(L, 1); continue; } @@ -189,6 +190,11 @@ static apr_array_header_t *resolve_tfns(lua_State *L, int idx, modsec_rec *msr, } else { *(msre_tfn_metadata **)apr_array_push(tfn_arr) = tfn; } + + /* Pop the entry, otherwise a table longer than LUA_MINSTACK + * writes past the stack. name isn't used after this. + */ + lua_pop(L, 1); } } else if (lua_isstring(L, idx)) { /* The second parameter may be a simple string? */ name = (char *)luaL_checkstring(L, idx); diff --git a/tests/regression/rule/00-script.t b/tests/regression/rule/00-script.t index e57bdda39..407ed0316 100644 --- a/tests/regression/rule/00-script.t +++ b/tests/regression/rule/00-script.t @@ -118,3 +118,22 @@ GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", ), }, +{ + type => "rule", + comment => "SecRuleScript m.getvar with a long transformation table", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRuleScript "tfn-long-table.lua" "phase:2,pass" + ), + match_log => { + debug => [ qr/long table: abc/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?a=ABC", + ), +}, diff --git a/tests/regression/server_root/conf/tfn-long-table.lua b/tests/regression/server_root/conf/tfn-long-table.lua new file mode 100644 index 000000000..10c8a9a56 --- /dev/null +++ b/tests/regression/server_root/conf/tfn-long-table.lua @@ -0,0 +1,7 @@ +function main() + local t = {}; + for i = 1, 1000 do t[i] = "lowercase"; end + local v = m.getvar("ARGS.a", t); + m.log(4, "long table: " .. tostring(v)); + return nil; +end From 14489f67d3e58c82c4acc49496dcd97f25d25dfb Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 2 Nov 2026 14:51:11 +0100 Subject: [PATCH 2/3] fix: skip a Lua transformation table entry that isn't a string (v2) 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 #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/ModSecurity#3737 --- apache2/msc_lua.c | 16 +++++++++++++--- tests/regression/rule/00-script.t | 19 +++++++++++++++++++ .../server_root/conf/tfn-not-a-string.lua | 5 +++++ 3 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 tests/regression/server_root/conf/tfn-not-a-string.lua diff --git a/apache2/msc_lua.c b/apache2/msc_lua.c index aee4676a0..3642b69fa 100644 --- a/apache2/msc_lua.c +++ b/apache2/msc_lua.c @@ -175,7 +175,17 @@ static apr_array_header_t *resolve_tfns(lua_State *L, int idx, modsec_rec *msr, for(i = 1; i <= n; i++) { lua_rawgeti(L, idx, i); - name = (char *)luaL_checkstring(L, -1); + + /* Skip anything but a string, a number included, instead of + * failing the whole script with luaL_checkstring(). + */ + if (lua_type(L, -1) != LUA_TSTRING) { + msr_log(msr, 8, "SecRuleScript: Transformation name must be a string, but found %s", + lua_typename(L, lua_type(L, -1))); + lua_pop(L, 1); + continue; + } + name = (char *)lua_tostring(L, -1); /* A "none" means start over */ if (strcmp("none", name) == 0) { @@ -196,8 +206,8 @@ static apr_array_header_t *resolve_tfns(lua_State *L, int idx, modsec_rec *msr, */ lua_pop(L, 1); } - } else if (lua_isstring(L, idx)) { /* The second parameter may be a simple string? */ - name = (char *)luaL_checkstring(L, idx); + } else if (lua_type(L, idx) == LUA_TSTRING) { /* The second parameter may be a simple string? */ + name = (char *)lua_tostring(L, idx); /* A "none" means start over */ if (strcmp("none", name) == 0) { diff --git a/tests/regression/rule/00-script.t b/tests/regression/rule/00-script.t index 407ed0316..37bb968d0 100644 --- a/tests/regression/rule/00-script.t +++ b/tests/regression/rule/00-script.t @@ -137,3 +137,22 @@ GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?a=ABC", ), }, +{ + type => "rule", + comment => "SecRuleScript m.getvar skips a transformation that isn't a string", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRuleScript "tfn-not-a-string.lua" "phase:2,pass" + ), + match_log => { + debug => [ qr/Transformation name must be a string, but found table.*Transformation name must be a string, but found number.*not a string: abc/s, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?a=ABC", + ), +}, diff --git a/tests/regression/server_root/conf/tfn-not-a-string.lua b/tests/regression/server_root/conf/tfn-not-a-string.lua new file mode 100644 index 000000000..5f3ad2278 --- /dev/null +++ b/tests/regression/server_root/conf/tfn-not-a-string.lua @@ -0,0 +1,5 @@ +function main() + local v = m.getvar("ARGS.a", { "lowercase", {}, 12 }); + m.log(4, "not a string: " .. tostring(v)); + return nil; +end From 6934b026a738585522d8a45261749a4b1b3343c4 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 2 Nov 2026 14:52:20 +0100 Subject: [PATCH 3/3] fix: log the right type for a bad Lua transformation parameter (v2) 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 #3738. Add a regression test with a boolean parameter. Refs owasp-modsecurity/ModSecurity#3737 --- apache2/msc_lua.c | 2 +- tests/regression/rule/00-script.t | 19 +++++++++++++++++++ .../server_root/conf/tfn-bad-param.lua | 5 +++++ 3 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 tests/regression/server_root/conf/tfn-bad-param.lua diff --git a/apache2/msc_lua.c b/apache2/msc_lua.c index 3642b69fa..c03dc3084 100644 --- a/apache2/msc_lua.c +++ b/apache2/msc_lua.c @@ -222,7 +222,7 @@ static apr_array_header_t *resolve_tfns(lua_State *L, int idx, modsec_rec *msr, } } } else { - msr_log(msr, 1, "SecRuleScript: Transformation parameter must be a transformation name or array of transformation names, but found \"%s\" (type %d).", lua_typename(L, idx), lua_type(L, idx)); + msr_log(msr, 1, "SecRuleScript: Transformation parameter must be a transformation name or array of transformation names, but found \"%s\" (type %d).", lua_typename(L, lua_type(L, idx)), lua_type(L, idx)); return NULL; } diff --git a/tests/regression/rule/00-script.t b/tests/regression/rule/00-script.t index 37bb968d0..ffb4ed76d 100644 --- a/tests/regression/rule/00-script.t +++ b/tests/regression/rule/00-script.t @@ -156,3 +156,22 @@ GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?a=ABC", ), }, +{ + type => "rule", + comment => "SecRuleScript m.getvar logs the type of a bad transformation parameter", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRuleScript "tfn-bad-param.lua" "phase:2,pass" + ), + match_log => { + debug => [ qr/but found "boolean" \(type 1\).*bad param: ABC/s, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?a=ABC", + ), +}, diff --git a/tests/regression/server_root/conf/tfn-bad-param.lua b/tests/regression/server_root/conf/tfn-bad-param.lua new file mode 100644 index 000000000..e070c83e9 --- /dev/null +++ b/tests/regression/server_root/conf/tfn-bad-param.lua @@ -0,0 +1,5 @@ +function main() + local v = m.getvar("ARGS.a", true); + m.log(4, "bad param: " .. tostring(v)); + return nil; +end