Security and correctness fixes, plus a DTLS 1.3 scheduled work API

doc/dox_comments/header_files/ssl.h

@@ -3951,6 +3951,87 @@ int  wolfSSL_dtls_got_timeout(WOLFSSL* ssl);
 */
 int wolfSSL_dtls_retransmit(WOLFSSL* ssl);
 
+/*!
+    \ingroup Setup
+
+    \brief Sends the DTLS 1.3 work that was scheduled while reading. With
+    WOLFSSL_RW_THREADED the read path never transmits, because that would race
+    the write thread over the output buffer and the sending key schedule, so
+    ACKs, retransmissions and a KeyUpdate the peer asked for are only sent from
+    the write side. An application that reads without writing must call this,
+    from the same thread it uses for writing, or those messages are never sent
+    and the peer keeps retransmitting what it is waiting to have acknowledged.
+    Not for write-dup applications, which drain through wolfSSL_write().
+
+    A KeyUpdate is only sent while none of ours is still unacknowledged, since
+    DTLS must not have two in flight. Completing one we started needs the
+    peer's acknowledgement processed, which rotates the sending keys and so
+    does not happen here, so in a WOLFSSL_RW_THREADED build a peer request
+    arriving after that point is held rather than answered.
+
+    A send that could only write part of a record returns WOLFSSL_FATAL_ERROR
+    with wolfSSL_get_error() reporting SSL_ERROR_WANT_WRITE. That is not a
+    failure of the connection: the record is held and the next call sends the
+    rest, so wolfSSL_dtls13_pending_work() keeps reporting work until it is
+    out. Treat it as a retry rather than as a reason to stop draining.
+
+    \return WOLFSSL_SUCCESS on success, including when there was nothing to do.
+    \return WOLFSSL_FATAL_ERROR if ssl is NULL, is not a DTLS 1.3 object, is
+    part of a write-dup pair, or the send failed. Call wolfSSL_get_error() to
+    tell a retryable SSL_ERROR_WANT_WRITE from a real failure.
+
+    \param ssl a pointer to a WOLFSSL structure, created using wolfSSL_new().
+
+    _Example_
+    \code
+    WOLFSSL* ssl;
+    ...
+    while (wolfSSL_dtls13_pending_work(ssl)) {
+        if (wolfSSL_dtls13_do_scheduled_work(ssl) != WOLFSSL_SUCCESS) {
+            if (wolfSSL_get_error(ssl, 0) == SSL_ERROR_WANT_WRITE) {
+                // the socket is full, wait for it and call again
+                break;
+            }
+            // a real error
+            break;
+        }
+    }
+    \endcode
+
+    \sa wolfSSL_dtls13_pending_work
+    \sa wolfSSL_dtls_retransmit
+*/
+int wolfSSL_dtls13_do_scheduled_work(WOLFSSL* ssl);
+
+/*!
+    \ingroup Setup
+
+    \brief Reports whether the object has DTLS 1.3 work waiting to be sent by
+    wolfSSL_dtls13_do_scheduled_work(). Only meaningful with
+    WOLFSSL_RW_THREADED. The answer is advisory and can change as soon as it is
+    returned. Only work the pump can actually carry out is reported, so a drain
+    loop over the pair terminates; waiting for the peer to acknowledge a key
+    update we sent is not reported, as there is nothing to send for it. A
+    record the pump could only write in part is reported, so that the retry it
+    owes is not lost.
+
+    \return 1 if there is work to send, or if it could not be determined.
+    \return 0 if there is nothing to do, or ssl is not supported here.
+
+    \param ssl a pointer to a WOLFSSL structure, created using wolfSSL_new().
+
+    _Example_
+    \code
+    WOLFSSL* ssl;
+    ...
+    if (wolfSSL_dtls13_pending_work(ssl))
+        wolfSSL_dtls13_do_scheduled_work(ssl);
+    \endcode
+
+    \sa wolfSSL_dtls13_do_scheduled_work
+*/
+int wolfSSL_dtls13_pending_work(WOLFSSL* ssl);
+
 /*!
     \brief This function is used to determine if the SSL session has been
     configured to use DTLS.

src/dtls.c

@@ -66,6 +66,11 @@
 
 void DtlsResetState(WOLFSSL* ssl)
 {
+#if defined(WOLFSSL_DTLS) && defined(WOLFSSL_DTLS_CID) && \
+    defined(WOLFSSL_RW_THREADED)
+    int locked;
+#endif
+
     /* Reset the state so that we can statelessly await the
      * ClientHello that contains the cookie. Don't gate on IsAtLeastTLSv1_3
      * to handle the edge case when the peer wants a lower version. */
