Implement RSA key exchange in constant time.

RSA key exchange is known to have multiple security weaknesses,
including being potentially susceptible to padding oracle and timing
attacks.

The RSA key exchange code that we inherited from OpenSSL was riddled
with timing leaks, many of which we fixed (or minimised) early on.
However, a number of issues still remained, particularly those
related to libcrypto's RSA decryption and padding checks.

Rework the RSA key exchange code such that we decrypt with
RSA_NO_PADDING and then check the padding ourselves in constant
time. In this case, the pre-master secret is of a known length,
hence the padding is also a known length based on the size of the
RSA key. This makes it easy to implement a check that is much safer
than having RSA_private_decrypt() depad for us.

Regardless, we still strongly recommend disabling RSA key exchange
and using other key exchange methods that provide perfect forward
secrecy and do not depend on client generated keys.

Thanks to Marcel Maehren, Nurullah Erinola, Robert Merget, Juraj
Somorovsky, Joerg Schwenk and Hubert Kario for raising these issues
with us at various points in time.

ok tb@

3 files changed, 73 insertions, 63 deletions

diff --git a/src/lib/libssl/Makefile b/src/lib/libssl/Makefile
index 38e5ba30e0..a2b710922d 100644
--- a/src/lib/libssl/Makefile
+++ b/src/lib/libssl/Makefile
@@ -1,4 +1,4 @@
-# $OpenBSD: Makefile,v 1.81 2023/11/22 15:55:28 tb Exp $
+# $OpenBSD: Makefile,v 1.82 2024/06/25 14:10:45 jsing Exp $
 
 .include <bsd.own.mk>
 .ifndef NOMAN
@@ -23,6 +23,7 @@ CFLAGS+= -DLIBRESSL_NAMESPACE
 CFLAGS+= -DTLS13_DEBUG
 .endif
 CFLAGS+= -I${.CURDIR}
+CFLAGS+= -I${.CURDIR}/../libcrypto
 CFLAGS+= -I${.CURDIR}/../libcrypto/hidden
 CFLAGS+= -I${.CURDIR}/../libcrypto/bio
 CFLAGS+= -I${.CURDIR}/hidden
diff --git a/src/lib/libssl/ssl_local.h b/src/lib/libssl/ssl_local.h
index eeef569fa9..db102212a8 100644
--- a/src/lib/libssl/ssl_local.h
+++ b/src/lib/libssl/ssl_local.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_local.h,v 1.16 2024/05/19 07:12:50 jsg Exp $ */
+/* $OpenBSD: ssl_local.h,v 1.17 2024/06/25 14:10:45 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -167,8 +167,10 @@
 
 __BEGIN_HIDDEN_DECLS
 
+#ifndef CTASSERT
 #define CTASSERT(x)     extern char  _ctassert[(x) ? 1 : -1 ]   \
                             __attribute__((__unused__))
+#endif
 
 #ifndef LIBRESSL_HAS_DTLS1_2
 #define LIBRESSL_HAS_DTLS1_2
diff --git a/src/lib/libssl/ssl_srvr.c b/src/lib/libssl/ssl_srvr.c
index 6d61a4e4fa..e9f14dc610 100644
--- a/src/lib/libssl/ssl_srvr.c
+++ b/src/lib/libssl/ssl_srvr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_srvr.c,v 1.160 2024/02/03 17:39:17 tb Exp $ */
+/* $OpenBSD: ssl_srvr.c,v 1.161 2024/06/25 14:10:45 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -163,6 +163,7 @@
 #include <openssl/x509.h>
 
 #include "bytestring.h"
+#include "crypto_internal.h"
 #include "dtls_local.h"
 #include "ssl_local.h"
 #include "ssl_sigalgs.h"
@@ -1621,98 +1622,104 @@ ssl3_send_certificate_request(SSL *s)
 static int
 ssl3_get_client_kex_rsa(SSL *s, CBS *cbs)
 {
-        unsigned char fakekey[SSL_MAX_MASTER_KEY_LENGTH];
-        unsigned char *pms = NULL;
-        unsigned char *p;
+        uint8_t fakepms[SSL_MAX_MASTER_KEY_LENGTH];
+        uint8_t *pms = NULL;
         size_t pms_len = 0;
+        size_t pad_len;
         EVP_PKEY *pkey = NULL;
         RSA *rsa = NULL;
         CBS enc_pms;
         int decrypt_len;
-        int al = -1;
+        uint8_t mask;
+        size_t i;
+        int valid = 1;
+        int ret = 0;
+
+        /*
+         * Handle key exchange in the form of an RSA-Encrypted Premaster Secret
+         * Message. See RFC 5246, section 7.4.7.1.
+         */
 
-        arc4random_buf(fakekey, sizeof(fakekey));
+        arc4random_buf(fakepms, sizeof(fakepms));
 
-        fakekey[0] = s->s3->hs.peer_legacy_version >> 8;
-        fakekey[1] = s->s3->hs.peer_legacy_version & 0xff;
+        fakepms[0] = s->s3->hs.peer_legacy_version >> 8;
+        fakepms[1] = s->s3->hs.peer_legacy_version & 0xff;
 
         pkey = s->cert->pkeys[SSL_PKEY_RSA].privatekey;
         if (pkey == NULL || (rsa = EVP_PKEY_get0_RSA(pkey)) == NULL) {
-                al = SSL_AD_HANDSHAKE_FAILURE;
                 SSLerror(s, SSL_R_MISSING_RSA_CERTIFICATE);
-                goto fatal_err;
+                ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
+                goto err;
         }
 
