fix: pop each entry of a Lua transformation table (v2)

apache2/msc_lua.c

@@ -175,11 +175,22 @@ 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) {
                 tfn_arr->nelts = 0;
+                lua_pop(L, 1);
                 continue;
             }
 
@@ -189,9 +200,14 @@ 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);
+    } 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) {
@@ -206,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;
     }
 

tests/regression/rule/00-script.t

@@ -118,3 +118,60 @@
 		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",
+	),
+},
+{
+	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",
+	),
+},
+{
+	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",
+	),
+},

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

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

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