From 3fddbb264e87a8cef2903cbd7b02b8e1a39a2a99 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 30 Jul 2020 12:02:06 +0100 Subject: Add an HMAC implementation that is TLS aware The TLS HMAC implementation should take care to calculate the MAC in constant time in the case of MAC-Then-Encrypt where we have a variable amount of padding. Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/12732) --- crypto/md5/build.info | 4 +- include/openssl/core_names.h | 9 +-- providers/implementations/macs/hmac_prov.c | 91 +++++++++++++++++++++++++++++- ssl/build.info | 1 + ssl/record/ssl3_record.c | 4 +- ssl/s3_cbc.c | 56 ++++++++++++++---- ssl/ssl_local.h | 3 +- 7 files changed, 146 insertions(+), 22 deletions(-) diff --git a/crypto/md5/build.info b/crypto/md5/build.info index d4494b2..afcf7c4 100644 --- a/crypto/md5/build.info +++ b/crypto/md5/build.info @@ -14,7 +14,9 @@ IF[{- !$disabled{asm} -}] ENDIF ENDIF -SOURCE[../../libcrypto]=md5_dgst.c md5_one.c md5_sha1.c $MD5ASM +$COMMON=md5_dgst.c md5_one.c md5_sha1.c $MD5ASM +SOURCE[../../libcrypto]=$COMMON +SOURCE[../../providers/libimplementations.a]=$COMMON # Implementations are now spread across several libraries, so the defines # need to be applied to all affected libraries and modules. diff --git a/include/openssl/core_names.h b/include/openssl/core_names.h index 3be69d5..fc8d2ce 100644 --- a/include/openssl/core_names.h +++ b/include/openssl/core_names.h @@ -153,10 +153,11 @@ extern "C" { * If "engine" or "properties" are specified, they should always be paired * with "cipher" or "digest". */ -#define OSSL_MAC_PARAM_CIPHER OSSL_ALG_PARAM_CIPHER /* utf8 string */ -#define OSSL_MAC_PARAM_DIGEST OSSL_ALG_PARAM_DIGEST /* utf8 string */ -#define OSSL_MAC_PARAM_PROPERTIES OSSL_ALG_PARAM_PROPERTIES /* utf8 string */ -#define OSSL_MAC_PARAM_SIZE "size" /* size_t */ +#define OSSL_MAC_PARAM_CIPHER OSSL_ALG_PARAM_CIPHER /* utf8 string */ +#define OSSL_MAC_PARAM_DIGEST OSSL_ALG_PARAM_DIGEST /* utf8 string */ +#define OSSL_MAC_PARAM_PROPERTIES OSSL_ALG_PARAM_PROPERTIES /* utf8 string */ +#define OSSL_MAC_PARAM_SIZE "size" /* size_t */ +#define OSSL_MAC_PARAM_TLS_DATA_SIZE "tls-data-size" /* size_t */ /* Known MAC names */ #define OSSL_MAC_NAME_BLAKE2BMAC "BLAKE2BMAC" diff --git a/providers/implementations/macs/hmac_prov.c b/providers/implementations/macs/hmac_prov.c index b30de6c..f328411 100644 --- a/providers/implementations/macs/hmac_prov.c +++ b/providers/implementations/macs/hmac_prov.c @@ -13,6 +13,8 @@ */ #include "internal/deprecated.h" +#include + #include #include #include @@ -47,8 +49,27 @@ struct hmac_data_st { void *provctx; HMAC_CTX *ctx; /* HMAC context */ PROV_DIGEST digest; + unsigned char *key; + size_t keylen; + /* Length of TLS data including the MAC but excluding padding */ + size_t tls_data_size; + unsigned char tls_header[13]; + int tls_header_set; + unsigned char tls_mac_out[EVP_MAX_MD_SIZE]; + size_t tls_mac_out_size; }; +/* Defined in ssl/s3_cbc.c */ +int ssl3_cbc_digest_record(const EVP_MD *md, + unsigned char *md_out, + size_t *md_out_size, + const unsigned char header[13], + const unsigned char *data, + size_t data_plus_mac_size, + size_t data_plus_mac_plus_padding_size, + const unsigned char *mac_secret, + size_t mac_secret_length, char is_sslv3); + static size_t hmac_size(void *vmacctx); static void *hmac_new(void *provctx) @@ -73,6 +94,7 @@ static void hmac_free(void *vmacctx) if (macctx != NULL) { HMAC_CTX_free(macctx->ctx); ossl_prov_digest_reset(&macctx->digest); + OPENSSL_secure_clear_free(macctx->key, macctx->keylen); OPENSSL_free(macctx); } } @@ -81,15 +103,30 @@ static void *hmac_dup(void *vsrc) { struct hmac_data_st *src = vsrc; struct hmac_data_st *dst = hmac_new(src->provctx); + HMAC_CTX *ctx; if (dst == NULL) return NULL; + ctx = dst->ctx; + *dst = *src; + dst->ctx = ctx; + dst->key = NULL; + if (!HMAC_CTX_copy(dst->ctx, src->ctx) || !ossl_prov_digest_copy(&dst->digest, &src->digest)) { hmac_free(dst); return NULL; } + if (src->key != NULL) { + /* There is no "secure" OPENSSL_memdup */ + dst->key = OPENSSL_secure_malloc(src->keylen); + if (dst->key == NULL) { + hmac_free(dst); + return 0; + } + memcpy(dst->key, src->key, src->keylen); + } return dst; } @@ -107,10 +144,10 @@ static int hmac_init(void *vmacctx) int rv = 1; /* HMAC_Init_ex doesn't tolerate all zero params, so we must be careful */ - if (digest != NULL) + if (macctx->tls_data_size == 0 && digest != NULL) rv = HMAC_Init_ex(macctx->ctx, NULL, 0, digest, ossl_prov_digest_engine(&macctx->digest)); - ossl_prov_digest_reset(&macctx->digest); + return rv; } @@ -119,6 +156,32 @@ static int hmac_update(void *vmacctx, const unsigned char *data, { struct hmac_data_st *macctx = vmacctx; + if (macctx->tls_data_size > 0) { + /* We're doing a TLS HMAC */ + if (!macctx->tls_header_set) { + /* We expect the first update call to contain the TLS header */ + if (datalen != sizeof(macctx->tls_header)) + return 0; + memcpy(macctx->tls_header, data, datalen); + macctx->tls_header_set = 1; + return 1; + } + /* datalen is macctx->tls_data_size plus the padding length */ + if (datalen < macctx->tls_data_size) + return 0; + + return ssl3_cbc_digest_record(ossl_prov_digest_md(&macctx->digest), + macctx->tls_mac_out, + &macctx->tls_mac_out_size, + macctx->tls_header, + data, + macctx->tls_data_size, + datalen, + macctx->key, + macctx->keylen, + 0); + } + return HMAC_Update(macctx->ctx, data, datalen); } @@ -128,6 +191,14 @@ static int hmac_final(void *vmacctx, unsigned char *out, size_t *outl, unsigned int hlen; struct hmac_data_st *macctx = vmacctx; + if (macctx->tls_data_size > 0) { + if (macctx->tls_mac_out_size == 0) + return 0; + if (outl != NULL) + *outl = macctx->tls_mac_out_size; + memcpy(out, macctx->tls_mac_out, macctx->tls_mac_out_size); + return 1; + } if (!HMAC_Final(macctx->ctx, out, &hlen)) return 0; *outl = hlen; @@ -158,6 +229,7 @@ static const OSSL_PARAM known_settable_ctx_params[] = { OSSL_PARAM_utf8_string(OSSL_MAC_PARAM_PROPERTIES, NULL, 0), OSSL_PARAM_octet_string(OSSL_MAC_PARAM_KEY, NULL, 0), OSSL_PARAM_int(OSSL_MAC_PARAM_FLAGS, NULL), + OSSL_PARAM_size_t(OSSL_MAC_PARAM_TLS_DATA_SIZE, NULL), OSSL_PARAM_END }; static const OSSL_PARAM *hmac_settable_ctx_params(ossl_unused void *provctx) @@ -190,12 +262,25 @@ static int hmac_set_ctx_params(void *vmacctx, const OSSL_PARAM params[]) if (p->data_type != OSSL_PARAM_OCTET_STRING) return 0; + if (macctx->keylen > 0) + OPENSSL_secure_clear_free(macctx->key, macctx->keylen); + /* Keep a copy of the key if we need it for TLS HMAC */ + macctx->key = OPENSSL_secure_malloc(p->data_size); + if (macctx->key == NULL) + return 0; + memcpy(macctx->key, p->data, p->data_size); + macctx->keylen = p->data_size; + if (!HMAC_Init_ex(macctx->ctx, p->data, p->data_size, ossl_prov_digest_md(&macctx->digest), NULL /* ENGINE */)) return 0; - ossl_prov_digest_reset(&macctx->digest); + } + if ((p = OSSL_PARAM_locate_const(params, + OSSL_MAC_PARAM_TLS_DATA_SIZE)) != NULL) { + if (!OSSL_PARAM_get_size_t(p, &macctx->tls_data_size)) + return 0; } return 1; } diff --git a/ssl/build.info b/ssl/build.info index cfcb2b1..e5b7bef 100644 --- a/ssl/build.info +++ b/ssl/build.info @@ -37,3 +37,4 @@ SOURCE[../libssl]=\ DEFINE[../libssl]=$AESDEF SOURCE[../providers/libcommon.a]=record/tls_pad.c +SOURCE[../providers/libimplementations.a]=s3_cbc.c diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index 634052d..70707da 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -1362,7 +1362,7 @@ int n_ssl3_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int sending) header[j++] = (unsigned char)(rec->length & 0xff); /* Final param == is SSLv3 */ - if (ssl3_cbc_digest_record(ssl, hash, + if (ssl3_cbc_digest_record(EVP_MD_CTX_md(hash), md, &md_size, header, rec->input, rec->length + md_size, rec->orig_len, @@ -1473,7 +1473,7 @@ int tls1_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int sending) * are hashing because that gives an attacker a timing-oracle. */ /* Final param == not SSLv3 */ - if (ssl3_cbc_digest_record(ssl, mac_ctx, + if (ssl3_cbc_digest_record(EVP_MD_CTX_md(mac_ctx), md, &md_size, header, rec->input, rec->length + md_size, rec->orig_len, diff --git a/ssl/s3_cbc.c b/ssl/s3_cbc.c index ec1f3cf..bffaebb 100644 --- a/ssl/s3_cbc.c +++ b/ssl/s3_cbc.c @@ -8,18 +8,60 @@ */ /* + * This file has no dependencies on the rest of libssl because it is shared + * with the providers. It contains functions for low level MAC calculations. + * Responsibility for this lies with the HMAC implementation in the + * providers. However there are legacy code paths in libssl which also need to + * do this. In time those legacy code paths can be removed and this file can be + * moved out of libssl. + */ + + +/* * MD5 and SHA-1 low level APIs are deprecated for public use, but still ok for * internal use. */ #include "internal/deprecated.h" #include "internal/constant_time.h" -#include "ssl_local.h" #include "internal/cryptlib.h" +#include #include #include +char ssl3_cbc_record_digest_supported(const EVP_MD_CTX *ctx); +int ssl3_cbc_digest_record(const EVP_MD *md, + unsigned char *md_out, + size_t *md_out_size, + const unsigned char header[13], + const unsigned char *data, + size_t data_plus_mac_size, + size_t data_plus_mac_plus_padding_size, + const unsigned char *mac_secret, + size_t mac_secret_length, char is_sslv3); + +# define l2n(l,c) (*((c)++)=(unsigned char)(((l)>>24)&0xff), \ + *((c)++)=(unsigned char)(((l)>>16)&0xff), \ + *((c)++)=(unsigned char)(((l)>> 8)&0xff), \ + *((c)++)=(unsigned char)(((l) )&0xff)) + +# define l2n6(l,c) (*((c)++)=(unsigned char)(((l)>>40)&0xff), \ + *((c)++)=(unsigned char)(((l)>>32)&0xff), \ + *((c)++)=(unsigned char)(((l)>>24)&0xff), \ + *((c)++)=(unsigned char)(((l)>>16)&0xff), \ + *((c)++)=(unsigned char)(((l)>> 8)&0xff), \ + *((c)++)=(unsigned char)(((l) )&0xff)) + +# define l2n8(l,c) (*((c)++)=(unsigned char)(((l)>>56)&0xff), \ + *((c)++)=(unsigned char)(((l)>>48)&0xff), \ + *((c)++)=(unsigned char)(((l)>>40)&0xff), \ + *((c)++)=(unsigned char)(((l)>>32)&0xff), \ + *((c)++)=(unsigned char)(((l)>>24)&0xff), \ + *((c)++)=(unsigned char)(((l)>>16)&0xff), \ + *((c)++)=(unsigned char)(((l)>> 8)&0xff), \ + *((c)++)=(unsigned char)(((l) )&0xff)) + /* * MAX_HASH_BIT_COUNT_BYTES is the maximum number of bytes in the hash's * length field. (SHA-384/512 have 128-bit length.) @@ -131,8 +173,7 @@ char ssl3_cbc_record_digest_supported(const EVP_MD_CTX *ctx) * padding too. ) * Returns 1 on success or 0 on error */ -int ssl3_cbc_digest_record(SSL *s, - const EVP_MD_CTX *ctx, +int ssl3_cbc_digest_record(const EVP_MD *md, unsigned char *md_out, size_t *md_out_size, const unsigned char header[13], @@ -168,7 +209,6 @@ int ssl3_cbc_digest_record(SSL *s, size_t md_length_size = 8; char length_is_big_endian = 1; int ret = 0; - const EVP_MD *md = NULL; /* * This is a, hopefully redundant, check that allows us to forget about @@ -177,7 +217,7 @@ int ssl3_cbc_digest_record(SSL *s, if (!ossl_assert(data_plus_mac_plus_padding_size < 1024 * 1024)) return 0; - switch (EVP_MD_CTX_type(ctx)) { + switch (EVP_MD_type(md)) { case NID_md5: if (MD5_Init((MD5_CTX *)md_state.c) <= 0) return 0; @@ -463,10 +503,7 @@ int ssl3_cbc_digest_record(SSL *s, md_ctx = EVP_MD_CTX_new(); if (md_ctx == NULL) goto err; - md = ssl_evp_md_fetch(s->ctx->libctx, EVP_MD_type(EVP_MD_CTX_md(ctx)), - s->ctx->propq); - if (md == NULL) - goto err; + if (EVP_DigestInit_ex(md_ctx, md, NULL /* engine */ ) <= 0) goto err; if (is_sslv3) { @@ -494,6 +531,5 @@ int ssl3_cbc_digest_record(SSL *s, ret = 1; err: EVP_MD_CTX_free(md_ctx); - ssl_evp_md_free(md); return ret; } diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h index f74f833..c54ced6 100644 --- a/ssl/ssl_local.h +++ b/ssl/ssl_local.h @@ -2761,8 +2761,7 @@ int ktls_configure_crypto(const SSL *s, const EVP_CIPHER *c, EVP_CIPHER_CTX *dd, /* s3_cbc.c */ __owur char ssl3_cbc_record_digest_supported(const EVP_MD_CTX *ctx); -__owur int ssl3_cbc_digest_record(SSL *s, - const EVP_MD_CTX *ctx, +__owur int ssl3_cbc_digest_record(const EVP_MD *md, unsigned char *md_out, size_t *md_out_size, const unsigned char header[13], -- cgit v1.1