Re: Crash Analysis: Finalizer Logic in singlestep function can lead to Sandbox Escape Exploit

[김지회 <pascal48@...>]

> void luaC_runtilstate (lua_State *L, int statesmask, int nofin) {
>    global_State *g = G(L);
>    while (!testbit(statesmask, g->gcstate)) {
>      if (nofin && g->gcstate == GCScallfin) {
>        g->gcstate = GCSpause;  /* skip finalization state */
>        if (testbit(statesmask, g->gcstate))
>          break;
>      }
>      singlestep(L);
>    }
> }
>
> When 'nofin' is true, the loop simply skips the GCScallfin state.

Hmm, I'm tried over a day to create proper patch to the problem.
literally a day. Trust me, I'm between jobs. :-(

But the only way I found to solve this problem is
"Just do not let GCTM to change the gckind, Recovering original mode 
after GCTM."

Because, the main problem is as I mentioned in the initial mail,

"singlestep function with case GCScallfin can change the mode of garbage 
collection via GCTM."

The core part of the crash is that GCTM can change the mode.
In a normal case, if we restrict GCTM to be called only after doing 
everything else by using a flag in luaC_runtilstate function, the 
problem will be gone.
crash1.lua can be handled well, too.

However, The problem arise again if GCTM is triggered by 
collectgarbage("step") statement in lua script.
Because the script explicitly call incstep function which lead to 
singlestep function without luaC_runtilstate.
Eventually, it can change the mode of garbage collector.
Besides, in crash1.lua we trigger good collection to change the mode of 
garbage collector, but  the mode can be changed via 
collectgarbage("generational") statement in lua script.
Note that we can trigger the change with collectgarbage("generational") 
lua script without recursive call of GCTM - it's exactly the same as 
crash2.lua!

As a result, even if we restrict luaC_runtilsate function by flag, the 
same problem can be triggered by using collectgarbage("step") and 
collectgarbage("generational").
you can find the exact example from crash2.lua script and ASAN report in 
the initial mail.
I copy them in this mail also for convenience.

---------- [ crash2.lua ] -----------
setmetatable({}, {
    __gc = function()
        setmetatable({}, {
            __gc = function()
                collectgarbage("generational") -- Explicitly change mode
                setmetatable({}, {
                    __gc = function()
                        collectgarbage("step")
                        collectgarbage("step")()
                    end
                })
            end
        })
        collectgarbage("step")
    end

})

---------- [ Result of crash2.lua ] -----------
root@Newbie:/home/Sandbox/temp/lua# ./lua ../../DebugLua/crash2.lua
...
==171896==ERROR: AddressSanitizer: heap-use-after-free on address
0xf51034fc at pc 0x5656e70e bp 0xffffbf78 sp 0xffffbf68
...
SUMMARY: AddressSanitizer: heap-use-after-free
(/home/Sandbox/temp/lua/lua+0x1970d) in funcnamefromcode
--------------------------------------------------


In short, my suggestion after a long-time thinking is



--------- [singlestep in lgc.c BEFORE patch] -------------

...
case GCScallfin: { /* call remaining finalizers */
    if (g->tobefnz && !g->gcemergency) {
        g->gcstopem = 0; /* ok collections during finalizers */
        work = runafewfinalizers(L, GCFINMAX) * GCFINALIZECOST;
    }
    else { /* emergency mode or no more finalizers */
        g->gcstate = GCSpause; /* finish collection */
        work = 0;
    }
...

--------- [singlestep in lgc.c AFTER patch] -------------

...
case GCScallfin: { /* call remaining finalizers */
    if (g->tobefnz && !g->gcemergency) {
        g->gcstopem = 0; /* ok collections during finalizers */
        work = runafewfinalizers(L, GCFINMAX) * GCFINALIZECOST;
        // <<<<<<PATCH LINE START>>>>>>
        if(l_unlikely(g->gckind == KGC_GEN)){
            int savedgcstate = g->gcstate;
            enterinc(g);
            g->gcstate = savedgcstate;
        }
        // <<<<<<PATCH LINE END>>>>>>
    }
    else { /* emergency mode or no more finalizers */
        g->gcstate = GCSpause; /* finish collection */
        work = 0;
    }

...

----------------------------------------------------

, which is actually the same as my first suggestion.
Or, we shall change logic of incstep function to handle the finalizer 
properly for every possible case.

By the way, I'm just wondering what the actual patch looks like.
Do we shall change incstep's logic or prevent GCTM to change mode by 
recovering?

Thank you.
--Regards, Jihoi.