Fix soundness problems with rlua

setmetatable now wraps a __gc method in a cclosure that aborts on error, also
'debug' library is no longer provided.  We could provide just the subset of the
debug library that is sound, though.

Author: kyren

src/ffi.rs

@@ -121,8 +121,26 @@ extern "C" {
     pub fn lua_error(state: *mut lua_State) -> !;
     pub fn lua_atpanic(state: *mut lua_State, panic: lua_CFunction) -> lua_CFunction;
 
+    pub fn luaopen_base(state: *mut lua_State) -> c_int;
+    pub fn luaopen_coroutine(state: *mut lua_State) -> c_int;
+    pub fn luaopen_table(state: *mut lua_State) -> c_int;
+    pub fn luaopen_io(state: *mut lua_State) -> c_int;
+    pub fn luaopen_os(state: *mut lua_State) -> c_int;
+    pub fn luaopen_string(state: *mut lua_State) -> c_int;
+    pub fn luaopen_utf8(state: *mut lua_State) -> c_int;
+    pub fn luaopen_math(state: *mut lua_State) -> c_int;
+    pub fn luaopen_debug(state: *mut lua_State) -> c_int;
+    pub fn luaopen_package(state: *mut lua_State) -> c_int;
+
     pub fn luaL_newstate() -> *mut lua_State;
     pub fn luaL_openlibs(state: *mut lua_State);
+    pub fn luaL_requiref(
+        state: *mut lua_State,
+        modname: *const c_char,
+        openf: lua_CFunction,
+        glb: c_int,
+    );
+
     pub fn luaL_loadbufferx(
         state: *mut lua_State,
         buf: *const c_char,

src/lua.rs

@@ -1302,7 +1302,18 @@ impl Lua {
             let state = ffi::luaL_newstate();
 
             stack_guard(state, 0, || {
-                ffi::luaL_openlibs(state);
+                // Do not open the debug library, currently it can be used to cause unsafety.
+                ffi::luaL_requiref(state, cstr!("_G"), ffi::luaopen_base, 1);
+                ffi::luaL_requiref(state, cstr!("base"), ffi::luaopen_base, 1);
+                ffi::luaL_requiref(state, cstr!("coroutine"), ffi::luaopen_coroutine, 1);
+                ffi::luaL_requiref(state, cstr!("table"), ffi::luaopen_table, 1);
+                ffi::luaL_requiref(state, cstr!("io"), ffi::luaopen_io, 1);
+                ffi::luaL_requiref(state, cstr!("os"), ffi::luaopen_os, 1);
+                ffi::luaL_requiref(state, cstr!("string"), ffi::luaopen_string, 1);
+                ffi::luaL_requiref(state, cstr!("utf8"), ffi::luaopen_utf8, 1);
+                ffi::luaL_requiref(state, cstr!("math"), ffi::luaopen_math, 1);
+                ffi::luaL_requiref(state, cstr!("package"), ffi::luaopen_package, 1);
+                ffi::lua_pop(state, 10);
 
                 // Create the userdata registry table
 
@@ -1348,7 +1359,8 @@ impl Lua {
 
                 ffi::lua_rawset(state, ffi::LUA_REGISTRYINDEX);
 
-                // Override pcall / xpcall
+                // Override pcall, xpcall, and setmetatable with versions that cannot be used to
+                // cause unsafety.
 
                 ffi::lua_rawgeti(state, ffi::LUA_REGISTRYINDEX, ffi::LUA_RIDX_GLOBALS);
 
@@ -1360,6 +1372,10 @@ impl Lua {
                 ffi::lua_pushcfunction(state, safe_xpcall);
                 ffi::lua_rawset(state, -3);
 
+                push_string(state, "setmetatable");
+                ffi::lua_pushcfunction(state, safe_setmetatable);
+                ffi::lua_rawset(state, -3);
+
                 ffi::lua_pop(state, 1);
             });
 

src/tests.rs

@@ -919,6 +919,33 @@ fn test_pcall_xpcall() {
     );
 }
 
+#[test]
+fn test_setmetatable_gc() {
+    let lua = Lua::new();
+    lua.exec::<()>(
+        r#"
+            val = nil
+            table = {}
+            setmetatable(table, {
+                __gc = function()
+                    val = "gcwascalled"
+                end
+            })
+            table_badgc = {}
+            setmetatable(table_badgc, {
+                __gc = "what's a gc"
+            })
+            table = nil
+            table_badgc = nil
+            collectgarbage("collect")
+        "#,
+        None,
+    ).unwrap();
+
+    let globals = lua.globals();
+    assert_eq!(globals.get::<_, String>("val").unwrap(), "gcwascalled");
+}
+
 // Need to use compiletest-rs or similar to make sure these don't compile.
 /*
 #[test]

src/util.rs

@@ -290,22 +290,19 @@ pub unsafe fn handle_error(state: *mut ffi::lua_State, err: c_int) -> Result<()>
                 }
                 ffi::LUA_ERRERR => Error::ErrorError(err_string),
                 ffi::LUA_ERRMEM => {
-                    // This is not impossible to hit, but this library is not set up
-                    // to handle this properly.  Lua does a longjmp on out of memory
-                    // (like all lua errors), but it can do this from a huge number
-                    // of lua functions, and it is extremely difficult to set up the
-                    // pcall protection for every lua function that might allocate.
-                    // If lua does this in an unprotected context, it will abort
-                    // anyway, so the best we can do right now is guarantee an abort
-                    // even in a protected context.
-                    println!("Lua memory error, aborting!");
+                    // TODO: We should provide lua with custom allocators that guarantee aborting on
+                    // OOM, instead of relying on the system allocator.  Currently, this is a
+                    // theoretical soundness bug, because an OOM could trigger a longjmp out of
+                    // arbitrary rust code that is unprotectedly calling a lua function marked as
+                    // potentially causing memory errors, which is most of them.
+                    eprintln!("Lua memory error, aborting!");
                     process::abort()
                 }
                 ffi::LUA_ERRGCMM => {
-                    // This should be impossible, or at least is indicative of an
-                    // internal bug.  Similarly to LUA_ERRMEM, this could indicate a
-                    // longjmp out of rust code, so we just abort.
-                    println!("Lua error during __gc, aborting!");
+                    // This should be impossible, since we wrap setmetatable to protect __gc
+                    // metamethods, but if we do end up here then the same logic as setmetatable
+                    // applies and we must abort.
+                    eprintln!("Lua error during __gc, aborting!");
                     process::abort()
                 }
                 _ => lua_panic!(state, "internal error: unrecognized lua error code"),
@@ -489,7 +486,44 @@ pub unsafe extern "C" fn safe_xpcall(state: *mut ffi::lua_State) -> c_int {
     }
 }
 
-/// Does not call checkstack, uses 1 stack space
+// Safely call setmetatable, if a __gc function is given, will wrap it in pcall, and panic on error.
+pub unsafe extern "C" fn safe_setmetatable(state: *mut ffi::lua_State) -> c_int {
+    if ffi::lua_gettop(state) < 2 {
+        push_string(state, "not enough arguments to setmetatable");
+        ffi::lua_error(state);
+    }
+
+    // Wrapping the __gc method in setmetatable ONLY works because Lua 5.3 only honors the __gc
+    // method when it exists upon calling setmetatable, and ignores it if it is set later.
+    push_string(state, "__gc");
+    if ffi::lua_rawget(state, -2) == ffi::LUA_TFUNCTION {
+        unsafe extern "C" fn safe_gc(state: *mut ffi::lua_State) -> c_int {
+            ffi::lua_pushvalue(state, ffi::lua_upvalueindex(1));
+            ffi::lua_insert(state, 1);
+            if ffi::lua_pcall(state, 1, 0, 0) != ffi::LUA_OK {
+                // If a user supplied __gc metamethod causes an error, we must always abort.  We may
+                // be inside a protected context due to being in a callback, but inside an
+                // unprotected ffi call that can cause memory errors, so may be at risk of
+                // longjmping over arbitrary rust.
+                eprintln!("Lua error during __gc, aborting!");
+                process::abort()
+            } else {
+                ffi::lua_gettop(state)
+            }
+        }
+
+        ffi::lua_pushcclosure(state, safe_gc, 1);
+        push_string(state, "__gc");
+        ffi::lua_insert(state, -2);
+        ffi::lua_rawset(state, -3);
+    } else {
+        ffi::lua_pop(state, 1);
+    }
+    ffi::lua_setmetatable(state, -2);
+    0
+}
+
+// Does not call checkstack, uses 1 stack space
 pub unsafe fn main_state(state: *mut ffi::lua_State) -> *mut ffi::lua_State {
     ffi::lua_rawgeti(state, ffi::LUA_REGISTRYINDEX, ffi::LUA_RIDX_MAINTHREAD);
     let main_state = ffi::lua_tothread(state, -1);