v3: Lua m.setvar() returns -1 on errors, so the script gets stack garbage
Maintainers usually reply within 1 day
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 72/100
Research direction
Start in src/engine/lua.cc at Lua::setvar(), where the three error paths return -1 (around lines 343, 351 and 366). Compare with getvar(), which calls luaL_checkstring() before any C++ locals exist. Done means the error paths return 0 (or push nil and return 1), the two luaL_checkstring() calls move ahead of the std::string locals, the typo is fixed, and a regression test runs the two reproduction scripts and checks the returned value.
Written by the indexing model from the issue text.
Description
@airween, I found this while reviewing #3730. It's in the same file but a separate problem, so I'm filing it on its own.
Problem
Lua::setvar() (src/engine/lua.cc, v3/master 9d45c6d7) returns -1 on its three error paths:
- line 343: wrong number of arguments;
- line 351:
luaL_checkstring()returnedNULL; - line 366: no
.in the name, so no collection.
A lua_CFunction must return the number of values it pushed onto the stack. Lua doesn't check for a negative count in release builds, so the call returns whatever happens to be on the stack.
Reproduction
I used two scripts run with exec: in phase 1 (macOS, Lua 5.5.1):
-- nodot.lua
function main()
local r = m.setvar("novar", "x");
m.log(4, "after setvar, got: " .. tostring(r));
return nil;
end
-- onearg.lua
function main()
local r = m.setvar("tx.a");
m.log(4, "after setvar, got: " .. tostring(r));
return nil;
end
Debug log:
[8] m.setvar: Must specify a collection using dot character - ie m.setvar(tx.myvar,mydata)
[4] after setvar, got: userdata: 0x16f5b2498
...
[8] m.setvar: Failed m.setvar funtion must has 2 arguments
[4] after setvar, got: [
In the first case the script gets back the __transaction pointer that setvar() pushed. In the second it gets a stray string. Nothing crashed here, but this is undefined behaviour, and the result depends on the Lua version and build.
A related, smaller point
luaL_checkstring() raises a Lua error, using longjmp with the usual C build of Lua, when an argument isn't a string, e.g. m.setvar("tx.a", {}). In setvar() that happens after the three std::string locals have been constructed, so their destructors are skipped. They're still empty at that point, so nothing leaks today, but it's fragile. getvar(), getvars() and log() call luaL_checkstring() before any C++ object exists, so they're fine.
Suggested fix
- Return
0(no results) on the three error paths, aslog()does. Or, if a script should see the failure, pushnilorfalseand return1. - Move the two
luaL_checkstring()calls before thestd::stringlocals, as ingetvar(). - While there: fix the typo in the debug message ("funtion must has" → "function must have").
A regression case with the two scripts above, checking that m.setvar() returns nil (or false), would cover it.
- Dominant language
- C++
- Stars
- 9.8k
- Forks
- 1.8k
- Avg merge
- 1d 9h
- Merged PRs (30d)
- 19
Getting set up
- No Dockerfile or Docker Compose file
- Has a pull request template
- No contributing guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from owasp-modsecurity/ModSecurity
-
v3: "General failure, action: 0x..." prints a pointer instead of the action namePossibly taken @Nikhil-jaiswal007 claimed this 2 days ago. Open3.x bug
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
owasp-modsecurity/ModSecurity#3708 ·
Maintainers usually reply within 1 day
-
v2: t:utf8toUnicode drops the last hex digit of code points above U+FFFF (regression since 2.9.9)Open
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
owasp-modsecurity/ModSecurity#3684 ·
Maintainers usually reply within 1 day
-
2.x bug Platform - IIS pr available windows
Difficulty 1/5 Under an hour Newbie friendliness 90/100
owasp-modsecurity/ModSecurity#3623 · 2 comments ·
Maintainers usually reply within 1 day
-
2.x bug Platform - IIS pr available windows
Difficulty 2/5 1-3 hours Newbie friendliness 82/100
owasp-modsecurity/ModSecurity#3621 · 2 comments ·
Maintainers usually reply within 1 day
-
2.x bug Platform - IIS pr available windows
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
owasp-modsecurity/ModSecurity#3619 · 2 comments ·
Maintainers usually reply within 1 day
All issues in owasp-modsecurity/ModSecurity
Similar issues
-
Issue-Bug Needs-Triage
Difficulty 2/5 1-3 hours Newbie friendliness 62/100
microsoft/winget-cli#6645 ·
Maintainers usually reply within 2 days
-
cleanup help wanted
Difficulty 2/5 1-3 hours Newbie friendliness 62/100
ESPresense/ESPresense#2602 ·
Maintainers usually reply within 1 day
-
devex good first issue
Difficulty 2/5 1-3 hours Newbie friendliness 62/100
Maintainers usually reply within 1 day
-
artifact:simulation-scenario domain:physics good first issue help wanted role:validation testing
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 62/100
Maintainers usually reply within 3 days