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 <agl@google.com>
diff --git a/crypto/chacha/chacha.c b/crypto/chacha/chacha.c index afe1b2a..1562089 100644 --- a/crypto/chacha/chacha.c +++ b/crypto/chacha/chacha.c
@@ -16,10 +16,13 @@ #include <openssl/chacha.h> +#include <assert.h> #include <string.h> #include <openssl/cpu.h> +#include "../internal.h" + #define U8TO32_LITTLE(p) \ (((uint32_t)((p)[0])) | ((uint32_t)((p)[1]) << 8) | \ @@ -36,8 +39,9 @@ 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 @@ 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 f364f98..0a5972f 100644 --- a/crypto/chacha/chacha_test.cc +++ b/crypto/chacha/chacha_test.cc
@@ -218,25 +218,16 @@ std::unique_ptr<uint8_t[]> 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<unsigned>(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<unsigned>(len), static_cast<unsigned>(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 b1db83d..57eecc1 100644 --- a/crypto/cipher/aead.c +++ b/crypto/cipher/aead.c
@@ -20,6 +20,7 @@ #include <openssl/err.h> #include "internal.h" +#include "../internal.h" size_t EVP_AEAD_key_length(const EVP_AEAD *aead) { return aead->key_len; } @@ -80,21 +81,15 @@ 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 @@ 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 @@ 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 f21291e..8bad93f 100644 --- a/crypto/cipher/aead_test.cc +++ b/crypto/cipher/aead_test.cc
@@ -225,84 +225,65 @@ 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<uint8_t> buffer(2 + valid_encryption_len); + uint8_t *in = buffer.data() + 1; + uint8_t *out1 = buffer.data(); + uint8_t *out2 = buffer.data() + 2; - std::vector<uint8_t> 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<unsigned>(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<unsigned>(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<uint8_t> 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<unsigned>(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<unsigned>(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<unsigned>(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<unsigned>(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 d9a640c..7895825 100644 --- a/include/openssl/aead.h +++ b/include/openssl/aead.h
@@ -226,7 +226,7 @@ * 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 @@ * 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 64713c2..3d035e6 100644 --- a/include/openssl/chacha.h +++ b/include/openssl/chacha.h
@@ -23,8 +23,8 @@ /* 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 6d96c6c..4cf7a2e 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -325,7 +325,7 @@ * 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,