BUG/MEDIUM: connection/http-reuse: fix address collision on unhandled address families

As described in GH #2765, there were situations where http connections
would be re-used for requests to different endpoints, which is obviously
unexpected. In GH #2765, this occured with httpclient and UNIX socket
combination, but later code analysis revealed that while disabling http
reuse on httpclient proxy helped, it didn't fix the underlying issue since
it was found that conn_calculate_hash_sockaddr() didn't take into account
families such as AF_UNIX or AF_CUST_SOCKPAIR, and because of that the
sock_addr part of the connection wasn't hashed.

To properly fix the issue, let's explicly handle UNIX (both regular and
ABNS) and AF_CUST_SOCKPAIR families, so that the destination address is
properly hashed. To prevent this bug from re-appearing: when the family
isn't known, instead of doing nothing like before, let's fall back to a
generic (unoptimal) hashing which hashes the whole sockaddr_storage struct

As a workaround, http-reuse may be disabled on impacted proxies.
(unfortunately this doesn't help for httpclient since reuse policy
defaults to safe and cannot be modified from the config)

It should be backported to all stable versions.

Shout out to @christopherhibbert for having reported the issue and
provided a trivial reproducer.

[ada: prior to 3.0, ctx adjt is required because conn_hash_update()'s
 prototype is slightly different]

(cherry picked from commit b5b40a9843e505ed84153327ab897ca0e8d9a571)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

diff --git a/src/connection.c b/src/connection.c
index 3fedad9..a3c6a01 100644 (file)
--- a/src/connection.c
+++ b/src/connection.c
@@ -2656,6 +2656,7 @@ static void conn_calculate_hash_sockaddr(const struct sockaddr_storage *ss,
 {
        struct sockaddr_in *addr;
        struct sockaddr_in6 *addr6;
+       struct sockaddr_un *un;
 
        switch (ss->ss_family) {
        case AF_INET:
@@ -2687,6 +2688,42 @@ static void conn_calculate_hash_sockaddr(const struct sockaddr_storage *ss,
                }
 
                break;
+
+       case AF_UNIX:
+               un = (struct sockaddr_un *)ss;
+
+               if (un->sun_path[0]) {
+                       /* regular UNIX socket */
+                       conn_hash_update(hash,
+                                        &un->sun_path, strlen(un->sun_path),
+                                        hash_flags, param_type_addr);
+               } else {
+                       /* ABNS UNIX socket */
+                       conn_hash_update(hash,
+                                        &un->sun_path, sizeof(un->sun_path),
+                                        hash_flags, param_type_addr);
+               }
+               break;
+
+       case AF_CUST_SOCKPAIR:
+               /* simply hash the fd */
+               conn_hash_update(hash,
+                                &((struct sockaddr_in *)ss)->sin_addr.s_addr,
+                                sizeof(int),
+                                hash_flags, param_type_addr);
+               break;
+
+       default:
+               /* We don't know how to specifically handle this address family.
+                * yet we cannot afford to have 2 sockaddr_storage structs
+                * colliding with different parameters. So instead of risking
+                * hash collision (which would result in HTTP reuse issues),
+                * let's switch to generic sockaddr storage hashing (unoptimal
+                * as it is slower and may not be able to group similar
+                * addresses if they're not byte to byte equivalents).
+                */
+               conn_hash_update(hash, ss, sizeof(*ss), hash_flags, param_type_addr);
+               break;
        }
 }