From 3ba84d5bd67513c7839cfad9eaf21e05cae1f461 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 20 Nov 2020 10:17:20 +0100 Subject: [PATCH 01/21] Improve documentation of cipher_auth_xxcrypt() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Document constraints on buffers/pointers NULLability explicitly. - Simplify terminology around IV/nonce: all AEADs implemented so far call that a nonce. Keep the parameter names (iv, iv_len) to avoid having to change the code (or having different names in the header and C files). - Align documentation to the code regarding parameter constraints: the documentation said the for ciphers with fixed nonce/tag length, the iv_len/tag_len arguments were ignored, while the code enforced them to be the expected value. This is more consistent with what's done with GCM/CCM, which for tag_len for example accept more than one value, but from a relatively small set, and will return errors for values outside that set. Accepting a single value is a particular case of that (the set of acceptable value only has one element). Don't document behaviour with NIST KW as we're about to change that. Note: this function is currently only defined if at least one of GCM, CCM or ChachaPoly is enabled, even though it's supposed to handle NIST KW as well. No need to fix this as the function will soon no longer support NIST KW. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/cipher.h | 80 +++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 30 deletions(-) diff --git a/include/mbedtls/cipher.h b/include/mbedtls/cipher.h index 8827e0b79..43bc1e270 100644 --- a/include/mbedtls/cipher.h +++ b/include/mbedtls/cipher.h @@ -862,25 +862,35 @@ int mbedtls_cipher_crypt( mbedtls_cipher_context_t *ctx, * * \param ctx The generic cipher context. This must be initialized and * bound to a key. - * \param iv The IV to use, or NONCE_COUNTER for CTR-mode ciphers. - * This must be a readable buffer of at least \p iv_len - * Bytes. - * \param iv_len The IV length for ciphers with variable-size IV. - * This parameter is discarded by ciphers with fixed-size IV. + * \param iv The nonce to use. This must be a readable buffer of + * at least \p iv_len Bytes and must not be \c NULL. + * \param iv_len The length of the nonce. This must satisfy the + * constraints imposed by the AEAD cipher used. * \param ad The additional data to authenticate. This must be a - * readable buffer of at least \p ad_len Bytes. + * readable buffer of at least \p ad_len Bytes, and may + * be \c NULL is \p ad_len is \c 0. * \param ad_len The length of \p ad. * \param input The buffer holding the input data. This must be a - * readable buffer of at least \p ilen Bytes. + * readable buffer of at least \p ilen Bytes, and may be + * \c NULL if \p ilen is \c 0. * \param ilen The length of the input data. - * \param output The buffer for the output data. This must be able to - * hold at least \p ilen Bytes. - * \param olen The length of the output data, to be updated with the - * actual number of Bytes written. This must not be - * \c NULL. + * \param output The buffer for the output data. This must be a + * writable buffer of at least \p ilen Bytes, and must + * not be \c NULL. + * \param olen This will be filled with the actual number of Bytes + * written to the \p output buffer. This must point to a + * writable object of type \c size_t. * \param tag The buffer for the authentication tag. This must be a - * writable buffer of at least \p tag_len Bytes. - * \param tag_len The desired length of the authentication tag. + * writable buffer of at least \p tag_len Bytes. See note + * below regarding restrictions with PSA-based contexts. + * \param tag_len The desired length of the authentication tag. This + * must match the constraints imposed by the AEAD cipher + * used, and in particuler must not be \c 0. + * + * \note If the context is based on PSA (that is, it was set up + * with mbedtls_cipher_setup_psa()), then it is required + * that \c tag == output + ilen. That is, the tag must be + * appended to the ciphertext as recommended by RFC 5116. * * \return \c 0 on success. * \return #MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA on @@ -903,25 +913,35 @@ int mbedtls_cipher_auth_encrypt( mbedtls_cipher_context_t *ctx, * * \param ctx The generic cipher context. This must be initialized and * and bound to a key. - * \param iv The IV to use, or NONCE_COUNTER for CTR-mode ciphers. - * This must be a readable buffer of at least \p iv_len - * Bytes. - * \param iv_len The IV length for ciphers with variable-size IV. - * This parameter is discarded by ciphers with fixed-size IV. - * \param ad The additional data to be authenticated. This must be a - * readable buffer of at least \p ad_len Bytes. + * \param iv The nonce to use. This must be a readable buffer of + * at least \p iv_len Bytes and must not be \c NULL. + * \param iv_len The length of the nonce. This must satisfy the + * constraints imposed by the AEAD cipher used. + * \param ad The additional data to authenticate. This must be a + * readable buffer of at least \p ad_len Bytes, and may + * be \c NULL is \p ad_len is \c 0. * \param ad_len The length of \p ad. * \param input The buffer holding the input data. This must be a - * readable buffer of at least \p ilen Bytes. + * readable buffer of at least \p ilen Bytes, and may be + * \c NULL if \p ilen is \c 0. * \param ilen The length of the input data. - * \param output The buffer for the output data. - * This must be able to hold at least \p ilen Bytes. - * \param olen The length of the output data, to be updated with the - * actual number of Bytes written. This must not be - * \c NULL. - * \param tag The buffer holding the authentication tag. This must be - * a readable buffer of at least \p tag_len Bytes. - * \param tag_len The length of the authentication tag. + * \param output The buffer for the output data. This must be a + * writable buffer of at least \p ilen Bytes, and must + * not be \c NULL. + * \param olen This will be filled with the actual number of Bytes + * written to the \p output buffer. This must point to a + * writable object of type \c size_t. + * \param tag The buffer for the authentication tag. This must be a + * readable buffer of at least \p tag_len Bytes. See note + * below regarding restrictions with PSA-based contexts. + * \param tag_len The length of the authentication tag. This must match + * the constraints imposed by the AEAD cipher used, and in + * particular must not be \c 0. + * + * \note If the context is based on PSA (that is, it was set up + * with mbedtls_cipher_setup_psa()), then it is required + * that \c tag == input + len. That is, the tag must be + * appended to the ciphertext as recommended by RFC 5116. * * \return \c 0 on success. * \return #MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA on From 9cc079db7a4f55cd7ad56e8af2c28ce0396710f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 25 Nov 2020 12:57:47 +0100 Subject: [PATCH 02/21] Declare cipher_auth_{en,de}crypt_ext() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Work in progress: next steps are to implement and test it. Compared to the existing non-ext version: - to separate tag parameter - explicit output_len parameter Also, this version will retain support for NIST_KW (hence documents it), while the non-ext version will lose it in a few commits. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/cipher.h | 108 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/include/mbedtls/cipher.h b/include/mbedtls/cipher.h index 43bc1e270..49b8b5e9e 100644 --- a/include/mbedtls/cipher.h +++ b/include/mbedtls/cipher.h @@ -957,6 +957,114 @@ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, const unsigned char *tag, size_t tag_len ); #endif /* MBEDTLS_CIPHER_MODE_AEAD */ +#if defined(MBEDTLS_CIPHER_MODE_AEAD) || defined(MBEDTLS_NIST_KW_C) +/** + * \brief The autenticated encryption (AEAD/NIST_KW) function. + * + * \note For AEAD modes, the tag will be appended to the + * ciphertext, as recommended by RFC 5116. + * (NIST_KW doesn't have a separate tag.) + * + * \param ctx The generic cipher context. This must be initialized and + * bound to a key. + * \param iv The nonce to use. This must be a readable buffer of + * at least \p iv_len Bytes and may be \c NULL if \p + * iv_len is \c 0. + * \param iv_len The length of the nonce. For AEAD ciphers, this must satisfy the + * constraints imposed by the cipher used. For NIST_KW, + * this must be \c 0. + * \param ad The additional data to authenticate. This must be a + * readable buffer of at least \p ad_len Bytes, and may + * be \c NULL is \p ad_len is \c 0. + * \param ad_len The length of \p ad. For NIST_KW, this must be \c 0. + * \param input The buffer holding the input data. This must be a + * readable buffer of at least \p ilen Bytes, and may be + * \c NULL if \p ilen is \c 0. + * \param ilen The length of the input data. + * \param output The buffer for the output data. This must be a + * writable buffer of at least \p output_len Bytes, and + * must not be \c NULL. + * \param output_len The length of the \p output buffer in Bytes. For AEAD + * ciphers, this must be at least \p ilen + \p tag_len. + * For NIST_KW, this must be at least \p ilen + 8 + * (rounded up to a multiple of 8 if KWP is used); + * \p ilen + 15 is always a safe value. + * \param olen This will be filled with the actual number of Bytes + * written to the \p output buffer. This must point to a + * writable object of type \c size_t. + * \param tag_len The desired length of the authentication tag. For AEAD + * ciphers, this must match the constraints imposed by + * the cipher used, and in particuler must not be \c 0. + * For NIST_KW, this must be \c 0. + * + * \return \c 0 on success. + * \return #MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA on + * parameter-verification failure. + * \return A cipher-specific error code on failure. + */ +int mbedtls_cipher_auth_encrypt_ext( mbedtls_cipher_context_t *ctx, + const unsigned char *iv, size_t iv_len, + const unsigned char *ad, size_t ad_len, + const unsigned char *input, size_t ilen, + unsigned char *output, size_t output_len, + size_t *olen, size_t tag_len ); + +/** + * \brief The autenticated encryption (AEAD/NIST_KW) function. + * + * \note If the data is not authentic, then the output buffer + * is zeroed out to prevent the unauthentic plaintext being + * used, making this interface safer. + * + * \note For AEAD modes, the tag must be appended to the + * ciphertext, as recommended by RFC 5116. + * (NIST_KW doesn't have a separate tag.) + * + * \param ctx The generic cipher context. This must be initialized and + * and bound to a key. + * \param iv The nonce to use. This must be a readable buffer of + * at least \p iv_len Bytes and may be \c NULL if \p + * iv_len is \c 0. + * \param iv_len The length of the nonce. For AEAD ciphers, this must satisfy the + * constraints imposed by the cipher used. For NIST_KW, + * this must be \c 0. + * \param ad The additional data to authenticate. This must be a + * readable buffer of at least \p ad_len Bytes, and may + * be \c NULL is \p ad_len is \c 0. + * \param ad_len The length of \p ad. For NIST_KW, this must be \c 0. + * \param input The buffer holding the input data. This must be a + * readable buffer of at least \p ilen Bytes, and may be + * \c NULL if \p ilen is \c 0. + * \param ilen The length of the input data. For AEAD ciphers this + * must be at least \p tag_len. For NIST_KW this must be + * at least \c 8. + * \param output The buffer for the output data. This must be a + * writable buffer of at least \p output_len Bytes, and + * may be \c NULL if \p output_len is \c 0. + * \param output_len The length of the \p output buffer in Bytes. For AEAD + * ciphers, this must be at least \p ilen - \p tag_len. + * For NIST_KW, this must be at least \p ilen - 8. + * \param olen This will be filled with the actual number of Bytes + * written to the \p output buffer. This must point to a + * writable object of type \c size_t. + * \param tag_len The actual length of the authentication tag. For AEAD + * ciphers, this must match the constraints imposed by + * the cipher used, and in particuler must not be \c 0. + * For NIST_KW, this must be \c 0. + * + * \return \c 0 on success. + * \return #MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA on + * parameter-verification failure. + * \return #MBEDTLS_ERR_CIPHER_AUTH_FAILED if data is not authentic. + * \return A cipher-specific error code on failure. + */ +int mbedtls_cipher_auth_decrypt_ext( mbedtls_cipher_context_t *ctx, + const unsigned char *iv, size_t iv_len, + const unsigned char *ad, size_t ad_len, + const unsigned char *input, size_t ilen, + unsigned char *output, size_t output_len, + size_t *olen, size_t tag_len ); +#endif /* MBEDTLS_CIPHER_MODE_AEAD || MBEDTLS_NIST_KW_C */ #ifdef __cplusplus } #endif From faddf98bea37e78f505e15fa85509282a038498b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 25 Nov 2020 13:39:47 +0100 Subject: [PATCH 03/21] Implement cipher_auth_{en,de}crypt_ext() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Work in progress: next step is to test it! Extract the part that is common with non-ext version to a new internal function. (We can't just use the non-ext version for that, as it's going to be deprecated.) Currently the NIST_KW part is somewhat duplicated between the ext and non-ext versions, but that's OK because it will soon be removed from the non-ext version. Signed-off-by: Manuel Pégourié-Gonnard --- library/cipher.c | 206 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 169 insertions(+), 37 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index 853eeec20..20e1e7b3a 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1288,23 +1288,16 @@ int mbedtls_cipher_crypt( mbedtls_cipher_context_t *ctx, #if defined(MBEDTLS_CIPHER_MODE_AEAD) /* - * Packet-oriented encryption for AEAD modes + * Packet-oriented encryption for AEAD modes: internal function shared by + * mbedtls_cipher_auth_encrypt() and mbedtls_cipher_auth_encrypt_ext(). */ -int mbedtls_cipher_auth_encrypt( mbedtls_cipher_context_t *ctx, +static int mbedtls_cipher_aead_encrypt( mbedtls_cipher_context_t *ctx, const unsigned char *iv, size_t iv_len, const unsigned char *ad, size_t ad_len, const unsigned char *input, size_t ilen, unsigned char *output, size_t *olen, unsigned char *tag, size_t tag_len ) { - CIPHER_VALIDATE_RET( ctx != NULL ); - CIPHER_VALIDATE_RET( iv != NULL ); - CIPHER_VALIDATE_RET( ad_len == 0 || ad != NULL ); - CIPHER_VALIDATE_RET( ilen == 0 || input != NULL ); - CIPHER_VALIDATE_RET( output != NULL ); - CIPHER_VALIDATE_RET( olen != NULL ); - CIPHER_VALIDATE_RET( tag_len == 0 || tag != NULL ); - #if defined(MBEDTLS_USE_PSA_CRYPTO) if( ctx->psa_enabled == 1 ) { @@ -1370,44 +1363,21 @@ int mbedtls_cipher_auth_encrypt( mbedtls_cipher_context_t *ctx, ilen, iv, ad, ad_len, input, output, tag ) ); } #endif /* MBEDTLS_CHACHAPOLY_C */ -#if defined(MBEDTLS_NIST_KW_C) - if( MBEDTLS_MODE_KW == ctx->cipher_info->mode || - MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) - { - mbedtls_nist_kw_mode_t mode = ( MBEDTLS_MODE_KW == ctx->cipher_info->mode ) ? - MBEDTLS_KW_MODE_KW : MBEDTLS_KW_MODE_KWP; - - /* There is no iv, tag or ad associated with KW and KWP, these length should be 0 */ - if( iv_len != 0 || tag_len != 0 || ad_len != 0 ) - { - return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); - } - - return( mbedtls_nist_kw_wrap( ctx->cipher_ctx, mode, input, ilen, output, olen, SIZE_MAX ) ); - } -#endif /* MBEDTLS_NIST_KW_C */ return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); } /* - * Packet-oriented decryption for AEAD modes + * Packet-oriented encryption for AEAD modes: internal function shared by + * mbedtls_cipher_auth_encrypt() and mbedtls_cipher_auth_encrypt_ext(). */ -int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, +static int mbedtls_cipher_aead_decrypt( mbedtls_cipher_context_t *ctx, const unsigned char *iv, size_t iv_len, const unsigned char *ad, size_t ad_len, const unsigned char *input, size_t ilen, unsigned char *output, size_t *olen, const unsigned char *tag, size_t tag_len ) { - CIPHER_VALIDATE_RET( ctx != NULL ); - CIPHER_VALIDATE_RET( iv != NULL ); - CIPHER_VALIDATE_RET( ad_len == 0 || ad != NULL ); - CIPHER_VALIDATE_RET( ilen == 0 || input != NULL ); - CIPHER_VALIDATE_RET( output != NULL ); - CIPHER_VALIDATE_RET( olen != NULL ); - CIPHER_VALIDATE_RET( tag_len == 0 || tag != NULL ); - #if defined(MBEDTLS_USE_PSA_CRYPTO) if( ctx->psa_enabled == 1 ) { @@ -1495,6 +1465,68 @@ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, return( ret ); } #endif /* MBEDTLS_CHACHAPOLY_C */ + + return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); +} + +/* + * Packet-oriented encryption for AEAD modes: public function. + */ +int mbedtls_cipher_auth_encrypt( mbedtls_cipher_context_t *ctx, + const unsigned char *iv, size_t iv_len, + const unsigned char *ad, size_t ad_len, + const unsigned char *input, size_t ilen, + unsigned char *output, size_t *olen, + unsigned char *tag, size_t tag_len ) +{ + CIPHER_VALIDATE_RET( ctx != NULL ); + CIPHER_VALIDATE_RET( iv != NULL ); + CIPHER_VALIDATE_RET( ad_len == 0 || ad != NULL ); + CIPHER_VALIDATE_RET( ilen == 0 || input != NULL ); + CIPHER_VALIDATE_RET( output != NULL ); + CIPHER_VALIDATE_RET( olen != NULL ); + CIPHER_VALIDATE_RET( tag_len == 0 || tag != NULL ); + +#if defined(MBEDTLS_NIST_KW_C) + if( MBEDTLS_MODE_KW == ctx->cipher_info->mode || + MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) + { + mbedtls_nist_kw_mode_t mode = ( MBEDTLS_MODE_KW == ctx->cipher_info->mode ) ? + MBEDTLS_KW_MODE_KW : MBEDTLS_KW_MODE_KWP; + + /* There is no iv, tag or ad associated with KW and KWP, these length should be 0 */ + if( iv_len != 0 || tag_len != 0 || ad_len != 0 ) + { + return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); + } + + return( mbedtls_nist_kw_wrap( ctx->cipher_ctx, mode, input, ilen, output, olen, SIZE_MAX ) ); + } +#endif /* MBEDTLS_NIST_KW_C */ + + return( mbedtls_cipher_aead_encrypt( ctx, iv, iv_len, ad, ad_len, + input, ilen, output, olen, + tag, tag_len ) ); +} + +/* + * Packet-oriented decryption for AEAD modes: public function. + */ +int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, + const unsigned char *iv, size_t iv_len, + const unsigned char *ad, size_t ad_len, + const unsigned char *input, size_t ilen, + unsigned char *output, size_t *olen, + const unsigned char *tag, size_t tag_len ) +{ + CIPHER_VALIDATE_RET( ctx != NULL ); + CIPHER_VALIDATE_RET( iv != NULL ); + CIPHER_VALIDATE_RET( ad_len == 0 || ad != NULL ); + CIPHER_VALIDATE_RET( ilen == 0 || input != NULL ); + CIPHER_VALIDATE_RET( output != NULL ); + CIPHER_VALIDATE_RET( olen != NULL ); + CIPHER_VALIDATE_RET( tag_len == 0 || tag != NULL ); + #if defined(MBEDTLS_NIST_KW_C) if( MBEDTLS_MODE_KW == ctx->cipher_info->mode || MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) @@ -1512,8 +1544,108 @@ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, } #endif /* MBEDTLS_NIST_KW_C */ - return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); + return( mbedtls_cipher_aead_decrypt( ctx, iv, iv_len, ad, ad_len, + input, ilen, output, olen, + tag, tag_len ) ); } #endif /* MBEDTLS_CIPHER_MODE_AEAD */ +#if defined(MBEDTLS_CIPHER_MODE_AEAD) || defined(MBEDTLS_NIST_KW_C) +/* + * Packet-oriented encryption for AEAD/NIST_KW: public function. + */ +int mbedtls_cipher_auth_encrypt_ext( mbedtls_cipher_context_t *ctx, + const unsigned char *iv, size_t iv_len, + const unsigned char *ad, size_t ad_len, + const unsigned char *input, size_t ilen, + unsigned char *output, size_t output_len, + size_t *olen, size_t tag_len ) +{ + CIPHER_VALIDATE_RET( ctx != NULL ); + CIPHER_VALIDATE_RET( iv != NULL ); + CIPHER_VALIDATE_RET( ad_len == 0 || ad != NULL ); + CIPHER_VALIDATE_RET( ilen == 0 || input != NULL ); + CIPHER_VALIDATE_RET( output != NULL ); + CIPHER_VALIDATE_RET( olen != NULL ); + +#if defined(MBEDTLS_NIST_KW_C) + if( MBEDTLS_MODE_KW == ctx->cipher_info->mode || + MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) + { + mbedtls_nist_kw_mode_t mode = ( MBEDTLS_MODE_KW == ctx->cipher_info->mode ) ? + MBEDTLS_KW_MODE_KW : MBEDTLS_KW_MODE_KWP; + + /* There is no iv, tag or ad associated with KW and KWP, + * so these length should be 0 as documented. */ + if( iv_len != 0 || tag_len != 0 || ad_len != 0 ) + return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); + + return( mbedtls_nist_kw_wrap( ctx->cipher_ctx, mode, input, ilen, + output, olen, output_len ) ); + } +#endif /* MBEDTLS_NIST_KW_C */ + +#if defined(MBEDTLS_CIPHER_MODE_AEAD) + /* AEAD case: check length before passing on to shared function */ + if( output_len < ilen + tag_len ) + return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); + + int ret = mbedtls_cipher_aead_encrypt( ctx, iv, iv_len, ad, ad_len, + input, ilen, output, olen, + output + ilen, tag_len ); + *olen += tag_len; + return( ret ); +#else + return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); +#endif /* MBEDTLS_CIPHER_MODE_AEAD */ +} + +/* + * Packet-oriented decryption for AEAD/NIST_KW: public function. + */ +int mbedtls_cipher_auth_decrypt_ext( mbedtls_cipher_context_t *ctx, + const unsigned char *iv, size_t iv_len, + const unsigned char *ad, size_t ad_len, + const unsigned char *input, size_t ilen, + unsigned char *output, size_t output_len, + size_t *olen, size_t tag_len ) +{ + CIPHER_VALIDATE_RET( ctx != NULL ); + CIPHER_VALIDATE_RET( iv != NULL ); + CIPHER_VALIDATE_RET( ad_len == 0 || ad != NULL ); + CIPHER_VALIDATE_RET( ilen == 0 || input != NULL ); + CIPHER_VALIDATE_RET( output_len == 0 || output != NULL ); + CIPHER_VALIDATE_RET( olen != NULL ); + +#if defined(MBEDTLS_NIST_KW_C) + if( MBEDTLS_MODE_KW == ctx->cipher_info->mode || + MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) + { + mbedtls_nist_kw_mode_t mode = ( MBEDTLS_MODE_KW == ctx->cipher_info->mode ) ? + MBEDTLS_KW_MODE_KW : MBEDTLS_KW_MODE_KWP; + + /* There is no iv, tag or ad associated with KW and KWP, + * so these length should be 0 as documented. */ + if( iv_len != 0 || tag_len != 0 || ad_len != 0 ) + return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); + + return( mbedtls_nist_kw_unwrap( ctx->cipher_ctx, mode, input, ilen, + output, olen, output_len ) ); + } +#endif /* MBEDTLS_NIST_KW_C */ + +#if defined(MBEDTLS_CIPHER_MODE_AEAD) + /* AEAD case: check length before passing on to shared function */ + if( ilen < tag_len || output_len < ilen - tag_len ) + return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); + + return( mbedtls_cipher_aead_decrypt( ctx, iv, iv_len, ad, ad_len, + input, ilen - tag_len, output, olen, + input + ilen - tag_len, tag_len ) ); +#else + return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); +#endif /* MBEDTLS_CIPHER_MODE_AEAD */ +} +#endif /* MBEDTLS_CIPHER_MODE_AEAD || MBEDTLS_NIST_KW_C */ + #endif /* MBEDTLS_CIPHER_C */ From 4c1a1006df81091a37996b18f59236d1afa9d98a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 26 Nov 2020 10:22:50 +0100 Subject: [PATCH 04/21] Improve comments/structure of auth_crypt test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We want to test both sets of functions (ext and non-ext) in turn, so goto exit is not really and option. Also, separate setting up the context (which is going to be the same for both ext and non-ext functions) from setting up the buffers (which will vary). Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_cipher.function | 149 ++++++++++++++---------- 1 file changed, 90 insertions(+), 59 deletions(-) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index ea1e9ada5..dc3bf3b21 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -13,6 +13,10 @@ #include "test/psa_crypto_helpers.h" #endif +#if defined(MBEDTLS_CIPHER_MODE_AEAD) || defined(MBEDTLS_NIST_KW_C) +#define MBEDTLS_CIPHER_AUTH_CRYPT +#endif + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -959,15 +963,17 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CIPHER_MODE_AEAD */ +/* BEGIN_CASE depends_on:MBEDTLS_CIPHER_AUTH_CRYPT */ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, data_t * ad, data_t * cipher, data_t * tag, char * result, data_t * clear, int use_psa ) { - /* Takes an AEAD ciphertext + tag and performs a pair - * of AEAD decryption and AEAD encryption. It checks that + /* + * Take an AEAD ciphertext + tag and perform a pair + * of AEAD decryption and AEAD encryption. Check that * this results in the expected plaintext, and that - * decryption and encryption are inverse to one another. */ + * decryption and encryption are inverse to one another. + */ int ret; unsigned char output[300]; /* Temporary buffer for results of @@ -984,31 +990,27 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, mbedtls_cipher_init( &ctx ); memset( output, 0xFF, sizeof( output ) ); - /* Prepare context */ -#if !defined(MBEDTLS_USE_PSA_CRYPTO) - (void) use_psa; + /* Initialize PSA Crypto */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) + if( use_psa == 1 ) + PSA_ASSERT( psa_crypto_init( ) ); #else + (void) use_psa; +#endif + + /* + * Prepare context for decryption + */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) if( use_psa == 1 ) { - PSA_ASSERT( psa_crypto_init( ) ); - - /* PSA requires that the tag immediately follows the ciphertext. */ - tmp_cipher = mbedtls_calloc( 1, cipher->len + tag->len ); - TEST_ASSERT( tmp_cipher != NULL ); - tmp_tag = tmp_cipher + cipher->len; - - memcpy( tmp_cipher, cipher->x, cipher->len ); - memcpy( tmp_tag, tag->x, tag->len ); - TEST_ASSERT( 0 == mbedtls_cipher_setup_psa( &ctx, mbedtls_cipher_info_from_type( cipher_id ), tag->len ) ); } else -#endif +#endif /* MBEDTLS_USE_PSA_CRYPTO */ { - tmp_tag = tag->x; - tmp_cipher = cipher->x; TEST_ASSERT( 0 == mbedtls_cipher_setup( &ctx, mbedtls_cipher_info_from_type( cipher_id ) ) ); } @@ -1016,7 +1018,30 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, TEST_ASSERT( 0 == mbedtls_cipher_setkey( &ctx, key->x, 8 * key->len, MBEDTLS_DECRYPT ) ); - /* decode buffer and check tag->x */ + /* + * Prepare buffers/pointers for decryption + */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) + if( use_psa == 1 ) + { + /* PSA requires that the tag immediately follows the ciphertext. */ + tmp_cipher = mbedtls_calloc( 1, cipher->len + tag->len ); + TEST_ASSERT( tmp_cipher != NULL ); + tmp_tag = tmp_cipher + cipher->len; + + memcpy( tmp_cipher, cipher->x, cipher->len ); + memcpy( tmp_tag, tag->x, tag->len ); + } + else +#endif /* MBEDTLS_USE_PSA_CRYPTO */ + { + tmp_tag = tag->x; + tmp_cipher = cipher->x; + } + + /* + * Authenticate and decrypt, and check result + */ /* Sanity check that we don't use overly long inputs. */ TEST_ASSERT( sizeof( output ) >= cipher->len ); @@ -1029,48 +1054,54 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, if( strcmp( result, "FAIL" ) == 0 ) { TEST_ASSERT( ret == MBEDTLS_ERR_CIPHER_AUTH_FAILED ); - goto exit; - } - - /* otherwise, make sure it was decrypted properly */ - TEST_ASSERT( ret == 0 ); - - TEST_ASSERT( outlen == clear->len ); - TEST_ASSERT( memcmp( output, clear->x, clear->len ) == 0 ); - - /* then encrypt the clear->x and make sure we get the same ciphertext and tag->x */ - mbedtls_cipher_free( &ctx ); -#if defined(MBEDTLS_USE_PSA_CRYPTO) - if( use_psa == 1 ) - { - TEST_ASSERT( 0 == mbedtls_cipher_setup_psa( &ctx, - mbedtls_cipher_info_from_type( cipher_id ), - tag->len ) ); } else -#endif { - TEST_ASSERT( 0 == mbedtls_cipher_setup( &ctx, - mbedtls_cipher_info_from_type( cipher_id ) ) ); + /* otherwise, make sure it was decrypted properly */ + TEST_ASSERT( ret == 0 ); + + TEST_ASSERT( outlen == clear->len ); + TEST_ASSERT( memcmp( output, clear->x, clear->len ) == 0 ); + + /* + * Prepare context for encryption + */ + mbedtls_cipher_free( &ctx ); +#if defined(MBEDTLS_USE_PSA_CRYPTO) + if( use_psa == 1 ) + { + TEST_ASSERT( 0 == mbedtls_cipher_setup_psa( &ctx, + mbedtls_cipher_info_from_type( cipher_id ), + tag->len ) ); + } + else +#endif + { + TEST_ASSERT( 0 == mbedtls_cipher_setup( &ctx, + mbedtls_cipher_info_from_type( cipher_id ) ) ); + } + TEST_ASSERT( 0 == mbedtls_cipher_setkey( &ctx, key->x, 8 * key->len, + MBEDTLS_ENCRYPT ) ); + + /* + * Encrypt and check the result + */ + memset( output, 0xFF, sizeof( output ) ); + outlen = 0; + + /* Sanity check that we don't use overly long inputs. */ + TEST_ASSERT( sizeof( output ) >= clear->len + tag->len ); + + output_tag = output + clear->len; + ret = mbedtls_cipher_auth_encrypt( &ctx, iv->x, iv->len, ad->x, ad->len, + clear->x, clear->len, output, &outlen, + output_tag, tag->len ); + TEST_ASSERT( ret == 0 ); + + TEST_ASSERT( outlen == cipher->len ); + TEST_ASSERT( memcmp( output, cipher->x, cipher->len ) == 0 ); + TEST_ASSERT( memcmp( output_tag, tag->x, tag->len ) == 0 ); } - TEST_ASSERT( 0 == mbedtls_cipher_setkey( &ctx, key->x, 8 * key->len, - MBEDTLS_ENCRYPT ) ); - - memset( output, 0xFF, sizeof( output ) ); - outlen = 0; - - /* Sanity check that we don't use overly long inputs. */ - TEST_ASSERT( sizeof( output ) >= clear->len + tag->len ); - - output_tag = output + clear->len; - ret = mbedtls_cipher_auth_encrypt( &ctx, iv->x, iv->len, ad->x, ad->len, - clear->x, clear->len, output, &outlen, - output_tag, tag->len ); - TEST_ASSERT( ret == 0 ); - - TEST_ASSERT( outlen == cipher->len ); - TEST_ASSERT( memcmp( output, cipher->x, cipher->len ) == 0 ); - TEST_ASSERT( memcmp( output_tag, tag->x, tag->len ) == 0 ); exit: From 89a8fe50fe6d07f434e28cf505ee378acb7cf8e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 27 Nov 2020 09:32:55 +0100 Subject: [PATCH 05/21] Extract helper function for repeated test code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_cipher.function | 76 ++++++++++++++----------- 1 file changed, 44 insertions(+), 32 deletions(-) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index dc3bf3b21..a40bfb5f2 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -17,6 +17,46 @@ #define MBEDTLS_CIPHER_AUTH_CRYPT #endif +#if defined(MBEDTLS_CIPHER_AUTH_CRYPT) +/* Helper for resetting key/direction + * + * The documentation doesn't explicitly say whether calling + * mbedtls_cipher_setkey() twice is allowed or not. This currently works with + * the default software implementation, but only by accident. It isn't + * guaranteed to work with new ciphers or with alternative implementations of + * individual ciphers, and it doesn't work with the PSA wrappers. So don't do + * it, and instead start with a fresh context. + */ +static void cipher_reset_key( mbedtls_cipher_context_t *ctx, int cipher_id, + int use_psa, size_t tag_len, const data_t *key, int direction ) +{ + mbedtls_cipher_free( ctx ); + mbedtls_cipher_init( ctx ); + +#if !defined(MBEDTLS_USE_PSA_CRYPTO) + (void) use_psa; + (void) tag_len; +#else + if( use_psa == 1 ) + { + TEST_ASSERT( 0 == mbedtls_cipher_setup_psa( ctx, + mbedtls_cipher_info_from_type( cipher_id ), + tag_len ) ); + } + else +#endif /* MBEDTLS_USE_PSA_CRYPTO */ + { + TEST_ASSERT( 0 == mbedtls_cipher_setup( ctx, + mbedtls_cipher_info_from_type( cipher_id ) ) ); + } + + TEST_ASSERT( 0 == mbedtls_cipher_setkey( ctx, key->x, 8 * key->len, + direction ) ); +exit: + ; +} +#endif /* MBEDTLS_CIPHER_AUTH_CRYPT */ + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -1001,22 +1041,8 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, /* * Prepare context for decryption */ -#if defined(MBEDTLS_USE_PSA_CRYPTO) - if( use_psa == 1 ) - { - TEST_ASSERT( 0 == mbedtls_cipher_setup_psa( &ctx, - mbedtls_cipher_info_from_type( cipher_id ), - tag->len ) ); - } - else -#endif /* MBEDTLS_USE_PSA_CRYPTO */ - { - TEST_ASSERT( 0 == mbedtls_cipher_setup( &ctx, - mbedtls_cipher_info_from_type( cipher_id ) ) ); - } - - TEST_ASSERT( 0 == mbedtls_cipher_setkey( &ctx, key->x, 8 * key->len, - MBEDTLS_DECRYPT ) ); + cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, + MBEDTLS_DECRYPT ); /* * Prepare buffers/pointers for decryption @@ -1066,22 +1092,8 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, /* * Prepare context for encryption */ - mbedtls_cipher_free( &ctx ); -#if defined(MBEDTLS_USE_PSA_CRYPTO) - if( use_psa == 1 ) - { - TEST_ASSERT( 0 == mbedtls_cipher_setup_psa( &ctx, - mbedtls_cipher_info_from_type( cipher_id ), - tag->len ) ); - } - else -#endif - { - TEST_ASSERT( 0 == mbedtls_cipher_setup( &ctx, - mbedtls_cipher_info_from_type( cipher_id ) ) ); - } - TEST_ASSERT( 0 == mbedtls_cipher_setkey( &ctx, key->x, 8 * key->len, - MBEDTLS_ENCRYPT ) ); + cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, + MBEDTLS_ENCRYPT ); /* * Encrypt and check the result From 53f10e70fda847ef8c4c80cb6877c83e58c03891 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 30 Nov 2020 10:17:01 +0100 Subject: [PATCH 06/21] Test cipher_auth_{en,de}crypt_ext() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_cipher.function | 168 ++++++++++++++++++++++-- 1 file changed, 157 insertions(+), 11 deletions(-) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index a40bfb5f2..7ea1a14a2 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1013,9 +1013,15 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, * of AEAD decryption and AEAD encryption. Check that * this results in the expected plaintext, and that * decryption and encryption are inverse to one another. + * + * Do that twice: + * - once with legacy functions auth_decrypt/auth_encrypt + * - once with new functions auth_decrypt_ext/auth_encrypt_ext + * This allows testing both without duplicating test cases. */ int ret; + int using_nist_kw, using_nist_kw_padding; unsigned char output[300]; /* Temporary buffer for results of * encryption and decryption. */ unsigned char *output_tag = NULL; /* Temporary buffer for tag in the @@ -1027,6 +1033,13 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, unsigned char *tmp_tag = NULL; unsigned char *tmp_cipher = NULL; + unsigned char *cipher_plus_tag = NULL; + size_t cipher_plus_tag_len; + unsigned char *decrypt_buf = NULL; + size_t decrypt_buf_len = 0; + unsigned char *encrypt_buf = NULL; + size_t encrypt_buf_len = 0; + mbedtls_cipher_init( &ctx ); memset( output, 0xFF, sizeof( output ) ); @@ -1038,6 +1051,17 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, (void) use_psa; #endif + /* + * Are we using NIST_KW? with padding? + */ + using_nist_kw_padding = cipher_id == MBEDTLS_CIPHER_AES_128_KWP || + cipher_id == MBEDTLS_CIPHER_AES_192_KWP || + cipher_id == MBEDTLS_CIPHER_AES_256_KWP; + using_nist_kw = cipher_id == MBEDTLS_CIPHER_AES_128_KW || + cipher_id == MBEDTLS_CIPHER_AES_192_KW || + cipher_id == MBEDTLS_CIPHER_AES_256_KW || + using_nist_kw_padding; + /* * Prepare context for decryption */ @@ -1045,24 +1069,146 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, MBEDTLS_DECRYPT ); /* - * Prepare buffers/pointers for decryption + * prepare buffer for decryption + * (we need the tag appended to the ciphertext) + */ + cipher_plus_tag_len = cipher->len + tag->len; + ASSERT_ALLOC( cipher_plus_tag, cipher_plus_tag_len ); + memcpy( cipher_plus_tag, cipher->x, cipher->len ); + memcpy( cipher_plus_tag + cipher->len, tag->x, tag->len ); + + /* + * Compute length of output buffer according to the documentation + */ + if( using_nist_kw ) + decrypt_buf_len = cipher_plus_tag_len - 8; + else + decrypt_buf_len = cipher_plus_tag_len - tag->len; + + + /* + * Try decrypting to a buffer that's 1B too small + */ + if( decrypt_buf_len != 0 ) + { + ASSERT_ALLOC( decrypt_buf, decrypt_buf_len - 1 ); + + outlen = 0; + ret = mbedtls_cipher_auth_decrypt_ext( &ctx, iv->x, iv->len, + ad->x, ad->len, cipher_plus_tag, cipher_plus_tag_len, + decrypt_buf, decrypt_buf_len - 1, &outlen, tag->len ); + TEST_ASSERT( ret == MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); + + mbedtls_free( decrypt_buf ); + decrypt_buf = NULL; + } + + /* + * Authenticate and decrypt, and check result + */ + ASSERT_ALLOC( decrypt_buf, decrypt_buf_len ); + + outlen = 0; + ret = mbedtls_cipher_auth_decrypt_ext( &ctx, iv->x, iv->len, + ad->x, ad->len, cipher_plus_tag, cipher_plus_tag_len, + decrypt_buf, decrypt_buf_len, &outlen, tag->len ); + + if( strcmp( result, "FAIL" ) == 0 ) + { + TEST_ASSERT( ret == MBEDTLS_ERR_CIPHER_AUTH_FAILED ); + } + else + { + TEST_ASSERT( ret == 0 ); + + TEST_ASSERT( outlen == clear->len ); + if( clear->len != 0 ) + TEST_ASSERT( memcmp( decrypt_buf, clear->x, clear->len ) == 0 ); + } + + /* Free this, but keep cipher_plus_tag for legacy function with PSA */ + mbedtls_free( decrypt_buf ); + decrypt_buf = NULL; + + /* + * Encrypt back if test data was authentic + */ + if( strcmp( result, "FAIL" ) != 0 ) + { + /* prepare context for encryption */ + cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, + MBEDTLS_ENCRYPT ); + + /* + * Compute size of output buffer according to documentation + */ + if( using_nist_kw ) + { + encrypt_buf_len = clear->len + 8; + if( using_nist_kw_padding && encrypt_buf_len % 8 != 0 ) + encrypt_buf_len += 8 - encrypt_buf_len % 8; + } + else + { + encrypt_buf_len = clear->len + tag->len; + } + + /* + * Try encrypting with an output buffer that's 1B too small + */ + ASSERT_ALLOC( encrypt_buf, encrypt_buf_len - 1 ); + + outlen = 0; + ret = mbedtls_cipher_auth_encrypt_ext( &ctx, iv->x, iv->len, + ad->x, ad->len, clear->x, clear->len, + encrypt_buf, encrypt_buf_len - 1, &outlen, tag->len ); + TEST_ASSERT( ret != 0 ); + + mbedtls_free( encrypt_buf ); + encrypt_buf = NULL; + + /* + * Encrypt and check the result + */ + ASSERT_ALLOC( encrypt_buf, encrypt_buf_len ); + + outlen = 0; + ret = mbedtls_cipher_auth_encrypt_ext( &ctx, iv->x, iv->len, + ad->x, ad->len, clear->x, clear->len, + encrypt_buf, encrypt_buf_len, &outlen, tag->len ); + TEST_ASSERT( ret == 0 ); + + TEST_ASSERT( outlen == cipher->len + tag->len ); + TEST_ASSERT( memcmp( encrypt_buf, cipher->x, cipher->len ) == 0 ); + TEST_ASSERT( memcmp( encrypt_buf + cipher->len, + tag->x, tag->len ) == 0 ); + + mbedtls_free( encrypt_buf ); + encrypt_buf = NULL; + } + + /* + * Prepare context for decryption + */ + cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, + MBEDTLS_DECRYPT ); + + /* + * Prepare pointers for decryption */ #if defined(MBEDTLS_USE_PSA_CRYPTO) if( use_psa == 1 ) { - /* PSA requires that the tag immediately follows the ciphertext. */ - tmp_cipher = mbedtls_calloc( 1, cipher->len + tag->len ); - TEST_ASSERT( tmp_cipher != NULL ); + /* PSA requires that the tag immediately follows the ciphertext. + * Fortunately, we already have that from testing the new API. */ + tmp_cipher = cipher_plus_tag; tmp_tag = tmp_cipher + cipher->len; - - memcpy( tmp_cipher, cipher->x, cipher->len ); - memcpy( tmp_tag, tag->x, tag->len ); } else #endif /* MBEDTLS_USE_PSA_CRYPTO */ { - tmp_tag = tag->x; tmp_cipher = cipher->x; + tmp_tag = tag->x; } /* @@ -1118,13 +1264,13 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, exit: mbedtls_cipher_free( &ctx ); + mbedtls_free( decrypt_buf ); + mbedtls_free( encrypt_buf ); + mbedtls_free( cipher_plus_tag ); #if defined(MBEDTLS_USE_PSA_CRYPTO) if( use_psa == 1 ) - { - mbedtls_free( tmp_cipher ); PSA_DONE( ); - } #endif /* MBEDTLS_USE_PSA_CRYPTO */ } /* END_CASE */ From f2ffbc43878c971235253f815f3d1ce61012c92c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 1 Dec 2020 09:57:55 +0100 Subject: [PATCH 07/21] Stop supporting NIST_KW in cipher_auth_xxcrypt() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/cipher.h | 30 ++++++++++++-------- library/cipher.c | 28 ++++++------------- tests/suites/test_suite_cipher.function | 37 ++++++++++++++++++------- 3 files changed, 54 insertions(+), 41 deletions(-) diff --git a/include/mbedtls/cipher.h b/include/mbedtls/cipher.h index 49b8b5e9e..24524c5c3 100644 --- a/include/mbedtls/cipher.h +++ b/include/mbedtls/cipher.h @@ -858,10 +858,14 @@ int mbedtls_cipher_crypt( mbedtls_cipher_context_t *ctx, #if defined(MBEDTLS_CIPHER_MODE_AEAD) /** - * \brief The generic autenticated encryption (AEAD) function. + * \brief The generic authenticated encryption (AEAD) function. + * + * \note This function only supports AEAD algorithms, not key + * wrapping algorithms such as NIST_KW; for this, see + * mbedtls_cipher_auth_encrypt_ext(). * * \param ctx The generic cipher context. This must be initialized and - * bound to a key. + * bound to a key associated with an AEAD algorithm. * \param iv The nonce to use. This must be a readable buffer of * at least \p iv_len Bytes and must not be \c NULL. * \param iv_len The length of the nonce. This must satisfy the @@ -885,7 +889,7 @@ int mbedtls_cipher_crypt( mbedtls_cipher_context_t *ctx, * below regarding restrictions with PSA-based contexts. * \param tag_len The desired length of the authentication tag. This * must match the constraints imposed by the AEAD cipher - * used, and in particuler must not be \c 0. + * used, and in particular must not be \c 0. * * \note If the context is based on PSA (that is, it was set up * with mbedtls_cipher_setup_psa()), then it is required @@ -905,14 +909,18 @@ int mbedtls_cipher_auth_encrypt( mbedtls_cipher_context_t *ctx, unsigned char *tag, size_t tag_len ); /** - * \brief The generic autenticated decryption (AEAD) function. + * \brief The generic authenticated decryption (AEAD) function. + * + * \note This function only supports AEAD algorithms, not key + * wrapping algorithms such as NIST_KW; for this, see + * mbedtls_cipher_auth_encrypt_ext(). * * \note If the data is not authentic, then the output buffer * is zeroed out to prevent the unauthentic plaintext being * used, making this interface safer. * * \param ctx The generic cipher context. This must be initialized and - * and bound to a key. + * bound to a key associated with an AEAD algorithm. * \param iv The nonce to use. This must be a readable buffer of * at least \p iv_len Bytes and must not be \c NULL. * \param iv_len The length of the nonce. This must satisfy the @@ -959,14 +967,14 @@ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, #if defined(MBEDTLS_CIPHER_MODE_AEAD) || defined(MBEDTLS_NIST_KW_C) /** - * \brief The autenticated encryption (AEAD/NIST_KW) function. + * \brief The authenticated encryption (AEAD/NIST_KW) function. * * \note For AEAD modes, the tag will be appended to the * ciphertext, as recommended by RFC 5116. * (NIST_KW doesn't have a separate tag.) * * \param ctx The generic cipher context. This must be initialized and - * bound to a key. + * bound to a key, with an AEAD algorithm or NIST_KW. * \param iv The nonce to use. This must be a readable buffer of * at least \p iv_len Bytes and may be \c NULL if \p * iv_len is \c 0. @@ -994,7 +1002,7 @@ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, * writable object of type \c size_t. * \param tag_len The desired length of the authentication tag. For AEAD * ciphers, this must match the constraints imposed by - * the cipher used, and in particuler must not be \c 0. + * the cipher used, and in particular must not be \c 0. * For NIST_KW, this must be \c 0. * * \return \c 0 on success. @@ -1010,7 +1018,7 @@ int mbedtls_cipher_auth_encrypt_ext( mbedtls_cipher_context_t *ctx, size_t *olen, size_t tag_len ); /** - * \brief The autenticated encryption (AEAD/NIST_KW) function. + * \brief The authenticated encryption (AEAD/NIST_KW) function. * * \note If the data is not authentic, then the output buffer * is zeroed out to prevent the unauthentic plaintext being @@ -1021,7 +1029,7 @@ int mbedtls_cipher_auth_encrypt_ext( mbedtls_cipher_context_t *ctx, * (NIST_KW doesn't have a separate tag.) * * \param ctx The generic cipher context. This must be initialized and - * and bound to a key. + * bound to a key, with an AEAD algorithm or NIST_KW. * \param iv The nonce to use. This must be a readable buffer of * at least \p iv_len Bytes and may be \c NULL if \p * iv_len is \c 0. @@ -1049,7 +1057,7 @@ int mbedtls_cipher_auth_encrypt_ext( mbedtls_cipher_context_t *ctx, * writable object of type \c size_t. * \param tag_len The actual length of the authentication tag. For AEAD * ciphers, this must match the constraints imposed by - * the cipher used, and in particuler must not be \c 0. + * the cipher used, and in particular must not be \c 0. * For NIST_KW, this must be \c 0. * * \return \c 0 on success. diff --git a/library/cipher.c b/library/cipher.c index 20e1e7b3a..47dafa4aa 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1491,16 +1491,10 @@ int mbedtls_cipher_auth_encrypt( mbedtls_cipher_context_t *ctx, if( MBEDTLS_MODE_KW == ctx->cipher_info->mode || MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) { - mbedtls_nist_kw_mode_t mode = ( MBEDTLS_MODE_KW == ctx->cipher_info->mode ) ? - MBEDTLS_KW_MODE_KW : MBEDTLS_KW_MODE_KWP; - - /* There is no iv, tag or ad associated with KW and KWP, these length should be 0 */ - if( iv_len != 0 || tag_len != 0 || ad_len != 0 ) - { - return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); - } - - return( mbedtls_nist_kw_wrap( ctx->cipher_ctx, mode, input, ilen, output, olen, SIZE_MAX ) ); + /* NIST_KW is not supported because we used to document the wrong size + * of the output buffer, so people should move to the _ext API, + * which has an explicit argument for buffer size. */ + return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); } #endif /* MBEDTLS_NIST_KW_C */ @@ -1531,16 +1525,10 @@ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, if( MBEDTLS_MODE_KW == ctx->cipher_info->mode || MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) { - mbedtls_nist_kw_mode_t mode = ( MBEDTLS_MODE_KW == ctx->cipher_info->mode ) ? - MBEDTLS_KW_MODE_KW : MBEDTLS_KW_MODE_KWP; - - /* There is no iv, tag or ad associated with KW and KWP, these length should be 0 */ - if( iv_len != 0 || tag_len != 0 || ad_len != 0 ) - { - return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); - } - - return( mbedtls_nist_kw_unwrap( ctx->cipher_ctx, mode, input, ilen, output, olen, SIZE_MAX ) ); + /* NIST_KW is not supported because we used to document the wrong size + * of the output buffer, so people should move to the _ext API, + * which has an explicit argument for buffer size. */ + return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); } #endif /* MBEDTLS_NIST_KW_C */ diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 7ea1a14a2..543ccf6fd 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1222,22 +1222,31 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, tmp_cipher, cipher->len, output, &outlen, tmp_tag, tag->len ); - /* make sure the message is rejected if it should be */ - if( strcmp( result, "FAIL" ) == 0 ) + if( using_nist_kw ) { + /* NIST_KW with legacy API */ + TEST_ASSERT( ret == MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); + } + else if( strcmp( result, "FAIL" ) == 0 ) + { + /* unauthentic message */ TEST_ASSERT( ret == MBEDTLS_ERR_CIPHER_AUTH_FAILED ); } else { - /* otherwise, make sure it was decrypted properly */ + /* authentic message: is the plaintext correct? */ TEST_ASSERT( ret == 0 ); TEST_ASSERT( outlen == clear->len ); TEST_ASSERT( memcmp( output, clear->x, clear->len ) == 0 ); + } - /* - * Prepare context for encryption - */ + /* + * Encrypt back if test data was authentic + */ + if( strcmp( result, "FAIL" ) != 0 ) + { + /* prepare context for encryption */ cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, MBEDTLS_ENCRYPT ); @@ -1254,11 +1263,19 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, ret = mbedtls_cipher_auth_encrypt( &ctx, iv->x, iv->len, ad->x, ad->len, clear->x, clear->len, output, &outlen, output_tag, tag->len ); - TEST_ASSERT( ret == 0 ); - TEST_ASSERT( outlen == cipher->len ); - TEST_ASSERT( memcmp( output, cipher->x, cipher->len ) == 0 ); - TEST_ASSERT( memcmp( output_tag, tag->x, tag->len ) == 0 ); + if( using_nist_kw ) + { + TEST_ASSERT( ret == MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); + } + else + { + TEST_ASSERT( ret == 0 ); + + TEST_ASSERT( outlen == cipher->len ); + TEST_ASSERT( memcmp( output, cipher->x, cipher->len ) == 0 ); + TEST_ASSERT( memcmp( output_tag, tag->x, tag->len ) == 0 ); + } } exit: From 513c2433178276b3e902e1133923eb5d194c15af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 1 Dec 2020 10:34:57 +0100 Subject: [PATCH 08/21] Deprecate mbedtls_cipher_auth_xxcrypt() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This temporarily breaks all.sh '*deprecated*' (deprecated functions still used in the library), which will be fix in the next commit. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/cipher.h | 20 +++++++++++-- library/cipher.c | 2 ++ tests/suites/test_suite_cipher.function | 39 +++++++++++++++++++------ 3 files changed, 49 insertions(+), 12 deletions(-) diff --git a/include/mbedtls/cipher.h b/include/mbedtls/cipher.h index 24524c5c3..9ae2f0609 100644 --- a/include/mbedtls/cipher.h +++ b/include/mbedtls/cipher.h @@ -857,9 +857,17 @@ int mbedtls_cipher_crypt( mbedtls_cipher_context_t *ctx, unsigned char *output, size_t *olen ); #if defined(MBEDTLS_CIPHER_MODE_AEAD) +#if ! defined(MBEDTLS_DEPRECATED_REMOVED) +#if defined(MBEDTLS_DEPRECATED_WARNING) +#define MBEDTLS_DEPRECATED __attribute__((deprecated)) +#else +#define MBEDTLS_DEPRECATED +#endif /* MBEDTLS_DEPRECATED_WARNING */ /** * \brief The generic authenticated encryption (AEAD) function. * + * \deprecated Superseded by mbedtls_cipher_auth_encrypt_ext(). + * * \note This function only supports AEAD algorithms, not key * wrapping algorithms such as NIST_KW; for this, see * mbedtls_cipher_auth_encrypt_ext(). @@ -906,14 +914,17 @@ int mbedtls_cipher_auth_encrypt( mbedtls_cipher_context_t *ctx, const unsigned char *ad, size_t ad_len, const unsigned char *input, size_t ilen, unsigned char *output, size_t *olen, - unsigned char *tag, size_t tag_len ); + unsigned char *tag, size_t tag_len ) + MBEDTLS_DEPRECATED; /** * \brief The generic authenticated decryption (AEAD) function. * + * \deprecated Superseded by mbedtls_cipher_auth_decrypt_ext(). + * * \note This function only supports AEAD algorithms, not key * wrapping algorithms such as NIST_KW; for this, see - * mbedtls_cipher_auth_encrypt_ext(). + * mbedtls_cipher_auth_decrypt_ext(). * * \note If the data is not authentic, then the output buffer * is zeroed out to prevent the unauthentic plaintext being @@ -962,7 +973,10 @@ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, const unsigned char *ad, size_t ad_len, const unsigned char *input, size_t ilen, unsigned char *output, size_t *olen, - const unsigned char *tag, size_t tag_len ); + const unsigned char *tag, size_t tag_len ) + MBEDTLS_DEPRECATED; +#undef MBEDTLS_DEPRECATED +#endif /* MBEDTLS_DEPRECATED_REMOVED */ #endif /* MBEDTLS_CIPHER_MODE_AEAD */ #if defined(MBEDTLS_CIPHER_MODE_AEAD) || defined(MBEDTLS_NIST_KW_C) diff --git a/library/cipher.c b/library/cipher.c index 47dafa4aa..44cba34bc 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1469,6 +1469,7 @@ static int mbedtls_cipher_aead_decrypt( mbedtls_cipher_context_t *ctx, return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); } +#if !defined(MBEDTLS_DEPRECATED_REMOVED) /* * Packet-oriented encryption for AEAD modes: public function. */ @@ -1536,6 +1537,7 @@ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, input, ilen, output, olen, tag, tag_len ) ); } +#endif /* !MBEDTLS_DEPRECATED_REMOVED */ #endif /* MBEDTLS_CIPHER_MODE_AEAD */ #if defined(MBEDTLS_CIPHER_MODE_AEAD) || defined(MBEDTLS_NIST_KW_C) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 543ccf6fd..3b6d1e307 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1022,17 +1022,10 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, int ret; int using_nist_kw, using_nist_kw_padding; - unsigned char output[300]; /* Temporary buffer for results of - * encryption and decryption. */ - unsigned char *output_tag = NULL; /* Temporary buffer for tag in the - * encryption step. */ mbedtls_cipher_context_t ctx; size_t outlen; - unsigned char *tmp_tag = NULL; - unsigned char *tmp_cipher = NULL; - unsigned char *cipher_plus_tag = NULL; size_t cipher_plus_tag_len; unsigned char *decrypt_buf = NULL; @@ -1040,8 +1033,19 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, unsigned char *encrypt_buf = NULL; size_t encrypt_buf_len = 0; - mbedtls_cipher_init( &ctx ); +#if !defined(MBEDTLS_DEPRECATED_WARNING) && \ + !defined(MBEDTLS_DEPRECATED_REMOVED) + unsigned char output[300]; /* Temporary buffer for results of + * encryption and decryption. */ + unsigned char *output_tag = NULL; /* Temporary buffer for tag in the + * encryption step. */ + unsigned char *tmp_tag = NULL; + unsigned char *tmp_cipher = NULL; + memset( output, 0xFF, sizeof( output ) ); +#endif /* !MBEDTLS_DEPRECATED_WARNING && !MBEDTLS_DEPRECATED_REMOVED */ + + mbedtls_cipher_init( &ctx ); /* Initialize PSA Crypto */ #if defined(MBEDTLS_USE_PSA_CRYPTO) @@ -1062,6 +1066,12 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, cipher_id == MBEDTLS_CIPHER_AES_256_KW || using_nist_kw_padding; + /**************************************************************** + * * + * Part 1: non-deprecated API * + * * + ****************************************************************/ + /* * Prepare context for decryption */ @@ -1126,7 +1136,7 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, TEST_ASSERT( memcmp( decrypt_buf, clear->x, clear->len ) == 0 ); } - /* Free this, but keep cipher_plus_tag for legacy function with PSA */ + /* Free this, but keep cipher_plus_tag for deprecated function with PSA */ mbedtls_free( decrypt_buf ); decrypt_buf = NULL; @@ -1187,6 +1197,15 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, encrypt_buf = NULL; } + /**************************************************************** + * * + * Part 2: deprecated API * + * * + ****************************************************************/ + +#if !defined(MBEDTLS_DEPRECATED_WARNING) && \ + !defined(MBEDTLS_DEPRECATED_REMOVED) + /* * Prepare context for decryption */ @@ -1278,6 +1297,8 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, } } +#endif /* !MBEDTLS_DEPRECATED_WARNING && !MBEDTLS_DEPRECATED_REMOVED */ + exit: mbedtls_cipher_free( &ctx ); From f5cf71e14adab82533a7d8cd12cbdb998948d2d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 1 Dec 2020 11:43:40 +0100 Subject: [PATCH 09/21] Stop using deprecated functions in the library MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit all.sh -k '*deprecated*' now passes again Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_msg.c | 22 +++++++++++----------- library/ssl_ticket.c | 23 ++++++++++------------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index bdf882d87..597494ead 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -850,20 +850,21 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, * Encrypt and authenticate */ - if( ( ret = mbedtls_cipher_auth_encrypt( &transform->cipher_ctx_enc, + if( ( ret = mbedtls_cipher_auth_encrypt_ext( &transform->cipher_ctx_enc, iv, transform->ivlen, - add_data, add_data_len, /* add data */ - data, rec->data_len, /* source */ - data, &rec->data_len, /* destination */ - data + rec->data_len, transform->taglen ) ) != 0 ) + add_data, add_data_len, + data, rec->data_len, /* src */ + data, rec->buf_len - (data - rec->buf), /* dst */ + &rec->data_len, + transform->taglen ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_cipher_auth_encrypt", ret ); return( ret ); } MBEDTLS_SSL_DEBUG_BUF( 4, "after encrypt: tag", - data + rec->data_len, transform->taglen ); + data + rec->data_len - transform->taglen, + transform->taglen ); /* Account for authentication tag. */ - rec->data_len += transform->taglen; post_avail -= transform->taglen; /* @@ -1420,12 +1421,11 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, /* * Decrypt and authenticate */ - if( ( ret = mbedtls_cipher_auth_decrypt( &transform->cipher_ctx_dec, + if( ( ret = mbedtls_cipher_auth_decrypt_ext( &transform->cipher_ctx_dec, iv, transform->ivlen, add_data, add_data_len, - data, rec->data_len, - data, &olen, - data + rec->data_len, + data, rec->data_len + transform->taglen, /* src */ + data, rec->buf_len - (data - rec->buf), &olen, /* dst */ transform->taglen ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_cipher_auth_decrypt", ret ); diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index e3e802315..626d137cc 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -209,7 +209,6 @@ int mbedtls_ssl_ticket_write( void *p_ticket, unsigned char *iv = start + TICKET_KEY_NAME_BYTES; unsigned char *state_len_bytes = iv + TICKET_IV_BYTES; unsigned char *state = state_len_bytes + TICKET_CRYPT_LEN_BYTES; - unsigned char *tag; size_t clear_len, ciph_len; *tlen = 0; @@ -250,23 +249,23 @@ int mbedtls_ssl_ticket_write( void *p_ticket, state_len_bytes[1] = ( clear_len ) & 0xff; /* Encrypt and authenticate */ - tag = state + clear_len; - if( ( ret = mbedtls_cipher_auth_encrypt( &key->ctx, + if( ( ret = mbedtls_cipher_auth_encrypt_ext( &key->ctx, iv, TICKET_IV_BYTES, /* Additional data: key name, IV and length */ key_name, TICKET_ADD_DATA_LEN, - state, clear_len, state, &ciph_len, - tag, TICKET_AUTH_TAG_BYTES ) ) != 0 ) + state, clear_len, + state, end - state, &ciph_len, + TICKET_AUTH_TAG_BYTES ) ) != 0 ) { goto cleanup; } - if( ciph_len != clear_len ) + if( ciph_len != clear_len + TICKET_AUTH_TAG_BYTES ) { ret = MBEDTLS_ERR_SSL_INTERNAL_ERROR; goto cleanup; } - *tlen = TICKET_MIN_LEN + ciph_len; + *tlen = TICKET_MIN_LEN + ciph_len - TICKET_AUTH_TAG_BYTES; cleanup: #if defined(MBEDTLS_THREADING_C) @@ -308,7 +307,6 @@ int mbedtls_ssl_ticket_parse( void *p_ticket, unsigned char *iv = buf + TICKET_KEY_NAME_BYTES; unsigned char *enc_len_p = iv + TICKET_IV_BYTES; unsigned char *ticket = enc_len_p + TICKET_CRYPT_LEN_BYTES; - unsigned char *tag; size_t enc_len, clear_len; if( ctx == NULL || ctx->f_rng == NULL ) @@ -326,7 +324,6 @@ int mbedtls_ssl_ticket_parse( void *p_ticket, goto cleanup; enc_len = ( enc_len_p[0] << 8 ) | enc_len_p[1]; - tag = ticket + enc_len; if( len != TICKET_MIN_LEN + enc_len ) { @@ -344,13 +341,13 @@ int mbedtls_ssl_ticket_parse( void *p_ticket, } /* Decrypt and authenticate */ - if( ( ret = mbedtls_cipher_auth_decrypt( &key->ctx, + if( ( ret = mbedtls_cipher_auth_decrypt_ext( &key->ctx, iv, TICKET_IV_BYTES, /* Additional data: key name, IV and length */ key_name, TICKET_ADD_DATA_LEN, - ticket, enc_len, - ticket, &clear_len, - tag, TICKET_AUTH_TAG_BYTES ) ) != 0 ) + ticket, enc_len + TICKET_AUTH_TAG_BYTES, + ticket, enc_len, &clear_len, + TICKET_AUTH_TAG_BYTES ) ) != 0 ) { if( ret == MBEDTLS_ERR_CIPHER_AUTH_FAILED ) ret = MBEDTLS_ERR_SSL_INVALID_MAC; From 9b2a78966f94032869c8cff1d522a3e09f0f5a5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 3 Dec 2020 11:09:46 +0100 Subject: [PATCH 10/21] Use exact-size buffers for testing auth_xxcrypt() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_cipher.function | 58 ++++++++++++++++--------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 3b6d1e307..0aa2ad8c0 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1035,14 +1035,9 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, #if !defined(MBEDTLS_DEPRECATED_WARNING) && \ !defined(MBEDTLS_DEPRECATED_REMOVED) - unsigned char output[300]; /* Temporary buffer for results of - * encryption and decryption. */ - unsigned char *output_tag = NULL; /* Temporary buffer for tag in the - * encryption step. */ unsigned char *tmp_tag = NULL; unsigned char *tmp_cipher = NULL; - - memset( output, 0xFF, sizeof( output ) ); + unsigned char *tag_buf = NULL; #endif /* !MBEDTLS_DEPRECATED_WARNING && !MBEDTLS_DEPRECATED_REMOVED */ mbedtls_cipher_init( &ctx ); @@ -1234,11 +1229,11 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, * Authenticate and decrypt, and check result */ - /* Sanity check that we don't use overly long inputs. */ - TEST_ASSERT( sizeof( output ) >= cipher->len ); - + /* We can't pass a NULL output buffer to this funciton */ + ASSERT_ALLOC( decrypt_buf, cipher->len ? cipher->len : 1 ); + outlen = 0; ret = mbedtls_cipher_auth_decrypt( &ctx, iv->x, iv->len, ad->x, ad->len, - tmp_cipher, cipher->len, output, &outlen, + tmp_cipher, cipher->len, decrypt_buf, &outlen, tmp_tag, tag->len ); if( using_nist_kw ) @@ -1257,9 +1252,14 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, TEST_ASSERT( ret == 0 ); TEST_ASSERT( outlen == clear->len ); - TEST_ASSERT( memcmp( output, clear->x, clear->len ) == 0 ); + TEST_ASSERT( memcmp( decrypt_buf, clear->x, clear->len ) == 0 ); } + mbedtls_free( decrypt_buf ); + decrypt_buf = NULL; + mbedtls_free( cipher_plus_tag ); + cipher_plus_tag = NULL; + /* * Encrypt back if test data was authentic */ @@ -1269,19 +1269,31 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, MBEDTLS_ENCRYPT ); + /* prepare buffers for encryption */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) + if( use_psa ) + { + ASSERT_ALLOC( cipher_plus_tag, cipher->len + tag->len ); + tmp_cipher = cipher_plus_tag; + tmp_tag = cipher_plus_tag + cipher->len; + } + else +#endif /* MBEDTLS_USE_PSA_CRYPTO */ + { + /* can't pass a NULL output buffer to this function */ + ASSERT_ALLOC( encrypt_buf, cipher->len ? cipher->len : 1 ); + ASSERT_ALLOC( tag_buf, tag->len ); + tmp_cipher = encrypt_buf; + tmp_tag = tag_buf; + } + /* * Encrypt and check the result */ - memset( output, 0xFF, sizeof( output ) ); outlen = 0; - - /* Sanity check that we don't use overly long inputs. */ - TEST_ASSERT( sizeof( output ) >= clear->len + tag->len ); - - output_tag = output + clear->len; ret = mbedtls_cipher_auth_encrypt( &ctx, iv->x, iv->len, ad->x, ad->len, - clear->x, clear->len, output, &outlen, - output_tag, tag->len ); + clear->x, clear->len, tmp_cipher, &outlen, + tmp_tag, tag->len ); if( using_nist_kw ) { @@ -1292,8 +1304,8 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, TEST_ASSERT( ret == 0 ); TEST_ASSERT( outlen == cipher->len ); - TEST_ASSERT( memcmp( output, cipher->x, cipher->len ) == 0 ); - TEST_ASSERT( memcmp( output_tag, tag->x, tag->len ) == 0 ); + TEST_ASSERT( memcmp( tmp_cipher, cipher->x, cipher->len ) == 0 ); + TEST_ASSERT( memcmp( tmp_tag, tag->x, tag->len ) == 0 ); } } @@ -1305,6 +1317,10 @@ exit: mbedtls_free( decrypt_buf ); mbedtls_free( encrypt_buf ); mbedtls_free( cipher_plus_tag ); +#if !defined(MBEDTLS_DEPRECATED_WARNING) && \ + !defined(MBEDTLS_DEPRECATED_REMOVED) + mbedtls_free( tag_buf ); +#endif /* !MBEDTLS_DEPRECATED_WARNING && !MBEDTLS_DEPRECATED_REMOVED */ #if defined(MBEDTLS_USE_PSA_CRYPTO) if( use_psa == 1 ) From 86796bc8a59f8fc9930ba4722feb5a8aaddef9d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 3 Dec 2020 11:29:22 +0100 Subject: [PATCH 11/21] Add check_param test for cipher_auth_xxcrypt_ext() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_cipher.function | 102 ++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 0aa2ad8c0..0fcbd3749 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -529,6 +529,108 @@ void cipher_invalid_param_conditional( ) NULL, valid_size ) ); #endif /* defined(MBEDTLS_CIPHER_MODE_AEAD) */ +#if defined(MBEDTLS_CIPHER_MODE_AEAD) || defined(MBEDTLS_NIST_KW_C) + /* mbedtls_cipher_auth_encrypt_ext */ + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_encrypt_ext( NULL, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, &size_t_var, + valid_size ) ); + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_encrypt_ext( &valid_ctx, + NULL, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, &size_t_var, + valid_size ) ); + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_encrypt_ext( &valid_ctx, + valid_buffer, valid_size, + NULL, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, &size_t_var, + valid_size ) ); + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_encrypt_ext( &valid_ctx, + valid_buffer, valid_size, + valid_buffer, valid_size, + NULL, valid_size, + valid_buffer, valid_size, &size_t_var, + valid_size ) ); + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_encrypt_ext( &valid_ctx, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, + NULL, valid_size, &size_t_var, + valid_size ) ); + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_encrypt_ext( &valid_ctx, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, NULL, + valid_size ) ); + + /* mbedtls_cipher_auth_decrypt_ext */ + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_decrypt_ext( NULL, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, &size_t_var, + valid_size ) ); + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_decrypt_ext( &valid_ctx, + NULL, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, &size_t_var, + valid_size ) ); + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_decrypt_ext( &valid_ctx, + valid_buffer, valid_size, + NULL, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, &size_t_var, + valid_size ) ); + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_decrypt_ext( &valid_ctx, + valid_buffer, valid_size, + valid_buffer, valid_size, + NULL, valid_size, + valid_buffer, valid_size, &size_t_var, + valid_size ) ); + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_decrypt_ext( &valid_ctx, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, + NULL, valid_size, &size_t_var, + valid_size ) ); + TEST_INVALID_PARAM_RET( + MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, + mbedtls_cipher_auth_decrypt_ext( &valid_ctx, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, + valid_buffer, valid_size, NULL, + valid_size ) ); +#endif /* MBEDTLS_CIPHER_MODE_AEAD || MBEDTLS_NIST_KW_C */ + /* mbedtls_cipher_free() */ TEST_VALID_PARAM( mbedtls_cipher_free( NULL ) ); exit: From f215ef82aff029b376c1a10535fa6c7baccf12aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 3 Dec 2020 12:33:31 +0100 Subject: [PATCH 12/21] Test that auth_decrypt{,_ext}() zeroize on failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The documentation says it does, so it should be tested. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_cipher.function | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 0fcbd3749..b77d3696f 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -55,6 +55,19 @@ static void cipher_reset_key( mbedtls_cipher_context_t *ctx, int cipher_id, exit: ; } + +/* + * Check if a buffer is all-0 bytes: + * return 1 if it is, + * 0 if it isn't. + */ +int buffer_is_all_zero( const uint8_t *buf, size_t size ) +{ + for( size_t i = 0; i < size; i++ ) + if( buf[i] != 0 ) + return 0; + return 1; +} #endif /* MBEDTLS_CIPHER_AUTH_CRYPT */ /* END_HEADER */ @@ -1223,6 +1236,7 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, if( strcmp( result, "FAIL" ) == 0 ) { TEST_ASSERT( ret == MBEDTLS_ERR_CIPHER_AUTH_FAILED ); + TEST_ASSERT( buffer_is_all_zero( decrypt_buf, decrypt_buf_len ) ); } else { @@ -1347,6 +1361,7 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, { /* unauthentic message */ TEST_ASSERT( ret == MBEDTLS_ERR_CIPHER_AUTH_FAILED ); + TEST_ASSERT( buffer_is_all_zero( decrypt_buf, cipher->len ) ); } else { From 6df90523e179173baeb841aea40eda17d6a1dab7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 3 Dec 2020 13:00:58 +0100 Subject: [PATCH 13/21] Add ChangeLog entries for auth_crypt changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/cipher-auth-crypt-nist-kw.txt | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 ChangeLog.d/cipher-auth-crypt-nist-kw.txt diff --git a/ChangeLog.d/cipher-auth-crypt-nist-kw.txt b/ChangeLog.d/cipher-auth-crypt-nist-kw.txt new file mode 100644 index 000000000..63519a126 --- /dev/null +++ b/ChangeLog.d/cipher-auth-crypt-nist-kw.txt @@ -0,0 +1,22 @@ +API changes + * The functions mbedtls_cipher_auth_encrypt() and + mbedtls_cipher_auth_decrypt() no longer accept NIST_KW contexts, + as they have no way to check if the output buffer is large enough. + Please use mbedtls_cipher_auth_encrypt_ext() and + mbedtls_cipher_auth_decrypt_ext() instead. + +Security + * The functions mbedtls_cipher_auth_encrypt() and + mbedtls_cipher_auth_decrypt() would write past the minimum documented + size of the output buffer when used with NIST_KW. As a result, code using + those functions as documented with NIST_KW could have a buffer overwrite + of up to 15 bytes, with consequences ranging up to arbitrary code + execution depending on the location of the output buffer. + +New deprecations + * The functions mbedtls_cipher_auth_encrypt() and + mbedtls_cipher_auth_decrypt() are deprecated in favour of the new + functions mbedtls_cipher_auth_encrypt_ext() and + mbedtls_cipher_auth_decrypt_ext(). Please note that with AEAD ciphers, + these new functions always append the tag to the ciphertext, and include + the tag in the ciphertext length. From b23e31d86ac58d160d1a236da4120d02800740c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 7 Dec 2020 09:57:35 +0100 Subject: [PATCH 14/21] Minor documentation/comment fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit typos, overlong lines Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/cipher.h | 12 ++++++------ tests/suites/test_suite_cipher.function | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/mbedtls/cipher.h b/include/mbedtls/cipher.h index 9ae2f0609..1cafa6ec2 100644 --- a/include/mbedtls/cipher.h +++ b/include/mbedtls/cipher.h @@ -992,9 +992,9 @@ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, * \param iv The nonce to use. This must be a readable buffer of * at least \p iv_len Bytes and may be \c NULL if \p * iv_len is \c 0. - * \param iv_len The length of the nonce. For AEAD ciphers, this must satisfy the - * constraints imposed by the cipher used. For NIST_KW, - * this must be \c 0. + * \param iv_len The length of the nonce. For AEAD ciphers, this must + * satisfy the constraints imposed by the cipher used. + * For NIST_KW, this must be \c 0. * \param ad The additional data to authenticate. This must be a * readable buffer of at least \p ad_len Bytes, and may * be \c NULL is \p ad_len is \c 0. @@ -1047,9 +1047,9 @@ int mbedtls_cipher_auth_encrypt_ext( mbedtls_cipher_context_t *ctx, * \param iv The nonce to use. This must be a readable buffer of * at least \p iv_len Bytes and may be \c NULL if \p * iv_len is \c 0. - * \param iv_len The length of the nonce. For AEAD ciphers, this must satisfy the - * constraints imposed by the cipher used. For NIST_KW, - * this must be \c 0. + * \param iv_len The length of the nonce. For AEAD ciphers, this must + * satisfy the constraints imposed by the cipher used. + * For NIST_KW, this must be \c 0. * \param ad The additional data to authenticate. This must be a * readable buffer of at least \p ad_len Bytes, and may * be \c NULL is \p ad_len is \c 0. diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index b77d3696f..47a763cc8 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1345,7 +1345,7 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, * Authenticate and decrypt, and check result */ - /* We can't pass a NULL output buffer to this funciton */ + /* We can't pass a NULL output buffer to this function */ ASSERT_ALLOC( decrypt_buf, cipher->len ? cipher->len : 1 ); outlen = 0; ret = mbedtls_cipher_auth_decrypt( &ctx, iv->x, iv->len, ad->x, ad->len, From 70edd689a8cc774e6359ebee982e0c8bcd504453 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 3 Dec 2020 20:27:27 +0100 Subject: [PATCH 15/21] cipher_auth_xxcrypt(): fix some null pointer handling Make sure that if a buffer is allowed to be empty, a null pointer is accepted if the buffer length is 0. This was already the case for most but not all arguments to mbedtls_cipher_auth_{en,de}crypt{,_ext}. Make sure to pass NULL for an empty buffer in the tests. Signed-off-by: Gilles Peskine --- library/cipher.c | 16 ++++++++-------- tests/suites/test_suite_cipher.function | 19 ++++++++++++++----- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index 44cba34bc..cf45446f7 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1313,7 +1313,7 @@ static int mbedtls_cipher_aead_encrypt( mbedtls_cipher_context_t *ctx, /* PSA Crypto API always writes the authentication tag * at the end of the encrypted message. */ - if( tag != output + ilen ) + if( output == NULL || tag != output + ilen ) return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); status = psa_aead_encrypt( cipher_psa->slot, @@ -1393,7 +1393,7 @@ static int mbedtls_cipher_aead_decrypt( mbedtls_cipher_context_t *ctx, /* PSA Crypto API always writes the authentication tag * at the end of the encrypted message. */ - if( tag != input + ilen ) + if( input == NULL || tag != input + ilen ) return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); status = psa_aead_decrypt( cipher_psa->slot, @@ -1481,10 +1481,10 @@ int mbedtls_cipher_auth_encrypt( mbedtls_cipher_context_t *ctx, unsigned char *tag, size_t tag_len ) { CIPHER_VALIDATE_RET( ctx != NULL ); - CIPHER_VALIDATE_RET( iv != NULL ); + CIPHER_VALIDATE_RET( iv_len == 0 || iv != NULL ); CIPHER_VALIDATE_RET( ad_len == 0 || ad != NULL ); CIPHER_VALIDATE_RET( ilen == 0 || input != NULL ); - CIPHER_VALIDATE_RET( output != NULL ); + CIPHER_VALIDATE_RET( ilen == 0 || output != NULL ); CIPHER_VALIDATE_RET( olen != NULL ); CIPHER_VALIDATE_RET( tag_len == 0 || tag != NULL ); @@ -1515,10 +1515,10 @@ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, const unsigned char *tag, size_t tag_len ) { CIPHER_VALIDATE_RET( ctx != NULL ); - CIPHER_VALIDATE_RET( iv != NULL ); + CIPHER_VALIDATE_RET( iv_len == 0 || iv != NULL ); CIPHER_VALIDATE_RET( ad_len == 0 || ad != NULL ); CIPHER_VALIDATE_RET( ilen == 0 || input != NULL ); - CIPHER_VALIDATE_RET( output != NULL ); + CIPHER_VALIDATE_RET( ilen == 0 || output != NULL ); CIPHER_VALIDATE_RET( olen != NULL ); CIPHER_VALIDATE_RET( tag_len == 0 || tag != NULL ); @@ -1552,7 +1552,7 @@ int mbedtls_cipher_auth_encrypt_ext( mbedtls_cipher_context_t *ctx, size_t *olen, size_t tag_len ) { CIPHER_VALIDATE_RET( ctx != NULL ); - CIPHER_VALIDATE_RET( iv != NULL ); + CIPHER_VALIDATE_RET( iv_len == 0 || iv != NULL ); CIPHER_VALIDATE_RET( ad_len == 0 || ad != NULL ); CIPHER_VALIDATE_RET( ilen == 0 || input != NULL ); CIPHER_VALIDATE_RET( output != NULL ); @@ -1601,7 +1601,7 @@ int mbedtls_cipher_auth_decrypt_ext( mbedtls_cipher_context_t *ctx, size_t *olen, size_t tag_len ) { CIPHER_VALIDATE_RET( ctx != NULL ); - CIPHER_VALIDATE_RET( iv != NULL ); + CIPHER_VALIDATE_RET( iv_len == 0 || iv != NULL ); CIPHER_VALIDATE_RET( ad_len == 0 || ad != NULL ); CIPHER_VALIDATE_RET( ilen == 0 || input != NULL ); CIPHER_VALIDATE_RET( output_len == 0 || output != NULL ); diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 47a763cc8..ffe328458 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1155,6 +1155,16 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, unsigned char *tag_buf = NULL; #endif /* !MBEDTLS_DEPRECATED_WARNING && !MBEDTLS_DEPRECATED_REMOVED */ + /* Null pointers are documented as valid for inputs of length 0. + * The test framework passes non-null pointers, so set them to NULL. + * key, cipher and tag can't be empty. */ + if( iv->len == 0 ) + iv->x = NULL; + if( ad->len == 0 ) + ad->x = NULL; + if( clear->len == 0 ) + clear->x = NULL; + mbedtls_cipher_init( &ctx ); /* Initialize PSA Crypto */ @@ -1345,8 +1355,7 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, * Authenticate and decrypt, and check result */ - /* We can't pass a NULL output buffer to this function */ - ASSERT_ALLOC( decrypt_buf, cipher->len ? cipher->len : 1 ); + ASSERT_ALLOC( decrypt_buf, cipher->len ); outlen = 0; ret = mbedtls_cipher_auth_decrypt( &ctx, iv->x, iv->len, ad->x, ad->len, tmp_cipher, cipher->len, decrypt_buf, &outlen, @@ -1397,8 +1406,7 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, else #endif /* MBEDTLS_USE_PSA_CRYPTO */ { - /* can't pass a NULL output buffer to this function */ - ASSERT_ALLOC( encrypt_buf, cipher->len ? cipher->len : 1 ); + ASSERT_ALLOC( encrypt_buf, cipher->len ); ASSERT_ALLOC( tag_buf, tag->len ); tmp_cipher = encrypt_buf; tmp_tag = tag_buf; @@ -1421,7 +1429,8 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, TEST_ASSERT( ret == 0 ); TEST_ASSERT( outlen == cipher->len ); - TEST_ASSERT( memcmp( tmp_cipher, cipher->x, cipher->len ) == 0 ); + if( cipher->len != 0 ) + TEST_ASSERT( memcmp( tmp_cipher, cipher->x, cipher->len ) == 0 ); TEST_ASSERT( memcmp( tmp_tag, tag->x, tag->len ) == 0 ); } } From a2971ea62cfdd930ec89ab99f00800550d50398b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 3 Dec 2020 20:36:02 +0100 Subject: [PATCH 16/21] Simplify some buffer comparisons in tests Signed-off-by: Gilles Peskine --- tests/suites/test_suite_cipher.function | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index ffe328458..5e9a1e3f2 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1251,10 +1251,7 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, else { TEST_ASSERT( ret == 0 ); - - TEST_ASSERT( outlen == clear->len ); - if( clear->len != 0 ) - TEST_ASSERT( memcmp( decrypt_buf, clear->x, clear->len ) == 0 ); + ASSERT_COMPARE( decrypt_buf, outlen, clear->x, clear->len ); } /* Free this, but keep cipher_plus_tag for deprecated function with PSA */ @@ -1376,9 +1373,7 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, { /* authentic message: is the plaintext correct? */ TEST_ASSERT( ret == 0 ); - - TEST_ASSERT( outlen == clear->len ); - TEST_ASSERT( memcmp( decrypt_buf, clear->x, clear->len ) == 0 ); + ASSERT_COMPARE( decrypt_buf, outlen, clear->x, clear->len ); } mbedtls_free( decrypt_buf ); From 8a3d2348596d3972eb57a759bce1f6c799dc4066 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 3 Dec 2020 21:06:15 +0100 Subject: [PATCH 17/21] Fail the test case immediately if cipher_reset_key fails Signed-off-by: Gilles Peskine --- tests/suites/test_suite_cipher.function | 26 +++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 5e9a1e3f2..1d98f3db0 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -27,7 +27,7 @@ * individual ciphers, and it doesn't work with the PSA wrappers. So don't do * it, and instead start with a fresh context. */ -static void cipher_reset_key( mbedtls_cipher_context_t *ctx, int cipher_id, +static int cipher_reset_key( mbedtls_cipher_context_t *ctx, int cipher_id, int use_psa, size_t tag_len, const data_t *key, int direction ) { mbedtls_cipher_free( ctx ); @@ -52,8 +52,10 @@ static void cipher_reset_key( mbedtls_cipher_context_t *ctx, int cipher_id, TEST_ASSERT( 0 == mbedtls_cipher_setkey( ctx, key->x, 8 * key->len, direction ) ); + return( 1 ); + exit: - ; + return( 0 ); } /* @@ -1195,8 +1197,9 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, /* * Prepare context for decryption */ - cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, - MBEDTLS_DECRYPT ); + if( ! cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, + MBEDTLS_DECRYPT ) ) + goto exit; /* * prepare buffer for decryption @@ -1264,8 +1267,9 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, if( strcmp( result, "FAIL" ) != 0 ) { /* prepare context for encryption */ - cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, - MBEDTLS_ENCRYPT ); + if( ! cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, + MBEDTLS_ENCRYPT ) ) + goto exit; /* * Compute size of output buffer according to documentation @@ -1327,8 +1331,9 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, /* * Prepare context for decryption */ - cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, - MBEDTLS_DECRYPT ); + if( ! cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, + MBEDTLS_DECRYPT ) ) + goto exit; /* * Prepare pointers for decryption @@ -1387,8 +1392,9 @@ void auth_crypt_tv( int cipher_id, data_t * key, data_t * iv, if( strcmp( result, "FAIL" ) != 0 ) { /* prepare context for encryption */ - cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, - MBEDTLS_ENCRYPT ); + if( ! cipher_reset_key( &ctx, cipher_id, use_psa, tag->len, key, + MBEDTLS_ENCRYPT ) ) + goto exit; /* prepare buffers for encryption */ #if defined(MBEDTLS_USE_PSA_CRYPTO) From e09aeb4923f17449be7b8cda9a998e5c31ba273c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Dec 2020 00:31:09 +0100 Subject: [PATCH 18/21] Remove redundant NIST_KW checks in cipher_auth_xxcrypt() The internal functions mbedtls_cipher_aead_{encrypt,decrypt} reject unsupported algorithms, so there's no need for an additional check in the legacy wrappers. Signed-off-by: Gilles Peskine --- library/cipher.c | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index cf45446f7..503109253 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1488,17 +1488,6 @@ int mbedtls_cipher_auth_encrypt( mbedtls_cipher_context_t *ctx, CIPHER_VALIDATE_RET( olen != NULL ); CIPHER_VALIDATE_RET( tag_len == 0 || tag != NULL ); -#if defined(MBEDTLS_NIST_KW_C) - if( MBEDTLS_MODE_KW == ctx->cipher_info->mode || - MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) - { - /* NIST_KW is not supported because we used to document the wrong size - * of the output buffer, so people should move to the _ext API, - * which has an explicit argument for buffer size. */ - return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); - } -#endif /* MBEDTLS_NIST_KW_C */ - return( mbedtls_cipher_aead_encrypt( ctx, iv, iv_len, ad, ad_len, input, ilen, output, olen, tag, tag_len ) ); @@ -1522,17 +1511,6 @@ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, CIPHER_VALIDATE_RET( olen != NULL ); CIPHER_VALIDATE_RET( tag_len == 0 || tag != NULL ); -#if defined(MBEDTLS_NIST_KW_C) - if( MBEDTLS_MODE_KW == ctx->cipher_info->mode || - MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) - { - /* NIST_KW is not supported because we used to document the wrong size - * of the output buffer, so people should move to the _ext API, - * which has an explicit argument for buffer size. */ - return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); - } -#endif /* MBEDTLS_NIST_KW_C */ - return( mbedtls_cipher_aead_decrypt( ctx, iv, iv_len, ad, ad_len, input, ilen, output, olen, tag, tag_len ) ); From a56d3d9e758e27ba529ea6be561dc716bf5d3ce3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Dec 2020 00:47:07 +0100 Subject: [PATCH 19/21] cipher_auth_xxcrypt_ext: Make NIST_KW case more robust Don't invoke classic NIST_KW in case PSA gains support for NIST_KW. Signed-off-by: Gilles Peskine --- library/cipher.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index 503109253..119e79600 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1537,8 +1537,12 @@ int mbedtls_cipher_auth_encrypt_ext( mbedtls_cipher_context_t *ctx, CIPHER_VALIDATE_RET( olen != NULL ); #if defined(MBEDTLS_NIST_KW_C) - if( MBEDTLS_MODE_KW == ctx->cipher_info->mode || - MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) + if( +#if defined(MBEDTLS_USE_PSA_CRYPTO) + ctx->psa_enabled == 0 && +#endif + ( MBEDTLS_MODE_KW == ctx->cipher_info->mode || + MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) ) { mbedtls_nist_kw_mode_t mode = ( MBEDTLS_MODE_KW == ctx->cipher_info->mode ) ? MBEDTLS_KW_MODE_KW : MBEDTLS_KW_MODE_KWP; @@ -1586,8 +1590,12 @@ int mbedtls_cipher_auth_decrypt_ext( mbedtls_cipher_context_t *ctx, CIPHER_VALIDATE_RET( olen != NULL ); #if defined(MBEDTLS_NIST_KW_C) - if( MBEDTLS_MODE_KW == ctx->cipher_info->mode || - MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) + if( +#if defined(MBEDTLS_USE_PSA_CRYPTO) + ctx->psa_enabled == 0 && +#endif + ( MBEDTLS_MODE_KW == ctx->cipher_info->mode || + MBEDTLS_MODE_KWP == ctx->cipher_info->mode ) ) { mbedtls_nist_kw_mode_t mode = ( MBEDTLS_MODE_KW == ctx->cipher_info->mode ) ? MBEDTLS_KW_MODE_KW : MBEDTLS_KW_MODE_KWP; From 4e0a4d444d44d6722fd2e71fcdc4703365dd2ea2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Dec 2020 00:48:14 +0100 Subject: [PATCH 20/21] Clarifying comment Signed-off-by: Gilles Peskine --- library/cipher.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index 119e79600..ee365a185 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1471,7 +1471,7 @@ static int mbedtls_cipher_aead_decrypt( mbedtls_cipher_context_t *ctx, #if !defined(MBEDTLS_DEPRECATED_REMOVED) /* - * Packet-oriented encryption for AEAD modes: public function. + * Packet-oriented encryption for AEAD modes: public legacy function. */ int mbedtls_cipher_auth_encrypt( mbedtls_cipher_context_t *ctx, const unsigned char *iv, size_t iv_len, @@ -1494,7 +1494,7 @@ int mbedtls_cipher_auth_encrypt( mbedtls_cipher_context_t *ctx, } /* - * Packet-oriented decryption for AEAD modes: public function. + * Packet-oriented decryption for AEAD modes: public legacy function. */ int mbedtls_cipher_auth_decrypt( mbedtls_cipher_context_t *ctx, const unsigned char *iv, size_t iv_len, From 841b6fa97fa3221ea8f1ce31f99c3673ba8940e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 7 Dec 2020 10:42:21 +0100 Subject: [PATCH 21/21] Fix unused param warnings in auth_xxcrypt_ext() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/cipher.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/cipher.c b/library/cipher.c index ee365a185..457f8f660 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1552,6 +1552,9 @@ int mbedtls_cipher_auth_encrypt_ext( mbedtls_cipher_context_t *ctx, if( iv_len != 0 || tag_len != 0 || ad_len != 0 ) return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); + (void) iv; + (void) ad; + return( mbedtls_nist_kw_wrap( ctx->cipher_ctx, mode, input, ilen, output, olen, output_len ) ); } @@ -1605,6 +1608,9 @@ int mbedtls_cipher_auth_decrypt_ext( mbedtls_cipher_context_t *ctx, if( iv_len != 0 || tag_len != 0 || ad_len != 0 ) return( MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); + (void) iv; + (void) ad; + return( mbedtls_nist_kw_unwrap( ctx->cipher_ctx, mode, input, ilen, output, olen, output_len ) ); }