From: Aurelien DARRAGON <adarragon@haproxy.com>
Date: Tue, 24 Dec 2024 15:28:35 +0000 (+0100)
Subject: BUG/MINOR: stktable: invalid use of stkctr_set_entry() with mixed table types
X-Git-Tag: v3.2-dev3~25
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5bbdd14f56a2a672e8fef4449e4143b0f0642812;p=thirdparty%2Fhaproxy.git

BUG/MINOR: stktable: invalid use of stkctr_set_entry() with mixed table types

Some actions such as "sc0_get_gpc0" (using smp_fetch_sc_stkctr()
internally) can take an optional table name as parameter to perform the
lookup on a different table from the tracked one but using the key from
the tracked entry. It is done by leveraging the stktable_lookup() function
which was originally meant to perform intra-table lookups.

Calling sc0_get_gpc0() with a different table name will result in
stktable_lookup() being called to perform lookup using a stktsess from
a different table. While it is theorically fine, it comes with a pitfall:
both tables (the one from where the stktsess originates and the actual
target table) should rely on the exact same key type and length.

Failure to do so actually results in undefined behavior, because the key
type and/or length from one table is used to perform the lookup in
another table, while the underlying lookup API expects explicit type and
key length.

For instance, consider the below example:

  peers testpeers
    bind 127.0.0.1:10001
    server localhost

    table test type binary len 1 size 100k expire 1h store gpc0
    table test2 type string size 100k expire 1h store gpc0

  listen test_px
    mode http
    bind 0.0.0.0:8080
    http-request track-sc0 bin(AA) table testpeers/test
    http-request track-sc1 str(ok) table testpeers/test2
    log-format "%[sc0_get_gpc0(testpeers/test2)]"
    log stdout format raw local0

    server s1 git.haproxy.org:80

Performing a curl request to localhost:8080 will cause unitialized reads
because string "ok" from test2 table will be compared as a string against
"AA" binary sample which is not NULL terminated:

==2450742== Conditional jump or move depends on uninitialised value(s)
==2450742==    at 0x484F238: strlen (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2450742==    by 0x27BCE6: stktable_lookup (stick_table.c:539)
==2450742==    by 0x281470: smp_fetch_sc_stkctr (stick_table.c:3580)
==2450742==    by 0x283083: smp_fetch_sc_get_gpc0 (stick_table.c:3788)
==2450742==    by 0x2A805C: sample_process (sample.c:1376)

So let's prevent that by adding some comments in stktable_set_entry()
func description, and by adding a check in smp_fetch_sc_stkctr() to ensure
both source stksess and target table share the same key properties.

While it could be relevant to backport this in all stable versions, it is
probably safer to wait for some time before doing so, to ensure that no
existing configs rely on this ambiguity because the fact that the target
table and source stksess entry need to share the same key type and length
is not explicitly documented.
---

diff --git a/src/stick_table.c b/src/stick_table.c
index 114fa46345..792a8da9e2 100644
--- a/src/stick_table.c
+++ b/src/stick_table.c
@@ -507,6 +507,9 @@ struct stksess *stktable_lookup_ptr(struct stktable *t, void *ptr)
 /*
  * Looks in table <t> for a sticky session with same key as <ts>.
  * Returns pointer on requested sticky session or NULL if none was found.
+ *
+ * <ts> must originate from a table with same key type and length than <t>,
+ * else it is undefined behavior.
  */
 struct stksess *__stktable_lookup(struct stktable *t, struct stksess *ts, uint shard)
 {
@@ -528,6 +531,9 @@ struct stksess *__stktable_lookup(struct stktable *t, struct stksess *ts, uint s
  * Returns pointer on requested sticky session or NULL if none was found.
  * The refcount of the found entry is increased and this function
  * is protected using the table lock
+ *
+ * <ts> must originate from a table with same key type and length than <t>,
+ * else it is undefined behavior.
  */
 struct stksess *stktable_lookup(struct stktable *t, struct stksess *ts)
 {
@@ -3575,7 +3581,12 @@ smp_fetch_sc_stkctr(struct session *sess, struct stream *strm, const struct arg
 		return NULL;
 
 	if (unlikely(args[arg].type == ARGT_TAB)) {
-		/* an alternate table was specified, let's look up the same key there */
+		/* an alternate table was specified, let's look up the same key there
+		 * unless the table key type or length differs from the tracked one
+		 */
+		if (args[arg].data.t->type != stkptr->table->type ||
+		    args[arg].data.t->key_size != stkptr->table->key_size)
+			return NULL;
 		stkctr->table = args[arg].data.t;
 		stkctr_set_entry(stkctr, stktable_lookup(stkctr->table, stksess));
 		return stkctr;