From ac028e551d3fda94f2584cf86a006cc78bd0d38d Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Wed, 10 Jun 2020 18:14:39 +0200 Subject: [PATCH] Code Review --- src/internal.c | 106 ++++++++++++++++++++++++++++++--------------- src/ssl.c | 24 ++-------- src/tls.c | 78 ++------------------------------- src/tls13.c | 4 +- wolfssl/internal.h | 10 +++-- 5 files changed, 87 insertions(+), 135 deletions(-) diff --git a/src/internal.c b/src/internal.c index 2789ad57b..3f58fe9c3 100644 --- a/src/internal.c +++ b/src/internal.c @@ -6723,12 +6723,7 @@ static WC_INLINE void GetSEQIncrement(WOLFSSL* ssl, int verify, word32 seq[2]) static WC_INLINE void DtlsGetSEQ(WOLFSSL* ssl, int order, word32 seq[2]) { #ifdef HAVE_SECURE_RENEGOTIATION - /* if ssl->secure_renegotiation->tmp_keys.dtls_epoch > ssl->keys.dtls_epoch then PREV_ORDER - * refers to the current epoch */ - if (order == PREV_ORDER && ssl->secure_renegotiation && - ssl->secure_renegotiation->tmp_keys.dtls_epoch > ssl->keys.dtls_epoch) { - order = CUR_ORDER; - } + order = DtlsCheckOrder(ssl, order); #endif if (order == PREV_ORDER) { /* Previous epoch case */ @@ -6776,12 +6771,7 @@ static WC_INLINE void DtlsSEQIncrement(WOLFSSL* ssl, int order) { word32 seq; #ifdef HAVE_SECURE_RENEGOTIATION - /* if ssl->secure_renegotiation->tmp_keys.dtls_epoch > ssl->keys.dtls_epoch then PREV_ORDER - * refers to the current epoch */ - if (order == PREV_ORDER && ssl->secure_renegotiation && - ssl->secure_renegotiation->tmp_keys.dtls_epoch > ssl->keys.dtls_epoch) { - order = CUR_ORDER; - } + order = DtlsCheckOrder(ssl, order); #endif if (order == PREV_ORDER) { @@ -6809,7 +6799,7 @@ static WC_INLINE void DtlsSEQIncrement(WOLFSSL* ssl, int order) #endif /* WOLFSSL_DTLS */ #if defined(WOLFSSL_DTLS) || !defined(WOLFSSL_NO_TLS12) -static WC_INLINE void WriteSEQ(WOLFSSL* ssl, int verifyOrder, byte* out) +void WriteSEQ(WOLFSSL* ssl, int verifyOrder, byte* out) { word32 seq[2] = {0, 0}; @@ -7232,9 +7222,14 @@ int VerifyForDtlsMsgPoolSend(WOLFSSL* ssl, byte type, word32 fragOffset) } +/** + * Verify if message `item` from `ssl->dtls_tx_msg_list` should be deleted + * depending on the current state of the handshake negotiation. + */ int VerifyForTxDtlsMsgDelete(WOLFSSL* ssl, DtlsMsg* item) { if (item->epoch < ssl->keys.dtls_epoch - 1) + /* Messages not from current or previous epoch can be deleted */ return 1; switch (ssl->options.side) { case WOLFSSL_CLIENT_END: @@ -7328,8 +7323,7 @@ int DtlsMsgPoolSend(WOLFSSL* ssl, int sendOnlyFirstPacket) * ssl->keys otherwise * PREV_ORDER will always use ssl->keys */ - if (ssl->secure_renegotiation && - ssl->secure_renegotiation->tmp_keys.dtls_epoch != 0) { + if (DtlsSCRKeysSet(ssl)) { if (pool->epoch == ssl->secure_renegotiation->tmp_keys.dtls_epoch) epochOrder = CUR_ORDER; else @@ -14098,17 +14092,10 @@ static WC_INLINE int DecryptDo(WOLFSSL* ssl, byte* plain, const byte* input, ssl->decrypt.additional + AEAD_LEN_OFFSET); #if defined(WOLFSSL_DTLS) && defined(HAVE_SECURE_RENEGOTIATION) - if (ssl->options.dtls && ssl->secure_renegotiation && - ssl->secure_renegotiation->tmp_keys.dtls_epoch != 0) { - if (ssl->keys.curEpoch == - ssl->secure_renegotiation->tmp_keys.dtls_epoch) - XMEMCPY(ssl->decrypt.nonce, - ssl->secure_renegotiation->tmp_keys.aead_dec_imp_IV, - AESGCM_IMP_IV_SZ); - else - XMEMCPY(ssl->decrypt.nonce, ssl->keys.aead_dec_imp_IV, - AESGCM_IMP_IV_SZ); - } + if (ssl->options.dtls && IsDtlsMsgSCRKeys(ssl)) + XMEMCPY(ssl->decrypt.nonce, + ssl->secure_renegotiation->tmp_keys.aead_dec_imp_IV, + AESGCM_IMP_IV_SZ); else #endif XMEMCPY(ssl->decrypt.nonce, ssl->keys.aead_dec_imp_IV, @@ -14237,8 +14224,7 @@ static WC_INLINE int Decrypt(WOLFSSL* ssl, byte* plain, const byte* input, case CIPHER_STATE_DO: { #if defined(WOLFSSL_DTLS) && defined(HAVE_SECURE_RENEGOTIATION) - if (ssl->options.dtls && ssl->secure_renegotiation && - ssl->secure_renegotiation->tmp_keys.dtls_epoch != 0) { + if (ssl->options.dtls && DtlsSCRKeysSet(ssl)) { /* For epochs >1 the current cipher parameters are located in * ssl->secure_renegotiation->tmp_keys. Previous cipher * parameters and for epoch 1 use ssl->keys */ @@ -16154,8 +16140,7 @@ int BuildMessage(WOLFSSL* ssl, byte* output, int outSz, const byte* input, ERROR_OUT(BAD_FUNC_ARG, exit_buildmsg); } #if defined(WOLFSSL_DTLS) && defined(HAVE_SECURE_RENEGOTIATION) - if (ssl->options.dtls && ssl->secure_renegotiation && - ssl->secure_renegotiation->tmp_keys.dtls_epoch != 0) { + if (ssl->options.dtls && DtlsSCRKeysSet(ssl)) { /* For epochs >1 the current cipher parameters are located in * ssl->secure_renegotiation->tmp_keys. Previous cipher * parameters and for epoch 1 use ssl->keys */ @@ -16408,10 +16393,7 @@ int BuildMessage(WOLFSSL* ssl, byte* output, int outSz, const byte* input, word16 dtls_sequence_number_hi; word32 dtls_sequence_number_lo; int swap_seq = ssl->options.dtls && epochOrder == PREV_ORDER && - ssl->secure_renegotiation && - ssl->secure_renegotiation->tmp_keys.dtls_epoch != 0 && - ssl->secure_renegotiation->tmp_keys.dtls_epoch == - ssl->keys.dtls_epoch; + DtlsUseSCRKeys(ssl); if (swap_seq) { dtls_epoch = ssl->keys.dtls_epoch; dtls_sequence_number_hi = ssl->keys.dtls_sequence_number_hi; @@ -17137,7 +17119,8 @@ int SendCertificateRequest(WOLFSSL* ssl) #endif } - sendSz += cipherExtraData(ssl); + if (IsEncryptionOn(ssl, 1)) + sendSz += cipherExtraData(ssl); /* check for available size */ if ((ret = CheckAvailableSize(ssl, sendSz)) != 0) @@ -17543,6 +17526,59 @@ int SendCertificateStatus(WOLFSSL* ssl) #endif /* WOLFSSL_NO_TLS12 */ + +#if defined(HAVE_SECURE_RENEGOTIATION) && defined(WOLFSSL_DTLS) +/** + * Check if the SCR keys are set in ssl->secure_renegotiation->tmp_keys. + */ +int DtlsSCRKeysSet(WOLFSSL* ssl) +{ + return ssl->secure_renegotiation && + ssl->secure_renegotiation->tmp_keys.dtls_epoch != 0; +} + +/** + * ssl->keys contains the current cipher parameters only for epoch 1. For + * epochs >1 ssl->secure_renegotiation->tmp_keys contains the current + * cipher parameters. This function checks if the message currently being + * processed should use ssl->keys or ssl->secure_renegotiation->tmp_keys. + */ +int IsDtlsMsgSCRKeys(WOLFSSL* ssl) +{ + return DtlsSCRKeysSet(ssl) && + ssl->keys.curEpoch == + ssl->secure_renegotiation->tmp_keys.dtls_epoch; +} + +/** + * ssl->keys contains the current cipher parameters only for epoch 1. For + * epochs >1 ssl->secure_renegotiation->tmp_keys contains the current + * cipher parameters. This function checks if the message currently being + * built should use ssl->keys or ssl->secure_renegotiation->tmp_keys. + */ +int DtlsUseSCRKeys(WOLFSSL* ssl) +{ + return DtlsSCRKeysSet(ssl) && + ssl->secure_renegotiation->tmp_keys.dtls_epoch == + ssl->keys.dtls_epoch; +} + +/** + * If ssl->secure_renegotiation->tmp_keys.dtls_epoch > ssl->keys.dtls_epoch + * then PREV_ORDER refers to the current epoch. + * */ +int DtlsCheckOrder(WOLFSSL* ssl, int order) +{ + if (order == PREV_ORDER && ssl->secure_renegotiation && + ssl->secure_renegotiation->tmp_keys.dtls_epoch > ssl->keys.dtls_epoch) { + return CUR_ORDER; + } + else { + return order; + } +} +#endif /* HAVE_SECURE_RENEGOTIATION && WOLFSSL_DTLS */ + /* If secure renegotiation is disabled, this will always return false. * Otherwise it checks to see if we are currently renegotiating. */ static WC_INLINE int IsSCR(WOLFSSL* ssl) diff --git a/src/ssl.c b/src/ssl.c index 3b74066e7..eeb024bed 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -3241,36 +3241,18 @@ const byte* wolfSSL_GetDtlsMacSecret(WOLFSSL* ssl, int verify, int epochOrder) return NULL; #ifdef HAVE_SECURE_RENEGOTIATION - /* ssl->keys contains the current cipher parameters only for epoch 1. For - * epochs >1 ssl->secure_renegotiation->tmp_keys contains the current - * cipher parameters */ switch (epochOrder) { case PEER_ORDER: - if (ssl->secure_renegotiation && - ssl->secure_renegotiation->tmp_keys.dtls_epoch != 0 && - ssl->keys.curEpoch == - ssl->secure_renegotiation->tmp_keys.dtls_epoch) + if (IsDtlsMsgSCRKeys(ssl)) keys = &ssl->secure_renegotiation->tmp_keys; else keys = &ssl->keys; break; case PREV_ORDER: - if (ssl->keys.dtls_epoch > 1 || - (ssl->secure_renegotiation && - ssl->secure_renegotiation->tmp_keys.dtls_epoch != 0)) - keys = &ssl->keys; - else { - WOLFSSL_MSG("No previous cipher epoch"); - return NULL; - } + keys = &ssl->keys; break; case CUR_ORDER: - if (ssl->secure_renegotiation && - ssl->secure_renegotiation->tmp_keys.dtls_epoch != 0 && - ssl->secure_renegotiation->tmp_keys.dtls_epoch == - ssl->keys.dtls_epoch) - /* new keys are in scr and are only current when the - * ssl->keys.dtls_epoch matches */ + if (DtlsUseSCRKeys(ssl)) keys = &ssl->secure_renegotiation->tmp_keys; else keys = &ssl->keys; diff --git a/src/tls.c b/src/tls.c index 18c911bfb..51ffe1757 100644 --- a/src/tls.c +++ b/src/tls.c @@ -643,79 +643,6 @@ int wolfSSL_make_eap_keys(WOLFSSL* ssl, void* msk, unsigned int len, } -static WC_INLINE void GetSEQIncrement(WOLFSSL* ssl, int verify, word32 seq[2]) -{ - if (verify) { - seq[0] = ssl->keys.peer_sequence_number_hi; - seq[1] = ssl->keys.peer_sequence_number_lo++; - if (seq[1] > ssl->keys.peer_sequence_number_lo) { - /* handle rollover */ - ssl->keys.peer_sequence_number_hi++; - } - } - else { - seq[0] = ssl->keys.sequence_number_hi; - seq[1] = ssl->keys.sequence_number_lo++; - if (seq[1] > ssl->keys.sequence_number_lo) { - /* handle rollover */ - ssl->keys.sequence_number_hi++; - } - } -} - - -#ifdef WOLFSSL_DTLS -static WC_INLINE void DtlsGetSEQ(WOLFSSL* ssl, int order, word32 seq[2]) -{ -#ifdef HAVE_SECURE_RENEGOTIATION - /* if ssl->secure_renegotiation->tmp_keys.dtls_epoch > ssl->keys.dtls_epoch then PREV_ORDER - * refers to the current epoch */ - if (order == PREV_ORDER && ssl->secure_renegotiation && - ssl->secure_renegotiation->tmp_keys.dtls_epoch > ssl->keys.dtls_epoch) { - order = CUR_ORDER; - } -#endif - if (order == PREV_ORDER) { - /* Previous epoch case */ - seq[0] = (((word32)ssl->keys.dtls_epoch - 1) << 16) | - (ssl->keys.dtls_prev_sequence_number_hi & 0xFFFF); - seq[1] = ssl->keys.dtls_prev_sequence_number_lo; - } - else if (order == PEER_ORDER) { - seq[0] = ((word32)ssl->keys.curEpoch << 16) | - (ssl->keys.curSeq_hi & 0xFFFF); - seq[1] = ssl->keys.curSeq_lo; /* explicit from peer */ - } - else { - seq[0] = ((word32)ssl->keys.dtls_epoch << 16) | - (ssl->keys.dtls_sequence_number_hi & 0xFFFF); - seq[1] = ssl->keys.dtls_sequence_number_lo; - } -} -#endif /* WOLFSSL_DTLS */ - - -static WC_INLINE void WriteSEQ(WOLFSSL* ssl, int verifyOrder, byte* out) -{ - word32 seq[2] = {0, 0}; - - if (!ssl->options.dtls) { - GetSEQIncrement(ssl, verifyOrder, seq); - } - else { -#ifdef WOLFSSL_DTLS - DtlsGetSEQ(ssl, verifyOrder, seq); -#endif - } - - c32toa(seq[0], out); - c32toa(seq[1], out + OPAQUE32_LEN); -} - - -/*** end copy ***/ - - /* return HMAC digest type in wolfSSL format */ int wolfSSL_GetHmacType(WOLFSSL* ssl) { @@ -1208,7 +1135,10 @@ int TLS_hmac(WOLFSSL* ssl, byte* digest, const byte* in, word32 sz, int padSz, } #endif - wolfSSL_SetTlsHmacInner(ssl, myInner, sz, content, epochOrder); + if (!ssl->options.dtls) + wolfSSL_SetTlsHmacInner(ssl, myInner, sz, content, verify); + else + wolfSSL_SetTlsHmacInner(ssl, myInner, sz, content, epochOrder); #if defined(WOLFSSL_RENESAS_TSIP_TLS) && \ !defined(NO_WOLFSSL_RENESAS_TSIP_TLS_SESSION) if (tsip_useable(ssl)) { diff --git a/src/tls13.c b/src/tls13.c index c70e37241..7507e417a 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -1474,7 +1474,7 @@ static void AddTls13FragHeaders(byte* output, word32 fragSz, word32 fragOffset, * verifyOrder Which set of sequence numbers to use. * out The buffer to write into. */ -static WC_INLINE void WriteSEQ(WOLFSSL* ssl, int verifyOrder, byte* out) +static WC_INLINE void WriteSEQTls13(WOLFSSL* ssl, int verifyOrder, byte* out) { word32 seq[2] = {0, 0}; @@ -1510,7 +1510,7 @@ static WC_INLINE void BuildTls13Nonce(WOLFSSL* ssl, byte* nonce, const byte* iv, int i; /* The nonce is the IV with the sequence XORed into the last bytes. */ - WriteSEQ(ssl, order, nonce + AEAD_NONCE_SZ - SEQ_SZ); + WriteSEQTls13(ssl, order, nonce + AEAD_NONCE_SZ - SEQ_SZ); for (i = 0; i < AEAD_NONCE_SZ - SEQ_SZ; i++) nonce[i] = iv[i]; for (; i < AEAD_NONCE_SZ; i++) diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 304ed443b..1aeec5125 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -4516,10 +4516,14 @@ WOLFSSL_LOCAL int GrowInputBuffer(WOLFSSL* ssl, int size, int usedLength); WOLFSSL_LOCAL int DtlsMsgPoolSend(WOLFSSL*, int); #endif /* WOLFSSL_DTLS */ -#ifndef NO_TLS +#if defined(HAVE_SECURE_RENEGOTIATION) && defined(WOLFSSL_DTLS) + WOLFSSL_LOCAL int DtlsSCRKeysSet(WOLFSSL* ssl); + WOLFSSL_LOCAL int IsDtlsMsgSCRKeys(WOLFSSL* ssl); + WOLFSSL_LOCAL int DtlsUseSCRKeys(WOLFSSL* ssl); + WOLFSSL_LOCAL int DtlsCheckOrder(WOLFSSL* ssl, int order); +#endif - -#endif /* NO_TLS */ + WOLFSSL_LOCAL void WriteSEQ(WOLFSSL* ssl, int verifyOrder, byte* out); #if defined(WOLFSSL_TLS13) && (defined(HAVE_SESSION_TICKET) || !defined(NO_PSK)) WOLFSSL_LOCAL word32 TimeNowInMilliseconds(void);