From c93f766860cd4e13aea7253c2d807f6048aa635e Mon Sep 17 00:00:00 2001 From: Hugo Landau Date: Tue, 23 May 2023 12:23:06 +0100 Subject: [PATCH] QUIC RXDP: Strictly enforce ACK PNs with regard to TX key epochs Reviewed-by: Tomas Mraz Reviewed-by: Matt Caswell Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/21029) --- ssl/quic/quic_channel.c | 1 + ssl/quic/quic_channel_local.h | 15 +++++++++----- ssl/quic/quic_rx_depack.c | 39 +++++++++++++++++++++++++++++++++-- 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c index 017a1ab28f7e8..951160a7f0788 100644 --- a/ssl/quic/quic_channel.c +++ b/ssl/quic/quic_channel.c @@ -691,6 +691,7 @@ static void rxku_detected(QUIC_PN pn, void *arg) ch->rxku_pending_confirm = 1; ch->rxku_trigger_pn = pn; ch->rxku_update_end_deadline = ossl_time_add(get_time(ch), pto); + ch->rxku_expected = 0; if (decision == DECISION_SOLICITED_TXKU) /* NOT gated by usual txku_allowed() */ diff --git a/ssl/quic/quic_channel_local.h b/ssl/quic/quic_channel_local.h index c4be7d792247b..607f109119d41 100644 --- a/ssl/quic/quic_channel_local.h +++ b/ssl/quic/quic_channel_local.h @@ -228,11 +228,11 @@ struct quic_channel_st { OSSL_TIME rxku_update_end_deadline; /* - * The first (application space) PN sent with a new key phase. Valid if - * txku_in_progress. Once a packet we sent with a PN p (p >= txku_pn) is - * ACKed, the TXKU is considered completed and txku_in_progress becomes 0. - * For sanity's sake, such a PN p should also be <= the highest PN we have - * ever sent, of course. + * The first (application space) PN sent with a new key phase. Valid if the + * QTX key epoch is greater than 0. Once a packet we sent with a PN p (p >= + * txku_pn) is ACKed, the TXKU is considered completed and txku_in_progress + * becomes 0. For sanity's sake, such a PN p should also be <= the highest + * PN we have ever sent, of course. */ QUIC_PN txku_pn; @@ -386,6 +386,11 @@ struct quic_channel_st { /* Temporary variable indicating rxku_pending_confirm is to become 0. */ unsigned int rxku_pending_confirm_done : 1; + + /* + * If set, RXKU is expected (because we initiated a spontaneous TXKU). + */ + unsigned int rxku_expected : 1; }; # endif diff --git a/ssl/quic/quic_rx_depack.c b/ssl/quic/quic_rx_depack.c index f22e658e8affe..12aae998d9578 100644 --- a/ssl/quic/quic_rx_depack.c +++ b/ssl/quic/quic_rx_depack.c @@ -60,7 +60,8 @@ static int depack_do_frame_ping(PACKET *pkt, QUIC_CHANNEL *ch, static int depack_do_frame_ack(PACKET *pkt, QUIC_CHANNEL *ch, int packet_space, OSSL_TIME received, - uint64_t frame_type) + uint64_t frame_type, + OSSL_QRX_PKT *qpacket) { OSSL_QUIC_FRAME_ACK ack; OSSL_QUIC_ACK_RANGE *ack_ranges = NULL; @@ -80,6 +81,40 @@ static int depack_do_frame_ack(PACKET *pkt, QUIC_CHANNEL *ch, if (!ossl_quic_wire_decode_frame_ack(pkt, ack_delay_exp, &ack, NULL)) goto malformed; + if (qpacket->hdr->type == QUIC_PKT_TYPE_1RTT + && (qpacket->key_epoch < ossl_qrx_get_key_epoch(ch->qrx) + || ch->rxku_expected) + && ack.ack_ranges[0].end >= ch->txku_pn) { + /* + * RFC 9001 s. 6.2: An endpoint that receives an acknowledgment that is + * carried in a packet protected with old keys where any acknowledged + * packet was protected with newer keys MAY treat that as a connection + * error of type KEY_UPDATE_ERROR. + * + * Two cases to handle here: + * + * - We did spontaneous TXKU, the peer has responded in kind and we + * have detected RXKU; !ch->rxku_expected, but then it sent a packet + * with old keys acknowledging a packet in the new key epoch. + * + * This also covers the case where we got RXKU and triggered + * solicited TXKU, and then for some reason the peer sent an ACK of + * a PN in our new TX key epoch with old keys. + * + * - We did spontaneous TXKU; ch->txku_pn is the starting PN of our + * new TX key epoch; the peer has not initiated a solicited TXKU in + * response (so we have not detected RXKU); in this case the RX key + * epoch has not incremented and ch->rxku_expected is still 1. + */ + ossl_quic_channel_raise_protocol_error(ch, + QUIC_ERR_KEY_UPDATE_ERROR, + frame_type, + "acked packet which initiated a " + "key update without a " + "corresponding key update"); + return 0; + } + if (!ossl_ackm_on_rx_ack_frame(ch->ackm, &ack, packet_space, received)) goto malformed; @@ -837,7 +872,7 @@ static int depack_process_frames(QUIC_CHANNEL *ch, PACKET *pkt, return 0; } if (!depack_do_frame_ack(pkt, ch, packet_space, received, - frame_type)) + frame_type, parent_pkt)) return 0; break;