@@ -98,13 +103,28 @@ void DtlsResetState(WOLFSSL* ssl)
     ssl->options.tls1_1 = 0;
     ssl->options.tls1_3 = 0;
 #if defined(WOLFSSL_DTLS) && defined(WOLFSSL_DTLS_CID)
+#ifdef WOLFSSL_RW_THREADED
+    /* wolfSSL_dtls_set_pending_peer() may run on another thread, so take the
+     * lock the record layer uses for these two fields before dropping what
+     * that call left behind. Callers must not already hold peerLock:
+     * re-acquiring a write lock is undefined and deadlocks rather than fails,
+     * so the only failure this can report is a broken or uninitialised lock.
+     * Clear the fields anyway in that case, since resetting the state is the
+     * whole point of this call and leaving a pending peer behind is worse than
+     * the race. dtlsProcessPendingPeer() and ProcessReplyEx() do the same. */
+    locked = (wc_LockRwLock_Wr(&ssl->buffers.dtlsCtx.peerLock) == 0);
+#endif
     ssl->buffers.dtlsCtx.processingPendingRecord = 0;
     /* Clear the pending peer in case user set */
     XFREE(ssl->buffers.dtlsCtx.pendingPeer.sa, ssl->heap,
           DYNAMIC_TYPE_SOCKADDR);
     ssl->buffers.dtlsCtx.pendingPeer.sa = NULL;
     ssl->buffers.dtlsCtx.pendingPeer.sz = 0;
     ssl->buffers.dtlsCtx.pendingPeer.bufSz = 0;
+#ifdef WOLFSSL_RW_THREADED
+    if (locked)
+        (void)wc_UnLockRwLock(&ssl->buffers.dtlsCtx.peerLock);
+#endif
 #endif
 }
 

src/internal.c