+        /*
+         * The minimum size of an encrypted premaster secret is 11 bytes of
+         * padding (00 02 <8 or more non-zero bytes> 00) (RFC 8017, section
+         * 9.2) and 48 bytes of premaster secret (RFC 5246, section 7.4.7.1).
+         * This means an RSA key size of at least 472 bits.
+         */
         pms_len = RSA_size(rsa);
-        if (pms_len < SSL_MAX_MASTER_KEY_LENGTH)
-                goto err;
-        if ((pms = malloc(pms_len)) == NULL)
+        if (pms_len < 11 + SSL_MAX_MASTER_KEY_LENGTH) {
+                SSLerror(s, SSL_R_DECRYPTION_FAILED);
+                ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECRYPT_ERROR);
                 goto err;
-        p = pms;
+        }
+        pad_len = pms_len - SSL_MAX_MASTER_KEY_LENGTH;
 
-        if (!CBS_get_u16_length_prefixed(cbs, &enc_pms))
-                goto decode_err;
-        if (CBS_len(cbs) != 0 || CBS_len(&enc_pms) != RSA_size(rsa)) {
+        if (!CBS_get_u16_length_prefixed(cbs, &enc_pms)) {
+                SSLerror(s, SSL_R_BAD_PACKET_LENGTH);
+                ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+                goto err;
+        }
+        if (CBS_len(&enc_pms) != pms_len || CBS_len(cbs) != 0) {
                 SSLerror(s, SSL_R_TLS_RSA_ENCRYPTED_VALUE_LENGTH_IS_WRONG);
+                ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
                 goto err;
         }
 
-        decrypt_len = RSA_private_decrypt(CBS_len(&enc_pms), CBS_data(&enc_pms),
-            pms, rsa, RSA_PKCS1_PADDING);
+        if ((pms = calloc(1, pms_len)) == NULL)
+                goto err;
 
-        ERR_clear_error();
+        decrypt_len = RSA_private_decrypt(CBS_len(&enc_pms), CBS_data(&enc_pms),
+            pms, rsa, RSA_NO_PADDING);
 
-        if (decrypt_len != SSL_MAX_MASTER_KEY_LENGTH) {
-                al = SSL_AD_DECODE_ERROR;
-                /* SSLerror(s, SSL_R_BAD_RSA_DECRYPT); */
+        if (decrypt_len != pms_len) {
+                SSLerror(s, SSL_R_DECRYPTION_FAILED);
+                ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECRYPT_ERROR);
+                goto err;
         }
 
-        if ((al == -1) && !((pms[0] == (s->s3->hs.peer_legacy_version >> 8)) &&
-            (pms[1] == (s->s3->hs.peer_legacy_version & 0xff)))) {
-                /*
-                 * The premaster secret must contain the same version number
-                 * as the ClientHello to detect version rollback attacks
-                 * (strangely, the protocol does not offer such protection for
-                 * DH ciphersuites).
-                 *
-                 * The Klima-Pokorny-Rosa extension of Bleichenbacher's attack
-                 * (http://eprint.iacr.org/2003/052/) exploits the version
-                 * number check as a "bad version oracle" -- an alert would
-                 * reveal that the plaintext corresponding to some ciphertext
-                 * made up by the adversary is properly formatted except that
-                 * the version number is wrong. To avoid such attacks, we should
-                 * treat this just like any other decryption error.
-                 */
-                al = SSL_AD_DECODE_ERROR;
-                /* SSLerror(s, SSL_R_BAD_PROTOCOL_VERSION_NUMBER); */
-        }
+        /*
+         * All processing from here on needs to avoid leaking any information
+         * about the decrypted content, in order to prevent oracle attacks and
+         * minimise timing attacks.
+         */
 
-        if (al != -1) {
-                /*
-                 * Some decryption failure -- use random value instead
-                 * as countermeasure against Bleichenbacher's attack
-                 * on PKCS #1 v1.5 RSA padding (see RFC 2246,
-                 * section 7.4.7.1).
-                 */
-                p = fakekey;
-        }
+        /* Check padding - 00 02 <8 or more non-zero bytes> 00 */
+        valid &= crypto_ct_eq_u8(pms[0], 0x00);
+        valid &= crypto_ct_eq_u8(pms[1], 0x02);
+        for (i = 2; i < pad_len - 1; i++)
+                valid &= crypto_ct_ne_u8(pms[i], 0x00);
+        valid &= crypto_ct_eq_u8(pms[pad_len - 1], 0x00);
 
-        if (!tls12_derive_master_secret(s, p, SSL_MAX_MASTER_KEY_LENGTH))
-                goto err;
+        /* Ensure client version in premaster secret matches ClientHello version. */
+        valid &= crypto_ct_eq_u8(pms[pad_len + 0], s->s3->hs.peer_legacy_version >> 8);
+        valid &= crypto_ct_eq_u8(pms[pad_len + 1], s->s3->hs.peer_legacy_version & 0xff);
 
-        freezero(pms, pms_len);
+        /* Use the premaster secret if padding is correct, if not use the fake. */
+        mask = crypto_ct_eq_mask_u8(valid, 1);
+        for (i = 0; i < SSL_MAX_MASTER_KEY_LENGTH; i++)
+                pms[i] = (pms[pad_len + i] & mask) | (fakepms[i] & ~mask);
 
-        return 1;
+        if (!tls12_derive_master_secret(s, pms, SSL_MAX_MASTER_KEY_LENGTH))
+                goto err;
+
+        ret = 1;
 
- decode_err:
-        al = SSL_AD_DECODE_ERROR;
-        SSLerror(s, SSL_R_BAD_PACKET_LENGTH);
- fatal_err:
-        ssl3_send_alert(s, SSL3_AL_FATAL, al);
  err:
         freezero(pms, pms_len);
 
-        return 0;
+        return ret;
 }
 
 static int