From c5b2808623c62d8f4a61562c648072b47cfbc424 Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Mon, 20 May 2019 16:44:07 +0100 Subject: [PATCH] QUIC: Fix broken Key Update support Use the standard TLS 1.3 Key Update variant (broken since draft -13). Fix key_phase change detection (gboolean is signed, and 1 != -1, so it would always trigger a key update when KP1). Fix typo that breaks Key Update for the client (server_pp -> pp_state). Tested with attachment 17132 from the linked bug. Bug: 13881 Change-Id: I0246816e99d2e3ed509aa3ebb8a57b753399dde4 Reviewed-on: https://code.wireshark.org/review/33279 Petri-Dish: Peter Wu Tested-by: Petri Dish Buildbot Reviewed-by: Alexis La Goutte --- epan/dissectors/packet-quic.c | 63 ++++++++--------------------------- 1 file changed, 13 insertions(+), 50 deletions(-) diff --git a/epan/dissectors/packet-quic.c b/epan/dissectors/packet-quic.c index 97ccc956e1..3a717309ba 100644 --- a/epan/dissectors/packet-quic.c +++ b/epan/dissectors/packet-quic.c @@ -1238,10 +1238,6 @@ dissect_quic_frame_type(tvbuff_t *tvb, packet_info *pinfo, proto_tree *quic_tree #endif /* HAVE_LIBGCRYPT_AEAD */ #ifdef HAVE_LIBGCRYPT_AEAD -static gcry_error_t -qhkdf_expand(int md, const guint8 *secret, guint secret_len, - const char *label, guint8 *out, guint out_len); - static gboolean quic_cipher_init(quic_cipher *cipher, int hash_algo, guint8 key_length, guint8 *secret); @@ -1498,43 +1494,6 @@ quic_create_decoders(packet_info *pinfo, quic_info_data_t *quic_info, quic_ciphe return TRUE; } -/** - * Computes QHKDF-Expand(Secret, Label, Length). - * Caller must ensure that "out" is large enough for "out_len". - */ -static gcry_error_t -qhkdf_expand(int md, const guint8 *secret, guint secret_len, - const char *label, guint8 *out, guint out_len) -{ - /* https://tools.ietf.org/html/draft-ietf-quic-tls-10#section-5.2.1 - * QHKDF-Expand(Secret, Label, Length) = - * HKDF-Expand(Secret, QhkdfLabel, Length) - * struct { - * uint16 length = Length; - * opaque label<6..255> = "QUIC " + Label; - * } QhkdfLabel; - */ - gcry_error_t err; - const guint label_length = (guint) strlen(label); - - /* Some sanity checks */ - DISSECTOR_ASSERT(label_length > 0 && 5 + label_length <= 255); - - /* info = QhkdfLabel { length, label } */ - GByteArray *info = g_byte_array_new(); - const guint16 length = g_htons(out_len); - g_byte_array_append(info, (const guint8 *)&length, sizeof(length)); - - const guint8 label_vector_length = 5 + label_length; - g_byte_array_append(info, &label_vector_length, 1); - g_byte_array_append(info, "QUIC ", 5); - g_byte_array_append(info, label, label_length); - - err = hkdf_expand(md, secret, secret_len, info->data, info->len, out, out_len); - g_byte_array_free(info, TRUE); - return err; -} - /** * Tries to obtain the QUIC application traffic secrets. */ @@ -1579,12 +1538,13 @@ quic_cipher_init(quic_cipher *cipher, int hash_algo, guint8 key_length, guint8 * * Updates the packet protection secret to the next one. */ static void -quic_update_key(int hash_algo, quic_pp_state_t *pp_state, gboolean from_client) +quic_update_key(int hash_algo, quic_pp_state_t *pp_state) { guint hash_len = gcry_md_get_algo_dlen(hash_algo); - qhkdf_expand(hash_algo, pp_state->next_secret, hash_len, - from_client ? "client 1rtt" : "server 1rtt", - pp_state->next_secret, hash_len); + gboolean ret = quic_hkdf_expand_label(hash_algo, pp_state->next_secret, hash_len, + "traffic upd", pp_state->next_secret, hash_len); + /* This must always succeed as our hash algorithm was already validated. */ + DISSECTOR_ASSERT(ret); } /** @@ -1621,7 +1581,7 @@ quic_get_1rtt_hp_cipher(packet_info *pinfo, quic_info_data_t *quic_info, gboolea return NULL; } - /* Create initial cipher handles for KEY_PHASE 0 and 1. */ + // Create initial cipher handles for KEY_PHASE 0 using the 1-RTT keys. if (!quic_cipher_prepare(&client_pp->cipher[0], quic_info->hash_algo, quic_info->cipher_algo, quic_info->cipher_mode, client_pp->next_secret, &error) || !quic_cipher_prepare(&server_pp->cipher[0], quic_info->hash_algo, @@ -1629,7 +1589,9 @@ quic_get_1rtt_hp_cipher(packet_info *pinfo, quic_info_data_t *quic_info, gboolea quic_info->skip_decryption = TRUE; return NULL; } - quic_update_key(quic_info->hash_algo, pp_state, !from_server); + // Rotate the 1-RTT key for the client and server for the next key update. + quic_update_key(quic_info->hash_algo, client_pp); + quic_update_key(quic_info->hash_algo, server_pp); } // Note: Header Protect cipher does not change after Key Update. @@ -1659,13 +1621,14 @@ quic_get_pp_cipher(gboolean key_phase, quic_info_data_t *quic_info, gboolean fro * If the key phase changed, try to decrypt the packet using the new cipher. * If that fails, then it is either a malicious packet or out-of-order. * In that case, try the previous cipher (unless it is the very first KP1). + * '!!' is due to key_phase being a signed bitfield, it forces -1 into 1. */ - if (key_phase != pp_state->key_phase) { + if (key_phase != !!pp_state->key_phase) { quic_cipher new_cipher; memset(&new_cipher, 0, sizeof(quic_cipher)); if (!quic_cipher_prepare(&new_cipher, quic_info->hash_algo, - quic_info->cipher_algo, quic_info->cipher_mode, server_pp->next_secret, &error)) { + quic_info->cipher_algo, quic_info->cipher_mode, pp_state->next_secret, &error)) { /* This should never be reached, if the parameters were wrong * before, then it should have set "skip_decryption". */ REPORT_DISSECTOR_BUG("quic_cipher_prepare unexpectedly failed: %s", error); @@ -1679,7 +1642,7 @@ quic_get_pp_cipher(gboolean key_phase, quic_info_data_t *quic_info, gboolean fro /* Verified the cipher, use it from now on and rotate the key. */ quic_cipher_reset(&pp_state->cipher[key_phase]); pp_state->cipher[key_phase] = new_cipher; - quic_update_key(quic_info->hash_algo, pp_state, !from_server); + quic_update_key(quic_info->hash_algo, pp_state); pp_state->key_phase = key_phase; //pp_state->changed_in_pkn = pkn;