[PATCH] BUG/MEDIUM: Expose all converters & fetches

[Robin H. Johnson]

One of my coworkers was having some trouble trying to escape data for
JSON in Lua, using the 'json' converter, based on the documentation, and
this lead to a deep bug discovery.

The Lua documentation [1] states that JSON escaping converter is exposed
in Lua, but it turns out that's not quite true.

Creatively dumping the function metatable [2] (see code at the end)
shows only a subset of converters exposed, and notable is missing at
least the following as of
v1.8.12:
- add *
- and *
- div *
- field
- json
- mod *
- mul *
- or *
- regsub
- sub *
- word
- xor *

Some fetches are also omitted:
- bool
- bin
- meth
- map*
- var
- set-var
- unset-var
(there might be more, I didn't have a comprehensive list of converters &
fetches).

All of these have in common that they have a validation of arguments.
Those with a * gained validation of arguments in commit
5d86fae2344dbfacce5479ba86bd2d2866bf5474 (v1.6-dev2-52-g5d86fae23)

This bug has been around since the start of Lua fetchers & converters.

hlua_run_sample_conv is capable of running the args checker [3]:
```c
/* Run the special args checker. */
if (conv->val_args && !conv->val_args(args, conv, "", 0, NULL)) {
hlua_pusherror(L, "error in arguments");
WILL_LJMP(lua_error(L));
}
```

But any converters with arguments checking functions are not registered [4]:
```c
/* Dont register the keywork if the arguments check function are
* not safe during the runtime.
*/
if (sc->val_args != NULL)
continue;
```

Fetchers have a similar issue, but some checkers were explicitly permitted:
```c
/* Dont register the keywork if the arguments check function are
* not safe during the runtime.
*/
if ((sf->val_args != NULL) &&
(sf->val_args != val_payload_lv) &&
(sf->val_args != val_hdr))
continue;
```

Here's a Lua AppletHTTP [5] that lets you dump out which fetchers & converters are exposed:
```lua
core.register_service("dump", "http", function(applet)
local response = core.concat()
response:add('---\n')
response:add('applet:\n')
members = { 'sf', 'f', 'sc', 'c' }

for i,m in ipairs(members) do
response:add('applet.' .. m .. ':\n')
for k, v in pairs(getmetatable(applet[m])) do
if k == "__index" then
local funcs = {}
for i, j in pairs(v) do
table.insert(funcs, i)
end
table.sort(funcs)
for i,j in ipairs(funcs) do
response:add('- "' .. tostring(j) .. '"\n')
end
break
end
end
response:add('\n')
end
response = response:dump()
applet:set_status(200)
applet:add_header("content-length", string.len(response))
applet:add_header("content-type", "text/yaml")
applet:start_response()
applet:send(response)
end)
```

[1] https://www.arpalert.org/src/haproxy-lua-api/1.8/index.html#Converters
[2] https://github.com/haproxy/haproxy/blob/master/doc/lua.txt#L297-L307
[3] https://github.com/haproxy/haproxy/commit/594afe76e4694d9faf281ae87f2d026506f7a9d9#diff-fc1678dd7de891cf951a19f59a9a7375R2643
[4] https://github.com/haproxy/haproxy/commit/594afe76e4694d9faf281ae87f2d026506f7a9d9#diff-fc1678dd7de891cf951a19f59a9a7375R4003
[5] https://gist.github.com/robbat2/6c75f78e0d857b6d8649d591bc44c452

Initial-Discovery: Yue Zhu <***@digitalocean.com>
Tracing: Robin H. Johnson <***@digitalocean.com>
Signed-off-by: Robin H. Johnson <***@digitalocean.com>
Signed-off-by: Robin H. Johnson <***@gentoo.org>
---
src/hlua.c | 15 ---------------
1 file changed, 15 deletions(-)

diff --git a/src/hlua.c b/src/hlua.c
index 4715639a1..d575f31ea 100644
--- a/src/hlua.c
+++ b/src/hlua.c
@@ -7737,15 +7737,6 @@ void hlua_init(void)
*/
sf = NULL;
while ((sf = sample_fetch_getnext(sf, &idx)) != NULL) {
-
- /* Dont register the keywork if the arguments check function are
- * not safe during the runtime.
- */
- if ((sf->val_args != NULL) &&
- (sf->val_args != val_payload_lv) &&
- (sf->val_args != val_hdr))
- continue;
-
/* gL.Tua doesn't support '.' and '-' in the function names, replace it
* by an underscore.
*/
@@ -7785,12 +7776,6 @@ void hlua_init(void)
*/
sc = NULL;
while ((sc = sample_conv_getnext(sc, &idx)) != NULL) {
- /* Dont register the keywork if the arguments check function are
- * not safe during the runtime.
- */
- if (sc->val_args != NULL)
- continue;
-
/* gL.Tua doesn't support '.' and '-' in the function names, replace it
* by an underscore.
*/

[Willy Tarreau]

Hello,

Post by Robin H. Johnson
One of my coworkers was having some trouble trying to escape data for
JSON in Lua, using the 'json' converter, based on the documentation, and
this lead to a deep bug discovery.
The Lua documentation [1] states that JSON escaping converter is exposed
in Lua, but it turns out that's not quite true.
Creatively dumping the function metatable [2] (see code at the end)
shows only a subset of converters exposed, and notable is missing at
least the following as of

(...)

Post by Robin H. Johnson
All of these have in common that they have a validation of arguments.
Those with a * gained validation of arguments in commit
5d86fae2344dbfacce5479ba86bd2d2866bf5474 (v1.6-dev2-52-g5d86fae23)
This bug has been around since the start of Lua fetchers & converters.
```c
/* Run the special args checker. */
if (conv->val_args && !conv->val_args(args, conv, "", 0, NULL)) {
hlua_pusherror(L, "error in arguments");
WILL_LJMP(lua_error(L));
}
```
```c
/* Dont register the keywork if the arguments check function are
* not safe during the runtime.
*/
if (sc->val_args != NULL)
continue;
```
```c
/* Dont register the keywork if the arguments check function are
* not safe during the runtime.
*/
if ((sf->val_args != NULL) &&
(sf->val_args != val_payload_lv) &&
(sf->val_args != val_hdr))
continue;
```

Well, for me the reason here is clearly mentioned in the comments, which
is that only those explicitly whitelisted there are safe for use at runtime.
Maybe since then others became safe and should be added, but your patch
simply removes these tests and will lead to bad things happening at run
time for people using unsafe functions.

I had a quick look, some converters use check_operator() which creates
a variable upon each invocation of the parsing function. Some people
might inadvertently get caught by using these ones to look up cookie
values or session identifiers for example and not realize that zoombie
variables are lying behind. Another one is smp_check_const_bin() which
will call parse_binary(), returning an allocated memory block representing
the binary string, that nobody will free either. And for converters you
can see that all map-related functions use sample_load_map(), which will
attempt to load a file from the file system. Not only this must not be
attempted at run time for obvious performance reasons (will not work if
the config properly uses chroot anyway), but it may also use huge amounts
of memory on each call.

For the time being, I think that instead a solution could be to review
the various val_args functions to see which ones are safe and may be
whitelisted for use with Lua, and to comment at the top of these functions
that they have to remain safe for use at run time.

Thanks,
Willy