osmocom
/
wireshark
11
0
Fork 1
Code Issues Pull Requests Projects Releases Wiki Activity

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 <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot
Reviewed-by: Alexis La Goutte <alexis.lagoutte@gmail.com>
This commit is contained in:
Peter Wu 2019-05-20 16:44:07 +01:00 committed by Alexis La Goutte
parent de5fd1634b
commit c5b2808623
1 changed files with 13 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

63
epan/dissectors/packet-quic.c
View File

@ -1238,10 +1238,6 @@ dissect_quic_frame_type(tvbuff_t *tvb, packet_info *pinfo, proto_tree *quic_tree
#endif /* HAVE_LIBGCRYPT_AEAD */ #endif /* HAVE_LIBGCRYPT_AEAD */
#ifdef 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 static gboolean
quic_cipher_init(quic_cipher *cipher, int hash_algo, guint8 key_length, guint8 *secret); 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; 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. * 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. * Updates the packet protection secret to the next one.
*/ */
static void 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); guint hash_len = gcry_md_get_algo_dlen(hash_algo);
qhkdf_expand(hash_algo, pp_state->next_secret, hash_len, gboolean ret = quic_hkdf_expand_label(hash_algo, pp_state->next_secret, hash_len,
from_client ? "client 1rtt" : "server 1rtt", "traffic upd", pp_state->next_secret, hash_len);
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; 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, 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_info->cipher_algo, quic_info->cipher_mode, client_pp->next_secret, &error) ||
!quic_cipher_prepare(&server_pp->cipher[0], quic_info->hash_algo, !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; quic_info->skip_decryption = TRUE;
return NULL; 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. // 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 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. * 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). * 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; quic_cipher new_cipher;
memset(&new_cipher, 0, sizeof(quic_cipher)); memset(&new_cipher, 0, sizeof(quic_cipher));
if (!quic_cipher_prepare(&new_cipher, quic_info->hash_algo, 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 /* This should never be reached, if the parameters were wrong
* before, then it should have set "skip_decryption". */ * before, then it should have set "skip_decryption". */
REPORT_DISSECTOR_BUG("quic_cipher_prepare unexpectedly failed: %s", error); 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. */ /* Verified the cipher, use it from now on and rotate the key. */
quic_cipher_reset(&pp_state->cipher[key_phase]); quic_cipher_reset(&pp_state->cipher[key_phase]);
pp_state->cipher[key_phase] = new_cipher; 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->key_phase = key_phase;
//pp_state->changed_in_pkn = pkn; //pp_state->changed_in_pkn = pkn;