QUIC CONFORMANCE: RFC 9000 s. 12.3: PN duplicate suppression

Make sure PN duplicate suppression is side-channel safe by doing the duplicate test after AEAD verification. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #21135)
openssl · Jul 16, 2023 · dfe5e7f · dfe5e7f
1 parent 85bbef2
commit dfe5e7f
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 23 deletions.
diff --git a/include/internal/quic_record_rx.h b/include/internal/quic_record_rx.h
@@ -309,29 +309,28 @@ int ossl_qrx_unprocessed_read_pending(OSSL_QRX *qrx);
 uint64_t ossl_qrx_get_bytes_received(OSSL_QRX *qrx, int clear);
 
 /*
- * Sets a callback which is called when a packet is received and being
- * validated before being queued in the read queue. This is called before packet
- * body decryption. pn_space is a QUIC_PN_SPACE_* value denoting which PN space
- * the PN belongs to.
+ * Sets a callback which is called when a packet is received and being validated
+ * before being queued in the read queue. This is called after packet body
+ * decryption and authentication to prevent exposing side channels. pn_space is
+ * a QUIC_PN_SPACE_* value denoting which PN space the PN belongs to.
  *
  * If this callback returns 1, processing continues normally.
  * If this callback returns 0, the packet is discarded.
  *
  * Other packets in the same datagram will still be processed where possible.
  *
- * The intended use for this function is to allow early validation of whether
- * a PN is a potential duplicate before spending CPU time decrypting the
- * packet payload.
+ * The intended use for this function is to allow validation of whether a PN is
+ * a potential duplicate before spending CPU time decrypting the packet payload.
  *
  * The callback is optional and can be unset by passing NULL for cb.
  * cb_arg is an opaque value passed to cb.
  */
-typedef int (ossl_qrx_early_validation_cb)(QUIC_PN pn, int pn_space,
-                                           void *arg);
+typedef int (ossl_qrx_late_validation_cb)(QUIC_PN pn, int pn_space,
+                                          void *arg);
 
-int ossl_qrx_set_early_validation_cb(OSSL_QRX *qrx,
-                                     ossl_qrx_early_validation_cb *cb,
-                                     void *cb_arg);
+int ossl_qrx_set_late_validation_cb(OSSL_QRX *qrx,
+                                    ossl_qrx_late_validation_cb *cb,
+                                    void *cb_arg);
 
 /*
  * Forcibly injects a URXE which has been issued by the DEMUX into the QRX for

diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c
@@ -66,7 +66,7 @@ static int ch_on_crypto_send(const unsigned char *buf, size_t buf_len,
                              size_t *consumed, void *arg);
 static OSSL_TIME get_time(void *arg);
 static uint64_t get_stream_limit(int uni, void *arg);
-static int rx_early_validate(QUIC_PN pn, int pn_space, void *arg);
+static int rx_late_validate(QUIC_PN pn, int pn_space, void *arg);
 static void rxku_detected(QUIC_PN pn, void *arg);
 static int ch_retry(QUIC_CHANNEL *ch,
                     const unsigned char *retry_token,
@@ -252,9 +252,9 @@ static int ch_init(QUIC_CHANNEL *ch)
     if ((ch->qrx = ossl_qrx_new(&qrx_args)) == NULL)
         goto err;
 
-    if (!ossl_qrx_set_early_validation_cb(ch->qrx,
-                                          rx_early_validate,
-                                          ch))
+    if (!ossl_qrx_set_late_validation_cb(ch->qrx,
+                                         rx_late_validate,
+                                         ch))
         goto err;
 
     if (!ossl_qrx_set_key_update_cb(ch->qrx,
@@ -522,7 +522,7 @@ static uint64_t get_stream_limit(int uni, void *arg)
  * Called by QRX to determine if a packet is potentially invalid before trying
  * to decrypt it.
  */
-static int rx_early_validate(QUIC_PN pn, int pn_space, void *arg)
+static int rx_late_validate(QUIC_PN pn, int pn_space, void *arg)
 {
     QUIC_CHANNEL *ch = arg;
 

diff --git a/ssl/quic/quic_record_rx.c b/ssl/quic/quic_record_rx.c
@@ -143,7 +143,7 @@ struct ossl_qrx_st {
     uint64_t                    forged_pkt_count;
 
     /* Validation callback. */
-    ossl_qrx_early_validation_cb   *validation_cb;
+    ossl_qrx_late_validation_cb    *validation_cb;
     void                           *validation_cb_arg;
 
     /* Key update callback. */
@@ -545,6 +545,14 @@ static int qrx_validate_hdr(OSSL_QRX *qrx, RXE *rxe)
                                           &rxe->pn))
         return 0;
 
+    return 1;
+}
+
+/* Late packet header validation. */
+static int qrx_validate_hdr_late(OSSL_QRX *qrx, RXE *rxe)
+{
+    int pn_space = rxe_determine_pn_space(rxe);
+
     /*
      * Allow our user to decide whether to discard the packet before we try and
      * decrypt it.
@@ -957,9 +965,14 @@ static int qrx_process_pkt(OSSL_QRX *qrx, QUIC_URXE *urxe,
      * -----------------------------------------------------
      *
      * At this point, we have successfully authenticated the AEAD tag and no
-     * longer need to worry about exposing the Key Phase bit in timing channels.
-     * Check for a Key Phase bit differing from our expectation.
+     * longer need to worry about exposing the PN, PN length or Key Phase bit in
+     * timing channels. Invoke any configured validation callback to allow for
+     * rejection of duplicate PNs.
      */
+    if (!qrx_validate_hdr_late(qrx, rxe))
+        goto malformed;
+
+    /* Check for a Key Phase bit differing from our expectation. */
     if (rxe->hdr.type == QUIC_PKT_TYPE_1RTT
         && rxe->hdr.key_phase != (el->key_epoch & 1))
         qrx_key_update_initiated(qrx, rxe->pn);
@@ -1215,9 +1228,9 @@ uint64_t ossl_qrx_get_bytes_received(OSSL_QRX *qrx, int clear)
     return v;
 }
 
-int ossl_qrx_set_early_validation_cb(OSSL_QRX *qrx,
-                                     ossl_qrx_early_validation_cb *cb,
-                                     void *cb_arg)
+int ossl_qrx_set_late_validation_cb(OSSL_QRX *qrx,
+                                    ossl_qrx_late_validation_cb *cb,
+                                    void *cb_arg)
 {
     qrx->validation_cb       = cb;
     qrx->validation_cb_arg   = cb_arg;