fix(quic): lifecycle hangs + TLS hardening from second-pass audit

Six fixes from the round-2 audit, all of which would either hang the client on failure modes or weaken security against a misbehaving server. QuicConnection / awaitHandshake hang fixes: - QuicConnectionClosedException + markClosedExternally(reason). - close() now also calls signalHandshakeFailed if !handshakeComplete. - QuicConnectionParser inbound CONNECTION_CLOSE → markClosedExternally (was: just flip status, leaving awaiters hanging forever). - QuicConnectionDriver.readLoop has a finally-block that calls markClosedExternally + wakeup so when the socket closes mid-handshake or the server closes uncleanly, awaitHandshake() throws instead of suspending forever. QuicConnectionDriver.close() no longer cancels its own caller scope: Previously close() was suspend, called connection.close (acquires lock) → wakeup → scope.cancel → socket.close. If close() is invoked from inside the driver scope (e.g. by the WT factory's exception cleanup path), scope.cancel cancels the very coroutine running close, so socket.close may never run. close() is now non-suspend and dispatches the teardown onto parentScope.launch so the cancel never reaches its caller. QuicWebTransportFactory.connect: - readResponseStatus wrapped in withTimeoutOrNull(connectTimeoutMillis, default 10s). Dead network → HandshakeFailed instead of forever-hang. - Whole post-handshake setup (open control + request streams + read response) now in try/catch that calls driver.close() on any unexpected exception. Previously a thrown SocketException between awaitHandshake() and the explicit non-2xx branch leaked the driver + UDP socket. JdkCertificateValidator.validateChain authType: Was hardcoded "ECDHE_ECDSA". Now derived from the leaf cert's public-key algorithm: "RSA" → ECDHE_RSA, "EC"/"EdDSA" → ECDHE_ECDSA. Some Android trust managers (RootTrustManager, NetworkSecurityConfig) gate algorithm-specific pinning rules on this string and may reject mismatched combos. TlsExtension.decodeList bounds: Inner reads were unbounded against the extension-list end. A malicious server could claim totalLen=4 but encode an extension whose data length declared 1000, reading past the supposed extension-list end into trailing handshake bytes. Now: validates totalLen ≤ r.limit upfront and asserts r.position ≤ end after each inner decode. TlsClient KeyUpdate close-on-receipt: Previously HS_KEY_UPDATE was silently dropped. RFC 9001 §6: if the peer rotates keys and we keep using the old ones, AEAD opens silently fail → connection wedges. Until we implement rotation, KeyUpdate must throw so the QUIC layer closes cleanly with a fatal error instead of desynchronizing. All :quic:jvmTest + :nestsClient:jvmTest pass. https://claude.ai/code/session_01EC1tfXfap8k8GyKvrxkxZx
2026-04-25 22:42:10 +00:00
parent 9d0a7ead7a
commit 94a3d32a6d
7 changed files with 161 additions and 52 deletions
@@ -148,7 +148,7 @@ class QuicConnection(
        handshakeDoneSignal.await()
    }

-    /** Mark the handshake as failed (called by the driver when read loop dies during handshaking). */
+    /** Mark the handshake as failed (called when read loop dies, peer closes, or local close runs). */
    internal fun signalHandshakeFailed(cause: Throwable) {
        if (!handshakeDoneSignal.isCompleted) handshakeDoneSignal.completeExceptionally(cause)
    }
@@ -256,11 +256,27 @@ class QuicConnection(
    suspend fun close(
        errorCode: Long,
        reason: String,
-    ) = lock.withLock {
-        if (status == Status.CLOSED || status == Status.CLOSING) return@withLock
-        closeErrorCode = errorCode
-        closeReason = reason
-        status = Status.CLOSING
+    ) {
+        lock.withLock {
+            if (status == Status.CLOSED || status == Status.CLOSING) return@withLock
+            closeErrorCode = errorCode
+            closeReason = reason
+            status = Status.CLOSING
+        }
+        // If a caller is suspended on awaitHandshake() and we're tearing down
+        // before completion, fail the deferred so the caller throws instead
+        // of hanging forever.
+        if (!handshakeComplete) {
+            signalHandshakeFailed(QuicConnectionClosedException("connection closed before handshake completed: $reason"))
+        }
+    }
+
+    /** Called by the parser on inbound CONNECTION_CLOSE or by the driver on read-loop death. */
+    internal fun markClosedExternally(reason: String) {
+        if (status != Status.CLOSED) status = Status.CLOSED
+        if (!handshakeComplete) {
+            signalHandshakeFailed(QuicConnectionClosedException("connection closed externally: $reason"))
+        }
    }

    /**
@@ -303,3 +319,8 @@ class QuicConnection(
    /** Caller must hold [lock]. */
    internal fun streamByIdLocked(id: Long): QuicStream? = streams[id]
 }
+
+/** Connection was closed (locally or by peer) before reaching CONNECTED. */
+class QuicConnectionClosedException(
+    message: String,
+) : RuntimeException(message)
@@ -46,7 +46,7 @@ import kotlinx.coroutines.sync.withLock
 class QuicConnectionDriver(
    val connection: QuicConnection,
    private val socket: UdpSocket,
-    parentScope: CoroutineScope,
+    private val parentScope: CoroutineScope,
    private val nowMillis: () -> Long = { System.currentTimeMillis() },
 ) {
    private val job = SupervisorJob(parentScope.coroutineContext[Job])
@@ -67,13 +67,21 @@ class QuicConnectionDriver(
    }

    private suspend fun readLoop() {
-        while (connection.status != QuicConnection.Status.CLOSED) {
-            val datagram = socket.receive() ?: break
-            connection.lock.withLock {
-                feedDatagram(connection, datagram, nowMillis())
+        try {
+            while (connection.status != QuicConnection.Status.CLOSED) {
+                val datagram = socket.receive() ?: break
+                connection.lock.withLock {
+                    feedDatagram(connection, datagram, nowMillis())
+                }
+                // Inbound data may have produced new outbound (acks, crypto, etc.).
+                wakeup()
            }
-            // Inbound data may have produced new outbound (acks, crypto, etc.).
-            wakeup()
+        } finally {
+            // If the read loop exits while the handshake is still pending,
+            // unblock anyone awaiting the handshake — otherwise awaitHandshake()
+            // suspends forever.
+            connection.markClosedExternally("read loop exited (socket closed or peer closed)")
+            wakeup() // let the send loop notice CLOSED and exit
        }
    }

@@ -90,10 +98,24 @@ class QuicConnectionDriver(
        }
    }

-    suspend fun close() {
-        connection.close(0L, "")
-        wakeup() // let the send loop emit CONNECTION_CLOSE
-        scope.cancel()
-        socket.close()
+    /**
+     * Cleanly tear down the driver. Safe to call from inside the driver scope —
+     * the actual cancel-and-close runs on [parentScope] so the caller's coroutine
+     * (which may itself be in [scope]) doesn't get cancelled before the close
+     * completes.
+     */
+    fun close() {
+        // Drive the close on the parent scope so we don't cancel our own caller.
+        parentScope.launch {
+            try {
+                connection.close(0L, "")
+                wakeup() // let the send loop emit CONNECTION_CLOSE
+                // Give the send loop one tick to flush the close packet, then tear down.
+                kotlinx.coroutines.yield()
+            } finally {
+                scope.cancel()
+                socket.close()
+            }
+        }
    }
 }
@@ -204,7 +204,7 @@ private fun dispatchFrames(
            }

            is ConnectionCloseFrame -> {
-                conn.status = QuicConnection.Status.CLOSED
+                conn.markClosedExternally("peer CONNECTION_CLOSE: ${frame.reason}")
            }

            is HandshakeDoneFrame -> {
@@ -242,16 +242,24 @@ class TlsClient(
            }

            State.SENT_CLIENT_FINISHED -> {
-                // Post-handshake messages on Application level: the server
-                // commonly sends NewSessionTicket (we don't resume, so we
-                // ignore them safely) and may send KeyUpdate. We don't
-                // implement key rotation yet, so we silently drop. If the
-                // server ever sends a CertificateRequest post-handshake we
-                // also ignore — we don't do client auth.
+                // Post-handshake messages on Application level. NewSessionTicket
+                // is safe to ignore (we don't do session resumption). KeyUpdate
+                // is NOT safe to ignore — if the peer rotates keys and we keep
+                // using the old ones, subsequent AEAD opens will silently fail
+                // and the connection wedges. We don't implement RFC 9001 §6 key
+                // updates yet, so KeyUpdate must surface as a fatal error so
+                // the QUIC layer closes the connection cleanly instead of
+                // silently desynchronizing.
                when (type) {
-                    TlsConstants.HS_NEW_SESSION_TICKET, TlsConstants.HS_KEY_UPDATE -> {
-                        // Don't append to transcript — these are not part of the
-                        // handshake transcript (RFC 8446 §4.4.1).
+                    TlsConstants.HS_NEW_SESSION_TICKET -> {
+                        // Don't append to transcript — NewSessionTicket is not
+                        // part of the handshake transcript per RFC 8446 §4.4.1.
+                    }
+
+                    TlsConstants.HS_KEY_UPDATE -> {
+                        throw QuicCodecException(
+                            "TLS KeyUpdate received but rotation not implemented; closing connection",
+                        )
                    }

                    else -> {
@@ -46,14 +46,28 @@ class TlsExtension(

        /**
         * Decode an Extension list (`extensions<0..2^16-1>`) from [r] until
-         * the inner length is consumed.
+         * the inner length is consumed. The inner reads are bounded against
+         * `end` so a malicious server can't claim a small extensions block
+         * but encode an extension whose `data` length escapes past the end
+         * and into trailing bytes (e.g. compression_method on a ServerHello).
         */
        fun decodeList(r: QuicReader): List<TlsExtension> {
            val totalLen = r.readUint16()
            val end = r.position + totalLen
+            if (end > r.limit) {
+                throw com.vitorpamplona.quic.QuicCodecException(
+                    "TLS extensions length $totalLen exceeds record bounds (have ${r.limit - r.position})",
+                )
+            }
            val out = mutableListOf<TlsExtension>()
            while (r.position < end) {
-                out += decode(r)
+                val ext = decode(r)
+                if (r.position > end) {
+                    throw com.vitorpamplona.quic.QuicCodecException(
+                        "TLS extension type=${ext.type} overran extension-list end",
+                    )
+                }
+                out += ext
            }
            return out
        }
@@ -57,8 +57,23 @@ class JdkCertificateValidator(
                cf.generateCertificate(ByteArrayInputStream(it)) as X509Certificate
            }
        try {
-            // RFC 8446 §4.4.2.4 — TLS 1.3 over QUIC negotiates ALPN h3.
-            trustManager.checkServerTrusted(parsed.toTypedArray(), "ECDHE_ECDSA")
+            // X509TrustManager auth-type string is the TLS key-exchange / sig-alg
+            // pair derived from the cipher suite name — for TLS 1.3 we use the
+            // leaf cert's public-key algorithm to pick the right value, since
+            // some Android trust managers (RootTrustManager, NetworkSecurityConfig)
+            // gate algorithm-specific pinning on this string.
+            val authType =
+                when (parsed[0].publicKey.algorithm) {
+                    "RSA" -> "ECDHE_RSA"
+
+                    "EC" -> "ECDHE_ECDSA"
+
+                    "EdDSA" -> "ECDHE_ECDSA"
+
+                    // RFC 8422 ext, no dedicated TLS 1.3 string
+                    else -> "ECDHE_ECDSA"
+                }
+            trustManager.checkServerTrusted(parsed.toTypedArray(), authType)
        } catch (t: Throwable) {
            throw QuicCodecException("certificate chain validation failed: ${t.message}", t)
        }