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]

diff --git a/src/connection.c b/src/connection.c
index 668e626f5ec4a74f7a3155f438da29b1282a118f..2e766c896c4ad3a9822fc7848af9cd350038bcda 100644 (file)
--- a/src/connection.c
+++ b/src/connection.c
@@ -2644,6 +2644,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:
@@ -2675,6 +2676,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;
        }
 }