Page MenuHomePhabricator

Add a changelog and apply a custom version number to our patched bundled Lua binaries
Open, MediumPublicSecurity

Description

A security review of one of the MediaWiki installation's I help manage has flagged lua as being vulnerable to CVE-2014-5461 and CVE-2021-43519. While it looks like CVE-2014-5461 has been patched, there is no indication of this in the version number and it isn't clear that CVE-2021-43519 has been addressed at all.

The patches for these binaries should be provided in version control somewhere (I can't find that linked on T72541) and the version numbers should be updated to indicate that wmf has patched them. Kind of like Debian's -bpo indications.

Details

Risk Rating
Low
Author Affiliation
Wikimedia Communities

Event Timeline

it isn't clear that CVE-2021-43519 has been addressed at all.

That looks like only applicable to coroutines, which i think we disable (or at least luasandbox disables, im not sure about lua standalone)

Reedy renamed this task from Ensure CVE-2021-43519 is addressed and Provide versioned binaries for Lua indicating that they have been patched to Ensure CVE-2021-43519 is addressed and provide versioned binaries for Lua indicating that they have been patched.Thu, Jun 6, 5:30 PM
Reedy added a project: Scribunto.
Reedy updated the task description. (Show Details)

The patches for these binaries should be provided in version control somewhere (I can't find that linked on T72541)

It was added in rELUA237d059ea167: Add lua5.1 patch for CVE-2014-5461 (after not being included in the original change).

and the version numbers should be updated to indicate that wmf has patched them. Kind of like Debian's -bpo indications.

Should be trivially enough doable using the generic.patch that was introduced in rELUA54cedd69b8f9: Introduced standalone interpreter, implemented module isolation, which already touches the Makefile; though some other files may need adjusting too, specifcally etc/lua.pc and src/lua.h:

% grep -R 5.1.5 *
Makefile:R= 5.1.5
doc/contents.html:Last change: revised for Lua 5.1.5
doc/readme.html:This is the documentation included in the source distribution of Lua 5.1.5.
doc/manual.html:Last change: revised for Lua 5.1.5
etc/lua.pc:R= 5.1.5
src/lcode.c:** $Id: lcode.c,v 2.25.1.5 2011/01/31 14:53:16 roberto Exp $
src/lapi.c:** $Id: lapi.c,v 2.55.1.5 2008/07/04 18:41:18 roberto Exp $
src/lua.h:#define LUA_RELEASE	"Lua 5.1.5"

The patch for CVE-2021-43519 seems to be https://github.com/lua/lua/commit/74d99057a5146755e737c479850f87fd0e3b6868 (unattributed that it fixes the CVE), but made only against 5.4.

https://github.com/lua/lua/commit/74d99057a5146755e737c479850f87fd0e3b6868.patch

From 74d99057a5146755e737c479850f87fd0e3b6868 Mon Sep 17 00:00:00 2001
From: Roberto Ierusalimschy <roberto@inf.puc-rio.br>
Date: Wed, 3 Nov 2021 15:04:18 -0300
Subject: [PATCH] Bug: C stack overflow with coroutines

'coroutine.resume' did not increment counter of C calls when
continuing execution after a protected error (that is,
while running 'precover').
---
 ldo.c             |  6 ++++--
 testes/cstack.lua | 14 ++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/ldo.c b/ldo.c
index d0edc8b4f..66f890364 100644
--- a/ldo.c
+++ b/ldo.c
@@ -759,11 +759,10 @@ static void resume (lua_State *L, void *ud) {
   StkId firstArg = L->top - n;  /* first argument */
   CallInfo *ci = L->ci;
   if (L->status == LUA_OK)  /* starting a coroutine? */
-    ccall(L, firstArg - 1, LUA_MULTRET, 1);  /* just call its body */
+    ccall(L, firstArg - 1, LUA_MULTRET, 0);  /* just call its body */
   else {  /* resuming from previous yield */
     lua_assert(L->status == LUA_YIELD);
     L->status = LUA_OK;  /* mark that it is running (again) */
-    luaE_incCstack(L);  /* control the C stack */
     if (isLua(ci)) {  /* yielded inside a hook? */
       L->top = firstArg;  /* discard arguments */
       luaV_execute(L, ci);  /* just continue running Lua code */
@@ -814,6 +813,9 @@ LUA_API int lua_resume (lua_State *L, lua_State *from, int nargs,
   else if (L->status != LUA_YIELD)  /* ended with errors? */
     return resume_error(L, "cannot resume dead coroutine", nargs);
   L->nCcalls = (from) ? getCcalls(from) : 0;
+  if (getCcalls(L) >= LUAI_MAXCCALLS)
+    return resume_error(L, "C stack overflow", nargs);
+  L->nCcalls++;
   luai_userstateresume(L, nargs);
   api_checknelems(L, (L->status == LUA_OK) ? nargs + 1 : nargs);
   status = luaD_rawrunprotected(L, resume, &nargs);
diff --git a/testes/cstack.lua b/testes/cstack.lua
index 213d15d47..ca76c8729 100644
--- a/testes/cstack.lua
+++ b/testes/cstack.lua
@@ -103,6 +103,20 @@ do
 end
 
 
+do    -- bug in 5.4.2
+  print("nesting coroutines running after recoverable errors")
+  local count = 0
+  local function foo()
+    count = count + 1
+    pcall(1)   -- create an error
+    -- running now inside 'precover' ("protected recover")
+    coroutine.wrap(foo)()   -- call another coroutine
+  end
+  checkerror("C stack overflow", foo)
+  print("final count: ", count)
+end
+
+
 if T then
   print("testing stack recovery")
   local N = 0      -- trace number of calls

Very helpfully, https://github.com/lua/lua doesn't include a 5.1.5 tag...

https://www.lua.org/ftp/lua-5.1.5.tar.gz exists...

It's not a trivial patch application though.

The ccall doesn't have the 4th parameter in 5.1.5. The luaE_incCstack doesn't exist. And where the 3 lines before the luai_userstateresume call are added... Well, the luai_userstateresume call doesn't exist either.

So in 5.1.5:

static void resume (lua_State *L, void *ud) {
  StkId firstArg = cast(StkId, ud);
  CallInfo *ci = L->ci;
  if (L->status == 0) {  /* start coroutine? */
    lua_assert(ci == L->base_ci && firstArg > L->base);
    if (luaD_precall(L, firstArg - 1, LUA_MULTRET) != PCRLUA)
      return;
  }
  else {  /* resuming from previous yield */
    lua_assert(L->status == LUA_YIELD);
    L->status = 0;
    if (!f_isLua(ci)) {  /* `common' yield? */
      /* finish interrupted execution of `OP_CALL' */
      lua_assert(GET_OPCODE(*((ci-1)->savedpc - 1)) == OP_CALL ||
                 GET_OPCODE(*((ci-1)->savedpc - 1)) == OP_TAILCALL);
      if (luaD_poscall(L, firstArg))  /* complete it... */
        L->top = L->ci->top;  /* and correct top if not multiple results */
    }
    else  /* yielded inside a hook: just continue its execution */
      L->base = L->ci->base;
  }
  luaV_execute(L, cast_int(L->ci - L->base_ci));
}

Whereas the patch is against (and GitHub reckons it's in v5.4.4 onwards):

/*
** Do the work for 'lua_resume' in protected mode. Most of the work
** depends on the status of the coroutine: initial state, suspended
** inside a hook, or regularly suspended (optionally with a continuation
** function), plus erroneous cases: non-suspended coroutine or dead
** coroutine.
*/
static void resume (lua_State *L, void *ud) {
  int n = *(cast(int*, ud));  /* number of arguments */
  StkId firstArg = L->top - n;  /* first argument */
  CallInfo *ci = L->ci;
  if (L->status == LUA_OK)  /* starting a coroutine? */
    ccall(L, firstArg - 1, LUA_MULTRET, 1);  /* just call its body */
  else {  /* resuming from previous yield */
    lua_assert(L->status == LUA_YIELD);
    L->status = LUA_OK;  /* mark that it is running (again) */
    luaE_incCstack(L);  /* control the C stack */
    if (isLua(ci)) {  /* yielded inside a hook? */
      L->top = firstArg;  /* discard arguments */
      luaV_execute(L, ci);  /* just continue running Lua code */
    }
    else {  /* 'common' yield */
      if (ci->u.c.k != NULL) {  /* does it have a continuation function? */
        lua_unlock(L);
        n = (*ci->u.c.k)(L, LUA_YIELD, ci->u.c.ctx); /* call continuation */
        lua_lock(L);
        api_checknelems(L, n);
      }
      luaD_poscall(L, ci, n);  /* finish 'luaD_call' */
    }
    unroll(L, NULL);  /* run continuation */
  }
}

According to https://www.cvedetails.com/cve/CVE-2021-43519/ Versions from including (>=) 5.1.0 and before (<) 5.3.5 and Versions from including (>=) 5.4.0 and before (<) 5.4.4 are vulnerable.

The CVE is dated as Published 2021-11-09 13:15:09

https://lua.org/versions.html#5.3

The last release was Lua 5.3.6, released on 25 Sep 2020. There will be no further releases of Lua 5.3.

Other pages such as https://nvd.nist.gov/vuln/detail/CVE-2021-43519 and https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-43519 suggest 5.1.0~5.4.4, so I suspect cvedetails.com is just incorrect about its version strings.

Looking at https://security-tracker.debian.org/tracker/CVE-2021-43519, they have not made a patch for anything before 5.4.4 either...

Debian also attributes the commit at fault as https://github.com/lua/lua/commit/287b302acb8d925178e9edb800f0a8d18c7d35f6 which is in 5.4.2, and you can see the change code that is later changed by the CVE fix.

And the two referenced mailing lists posts seem to agree - http://lua-users.org/lists/lua-l/2023-06/msg00059.html and http://lua-users.org/lists/lua-l/2023-06/msg00063.html

Quoting from the first post:

I'm able to reproduce the crash with v5.0 to v5.1.2, but I believe this is an
unrelated issue which is listed at https://www.lua.org/bugs.html#5.1.2-4 .
Similarly, the original PoC yields a crash with v5.2.0, which I believe is due
to this unrelated issue: https://www.lua.org/bugs.html#5.2.0-4 . Either way,
I'm unable to reproduce the crash with v5.1.x from v5.1.3, nor with v5.2.x from
v5.2.1.

it isn't clear that CVE-2021-43519 has been addressed at all.

That looks like only applicable to coroutines, which i think we disable (or at least luasandbox disables, im not sure about lua standalone)

^ Depending on the situation here... This may not be worth spending much more time on either...

Especially if the CVE record is wrong! And I'd be inclined to believe Debian and the references they are using for that too.

That, of course, doesn't mean we shouldn't probably try and do the version number correction, and introduce our own changelog at least within the MW extension itself for the changes included ontop of the public release.

Then it depends on someone having a build env to actually build it for linux/macos/windows.

But this probably means this doesn't need to stay as a private security bug either.

it isn't clear that CVE-2021-43519 has been addressed at all.

That looks like only applicable to coroutines, which i think we disable (or at least luasandbox disables, im not sure about lua standalone)

How can I verify that coroutines are disabled or not possible from MediaWiki?

How can I verify that coroutines are disabled or not possible from MediaWiki?

Looke like there is a test in the original report.

I think the results I got on my local wiki means this is safe. On a page in the module namespace, I have

local p = {}
function p.hello( frame )
	coroutine.wrap(p.hello)()
	return "Hello, World!"
end
return p

and, in the debug console of that page running`mw.logObject(p.hello())` results in:

Lua error at line 7: attempt to index global 'coroutine' (a nil value).

Backtrace:

    (tail call): ?
    Module:Loop:7: in function "hello"
    console input:7: ?
    (tail call): ?
    [C]: in function "xpcall"
    MWServer.lua:99: in function "handleCall"
    MWServer.lua:313: in function "dispatch"
    MWServer.lua:52: in function "execute"
    mw_main.lua:7: in main chunk
    [C]: ?

I wish I understood how coroutines were disabled, but searching the Scribunto codebase found only a couple of references to the word coroutine in the tests. Fiddling ignorantly with those tests didn't seem to provide any meaningful change.

If coroutines are disabled, this bug can be made public since there are no other issues that I see here.

I wish I understood how coroutines were disabled, but searching the Scribunto codebase found only a couple of references to the word coroutine in the tests. Fiddling ignorantly with those tests didn't seem to provide any meaningful change.

We build our own global namespace and just don't include coroutine. It's similar to how we disable things like os.execute(). Functions are whitelisted rather than blacklisted.

sbassett added a project: SecTeam-Processed.
sbassett subscribed.

Looks like @Reedy and @tstarling answered your questions, @MarkAHershberger? Did you need anything else right now or can we resolve this?

Looks like @Reedy and @tstarling answered your questions, @MarkAHershberger? Did you need anything else right now or can we resolve this?

The security issues are resolved.

However, the version number item is not resolved. As mentioned above, the bundled lua binaries should to have a clear indication added to their version numbers so that they don't trigger false positives during security audits.

From my pov, since no security issue needs to be resolved, this could be made public.

sbassett triaged this task as Medium priority.Mon, Jun 10, 5:38 PM
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett changed Risk Rating from N/A to Low.
Reedy renamed this task from Ensure CVE-2021-43519 is addressed and provide versioned binaries for Lua indicating that they have been patched to Add a changelog and apply a custom version number to our patched bundled Lua binaries.Wed, Jun 12, 1:26 PM

I created a couple of patch files that can be applied to a pristine copy of a git repo for lua 5.1.5 (as noted elsewhere, this isn't tagged in the github repo for lua) that will hopefully be of use. I've left stubs in the README-WMF for someone to document build instructions so that the binaries could easily be reproduced.