CVE-2022-24736: Lua readonly tables (CVE-2022-24736, CVE-2022-24735) by oranagra · Pull Request #10651 · redis/redis

Lua readonly tables

The PR adds support for readonly tables on Lua to prevent security vulnerabilities:

• (CVE-2022-24736) An attacker attempting to load a specially crafted Lua script
  can cause NULL pointer dereference which will result with a crash of the
  redis-server process. This issue affects all versions of Redis.
• (CVE-2022-24735) By exploiting weaknesses in the Lua script execution
  environment, an attacker with access to Redis can inject Lua code that will
  execute with the (potentially higher) privileges of another Redis user.

The PR is spitted into 4 commits.

Change Lua to support readonly tables

This PR modifies the Lua interpreter code to support a new flag on tables. The new flag indicating that the table is readonly and any attempt to perform any writes on such a table will result in an error. The new feature can be turned off and on using the new lua_enablereadonlytable Lua API. The new API can be used only from C code. Changes to support this feature was taken from https://luau-lang.org/

Change eval script to set user code on Lua registry

Today, Redis wrap the user Lua code with a Lua function. For example, assuming the user code is:

return redis.call('ping')

The actual code that would have sent to the Lua interpreter was:

f_b3a02c833904802db9c34a3cf1292eee3246df3c() return redis.call('ping') end

The warped code would have been saved on the global dictionary with the following name: f_<script sha> (in our example f_b3a02c833904802db9c34a3cf1292eee3246df3c). This approach allows one user to easily override the implementation of another user code, example:

f_b3a02c833904802db9c34a3cf1292eee3246df3c = function() return 'hacked' end

Running the above code will cause evalsha b3a02c833904802db9c34a3cf1292eee3246df3c 0 to return hacked although it should have returned pong. Another disadvantage is that Redis basically runs code on the loading (compiling) phase without been aware of it. User can do code injection like this:

return 1 end <run code on compling phase> function() return 1

The warped code will look like this and the entire <run code on compiling phase> block will run outside of eval or evalsha context:

f_<sha>() return 1 end <run code on compling phase> function() return 1 end

The commits puts the user code on a special Lua table called the registry. This table is not accessible to the user so it can not be manipulated by him. Also there is no longer a need to warp the user code so there is no risk in code injection which will cause running code in the wrong context.

Use lua_enablereadonlytable to protect global tables on eval and function

The commit uses the new lua_enablereadonlytable Lua API to protect the global tables of both evals scripts and functions. For eval scripts, the implementation is easy, We simply call lua_enablereadonlytable on the global table to turn it into a readonly table.

On functions its more complected, we want to be able to switch globals between load run and function run. To achieve this, we create a new empty table that acts as the globals table for function, we control the actual globals using metatable manipulations. Notice that even if the user gets a pointer to the original tables, all the tables are set to be readonly (using lua_enablereadonlytable Lua API) so he can not change them. The following better explains the solution:

Global table {} <- global table metatable {.__index = __real_globals__}

The real_globals is depends on the run context (function load or function call).

Why is this solution needed and its not enough to simply switch globals? When we run in the context of function load and create our functions, our function gets the current globals that was set when they were created. Replacing the globals after the creation will not effect them. This is why this trick it mandatory.

Protect the rest of the global API and add an allowed list to the provided API

The allowed list is done by setting a metatable on the global table before initialising any library. The metatable set the __newindex field to a function that check the allowed list before adding the field to the table. Fields which is not on the
allowed list are simply ignored.

After initialisation phase is done we protect the global table and each table that might be reachable from the global table. For each table we also protect the table metatable if exists.

Performance

Performance tests was done on a private computer and its only purpose is to show that this fix is not causing any performance regression.

case 1: return redis.call(‘ping’)
case 2: for i=1,10000000 do redis.call(‘ping’) end

Unstable eval

Unstable function

lua_readonly_tables eval

lua_readonly_tables function

case1 ops/sec

235904.70

236406.62

232180.16

230574.14

case1 avg latency ms

0.175

0.164

0.178

0.149

case2 total time in seconds

3.373

3.444s

3.268

3.278

Breaking changes

• print function was removed from Lua because it can potentially cause the Redis processes to get stuck (if no one reads from stdout). Users should use redis.log. An alternative is to override the print implementation and print the message to the log file.

todo:

• Add commit message about where we took the code from
• Remember its not going to be squashed
• check performance
• check debugger
• Remove print from white list

All the work by @MeirShpilraien, i’m just publishing it.