TLS: Generate the server random before processing ClientHello extensions

The server random was only generated after all of the ClientHello extensions had
been processed. In (D)TLSv1.3 the PSK callbacks are invoked while the
pre_shared_key extension is being parsed, so a call to SSL_get_server_random()
from within them returned all zeros. The TLSv1.2 psk_server_callback runs during
ClientKeyExchange processing and so already observed the real value, making the
behaviour inconsistent between protocol versions.

Move the generation of the server random to immediately after the protocol
version has been selected  and before any of the remaining ClientHello
processing. As a result the server random is also visible from the other
callbacks invoked during ClientHello processing, such as the session ticket and
servername callbacks.

Fixes openssl#26404

Assisted-by: Claude:claude-fable-5-1
Co-authored-by: Frederik Wedel-Heinen <frederik.wedel-heinen@dencrypt.dk>

Author: n13l

doc/man3/SSL_get_client_random.pod

@@ -74,6 +74,19 @@ Finally, though the "client_random" and "server_random" values are called
 "random", many TLS implementations will generate four bytes of those
 values based on their view of the current time.
 
+On the server side the server_random value is generated as soon as the
+protocol version has been selected during ClientHello processing. It is
+therefore already available to callbacks invoked later during that
+processing, such as the servername callback, the session ticket callbacks
+and the PSK callbacks (see L<SSL_CTX_set_psk_find_session_callback(3)> and
+L<SSL_CTX_set_psk_server_callback(3)>), including in (D)TLSv1.3. Note that if a
+HelloRetryRequest is sent a fresh server_random is generated when the second
+ClientHello is processed. When Encrypted ClientHello (ECH) is accepted, the
+final eight bytes of the server_random are replaced with the ECH acceptance
+confirmation while the ServerHello is being constructed, so a value observed
+from a callback before that point will differ in those bytes from the value
+sent to the client. On the client side the value is available once the
+ServerHello has been received.
 
 =head1 RETURN VALUES
 

ssl/statem/statem_srvr.c

@@ -2085,6 +2085,7 @@ static int tls_early_post_process_client_hello(SSL_CONNECTION *s)
     SSL_CTX *sctx = SSL_CONNECTION_GET_CTX(s);
     SSL *ssl = SSL_CONNECTION_GET_SSL(s);
     SSL *ussl = SSL_CONNECTION_GET_USER_SSL(s);
+    unsigned char *snonce = s->s3.server_random;
 
     /* Finished parsing the ClientHello, now we can start processing it */
     /* Give the ClientHello callback a crack at things */
@@ -2161,6 +2162,21 @@ static int tls_early_post_process_client_hello(SSL_CONNECTION *s)
         }
     }
 
+    /*
+     * Generate the server_random now that the protocol version (and hence any
+     * downgrade sentinel) is known, but before any of the remaining ClientHello
+     * processing. This ensures that callbacks invoked from here on such as
+     * the session ticket callbacks, the servername callback, the (D)TLSv1.3
+     * PSK callbacks and tls_session_secret_cb can observe the value via
+     * SSL_get_server_random(). Note that in the TLSv1.2 flow
+     * tls_session_secret_cb in particular needs it for SessionTicket key
+     * derivation.
+     */
+    if (ssl_fill_hello_random(s, 1, snonce, SSL3_RANDOM_SIZE, dgrd) <= 0) {
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+        goto err;
+    }
+
     s->hit = 0;
 
     if (!ssl_cache_cipherlist(s, &clienthello->ciphersuites)
@@ -2330,21 +2346,6 @@ static int tls_early_post_process_client_hello(SSL_CONNECTION *s)
         goto err;
     }
 
