Fenrir: TLS/DTLS conformance and crypto hardening fixes

src/dtls13.c

@@ -2369,7 +2369,6 @@ static Dtls13Epoch* Dtls13NewEpochSlot(WOLFSSL* ssl)
     w64wrapper oldestNumber;
     int i;
 
-    /* FIXME: add max function */
     oldestNumber = w64From32((word32)-1, (word32)-1);
     oldest = NULL;
 
@@ -2380,8 +2379,10 @@ static Dtls13Epoch* Dtls13NewEpochSlot(WOLFSSL* ssl)
 
         if (!w64Equal(e->epochNumber, ssl->dtls13Epoch) &&
             !w64Equal(e->epochNumber, ssl->dtls13PeerEpoch) &&
-            w64LT(e->epochNumber, oldestNumber))
+            w64LT(e->epochNumber, oldestNumber)) {
             oldest = e;
+            oldestNumber = e->epochNumber;
+        }
     }
 
     if (oldest == NULL)
@@ -2720,17 +2721,27 @@ static int Dtls13RtxIsTrackedByRn(const Dtls13RtxRecord* r, w64wrapper epoch,
 static int Dtls13KeyUpdateAckReceived(WOLFSSL* ssl)
 {
     int ret;
+    /* Validate on a local copy so ssl->dtls13Epoch is left untouched when a
+     * check fails. */
+    w64wrapper newEpoch = ssl->dtls13Epoch;
 
     ret = DeriveTls13Keys(ssl, update_traffic_key, ENCRYPT_SIDE_ONLY, 1);
     if (ret != 0)
         return ret;
 
-    w64Increment(&ssl->dtls13Epoch);
+    w64Increment(&newEpoch);
 
     /* Epoch wrapped up */
-    if (w64IsZero(ssl->dtls13Epoch))
+    if (w64IsZero(newEpoch))
+        return BAD_STATE_E;
+
+    /* RFC 9147 Section 4.2.1: the epoch must not exceed 2^48-1. */
+    if (w64GT(newEpoch,
+              w64From32(DTLS13_EPOCH_MAX_HI32, DTLS13_EPOCH_MAX_LO32)))
         return BAD_STATE_E;
 
+    ssl->dtls13Epoch = newEpoch;
+
     return Dtls13SetEpochKeys(ssl, ssl->dtls13Epoch, ENCRYPT_SIDE_ONLY);
 }
 

src/internal.c

@@ -3282,6 +3282,7 @@ static void FreeCiphersSide(Ciphers *cipher, void* heap)
     cipher->aria = NULL;
 #endif
 #ifdef HAVE_CAMELLIA
+    wc_CamelliaFree(cipher->cam);
     XFREE(cipher->cam, heap, DYNAMIC_TYPE_CIPHER);
     cipher->cam = NULL;
 #endif

src/pk.c

@@ -4977,8 +4977,7 @@ static int _DH_compute_key(unsigned char* key, const WOLFSSL_BIGNUM* otherPub,
              * correctly.
              */
             if (keySz < padded_keySz) {
-                XMEMMOVE(key, key + (padded_keySz - keySz),
-                         padded_keySz - keySz);
+                XMEMMOVE(key + (padded_keySz - keySz), key, keySz);
                 XMEMSET(key, 0, padded_keySz - keySz);
                 keySz = padded_keySz;
             }

src/ssl.c

@@ -3319,8 +3319,8 @@ int  wolfSSL_CTX_set1_groups(WOLFSSL_CTX* ctx, int* groups,
     int i;
     int _groups[WOLFSSL_MAX_GROUP_COUNT];
     WOLFSSL_ENTER("wolfSSL_CTX_set1_groups");
-    if (count == 0) {
-        WOLFSSL_MSG("Group count is zero");
+    if (count <= 0) {
+        WOLFSSL_MSG("Group count is not positive");
         return WOLFSSL_FAILURE;
     }
     if (count > WOLFSSL_MAX_GROUP_COUNT) {
@@ -3358,8 +3358,8 @@ int  wolfSSL_set1_groups(WOLFSSL* ssl, int* groups, int count)
     int i;
     int _groups[WOLFSSL_MAX_GROUP_COUNT];
     WOLFSSL_ENTER("wolfSSL_CTX_set1_groups");
-    if (count == 0) {
-        WOLFSSL_MSG("Group count is zero");
+    if (count <= 0) {
+        WOLFSSL_MSG("Group count is not positive");
         return WOLFSSL_FAILURE;
     }
     if (count > WOLFSSL_MAX_GROUP_COUNT) {
@@ -13025,7 +13025,10 @@ size_t wolfSSL_get_peer_finished(const WOLFSSL *ssl, void *buf, size_t count)
 long wolfSSL_get_verify_result(const WOLFSSL *ssl)
 {
     if (ssl == NULL) {
-        return WOLFSSL_FAILURE;
+        /* Return a non-zero error so the OpenSSL-idiomatic
+         * "!= X509_V_OK" check does not mistake a NULL ssl for a
+         * successful verification (X509_V_OK is 0). */
+        return WOLFSSL_X509_V_ERR_APPLICATION_VERIFICATION;
     }
 
     return (long)ssl->peerVerifyRet;
@@ -16199,9 +16202,24 @@ int wolfSSL_CTX_set_servername_arg(WOLFSSL_CTX* ctx, void* arg)
 
 int wolfSSL_CRYPTO_memcmp(const void *a, const void *b, size_t size)
 {
+    int ret = 0;
+    int chunk;
+    const byte* pa = (const byte*)a;
+    const byte* pb = (const byte*)b;
+
     if (!a || !b)
         return -1;
-    return ConstantCompare((const byte*)a, (const byte*)b, (int)size);
+    /* ConstantCompare takes an int length. Compare in chunks of at most
+     * INT_MAX so a size that does not fit in an int is not narrowed into a
+     * negative or truncated length, which could wrongly report equality. */
+    while (size > 0) {
+        chunk = (size > (size_t)INT_MAX) ? INT_MAX : (int)size;
+        ret |= ConstantCompare(pa, pb, chunk);
+        pa += chunk;
+        pb += chunk;
+        size -= (size_t)chunk;
+    }
+    return ret;
 }
 
 unsigned long wolfSSL_ERR_peek_last_error(void)

src/tls.c

@@ -391,15 +391,17 @@ ProtocolVersion MakeTLSv1_3(void)
  * ctx     SSL/TLS context object.
  * groups  Array of groups.
  * count   Number of groups in array.
- * returns BAD_FUNC_ARG when ctx or groups is NULL, not using TLS v1.3 or
- * count is greater than WOLFSSL_MAX_GROUP_COUNT and WOLFSSL_SUCCESS on success.
+ * returns BAD_FUNC_ARG when ctx or groups is NULL, not using TLS v1.3, count is
+ * not positive or count is greater than WOLFSSL_MAX_GROUP_COUNT and
+ * WOLFSSL_SUCCESS on success.
  */
 int wolfSSL_CTX_set_groups(WOLFSSL_CTX* ctx, int* groups, int count)
 {
     int ret, i;
 
     WOLFSSL_ENTER("wolfSSL_CTX_set_groups");
-    if (ctx == NULL || groups == NULL || count > WOLFSSL_MAX_GROUP_COUNT)
+    if (ctx == NULL || groups == NULL || count <= 0 ||
+            count > WOLFSSL_MAX_GROUP_COUNT)
         return BAD_FUNC_ARG;
     if (!IsTLS_ex(ctx->method->version))
         return BAD_FUNC_ARG;
@@ -436,15 +438,17 @@ int wolfSSL_CTX_set_groups(WOLFSSL_CTX* ctx, int* groups, int count)
  * ssl     SSL/TLS object.
  * groups  Array of groups.
  * count   Number of groups in array.
- * returns BAD_FUNC_ARG when ssl or groups is NULL, not using TLS v1.3 or
- * count is greater than WOLFSSL_MAX_GROUP_COUNT and WOLFSSL_SUCCESS on success.
+ * returns BAD_FUNC_ARG when ssl or groups is NULL, not using TLS v1.3, count is
+ * not positive or count is greater than WOLFSSL_MAX_GROUP_COUNT and
+ * WOLFSSL_SUCCESS on success.
  */
 int wolfSSL_set_groups(WOLFSSL* ssl, int* groups, int count)
 {
     int ret, i;
 
     WOLFSSL_ENTER("wolfSSL_set_groups");
-    if (ssl == NULL || groups == NULL || count > WOLFSSL_MAX_GROUP_COUNT)
+    if (ssl == NULL || groups == NULL || count <= 0 ||
+            count > WOLFSSL_MAX_GROUP_COUNT)
         return BAD_FUNC_ARG;
     if (!IsTLS_ex(ssl->version))
         return BAD_FUNC_ARG;
@@ -2065,6 +2069,15 @@ static int TLSX_ALPN_ParseAndSet(WOLFSSL *ssl, const byte *input, word16 length,
         byte sel_len = 0;
         TLSX *extension = NULL;
 
+        /* RFC 7301 Section 3.1: a ServerHello ALPN extension MUST contain
+         * exactly one protocol name. The first name's length byte plus its
+         * payload must therefore span the whole list. */
+        if ((word16)(input[offset] + OPAQUE8_LEN) != size) {
+            SendAlert(ssl, alert_fatal, illegal_parameter);
+            WOLFSSL_ERROR_VERBOSE(BUFFER_ERROR);
+            return BUFFER_ERROR;
+        }
+
         r = ALPN_find_match(ssl, &extension, &sel, &sel_len, input + offset, size);
         if (r != 0)
             return r;
@@ -3251,9 +3264,27 @@ static int TLSX_MFL_Parse(WOLFSSL* ssl, const byte* input, word16 length,
 #ifdef WOLFSSL_OLD_UNSUPPORTED_EXTENSION
     (void) isRequest;
 #else
-    if (!isRequest)
+    if (!isRequest) {
+        TLSX* extension;
+
         if (TLSX_CheckUnsupportedExtension(ssl, TLSX_MAX_FRAGMENT_LENGTH))
             return TLSX_HandleUnsupportedExtension(ssl);
+
+        /* RFC 6066 Section 4: the server's response value must match the
+         * value the client requested. The request may have been configured on
+         * the WOLFSSL object or inherited from the WOLFSSL_CTX. */
+        extension = TLSX_Find(ssl->extensions, TLSX_MAX_FRAGMENT_LENGTH);
+        if (extension == NULL) {
+            extension = TLSX_Find(ssl->ctx->extensions,
+                    TLSX_MAX_FRAGMENT_LENGTH);
+        }
+        if (extension == NULL || extension->data == NULL ||
+                ((byte*)extension->data)[0] != *input) {
+            SendAlert(ssl, alert_fatal, illegal_parameter);
+            WOLFSSL_ERROR_VERBOSE(UNKNOWN_MAX_FRAG_LEN_E);
+            return UNKNOWN_MAX_FRAG_LEN_E;
+        }
+    }
 #endif
 
     switch (*input) {

src/tls13.c

@@ -6100,11 +6100,24 @@ static int DoTls13CertificateRequest(WOLFSSL* ssl, const byte* input,
         return BUFFER_ERROR;
     /* INVALID_PARAMETER does not map to illegal_parameter in the central
      * alert path, so emit the alert explicitly before returning. */
-    if (ssl->options.connectState < FINISHED_DONE && len > 0) {
+    if (ssl->options.connectState < FINISHED_DONE) {
+        /* RFC 8446 Section 4.3.2: in the handshake the context is zero
+         * length. */
+        if (len > 0) {
+            SendAlert(ssl, alert_fatal, illegal_parameter);
+            WOLFSSL_ERROR_VERBOSE(INVALID_PARAMETER);
+            return INVALID_PARAMETER;
+        }
+    }
+#ifdef WOLFSSL_POST_HANDSHAKE_AUTH
+    else if (len == 0) {
+        /* RFC 8446 Section 4.3.2: a post-handshake CertificateRequest context
+         * MUST be non-empty and unique for the connection. */
         SendAlert(ssl, alert_fatal, illegal_parameter);
         WOLFSSL_ERROR_VERBOSE(INVALID_PARAMETER);
         return INVALID_PARAMETER;
     }
+#endif
 
 #ifdef WOLFSSL_POST_HANDSHAKE_AUTH
     /* Remember the request context bytes; the CertReqCtx allocation and
@@ -6113,6 +6126,18 @@ static int DoTls13CertificateRequest(WOLFSSL* ssl, const byte* input,
      */
     reqCtxLen = len;
     reqCtxData = input + *inOutIdx;
+    /* Reject a context that duplicates one still pending on the connection. */
+    if (ssl->options.connectState >= FINISHED_DONE) {
+        CertReqCtx* dup;
+        for (dup = ssl->certReqCtx; dup != NULL; dup = dup->next) {
+            if (dup->len == reqCtxLen &&
+                    XMEMCMP(&dup->ctx, reqCtxData, reqCtxLen) == 0) {
+                SendAlert(ssl, alert_fatal, illegal_parameter);
+                WOLFSSL_ERROR_VERBOSE(INVALID_PARAMETER);
+                return INVALID_PARAMETER;
+            }
+        }
+    }
 #endif
     *inOutIdx += len;
 
@@ -6134,6 +6159,14 @@ static int DoTls13CertificateRequest(WOLFSSL* ssl, const byte* input,
     }
     *inOutIdx += len;
 
+    /* RFC 8446 Section 4.3.2: the signature_algorithms extension MUST be
+     * present in a CertificateRequest. */
+    if (peerSuites.hashSigAlgoSz == 0) {
+        SendAlert(ssl, alert_fatal, missing_extension);
+        WOLFSSL_ERROR_VERBOSE(INVALID_PARAMETER);
+        return INVALID_PARAMETER;
+    }
+
 #ifdef WOLFSSL_CERT_SETUP_CB
     if ((ret = CertSetupCbWrapper(ssl)) != 0)
         return ret;
@@ -12172,8 +12205,16 @@ int SendTls13KeyUpdate(WOLFSSL* ssl)
     WOLFSSL_ENTER("SendTls13KeyUpdate");
 
 #ifdef WOLFSSL_DTLS13
-    if (ssl->options.dtls)
+    if (ssl->options.dtls) {
+        /* RFC 9147 Section 4.2.1: do not send a KeyUpdate that would advance
+         * the sending epoch beyond 2^48-1. */
+        if (w64GTE(ssl->dtls13Epoch,
+                   w64From32(DTLS13_EPOCH_MAX_HI32, DTLS13_EPOCH_MAX_LO32))) {
+            WOLFSSL_MSG("DTLS 1.3 sending epoch at maximum; refusing KeyUpdate");
+            return BAD_STATE_E;
+        }
         i = Dtls13GetRlHeaderLength(ssl, 1) + DTLS_HANDSHAKE_HEADER_SZ;
+    }
 #endif /* WOLFSSL_DTLS13 */
 
     outputSz = OPAQUE8_LEN + MAX_MSG_EXTRA;
@@ -12307,7 +12348,18 @@ static int DoTls13KeyUpdate(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
 
 #ifdef WOLFSSL_DTLS13
     if (ssl->options.dtls) {
-        w64Increment(&ssl->dtls13PeerEpoch);
+        w64wrapper newEpoch = ssl->dtls13PeerEpoch;
+        w64Increment(&newEpoch);
+
+        /* RFC 9147 Section 4.2.1: the epoch must not exceed 2^48-1. Reject a
+         * peer KeyUpdate that would advance the receiving epoch past the
+         * limit. Validate on a local copy so ssl->dtls13PeerEpoch is left
+         * untouched when the check fails. */
+        if (w64GT(newEpoch,
+                  w64From32(DTLS13_EPOCH_MAX_HI32, DTLS13_EPOCH_MAX_LO32)))
+            return BAD_STATE_E;
+
+        ssl->dtls13PeerEpoch = newEpoch;
 
         ret = Dtls13SetEpochKeys(ssl, ssl->dtls13PeerEpoch, DECRYPT_SIDE_ONLY);
         if (ret != 0)

tests/api.c

@@ -19069,7 +19069,10 @@ static int test_wolfSSL_verify_result(void)
     WOLFSSL_CTX* ctx = NULL;
     long         result = 0xDEADBEEF;
 
-    ExpectIntEQ(WC_NO_ERR_TRACE(WOLFSSL_FAILURE), wolfSSL_get_verify_result(ssl));
+    /* A NULL ssl returns a non-zero verify error (not X509_V_OK) so the
+     * OpenSSL-idiomatic "!= X509_V_OK" check is not fooled. See F-4594. */
+    ExpectIntEQ(WOLFSSL_X509_V_ERR_APPLICATION_VERIFICATION,
+        wolfSSL_get_verify_result(ssl));
 
     ExpectNotNull(ctx = wolfSSL_CTX_new(wolfSSLv23_client_method()));
     ExpectNotNull(ssl = SSL_new(ctx));

wolfssl/internal.h

@@ -5948,6 +5948,11 @@ enum  {
     DTLS13_EPOCH_TRAFFIC0 = 3
 };
 
+/* RFC 9147 Section 4.2.1: the DTLS 1.3 epoch is a 48-bit value and must not
+ * exceed 2^48-1. Expressed as the high/low 32-bit halves of a w64wrapper. */
+#define DTLS13_EPOCH_MAX_HI32 0x0000FFFFU
+#define DTLS13_EPOCH_MAX_LO32 0xFFFFFFFFU
+
 /* 64-bit epoch + 64-bit sequence number */
 #define DTLS13_RN_SIZE (OPAQUE64_LEN + OPAQUE64_LEN)
 /* Maximum number of ACK records allowed in an ACK message */

wolfssl/ssl.h

@@ -2732,6 +2732,7 @@ enum {
     WOLFSSL_X509_V_ERR_PATH_LENGTH_EXCEEDED              = 25,
     WOLFSSL_X509_V_ERR_CERT_REJECTED                     = 28,
     WOLFSSL_X509_V_ERR_SUBJECT_ISSUER_MISMATCH           = 29,
+    WOLFSSL_X509_V_ERR_APPLICATION_VERIFICATION          = 50,
     WOLFSSL_X509_V_ERR_HOSTNAME_MISMATCH                 = 62,
     WOLFSSL_X509_V_ERR_IP_ADDRESS_MISMATCH               = 64,
     WOLFSSL_X509_V_ERR_INVALID_CA                        = 79,