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