AEAD: allow _cleanup after failed _init.
This change makes it safe to call EVP_AEAD_CTX_cleanup after a failed
EVP_AEAD_CTX_init.
Change-Id: I608ed550e08d638cd7e941f5067edd3da4c850ab
Reviewed-on: https://boringssl-review.googlesource.com/4692
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/cipher/aead.c b/crypto/cipher/aead.c
index 6dad5a6..20d699d 100644
--- a/crypto/cipher/aead.c
+++ b/crypto/cipher/aead.c
@@ -35,6 +35,7 @@
ENGINE *impl) {
if (!aead->init) {
OPENSSL_PUT_ERROR(CIPHER, EVP_AEAD_CTX_init, CIPHER_R_NO_DIRECTION_SET);
+ ctx->aead = NULL;
return 0;
}
return EVP_AEAD_CTX_init_with_direction(ctx, aead, key, key_len, tag_len,
@@ -45,17 +46,27 @@
const uint8_t *key, size_t key_len,
size_t tag_len,
enum evp_aead_direction_t dir) {
- ctx->aead = aead;
if (key_len != aead->key_len) {
OPENSSL_PUT_ERROR(CIPHER, EVP_AEAD_CTX_init_with_direction,
CIPHER_R_UNSUPPORTED_KEY_SIZE);
+ ctx->aead = NULL;
return 0;
}
+
+ ctx->aead = aead;
+
+ int ok;
if (aead->init) {
- return aead->init(ctx, key, key_len, tag_len);
+ ok = aead->init(ctx, key, key_len, tag_len);
} else {
- return aead->init_with_direction(ctx, key, key_len, tag_len, dir);
+ ok = aead->init_with_direction(ctx, key, key_len, tag_len, dir);
}
+
+ if (!ok) {
+ ctx->aead = NULL;
+ }
+
+ return ok;
}
void EVP_AEAD_CTX_cleanup(EVP_AEAD_CTX *ctx) {
diff --git a/crypto/cipher/aead_test.cc b/crypto/cipher/aead_test.cc
index bdb333e..e4b75d6 100644
--- a/crypto/cipher/aead_test.cc
+++ b/crypto/cipher/aead_test.cc
@@ -183,6 +183,38 @@
return true;
}
+static int TestCleanupAfterInitFailure(const EVP_AEAD *aead) {
+ EVP_AEAD_CTX ctx;
+ uint8_t key[128];
+
+ memset(key, 0, sizeof(key));
+ const size_t key_len = EVP_AEAD_key_length(aead);
+ if (key_len > sizeof(key)) {
+ fprintf(stderr, "Key length of AEAD too long.\n");
+ return 0;
+ }
+
+ if (EVP_AEAD_CTX_init(&ctx, aead, key, key_len,
+ 9999 /* a silly tag length to trigger an error */,
+ NULL /* ENGINE */) != 0) {
+ fprintf(stderr, "A silly tag length didn't trigger an error!\n");
+ return 0;
+ }
+
+ /* Running a second, failed _init should not cause a memory leak. */
+ if (EVP_AEAD_CTX_init(&ctx, aead, key, key_len,
+ 9999 /* a silly tag length to trigger an error */,
+ NULL /* ENGINE */) != 0) {
+ fprintf(stderr, "A silly tag length didn't trigger an error!\n");
+ return 0;
+ }
+
+ /* Calling _cleanup on an |EVP_AEAD_CTX| after a failed _init should be a
+ * no-op. */
+ EVP_AEAD_CTX_cleanup(&ctx);
+ return 1;
+}
+
struct AEADName {
const char name[40];
const EVP_AEAD *(*func)(void);
@@ -236,5 +268,9 @@
}
}
+ if (!TestCleanupAfterInitFailure(aead)) {
+ return 1;
+ }
+
return FileTestMain(TestAEAD, const_cast<EVP_AEAD*>(aead), argv[2]);
}
diff --git a/crypto/cipher/e_ssl3.c b/crypto/cipher/e_ssl3.c
index e6afaf9..1031d9b 100644
--- a/crypto/cipher/e_ssl3.c
+++ b/crypto/cipher/e_ssl3.c
@@ -115,6 +115,7 @@
!EVP_DigestInit_ex(&ssl3_ctx->md_ctx, md, NULL) ||
!EVP_DigestUpdate(&ssl3_ctx->md_ctx, key, mac_key_len)) {
aead_ssl3_cleanup(ctx);
+ ctx->aead_state = NULL;
return 0;
}
EVP_CIPHER_CTX_set_padding(&ssl3_ctx->cipher_ctx, 0);
diff --git a/crypto/cipher/e_tls.c b/crypto/cipher/e_tls.c
index 9e55b08..bed02cb 100644
--- a/crypto/cipher/e_tls.c
+++ b/crypto/cipher/e_tls.c
@@ -91,6 +91,7 @@
dir == evp_aead_seal) ||
!HMAC_Init_ex(&tls_ctx->hmac_ctx, key, mac_key_len, md, NULL)) {
aead_tls_cleanup(ctx);
+ ctx->aead_state = NULL;
return 0;
}
EVP_CIPHER_CTX_set_padding(&tls_ctx->cipher_ctx, 0);
diff --git a/crypto/cipher/internal.h b/crypto/cipher/internal.h
index 1ce2e58..605b8cb 100644
--- a/crypto/cipher/internal.h
+++ b/crypto/cipher/internal.h
@@ -79,11 +79,13 @@
uint8_t overhead;
uint8_t max_tag_len;
+ /* init initialises an |evp_aead_ctx_st|. If this call returns zero then
+ * |cleanup| will not be called for that context. */
int (*init)(struct evp_aead_ctx_st *, const uint8_t *key,
size_t key_len, size_t tag_len);
int (*init_with_direction)(struct evp_aead_ctx_st *, const uint8_t *key,
- size_t key_len, size_t tag_len,
- enum evp_aead_direction_t dir);
+ size_t key_len, size_t tag_len,
+ enum evp_aead_direction_t dir);
void (*cleanup)(struct evp_aead_ctx_st *);
int (*seal)(const struct evp_aead_ctx_st *ctx, uint8_t *out,
diff --git a/include/openssl/aead.h b/include/openssl/aead.h
index 0416bbe..dc453e3 100644
--- a/include/openssl/aead.h
+++ b/include/openssl/aead.h
@@ -228,7 +228,10 @@
* Authentication tags may be truncated by passing a size as |tag_len|. A
* |tag_len| of zero indicates the default tag length and this is defined as
* EVP_AEAD_DEFAULT_TAG_LENGTH for readability.
- * Returns 1 on success. Otherwise returns 0 and pushes to the error stack. */
+ *
+ * Returns 1 on success. Otherwise returns 0 and pushes to the error stack. In
+ * the error case, you do not need to call |EVP_AEAD_CTX_cleanup|, but it's
+ * harmless to do so. */
OPENSSL_EXPORT int EVP_AEAD_CTX_init(EVP_AEAD_CTX *ctx, const EVP_AEAD *aead,
const uint8_t *key, size_t key_len,
size_t tag_len, ENGINE *impl);
@@ -240,7 +243,9 @@
EVP_AEAD_CTX *ctx, const EVP_AEAD *aead, const uint8_t *key, size_t key_len,
size_t tag_len, enum evp_aead_direction_t dir);
-/* EVP_AEAD_CTX_cleanup frees any data allocated by |ctx|. */
+/* EVP_AEAD_CTX_cleanup frees any data allocated by |ctx|. It is a no-op to
+ * call |EVP_AEAD_CTX_cleanup| on a |EVP_AEAD_CTX| that has been |memset| to
+ * all zeros. */
OPENSSL_EXPORT void EVP_AEAD_CTX_cleanup(EVP_AEAD_CTX *ctx);
/* EVP_AEAD_CTX_seal encrypts and authenticates |in_len| bytes from |in| and