-    /*
-     * Check if we want to use external pre-shared secret for this handshake
-     * for not reused session only. We need to generate server_random before
-     * calling tls_session_secret_cb in order to allow SessionTicket
-     * processing to use it in key derivation.
-     */
-    {
-        unsigned char *pos;
-        pos = s->s3.server_random;
-        if (ssl_fill_hello_random(s, 1, pos, SSL3_RANDOM_SIZE, dgrd) <= 0) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
-            goto err;
-        }
-    }
-
     if (!s->hit && !tls1_set_server_sigalgs(s)) {
         /* SSLfatal() already called */
         goto err;
@@ -2809,7 +2810,7 @@ CON_FUNC_RETURN tls_construct_server_hello(SSL_CONNECTION *s, WPACKET *pkt)
     if (!WPACKET_put_bytes_u16(pkt, version)
         /*
          * Random stuff. Filling of the server_random takes place in
-         * tls_process_client_hello()
+         * tls_early_post_process_client_hello()
          */
         || !WPACKET_memcpy(pkt,
             s->hello_retry_request == SSL_HRR_PENDING

test/quicapitest.c

@@ -1336,11 +1336,24 @@ static int use_session_cb(SSL *ssl, const EVP_MD *md, const unsigned char **id,
     return 1;
 }
 
+/*
+ * The server random as observed from within find_session_cb(). Prior to the
+ * fix for issue #26404 the server random was generated only after the
+ * ClientHello extensions had been processed, so a PSK callback observed an
+ * all-zero value.
+ */
+static unsigned char cb_server_random[SSL3_RANDOM_SIZE];
+static size_t cb_server_random_len;
+
 static int find_session_cb(SSL *ssl, const unsigned char *identity,
     size_t identity_len, SSL_SESSION **sess)
 {
     find_session_cb_cnt++;
 
+    memset(cb_server_random, 0, sizeof(cb_server_random));
+    cb_server_random_len = SSL_get_server_random(ssl, cb_server_random,
+        sizeof(cb_server_random));
+
     if (serverpsk == NULL || !SSL_SESSION_up_ref(serverpsk))
         return 0;
 
@@ -1361,6 +1374,10 @@ static int test_quic_psk(void)
     SSL_CTX *cctx = SSL_CTX_new_ex(libctx, NULL, OSSL_QUIC_client_method());
     SSL *clientquic = NULL;
     QUIC_TSERVER *qtserv = NULL;
+    SSL *stream = NULL;
+    unsigned char zeros[SSL3_RANDOM_SIZE] = { 0 };
+    unsigned char server_random[SSL3_RANDOM_SIZE];
+    unsigned char client_random[SSL3_RANDOM_SIZE];
     int testresult = 0;
 
     if (!TEST_ptr(cctx)
@@ -1387,9 +1404,39 @@ static int test_quic_psk(void)
         || !TEST_true(SSL_session_reused(clientquic)))
         goto end;
 
+    /*
+     * The server PSK callback must have observed the server random actually
+     * used in the handshake. The client side SSL object is a QUIC connection
+     * object, on which SSL_get_server_random() and SSL_get_client_random()
+     * report the values from the underlying TLS handshake.
+     */
+    if (!TEST_size_t_eq(cb_server_random_len, SSL3_RANDOM_SIZE)
+        || !TEST_mem_ne(cb_server_random, sizeof(cb_server_random),
+            zeros, sizeof(zeros))
+        || !TEST_size_t_eq(SSL_get_server_random(clientquic, server_random,
+                               sizeof(server_random)),
+            SSL3_RANDOM_SIZE)
+        || !TEST_mem_eq(cb_server_random, sizeof(cb_server_random),
+            server_random, sizeof(server_random))
+        || !TEST_size_t_eq(SSL_get_client_random(clientquic, client_random,
+                               sizeof(client_random)),
+            SSL3_RANDOM_SIZE))
+        goto end;
+
+    /* On a QUIC stream object the getters are not applicable and return 0 */
+    if (!TEST_ptr(stream = SSL_new_stream(clientquic, 0))
+        || !TEST_size_t_eq(SSL_get_server_random(stream, server_random,
+                               sizeof(server_random)),
+            0)
+        || !TEST_size_t_eq(SSL_get_client_random(stream, client_random,
+                               sizeof(client_random)),
+            0))
+        goto end;
+
     testresult = 1;
 
 end:
+    SSL_free(stream);
     SSL_free(clientquic);
     ossl_quic_tserver_free(qtserv);
     SSL_CTX_free(cctx);