Skip to content

Commit

Permalink
Security: fix Lua cmsgpack library stack overflow.
Browse files Browse the repository at this point in the history
During an auditing effort, the Apple Vulnerability Research team discovered
a critical Redis security issue affecting the Lua scripting part of Redis.

-- Description of the problem

Several years ago I merged a pull request including many small changes at
the Lua MsgPack library (that originally I authored myself). The Pull
Request entered Redis in commit 90b6337, in 2014.
Unfortunately one of the changes included a variadic Lua function that
lacked the check for the available Lua C stack. As a result, calling the
"pack" MsgPack library function with a large number of arguments, results
into pushing into the Lua C stack a number of new values proportional to
the number of arguments the function was called with. The pushed values,
moreover, are controlled by untrusted user input.

This in turn causes stack smashing which we believe to be exploitable,
while not very deterministic, but it is likely that an exploit could be
created targeting specific versions of Redis executables. However at its
minimum the issue results in a DoS, crashing the Redis server.

-- Versions affected

Versions greater or equal to Redis 2.8.18 are affected.

-- Reproducing

Reproduce with this (based on the original reproduction script by
Apple security team):

https://gist.github.com/antirez/82445fcbea6d9b19f97014cc6cc79f8a

-- Verification of the fix

The fix was tested in the following way:

1) I checked that the problem is no longer observable running the trigger.
2) The Lua code was analyzed to understand the stack semantics, and that
actually enough stack is allocated in all the cases of mp_pack() calls.
3) The mp_pack() function was modified in order to show exactly what items
in the stack were being set, to make sure that there is no silent overflow
even after the fix.

-- Credits

Thank you to the Apple team and to the other persons that helped me
checking the patch and coordinating this communication.
  • Loading branch information
antirez committed Jun 13, 2018
1 parent 032ea65 commit 52a0020
Showing 1 changed file with 3 additions and 0 deletions.
3 changes: 3 additions & 0 deletions deps/lua/src/lua_cmsgpack.c
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,9 @@ int mp_pack(lua_State *L) {
if (nargs == 0)
return luaL_argerror(L, 0, "MessagePack pack needs input.");

if (!lua_checkstack(L, nargs))
return luaL_argerror(L, 0, "Too many arguments for MessagePack pack.");

buf = mp_buf_new(L);
for(i = 1; i <= nargs; i++) {
/* Copy argument i to top of stack for _encode processing;
Expand Down

5 comments on commit 52a0020

@Machiry
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any CVE assigned for this?

@antirez
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, should be one of the following two:

CVE-2018-11218
CVE-2018-11219

@Machiry
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank You @antirez, We are doing a study on the assignment of CVEs to security fixes.

Is there any reason, why CVE numbers were not part of the original commit message?

@antirez
Copy link
Contributor Author

@antirez antirez commented on 52a0020 Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's just that I don't care about CERT, CVEs, and all these things. A matter of attitude, I don't have anything against such organizations, simply I run away from everything that looks even remotely bureaucracy.

@Machiry
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay :)

Please sign in to comment.