BUG/MEDIUM: hlua: Never release a cosocket applet from the GC

The commit 38534d344b ("BUG/MEDIUM: hlua: Be able to garbage collect
uninitialized lua sockets") introduced a regression, leading to UAF
accesses.

Indeed, with a shared lua state, the garbage collector can be run on any
thread. So for a script using cosocket loaded via "lua-load" directive, the
lua socket garbage collector can run on any thread. So here there were two
problems:

  * appctx_free() must only be called by the thread owning the applet. It
    destroys the applet's task and releases the memory to the thread-local
    pool cache. Here it was possible to call appctx_free() from any other
    thread.

  * All other accesses to the applet's context were performed after the xref
    was disconnected. However, once disconnected, the applet is free to be
    released at any instant by the thread owning it, because
    hlua_socket_release() no longer finds any peer. Because the cosocket
    context is part of the applet's context, this was a use-after-free.

So now, the GC only signals the applet it must die and wakes it up, exactly
like hlua_socket_close_helper() already does, and it does so before
disconnecting the xref. This way:

  * The applet is always released by the thread owning it.

  * Because the work is performed before the xref is disconnected, a
    concurrent hlua_socket_release() is stalled on the xref lock and cannot
    release the applet in our back.

Note this also fixes the same issue in hlua_socket_close_helper(), which
still wakes up a possibly uninitialized applet. Closing a lua socket that
was never connected ("core.tcp():close()") should be enough to trigger it.

Thank you to Red Hat and AISLE Research for reporting this.

This patch must be backported to all stable versions containing the commit
38534d344b, so as far as 2.6.

diff --git a/src/hlua.c b/src/hlua.c
index a4d6826351aa41049eb63b59f0a0cae1519dbeed..211ba603a157f6f73ee1487d79ecb05bd0f9ec3b 100644 (file)
--- a/src/hlua.c
+++ b/src/hlua.c
@@ -2726,6 +2726,14 @@ static int hlua_socket_init(struct appctx *appctx)
        struct hlua_csk_ctx *csk_ctx = appctx->svcctx;
        struct stream *s;
 
+       /* The lua socket owner was released, by the garbage collector or by an
+        * explicit close, before any connect attempt. There is nothing to
+        * initialize, just report an error. This way, the applet is released by
+        * the thread owning it.
+        */
+       if (csk_ctx->die)
+               goto error;
+
        if (appctx_finalize_startup(appctx, socket_proxy, &BUF_NULL) == -1)
                goto error;
 
@@ -2793,19 +2801,22 @@ __LJMP static int hlua_socket_gc(lua_State *L)
 
        ctx = container_of(peer, struct hlua_csk_ctx, xref);
 
+       /* Notify the applet it must die and wake it up. It is important to do
+        * so before disconnecting the xref: the applet may be owned by another
+        * thread and, once the xref is disconnected, it is free to be released
+        * at any time. <ctx> is part of the applet's context, so it must no
+        * longer be dereferenced after that point.
+        *
+        * Note the applet may not be initialized yet (no connect performed). In
+        * this case, waking it up is safe: its init callback function will fail
+        * and the applet will be released by the thread owning it.
+        */
+       ctx->die = 1;
+       appctx_wakeup(ctx->appctx);
+
        /* Remove all reference between the Lua stack and the coroutine stream. */
        xref_disconnect(&socket->xref, peer);
 
-       if (se_fl_test(ctx->appctx->sedesc, SE_FL_ORPHAN)) {
-               /* The applet was never initialized, just release it */
-               appctx_free(ctx->appctx);
-       }
-       else {
-               /* Otherwise, notify it that is must die and wake it up */
-               ctx->die = 1;
-               appctx_wakeup(ctx->appctx);
-       }
-
        return 0;
 }