From: Hugo Landau Date: Tue, 6 Jun 2023 15:25:10 +0000 (+0100) Subject: QUIC CONFORMANCE: RFC 9000 s. 12.3: PN duplicate suppression X-Git-Tag: openssl-3.2.0-alpha1~450 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=dfe5e7fa987c0e79c165a677d6572a04105528e3;p=thirdparty%2Fopenssl.git 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 Reviewed-by: Matt Caswell Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/21135) --- diff --git a/include/internal/quic_record_rx.h b/include/internal/quic_record_rx.h index 29755e2df14..f9a69c6c537 100644 --- 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 index 76b117c32b9..4179b7e0862 100644 --- 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 index 7ee352a4710..4448a96556a 100644 --- 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;