BUG/MINOR: quic: Wrong cluster secret initialization

The function generate_random_cluster_secret() which initializes the cluster secret
when not supplied by configuration is buggy. There 1/256 that the cluster secret
string is empty.

To fix this, one stores the cluster as a reduced size first 128 bits of its own
SHA1 (160 bits) digest, if defined by configuration. If this is not the case, it
is initialized with a 128 bits random value. Furthermore, thus the cluster secret
is always initialized.

As the cluster secret is always initialized, there are several tests which
are for now on useless. This patch removes such tests (if(global.cluster_secret))
in the QUIC code part and at parsing time: no need to check that a cluster
secret was initialized with "quic-force-retry" option.

Must be backported as far as 2.6.

diff --git a/include/haproxy/global-t.h b/include/haproxy/global-t.h
index 3f025c59cbe62c26624c1c8d6614ba94a2e4584d..e9c0f2d9f87af4c438b8b222e7624b77fe03e81d 100644 (file)
--- a/include/haproxy/global-t.h
+++ b/include/haproxy/global-t.h
@@ -85,6 +85,8 @@
 #define GTUNE_LISTENER_MQ_OPT    (1<<28)
 #define GTUNE_LISTENER_MQ_ANY    (GTUNE_LISTENER_MQ_FAIR | GTUNE_LISTENER_MQ_OPT)
 