@@ -22730,11 +22730,13 @@ int TimingPadVerify(WOLFSSL* ssl, const byte* input, int padLen, int macSz,
 
     XMEMSET(verify, 0, WC_MAX_DIGEST_SIZE);
     good = MaskPadding(input, pLen, macSz);
-    /* 4th argument has potential to underflow, ssl->hmac function should
-     * either increment the size by (macSz + padLen + 1) before use or check on
-     * the size to make sure is valid. */
-    ret = ssl->hmac(ssl, verify, input, (word32)(pLen - macSz - padLen - 1), padLen,
-                                                        content, 1, PEER_ORDER);
+    /* An out of range padding length byte is already recorded in good, but the
+     * length handed to ssl->hmac must not underflow. Clamp it in constant time
+     * so that the same amount of hashing is done for every value of the
+     * padding length byte. */
+    padLen &= ctMaskIntGTE(pLen - macSz - 1, padLen);
+    ret = ssl->hmac(ssl, verify, input, (word32)(pLen - macSz - padLen - 1),
+                                        padLen, content, 1, PEER_ORDER);
     good |= MaskMac(input, pLen, ssl->specs.hash_size, verify);
 
     /* Non-zero on failure. */
@@ -23735,6 +23737,16 @@ static int dtlsRecordIsNewest(WOLFSSL* ssl)
  */
 static void dtlsProcessPendingPeer(WOLFSSL* ssl, int deprotected, int isNewest)
 {
+#ifdef WOLFSSL_RW_THREADED
+    int locked;
+
+    /* A failure here means the lock itself is broken, not that another holder
+     * is in the way, so carry on regardless: this bookkeeping is the caller's
+     * own and leaving it stale is worse than the race. Only the promotion into
+     * dtlsCtx.peer below is skipped, since EmbedSendTo reads that buffer
+     * without the lock. DtlsResetState() and ProcessReplyEx() do the same. */
+    locked = (wc_LockRwLock_Wr(&ssl->buffers.dtlsCtx.peerLock) == 0);
+#endif
     if (ssl->buffers.dtlsCtx.pendingPeer.sa != NULL) {
         if (!deprotected) {
             /* Here we have just read an entire record from the network. It is
@@ -23750,11 +23762,30 @@ static void dtlsProcessPendingPeer(WOLFSSL* ssl, int deprotected, int isNewest)
                     !ssl->buffers.dtlsCtx.processingPendingRecord;
         }
         else {
-            /* Pending peer present and record deprotected. Update the peer. */
-            if (isNewest) {
-                (void)wolfSSL_dtls_set_peer(ssl,
-                        ssl->buffers.dtlsCtx.pendingPeer.sa,
-                        ssl->buffers.dtlsCtx.pendingPeer.sz);
+            /* Pending peer present and record deprotected. Promote it here
+             * rather than through wolfSSL_dtls_set_peer, which would take this
+             * same lock again. */
+            if (isNewest
+        #ifdef WOLFSSL_RW_THREADED
+                    /* Only this touches the buffer the send path reads
+                     * without the lock, so it is the one thing a failed
+                     * acquisition has to skip. */
+                    && locked
+        #endif
+                ) {
+                WOLFSSL_SOCKADDR* from = &ssl->buffers.dtlsCtx.pendingPeer;
+
+                if (wolfssl_local_SockAddrSet(&ssl->buffers.dtlsCtx.peer,
+                        from->sa, from->sz, ssl->heap) == WOLFSSL_SUCCESS) {
+                    ssl->buffers.dtlsCtx.userSet = 1;
+                }
+                else {
+                    /* wolfSSL_dtls_set_peer clears this when it fails to store
+                     * a peer, and the receive path only checks the sender
+                     * while peer.sz is non zero, so leaving it set would drop
+                     * the check rather than fail safe. */
+                    ssl->buffers.dtlsCtx.userSet = 0;
+                }
             }
             ssl->buffers.dtlsCtx.processingPendingRecord = 0;
             dtlsClearPeer(&ssl->buffers.dtlsCtx.pendingPeer);
@@ -23763,6 +23794,10 @@ static void dtlsProcessPendingPeer(WOLFSSL* ssl, int deprotected, int isNewest)
     else {
         ssl->buffers.dtlsCtx.processingPendingRecord = 0;
     }
+#ifdef WOLFSSL_RW_THREADED
+    if (locked)
+        (void)wc_UnLockRwLock(&ssl->buffers.dtlsCtx.peerLock);
+#endif
 }
 #endif
 static int DoDecrypt(WOLFSSL *ssl)
@@ -25102,6 +25137,10 @@ int ProcessReply(WOLFSSL* ssl)
 int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr)
 {
     int ret;
+#if defined(WOLFSSL_DTLS) && defined(WOLFSSL_DTLS_CID) && \
+    defined(WOLFSSL_RW_THREADED)
+    int locked;
+#endif
 
     ret = DoProcessReplyEx(ssl, allowSocketErr);
 
@@ -25114,8 +25153,20 @@ int ProcessReplyEx(WOLFSSL* ssl, int allowSocketErr)
                 && ret != WC_NO_ERR_TRACE(WC_PENDING_E)
 #endif
             ) {
+        #ifdef WOLFSSL_RW_THREADED
+            /* Drop the pending peer even when the lock cannot be taken, as
+             * DtlsResetState() and dtlsProcessPendingPeer() do. A failure here
+             * means the lock is broken rather than held, and nothing below
+             * touches dtlsCtx.peer, which is the buffer the send path reads
+             * without this lock. */
+            locked = (wc_LockRwLock_Wr(&ssl->buffers.dtlsCtx.peerLock) == 0);
+        #endif
             dtlsClearPeer(&ssl->buffers.dtlsCtx.pendingPeer);
             ssl->buffers.dtlsCtx.processingPendingRecord = 0;
+        #ifdef WOLFSSL_RW_THREADED
+            if (locked)
+                (void)wc_UnLockRwLock(&ssl->buffers.dtlsCtx.peerLock);
+        #endif
         }
     }
 #endif

src/ssl.c

@@ -5679,7 +5679,94 @@ size_t wolfSSL_get_client_random(const WOLFSSL* ssl, unsigned char* out,
                 ssl->buffers.inputBuffer.bufferSize);
         #endif
         }
-        ssl->keys.encryptionOn = 0;
+        /* Recycling the object for a new connection must not carry the
+         * previous connection's key material along with it. A freshly
+         * created object has all of this zeroed. */
+        ForceZero(&ssl->keys, sizeof(Keys));
+    #ifdef WOLFSSL_MULTICAST
+        if (ssl->options.haveMcast) {
+            int i;
+
+            for (i = 0; i < WOLFSSL_DTLS_PEERSEQ_SZ; i++)
+                ssl->keys.peerSeq[i].peerId = INVALID_PEER_ID;
+        }
+    #endif
+    #ifdef WOLFSSL_TLS13
+        ForceZero(ssl->clientSecret, sizeof(ssl->clientSecret));
+        ForceZero(ssl->serverSecret, sizeof(ssl->serverSecret));
+        /* A key update the previous connection asked for has nothing to say
+         * about the next one, and the keys it would rotate are gone. */
+        ssl->options.sendKeyUpdate = 0;
+    #endif
+    #ifdef WOLFSSL_HAVE_TLS_UNIQUE
+        /* The channel binding of the connection that just ended. Leaving it
+         * in place would let the next caller bind to the wrong session. */
+        ForceZero(ssl->clientFinished, TLS_FINISHED_SZ_MAX);
+        ForceZero(ssl->serverFinished, TLS_FINISHED_SZ_MAX);
+        ssl->clientFinished_len = 0;
+        ssl->serverFinished_len = 0;
+    #endif
+    #ifdef WOLFSSL_DTLS13
+        /* Per-epoch traffic keys, IVs and sequence number keys. */
+        ForceZero(ssl->dtls13Epochs, sizeof(ssl->dtls13Epochs));
+        /* Only InitSSL() sets up the unprotected epoch 0 and aims the epoch
+         * pointers at it, and this object is being reused rather than freed,
+         * so put it back or the next handshake has no valid epoch. */
+        ssl->dtls13Epochs[0].isValid = 1;
+        ssl->dtls13Epochs[0].side = ENCRYPT_AND_DECRYPT_SIDE;
+        ssl->dtls13EncryptEpoch = &ssl->dtls13Epochs[0];
+        ssl->dtls13DecryptEpoch = &ssl->dtls13Epochs[0];
+        /* The numbers that say which epoch to use sit outside the table and
+         * only ever move up, so wiping the table has to be matched here by
+         * hand. InitSSL() gets this for free from clearing the whole object.
+         * Left behind, they ask the next handshake to send under an epoch the
+         * table no longer holds, and Dtls13SetEpochKeys() fails the connection
+         * with BAD_STATE_E. */
+        w64Zero(&ssl->dtls13Epoch);
+        w64Zero(&ssl->dtls13PeerEpoch);
+        w64Zero(&ssl->dtls13InvalidateBefore);
+        ssl->dtls13WaitKeyUpdateAck = 0;
+        ssl->dtls13DoKeyUpdate = 0;
+        ssl->dtls13SendingAckOrRtx = 0;
+        /* Anything still queued for retransmission or acknowledgement is
+         * tagged with an epoch that no longer exists, so it can never be sent
+         * and would only be released when the object is freed. */
+        Dtls13FreeFsmResources(ssl);
+        ssl->dtls13Rtx.sendAcks = 0;
+        ssl->dtls13Rtx.retransmit = 0;
+    #endif
+        /* The handshake arrays hold the master and pre-master secrets. Wipe
+         * those in place rather than releasing the arrays, so that the
+         * allocation stays valid for wolfSSL_set_secret(), the exporter and
+         * the other accessors that read from it once a connection has ended.
+         * An application that asked to keep the arrays still gets back
+         * everything the API can hand it, so only the rest goes. */
+        if (ssl->arrays != NULL) {
+            if (ssl->arrays->preMasterSecret != NULL) {
+                ForceZero(ssl->arrays->preMasterSecret, ENCRYPT_LEN);
+                /* The key agreement routines take this as the size of the
+                 * buffer they may write, so put back what a freshly
+                 * allocated Arrays would carry. */
+                ssl->arrays->preMasterSz = ENCRYPT_LEN;
+            }
+        #if defined(HAVE_SESSION_TICKET) || !defined(NO_PSK)
+            ForceZero(ssl->arrays->psk_key, MAX_PSK_KEY_LEN);
+            ssl->arrays->psk_keySz = 0;
+        #endif
+        #ifdef WOLFSSL_TLS13
+            /* The key schedule secret. No API hands this one back. */
+            ForceZero(ssl->arrays->secret, SECRET_LEN);
+        #endif
+            if (!ssl->options.saveArrays) {
+                ForceZero(ssl->arrays->masterSecret, SECRET_LEN);
+            #ifdef HAVE_KEYING_MATERIAL
+                /* Tls13_Exporter() reads this one, and exporting keying
+                 * material requires the arrays to be kept, so it only goes
+                 * when the application did not ask for that. */
+                ForceZero(ssl->arrays->exporterSecret, WC_MAX_DIGEST_SIZE);
+            #endif
+            }
+        }
         XMEMSET(&ssl->msgsReceived, 0, sizeof(ssl->msgsReceived));
 
         /* Discard any partial handshake-message reassembly on reuse. */