BUG/MEDIUM: quic: crash on key update phase change after a failed one

qc_pkt_decrypt() decrypts the first packet of a new key phase with
<qc->ku.nxt_rx.ctx> without checking it. quic_tls_key_update() leaves it NULL
when the derivation fails on a memory allocation error, and its caller only
dropped the packet, so the next key phase change crashed the process.

Closing the connection would be excessive: <tls_ctx->rx.ctx> is still valid and
the current key phase works. The next keys are now checked where they are used
and derived again if missing. <qc->ku.nxt_tx.ctx> is checked too, as
quic_tls_rotate_keys() moves it to <tls_ctx->tx.ctx>. If the derivation fails
again the packet is dropped and the peer retransmits it. A packet which
decrypted correctly is no longer dropped either.

This bug was introduced by commit a7d2c0946 ("MINOR: quic: Enable the Key
Update process").

Thanks to Bruno Henc for reporting this issue.

This must be backported as far as 2.6.

diff --git a/src/quic_rx.c b/src/quic_rx.c
index 594bd293aef142b46727aa6c42c9e9fadb88adc2..afdba0ce0f2ef047fb304343fb6b6dfaf9a43fb3 100644 (file)
--- a/src/quic_rx.c
+++ b/src/quic_rx.c
@@ -163,6 +163,16 @@ static int qc_pkt_decrypt(struct quic_conn *qc, struct quic_enc_level *qel,
                        else if (pkt->pn > qel->pktns->rx.largest_pn) {
                                /* Next key phase */
                                TRACE_PROTO("Key phase changed", QUIC_EV_CONN_RXPKT, qc);
+
+                               /* The next keys may be missing if a previous
+                                * update failed. Derive them again.
+                                */
+                               if ((!qc->ku.nxt_rx.ctx || !qc->ku.nxt_tx.ctx) &&
+                                   !quic_tls_key_update(qc)) {
+                                       TRACE_ERROR("quic_tls_key_update() failed", QUIC_EV_CONN_RXPKT, qc);
+                                       goto leave;
+                               }
+
                                kp_changed = 1;
                                rx_ctx = qc->ku.nxt_rx.ctx;
                                rx_iv  = qc->ku.nxt_rx.iv;
@@ -188,11 +198,11 @@ static int qc_pkt_decrypt(struct quic_conn *qc, struct quic_enc_level *qel,
                tls_ctx->flags ^= QUIC_FL_TLS_KP_BIT_SET;
                /* Store the lowest packet number received for the current key phase */
                tls_ctx->rx.pn = pkt->pn;
-               /* Prepare the next key update */
-               if (!quic_tls_key_update(qc)) {
+               /* Prepare the next key update. On failure, the keys are
+                * derived again at the next key phase change.
+                */
+               if (!quic_tls_key_update(qc))
                        TRACE_ERROR("quic_tls_key_update() failed", QUIC_EV_CONN_RXPKT, qc);
-                       goto leave;
-               }
        }
 
        /* Update the packet length (required to parse the frames). */