+extern int cluster_secret_isset; /* non zero means a cluster secret was initiliazed */
+
 /* SSL server verify mode */
 enum {
        SSL_SERVER_VERIFY_NONE = 0,
@@ -139,7 +141,7 @@ struct global {
        char *log_send_hostname;   /* set hostname in syslog header */
        char *server_state_base;   /* path to a directory where server state files can be found */
        char *server_state_file;   /* path to the file where server states are loaded from */
-       char *cluster_secret;      /* Secret defined as ASCII string */
+       unsigned char cluster_secret[16]; /* 128 bits of an SHA1 digest of a secret defined as ASCII string */
        struct {
                int maxpollevents; /* max number of poll events at once */
                int maxaccept;     /* max number of consecutive accept() */

diff --git a/src/cfgparse-global.c b/src/cfgparse-global.c
index 23f83f89dba8aa135c64963366dc26a9d5c68905..dce56e616eab74c0f262ee0f80c7c6ee08983c2f 100644 (file)
--- a/src/cfgparse-global.c
+++ b/src/cfgparse-global.c
@@ -11,6 +11,8 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include <import/sha1.h>
+
 #include <haproxy/buf.h>
 #include <haproxy/cfgparse.h>
 #ifdef USE_CPU_AFFINITY
@@ -23,6 +25,8 @@
 #include <haproxy/protocol.h>
 #include <haproxy/tools.h>
 
+int cluster_secret_isset;
+
 /* some keywords that are still being parsed using strcmp() and are not
  * registered anywhere. They are used as suggestions for mistyped words.
  */
@@ -514,6 +518,9 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm)
                global.tune.options &= GTUNE_USE_FAST_FWD;
        }
        else if (strcmp(args[0], "cluster-secret") == 0) {
+               blk_SHA_CTX sha1_ctx;
+               unsigned char sha1_out[20];
+
                if (alertif_too_many_args(1, file, linenum, args, &err_code))
                        goto out;
                if (*args[1] == 0) {
@@ -521,13 +528,18 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm)
                        err_code |= ERR_ALERT | ERR_FATAL;
                        goto out;
                }
-               if (global.cluster_secret != NULL) {
+               if (cluster_secret_isset) {
                        ha_alert("parsing [%s:%d] : '%s' already specified. Continuing.\n", file, linenum, args[0]);
                        err_code |= ERR_ALERT;
                        goto out;
                }
-               ha_free(&global.cluster_secret);
-               global.cluster_secret = strdup(args[1]);
+
+               blk_SHA1_Init(&sha1_ctx);
+               blk_SHA1_Update(&sha1_ctx, args[1], strlen(args[1]));
+               blk_SHA1_Final(sha1_out, &sha1_ctx);
+               BUG_ON(sizeof sha1_out < sizeof global.cluster_secret);
+               memcpy(global.cluster_secret, sha1_out, sizeof global.cluster_secret);
+               cluster_secret_isset = 1;
        }
        else if (strcmp(args[0], "uid") == 0) {
                if (alertif_too_many_args(1, file, linenum, args, &err_code))

diff --git a/src/cfgparse.c b/src/cfgparse.c
index 04da22bfaea00bf3d540c2ae65f4aafe8b7608c7..dd2a928ab953268de7e4ac686ebc0f80628943b3 100644 (file)
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -2803,7 +2803,6 @@ int check_config_validity()
        struct cfg_postparser *postparser;
        struct resolvers *curr_resolvers = NULL;
        int i;
-       int diag_no_cluster_secret = 0;
 
        bind_conf = NULL;
        /*
@@ -4318,13 +4317,6 @@ init_proxies_list_stage2:
 
 #ifdef USE_QUIC
                        if (listener->bind_conf->xprt == xprt_get(XPRT_QUIC)) {
-                               if (!global.cluster_secret) {
-                                       diag_no_cluster_secret = 1;
-                                       if (listener->bind_conf->options & BC_O_QUIC_FORCE_RETRY) {
-                                               ha_alert("QUIC listener with quic-force-retry requires global cluster-secret to be set.\n");
-                                               cfgerr++;
-                                       }
-                               }
 # ifdef USE_QUIC_OPENSSL_COMPAT
                                /* store the last checked bind_conf in bind_conf */
                                if (!(global.tune.options & GTUNE_NO_QUIC) &&
@@ -4375,12 +4367,6 @@ init_proxies_list_stage2:
                        goto init_proxies_list_stage2;
        }
 
-       if (diag_no_cluster_secret) {
-               ha_diag_warning("Generating a random cluster secret. "
-                               "You should define your own one in the configuration to ensure consistency "
-                               "after reload/restart or across your whole cluster.\n");
-       }
-
        /*
         * Recount currently required checks.
         */

diff --git a/src/haproxy.c b/src/haproxy.c
index 1730545ef6735b2221db64e7962c1458d10a2a7d..1b5be885721321ba3b6dcc30141cf23f58751ad4 100644 (file)
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -1925,17 +1925,16 @@ static void dump_registered_keywords(void)
 static void generate_random_cluster_secret()
 {
        /* used as a default random cluster-secret if none defined. */
-       uint64_t rand = ha_random64();
+       uint64_t rand;
 
        /* The caller must not overwrite an already defined secret. */
-       BUG_ON(global.cluster_secret);
-
-       global.cluster_secret = malloc(8);
-       if (!global.cluster_secret)
-               return;
+       BUG_ON(cluster_secret_isset);
 
+       rand = ha_random64();
        memcpy(global.cluster_secret, &rand, sizeof(rand));
-       global.cluster_secret[7] = '\0';
+       rand = ha_random64();
+       memcpy(global.cluster_secret + sizeof(rand), &rand, sizeof(rand));
+       cluster_secret_isset = 1;
 }
 
 /*
@@ -2657,7 +2656,7 @@ static void init(int argc, char **argv)
                exit(1);
        }
 
-       if (!global.cluster_secret)
+       if (!cluster_secret_isset)
                generate_random_cluster_secret();
 
        /*
@@ -2850,7 +2849,6 @@ void deinit(void)
        ha_free(&global.log_send_hostname);
        chunk_destroy(&global.log_tag);
        ha_free(&global.chroot);
-       ha_free(&global.cluster_secret);
        ha_free(&global.pidfile);
        ha_free(&global.node);
        ha_free(&global.desc);

diff --git a/src/quic_conn.c b/src/quic_conn.c
index be65c2e2058f6b5fda74a213f8f02bfcfdf35366..46776b6037226812f948574fa3ff69a2a1b5c8ba 100644 (file)
--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -477,8 +477,8 @@ int quic_stateless_reset_token_cpy(unsigned char *pos, size_t len,
                                    const unsigned char *salt, size_t saltlen)
 {
        /* Input secret */
-       const unsigned char *key = (const unsigned char *)global.cluster_secret;
-       size_t keylen = strlen(global.cluster_secret);
+       const unsigned char *key = global.cluster_secret;
+       size_t keylen = sizeof global.cluster_secret;
        /* Info */
        const unsigned char label[] = "stateless token";
        size_t labellen = sizeof label - 1;
@@ -494,25 +494,14 @@ int quic_stateless_reset_token_cpy(unsigned char *pos, size_t len,
  */
 static int quic_stateless_reset_token_init(struct quic_connection_id *conn_id)
 {
-       int ret;
-
-       if (global.cluster_secret) {
-               /* Output secret */
-               unsigned char *token = conn_id->stateless_reset_token;
-               size_t tokenlen = sizeof conn_id->stateless_reset_token;
-               /* Salt */
-               const unsigned char *cid = conn_id->cid.data;
-               size_t cidlen = conn_id->cid.len;
-
-               ret = quic_stateless_reset_token_cpy(token, tokenlen, cid, cidlen);
-       }
-       else {
-               /* TODO: RAND_bytes() should be replaced */
-               ret = RAND_bytes(conn_id->stateless_reset_token,
-                                sizeof conn_id->stateless_reset_token) == 1;
-       }
-
-       return ret;
+       /* Output secret */
+       unsigned char *token = conn_id->stateless_reset_token;
+       size_t tokenlen = sizeof conn_id->stateless_reset_token;
+       /* Salt */
+       const unsigned char *cid = conn_id->cid.data;
+       size_t cidlen = conn_id->cid.len;
+
+       return quic_stateless_reset_token_cpy(token, tokenlen, cid, cidlen);
 }
 
 /* Generate a CID directly derived from <orig> CID and <addr> address.

diff --git a/src/quic_rx.c b/src/quic_rx.c
index 3892b11ea368fcc5e9b08115fa9b42357696f986..39271a0fd41d287be981ef9c75b1efa1fd05f9bb 100644 (file)
--- a/src/quic_rx.c
+++ b/src/quic_rx.c
@@ -1722,8 +1722,8 @@ static int quic_retry_token_check(struct quic_rx_packet *pkt,
        const unsigned char *salt;
        unsigned char key[QUIC_TLS_KEY_LEN];
        unsigned char iv[QUIC_TLS_IV_LEN];
-       const unsigned char *sec = (const unsigned char *)global.cluster_secret;
-       size_t seclen = strlen(global.cluster_secret);
+       const unsigned char *sec = global.cluster_secret;
+       size_t seclen = sizeof global.cluster_secret;
        EVP_CIPHER_CTX *ctx = NULL;
        const EVP_CIPHER *aead = EVP_aes_128_gcm();
        const struct quic_version *qv = qc ? qc->original_version :
@@ -1732,7 +1732,7 @@ static int quic_retry_token_check(struct quic_rx_packet *pkt,
        TRACE_ENTER(QUIC_EV_CONN_LPKT, qc);
 
        /* The caller must ensure this. */
-       BUG_ON(!global.cluster_secret || !pkt->token_len);
+       BUG_ON(!pkt->token_len);
 
        prx = l->bind_conf->frontend;
        prx_counters = EXTRA_COUNTERS_GET(prx->extra_counters_fe, &quic_stats_module);
@@ -1906,7 +1906,7 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
        if (pkt->type == QUIC_PACKET_TYPE_INITIAL) {
                BUG_ON(!pkt->version); /* This must not happen. */
 
-               if (global.cluster_secret && pkt->token_len) {
+               if (pkt->token_len) {
                        if (!quic_retry_token_check(pkt, dgram, l, qc, &token_odcid))
                                goto err;
                }
@@ -1917,7 +1917,7 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
                        struct quic_connection_id *conn_id;
                        int ipv4;
 
-                       if (global.cluster_secret && !pkt->token_len && !(l->bind_conf->options & BC_O_QUIC_FORCE_RETRY) &&
+                       if (!pkt->token_len && !(l->bind_conf->options & BC_O_QUIC_FORCE_RETRY) &&
                            HA_ATOMIC_LOAD(&prx_counters->half_open_conn) >= global.tune.quic_retry_threshold) {
                                TRACE_PROTO("Initial without token, sending retry",
                                            QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
@@ -1994,7 +1994,7 @@ static struct quic_conn *quic_rx_pkt_retrieve_conn(struct quic_rx_packet *pkt,
        }
        else if (!qc) {
                TRACE_PROTO("RX non Initial pkt without connection", QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
-               if (global.cluster_secret && !send_stateless_reset(l, &dgram->saddr, pkt))
+               if (!send_stateless_reset(l, &dgram->saddr, pkt))
                        TRACE_ERROR("stateless reset not sent", QUIC_EV_CONN_LPKT, qc);
                goto err;
        }
@@ -2117,7 +2117,7 @@ static int quic_rx_pkt_parse(struct quic_rx_packet *pkt,
                        /* TODO Retry should be automatically activated if
                         * suspect network usage is detected.
                         */
-                       if (global.cluster_secret && !token_len) {
+                       if (!token_len) {
                                if (l->bind_conf->options & BC_O_QUIC_FORCE_RETRY) {
                                        TRACE_PROTO("Initial without token, sending retry",
                                                    QUIC_EV_CONN_LPKT, NULL, NULL, NULL, pkt->version);
@@ -2131,14 +2131,6 @@ static int quic_rx_pkt_parse(struct quic_rx_packet *pkt,
                                        goto drop_silent;
                                }
                        }
-                       else if (!global.cluster_secret && token_len) {
-                               /* Impossible case: a token was received without configured
-                                * cluster secret.
-                                */
-                               TRACE_PROTO("Packet dropped", QUIC_EV_CONN_LPKT,
-                                           NULL, NULL, NULL, pkt->version);
-                               goto drop;
-                       }
 
                        pkt->token = pos;
                        pkt->token_len = token_len;

diff --git a/src/quic_tx.c b/src/quic_tx.c
index 22ec33468fc88dd2d91713284ea633ec08ba01e1..e63b44a248183432ea00211e8308218ddf977f6e 100644 (file)
--- a/src/quic_tx.c
+++ b/src/quic_tx.c
@@ -1610,8 +1610,8 @@ static int quic_generate_retry_token(unsigned char *token, size_t len,
        unsigned char salt[QUIC_RETRY_TOKEN_SALTLEN];
        unsigned char key[QUIC_TLS_KEY_LEN];
        unsigned char iv[QUIC_TLS_IV_LEN];
-       const unsigned char *sec = (const unsigned char *)global.cluster_secret;
-       size_t seclen = strlen(global.cluster_secret);
+       const unsigned char *sec = global.cluster_secret;
+       size_t seclen = sizeof global.cluster_secret;
        EVP_CIPHER_CTX *ctx = NULL;
        const EVP_CIPHER *aead = EVP_aes_128_gcm();
        uint32_t timestamp = (uint32_t)date.tv_sec;