From 2446db0f52b8697f3e131db3315de8a66fd9e0fe Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 8 Jun 2016 18:31:42 -0400 Subject: [PATCH] Require in == out for in-place encryption. While most of OpenSSL's assembly allows out < in too, some of it doesn't. Upstream seems to not consider this a problem (or, at least, they're failing to make a decision on whether it is a problem, so we should assume they'll stay their course). Accordingly, require aliased buffers to exactly align so we don't have to keep chasing this down. Change-Id: I00eb3df3e195b249116c68f7272442918d7077eb Reviewed-on: https://boringssl-review.googlesource.com/8231 Reviewed-by: Adam Langley --- crypto/chacha/chacha.c | 10 ++- crypto/chacha/chacha_test.cc | 23 ++----- crypto/cipher/aead.c | 25 +++---- crypto/cipher/aead_test.cc | 123 +++++++++++++++-------------------- include/openssl/aead.h | 4 +- include/openssl/chacha.h | 4 +- ssl/internal.h | 2 +- 7 files changed, 82 insertions(+), 109 deletions(-) diff --git a/crypto/chacha/chacha.c b/crypto/chacha/chacha.c index afe1b2ad..15620894 100644 --- a/crypto/chacha/chacha.c +++ b/crypto/chacha/chacha.c @@ -16,10 +16,13 @@ #include +#include #include #include +#include "../internal.h" + #define U8TO32_LITTLE(p) \ (((uint32_t)((p)[0])) | ((uint32_t)((p)[1]) << 8) | \ @@ -36,8 +39,9 @@ void ChaCha20_ctr32(uint8_t *out, const uint8_t *in, size_t in_len, void CRYPTO_chacha_20(uint8_t *out, const uint8_t *in, size_t in_len, const uint8_t key[32], const uint8_t nonce[12], uint32_t counter) { - uint32_t counter_nonce[4]; - counter_nonce[0] = counter; + assert(!buffers_alias(out, in_len, in, in_len) || in == out); + + uint32_t counter_nonce[4]; counter_nonce[0] = counter; counter_nonce[1] = U8TO32_LITTLE(nonce + 0); counter_nonce[2] = U8TO32_LITTLE(nonce + 4); counter_nonce[3] = U8TO32_LITTLE(nonce + 8); @@ -118,6 +122,8 @@ static void chacha_core(uint8_t output[64], const uint32_t input[16]) { void CRYPTO_chacha_20(uint8_t *out, const uint8_t *in, size_t in_len, const uint8_t key[32], const uint8_t nonce[12], uint32_t counter) { + assert(!buffers_alias(out, in_len, in, in_len) || in == out); + uint32_t input[16]; uint8_t buf[64]; size_t todo, i; diff --git a/crypto/chacha/chacha_test.cc b/crypto/chacha/chacha_test.cc index f364f982..0a5972f7 100644 --- a/crypto/chacha/chacha_test.cc +++ b/crypto/chacha/chacha_test.cc @@ -218,25 +218,16 @@ static bool TestChaCha20(size_t len) { std::unique_ptr buf(new uint8_t[len]); CRYPTO_chacha_20(buf.get(), kInput, len, kKey, kNonce, kCounter); if (memcmp(buf.get(), kOutput, len) != 0) { - fprintf(stderr, "Mismatch at length %u.\n", static_cast(len)); + fprintf(stderr, "Mismatch at length %zu.\n", len); return false; } - // Test in-place at various offsets. - static const size_t kOffsets[] = { - 0, 1, 2, 8, 15, 16, 17, 31, 32, 33, 63, - 64, 65, 95, 96, 97, 127, 128, 129, 255, 256, 257, - }; - for (size_t offset : kOffsets) { - buf.reset(new uint8_t[len + offset]); - memcpy(buf.get() + offset, kInput, len); - CRYPTO_chacha_20(buf.get(), buf.get() + offset, len, kKey, kNonce, - kCounter); - if (memcmp(buf.get(), kOutput, len) != 0) { - fprintf(stderr, "Mismatch at length %u with in-place offset %u.\n", - static_cast(len), static_cast(offset)); - return false; - } + // Test in-place. + memcpy(buf.get(), kInput, len); + CRYPTO_chacha_20(buf.get(), buf.get(), len, kKey, kNonce, kCounter); + if (memcmp(buf.get(), kOutput, len) != 0) { + fprintf(stderr, "Mismatch at length %zu, in-place.\n", len); + return false; } return true; diff --git a/crypto/cipher/aead.c b/crypto/cipher/aead.c index b1db83d2..57eecc1b 100644 --- a/crypto/cipher/aead.c +++ b/crypto/cipher/aead.c @@ -20,6 +20,7 @@ #include #include "internal.h" +#include "../internal.h" size_t EVP_AEAD_key_length(const EVP_AEAD *aead) { return aead->key_len; } @@ -80,21 +81,15 @@ void EVP_AEAD_CTX_cleanup(EVP_AEAD_CTX *ctx) { ctx->aead = NULL; } -/* check_alias returns 0 if |out| points within the buffer determined by |in| - * and |in_len| and 1 otherwise. - * - * When processing, there's only an issue if |out| points within in[:in_len] - * and isn't equal to |in|. If that's the case then writing the output will - * stomp input that hasn't been read yet. - * - * This function checks for that case. */ -static int check_alias(const uint8_t *in, size_t in_len, const uint8_t *out) { - if (out <= in) { - return 1; - } else if (in + in_len <= out) { +/* check_alias returns 1 if |out| is compatible with |in| and 0 otherwise. If + * |in| and |out| alias, we require that |in| == |out|. */ +static int check_alias(const uint8_t *in, size_t in_len, const uint8_t *out, + size_t out_len) { + if (!buffers_alias(in, in_len, out, out_len)) { return 1; } - return 0; + + return in == out; } int EVP_AEAD_CTX_seal(const EVP_AEAD_CTX *ctx, uint8_t *out, size_t *out_len, @@ -108,7 +103,7 @@ int EVP_AEAD_CTX_seal(const EVP_AEAD_CTX *ctx, uint8_t *out, size_t *out_len, goto error; } - if (!check_alias(in, in_len, out)) { + if (!check_alias(in, in_len, out, max_out_len)) { OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_OUTPUT_ALIASES_INPUT); goto error; } @@ -130,7 +125,7 @@ int EVP_AEAD_CTX_open(const EVP_AEAD_CTX *ctx, uint8_t *out, size_t *out_len, size_t max_out_len, const uint8_t *nonce, size_t nonce_len, const uint8_t *in, size_t in_len, const uint8_t *ad, size_t ad_len) { - if (!check_alias(in, in_len, out)) { + if (!check_alias(in, in_len, out, max_out_len)) { OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_OUTPUT_ALIASES_INPUT); goto error; } diff --git a/crypto/cipher/aead_test.cc b/crypto/cipher/aead_test.cc index f21291e7..8bad93f5 100644 --- a/crypto/cipher/aead_test.cc +++ b/crypto/cipher/aead_test.cc @@ -225,84 +225,65 @@ static bool TestWithAliasedBuffers(const EVP_AEAD *aead) { return false; } - // First test with out > in, which we expect to fail. - for (auto offset : offsets) { - if (offset == 0) { - // Will be tested in the next loop. - continue; - } + // Test with out != in which we expect to fail. + std::vector buffer(2 + valid_encryption_len); + uint8_t *in = buffer.data() + 1; + uint8_t *out1 = buffer.data(); + uint8_t *out2 = buffer.data() + 2; - std::vector buffer(offset + valid_encryption_len); - memcpy(buffer.data(), kPlaintext, sizeof(kPlaintext)); - uint8_t *out = buffer.data() + offset; + memcpy(in, kPlaintext, sizeof(kPlaintext)); + size_t out_len; + if (EVP_AEAD_CTX_seal(ctx.get(), out1, &out_len, + sizeof(kPlaintext) + max_overhead, nonce.data(), + nonce_len, in, sizeof(kPlaintext), nullptr, 0) || + EVP_AEAD_CTX_seal(ctx.get(), out2, &out_len, + sizeof(kPlaintext) + max_overhead, nonce.data(), + nonce_len, in, sizeof(kPlaintext), nullptr, 0)) { + fprintf(stderr, "EVP_AEAD_CTX_seal unexpectedly succeeded.\n"); + return false; + } + ERR_clear_error(); - size_t out_len; - if (!EVP_AEAD_CTX_seal(ctx.get(), out, &out_len, - sizeof(kPlaintext) + max_overhead, nonce.data(), - nonce_len, buffer.data(), sizeof(kPlaintext), - nullptr, 0)) { - // We expect offsets where the output is greater than the input to fail. - ERR_clear_error(); - } else { - fprintf(stderr, - "EVP_AEAD_CTX_seal unexpectedly succeeded for offset %u.\n", - static_cast(offset)); - return false; - } + memcpy(in, valid_encryption.data(), valid_encryption_len); + if (EVP_AEAD_CTX_open(ctx.get(), out1, &out_len, valid_encryption_len, + nonce.data(), nonce_len, in, valid_encryption_len, + nullptr, 0) || + EVP_AEAD_CTX_open(ctx.get(), out2, &out_len, valid_encryption_len, + nonce.data(), nonce_len, in, valid_encryption_len, + nullptr, 0)) { + fprintf(stderr, "EVP_AEAD_CTX_open unexpectedly succeeded.\n"); + return false; + } + ERR_clear_error(); - memcpy(buffer.data(), valid_encryption.data(), valid_encryption_len); - if (!EVP_AEAD_CTX_open(ctx.get(), out, &out_len, valid_encryption_len, - nonce.data(), nonce_len, buffer.data(), - valid_encryption_len, nullptr, 0)) { - // We expect offsets where the output is greater than the input to fail. - ERR_clear_error(); - } else { - fprintf(stderr, - "EVP_AEAD_CTX_open unexpectedly succeeded for offset %u.\n", - static_cast(offset)); - ERR_print_errors_fp(stderr); - return false; - } + // Test with out == in, which we expect to work. + memcpy(in, kPlaintext, sizeof(kPlaintext)); + + if (!EVP_AEAD_CTX_seal(ctx.get(), in, &out_len, + sizeof(kPlaintext) + max_overhead, nonce.data(), + nonce_len, in, sizeof(kPlaintext), nullptr, 0)) { + fprintf(stderr, "EVP_AEAD_CTX_seal failed in-place.\n"); + return false; } - // Test with out <= in, which we expect to work. - for (auto offset : offsets) { - std::vector buffer(offset + valid_encryption_len); - uint8_t *const out = buffer.data(); - uint8_t *const in = buffer.data() + offset; - memcpy(in, kPlaintext, sizeof(kPlaintext)); + if (out_len != valid_encryption_len || + memcmp(in, valid_encryption.data(), out_len) != 0) { + fprintf(stderr, "EVP_AEAD_CTX_seal produced bad output in-place.\n"); + return false; + } - size_t out_len; - if (!EVP_AEAD_CTX_seal(ctx.get(), out, &out_len, - sizeof(kPlaintext) + max_overhead, nonce.data(), - nonce_len, in, sizeof(kPlaintext), nullptr, 0)) { - fprintf(stderr, "EVP_AEAD_CTX_seal failed for offset -%u.\n", - static_cast(offset)); - return false; - } + memcpy(in, valid_encryption.data(), valid_encryption_len); + if (!EVP_AEAD_CTX_open(ctx.get(), in, &out_len, valid_encryption_len, + nonce.data(), nonce_len, in, valid_encryption_len, + nullptr, 0)) { + fprintf(stderr, "EVP_AEAD_CTX_open failed in-place.\n"); + return false; + } - if (out_len != valid_encryption_len || - memcmp(out, valid_encryption.data(), out_len) != 0) { - fprintf(stderr, "EVP_AEAD_CTX_seal produced bad output for offset -%u.\n", - static_cast(offset)); - return false; - } - - memcpy(in, valid_encryption.data(), valid_encryption_len); - if (!EVP_AEAD_CTX_open(ctx.get(), out, &out_len, - offset + valid_encryption_len, nonce.data(), - nonce_len, in, valid_encryption_len, nullptr, 0)) { - fprintf(stderr, "EVP_AEAD_CTX_open failed for offset -%u.\n", - static_cast(offset)); - return false; - } - - if (out_len != sizeof(kPlaintext) || - memcmp(out, kPlaintext, out_len) != 0) { - fprintf(stderr, "EVP_AEAD_CTX_open produced bad output for offset -%u.\n", - static_cast(offset)); - return false; - } + if (out_len != sizeof(kPlaintext) || + memcmp(in, kPlaintext, out_len) != 0) { + fprintf(stderr, "EVP_AEAD_CTX_open produced bad output in-place.\n"); + return false; } return true; diff --git a/include/openssl/aead.h b/include/openssl/aead.h index d9a640c3..7895825c 100644 --- a/include/openssl/aead.h +++ b/include/openssl/aead.h @@ -226,7 +226,7 @@ OPENSSL_EXPORT void EVP_AEAD_CTX_cleanup(EVP_AEAD_CTX *ctx); * insufficient, zero will be returned. (In this case, |*out_len| is set to * zero.) * - * If |in| and |out| alias then |out| must be <= |in|. */ + * If |in| and |out| alias then |out| must be == |in|. */ OPENSSL_EXPORT int EVP_AEAD_CTX_seal(const EVP_AEAD_CTX *ctx, uint8_t *out, size_t *out_len, size_t max_out_len, const uint8_t *nonce, size_t nonce_len, @@ -251,7 +251,7 @@ OPENSSL_EXPORT int EVP_AEAD_CTX_seal(const EVP_AEAD_CTX *ctx, uint8_t *out, * insufficient, zero will be returned. (In this case, |*out_len| is set to * zero.) * - * If |in| and |out| alias then |out| must be <= |in|. */ + * If |in| and |out| alias then |out| must be == |in|. */ OPENSSL_EXPORT int EVP_AEAD_CTX_open(const EVP_AEAD_CTX *ctx, uint8_t *out, size_t *out_len, size_t max_out_len, const uint8_t *nonce, size_t nonce_len, diff --git a/include/openssl/chacha.h b/include/openssl/chacha.h index 64713c24..3d035e69 100644 --- a/include/openssl/chacha.h +++ b/include/openssl/chacha.h @@ -23,8 +23,8 @@ extern "C" { /* CRYPTO_chacha_20 encrypts |in_len| bytes from |in| with the given key and - * nonce and writes the result to |out|, which may be equal to |in|. The - * initial block counter is specified by |counter|. */ + * nonce and writes the result to |out|. If |in| and |out| alias, they must be + * equal. The initial block counter is specified by |counter|. */ OPENSSL_EXPORT void CRYPTO_chacha_20(uint8_t *out, const uint8_t *in, size_t in_len, const uint8_t key[32], const uint8_t nonce[12], uint32_t counter); diff --git a/ssl/internal.h b/ssl/internal.h index 6d96c6c4..4cf7a2e0 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -325,7 +325,7 @@ int SSL_AEAD_CTX_open(SSL_AEAD_CTX *ctx, CBS *out, uint8_t type, * writes the result to |out|. It returns one on success and zero on * error. |ctx| may be NULL to denote the null cipher. * - * If |in| and |out| alias then |out| + |explicit_nonce_len| must be <= |in| */ + * If |in| and |out| alias then |out| + |explicit_nonce_len| must be == |in|. */ int SSL_AEAD_CTX_seal(SSL_AEAD_CTX *ctx, uint8_t *out, size_t *out_len, size_t max_out, uint8_t type, uint16_t wire_version, const uint8_t seqnum[8], const uint8_t *in,