BUG/MEDIUM: stick-table: restrict key sizes to what fits in a buffer

Dmitri Kaslov reported an interesting bug in the stick-table key exchange
over peers: the few extra bytes of overhead and data storage are not
accounted when building the key to be transmitted, while a message must
fit into a single buffer. This means in practise that a stick-table key
as large as a buffer can theoretically crash the sender. But it's not
that trivial since such a key has to be made (which wasn't the case in
1.4 when this was designed, rules and converters to do this weren't that
rich). For strings it doesn't happen by accident though, but nowadays
it's possible to do it by tracking too large data or even applying an
expansion converter (b64dec, hex etc). For binary it's easier because
the whole block is always padded with zeros and the whole size is sent.

It has become easier to trigger with large buffers because it's now
possible to wait for a large body and track it as a large key. During
local tests, mostly applet issues were noticed, with the peers applet
not being able to serialize its message and looping back there trying
again. But with ASAN it's visible that a few bytes are overwritten at
the end of the buffer.

In order to address this, here we apply a limit to what will be
serialized. For binary keys, that cannot be trimmed, we simply refuse
to start with too large a key + data storage for the buffer size,
explaining what to do to fix it (hint: nobody sane uses 16kB keys).
For strings, they're simply trimmed to the room left once data are
subtracted from the buffer size.

This is sufficient to stop this theoretical problem without affecting
peers transfers during an upgrade (no protocol change), nor affecting
anyone with a realistic configuration.

Thanks to Dmitri Kaslov for this report with a reproducer.

This should be backported to all stable releases, along with these two
previous commits:

  MINOR: stick-table: provide a function to estimate on-wire data size
  MINOR: stick-table: calculate the on-wire size of each key

diff --git a/src/stick_table.c b/src/stick_table.c
index fa9bee4571587a18d50fb9f204f1445ee937a4e0..ca44cb4303aa346d5aea86934112e7652e17823e 100644 (file)
--- a/src/stick_table.c
+++ b/src/stick_table.c
@@ -191,8 +191,19 @@ void stksess_setkey(struct stktable *t, struct stksess *ts, struct stktable_key
        if (t->type != SMP_T_STR)
                memcpy(ts->key.key, key->key, t->key_size);
        else {
-               memcpy(ts->key.key, key->key, MIN(t->key_size - 1, key->key_len));
-               ts->key.key[MIN(t->key_size - 1, key->key_len)] = 0;
+               int key_len = key->key_len;
+
+               if (key_len > t->key_size - 1)
+                       key_len = t->key_size - 1;
+
+               if (key_len + t->proto_size > global.tune.bufsize)
+                       key_len = global.tune.bufsize - t->proto_size;
+
+               if (key_len < 0)
+                       key_len = 0;
+
+               memcpy(ts->key.key, key->key, key_len);
+               ts->key.key[key_len] = 0;
        }
 }
 
@@ -1687,6 +1698,13 @@ int parse_stick_table(const char *file, int linenum, char **args,
        if (t->type != SMP_T_STR)
                t->proto_size += t->key_size;
 
+       if (t->proto_size > global.tune.bufsize) {
+               ha_alert("parsing [%s:%d] : %s: too large key+data (%d bytes on wire) for bufsize (%u). Reduce key len, reduce stored data, or increase tune.bufsize.\n",
+                        file, linenum, args[0], t->proto_size, global.tune.bufsize);
+               err_code |= ERR_ALERT | ERR_FATAL;
+               goto out;
+       }
+
  out:
        return err_code;
 }