Add some missing error failure checks.

Found while diagnosing some crashes and hangs in the malloc tests. This (and
the follow-up) get us further but does not quite let the malloc tests pass
quietly, even without valgrind. DTLS silently ignores some malloc failures
(confusion with silently dropping bad packets) which then translate to hangs.

Change-Id: Ief06a671e0973d09d2883432b89a86259e346653
Reviewed-on: https://boringssl-review.googlesource.com/3482
Reviewed-by: Adam Langley <agl@google.com>

12 files changed, 69 insertions, 37 deletions

diff --git a/crypto/bn/div.c b/crypto/bn/div.c
index 2bf86de7..8b0152e8 100644
--- a/crypto/bn/div.c
+++ b/crypto/bn/div.c
@@ -362,7 +362,9 @@ int BN_div(BIGNUM *dv, BIGNUM *rm, const BIGNUM *num, const BIGNUM *divisor,
      * BN_rshift() will overwrite it.
      */
     int neg = num->neg;
-    BN_rshift(rm, snum, norm_shift);
+    if (!BN_rshift(rm, snum, norm_shift)) {
+      goto err;
+    }
     if (!BN_is_zero(rm)) {
       rm->neg = neg;
     }
diff --git a/crypto/bn/gcd.c b/crypto/bn/gcd.c
index 789a0942..3132c29e 100644
--- a/crypto/bn/gcd.c
+++ b/crypto/bn/gcd.c
@@ -258,12 +258,8 @@ BIGNUM *BN_mod_inverse(BIGNUM *out, const BIGNUM *a, const BIGNUM *n,
     goto err;
   }
 
-  BN_one(X);
   BN_zero(Y);
-  if (BN_copy(B, a) == NULL) {
-    goto err;
-  }
-  if (BN_copy(A, n) == NULL) {
+  if (!BN_one(X) || BN_copy(B, a) == NULL || BN_copy(A, n) == NULL) {
     goto err;
   }
   A->neg = 0;
@@ -570,12 +566,8 @@ static BIGNUM *BN_mod_inverse_no_branch(BIGNUM *out, const BIGNUM *a,
     goto err;
   }
 
-  BN_one(X);
   BN_zero(Y);
-  if (BN_copy(B, a) == NULL) {
-    goto err;
-  }
-  if (BN_copy(A, n) == NULL) {
+  if (!BN_one(X) || BN_copy(B, a) == NULL || BN_copy(A, n) == NULL) {
     goto err;
   }
   A->neg = 0;
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index cabc56db..8a9d686f 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1043,10 +1043,24 @@ OPENSSL_EXPORT int SSL_select_next_proto(uint8_t **out, uint8_t *outlen,
 #define OPENSSL_NPN_NEGOTIATED 1
 #define OPENSSL_NPN_NO_OVERLAP 2
 
+/* SSL_CTX_set_alpn_protos sets the ALPN protocol list on |ctx| to |protos|.
+ * |protos| must be in wire-format (i.e. a series of non-empty, 8-bit
+ * length-prefixed strings). It returns zero on success and one on failure.
+ *
+ * WARNING: this function is dangerous because it breaks the usual return value
+ * convention. */
 OPENSSL_EXPORT int SSL_CTX_set_alpn_protos(SSL_CTX *ctx, const uint8_t *protos,
                                            unsigned protos_len);
+
+/* SSL_set_alpn_protos sets the ALPN protocol list on |ssl| to |protos|.
+ * |protos| must be in wire-format (i.e. a series of non-empty, 8-bit
+ * length-prefixed strings). It returns zero on success and one on failure.
+ *
+ * WARNING: this function is dangerous because it breaks the usual return value
+ * convention. */
 OPENSSL_EXPORT int SSL_set_alpn_protos(SSL *ssl, const uint8_t *protos,
                                        unsigned protos_len);
+
 OPENSSL_EXPORT void SSL_CTX_set_alpn_select_cb(
     SSL_CTX *ctx, int (*cb)(SSL *ssl, const uint8_t **out, uint8_t *outlen,
                             const uint8_t *in, unsigned int inlen, void *arg),
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index a34d221e..ddfc1fe7 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -296,7 +296,7 @@ int ssl3_send_change_cipher_spec(SSL *s, int a, int b) {
   return ssl3_do_write(s, SSL3_RT_CHANGE_CIPHER_SPEC);
 }
 
-unsigned long ssl3_output_cert_chain(SSL *s, CERT_PKEY *cpk) {
+int ssl3_output_cert_chain(SSL *s, CERT_PKEY *cpk) {
   uint8_t *p;
   unsigned long l = 3 + SSL_HM_HEADER_LENGTH(s);
 
@@ -309,7 +309,7 @@ unsigned long ssl3_output_cert_chain(SSL *s, CERT_PKEY *cpk) {
   l2n3(l, p);
   l += 3;
   ssl_set_handshake_header(s, SSL3_MT_CERTIFICATE, l);
-  return l + SSL_HM_HEADER_LENGTH(s);
+  return 1;
 }
 
 /* Obtain handshake message of message type |msg_type| (any if |msg_type| == -1),
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index c51ba6d5..c6c2b429 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -1100,6 +1100,9 @@ int ssl3_get_server_key_exchange(SSL *s) {
        * initialized |sess_cert|. */
       if (s->session->sess_cert == NULL) {
         s->session->sess_cert = ssl_sess_cert_new();
+        if (s->session->sess_cert == NULL) {
+          return -1;
+        }
       }
 
       /* TODO(davidben): This should be reset in one place with the rest of the
@@ -1128,6 +1131,9 @@ int ssl3_get_server_key_exchange(SSL *s) {
     }
   } else {
     s->session->sess_cert = ssl_sess_cert_new();
+    if (s->session->sess_cert == NULL) {
+      return -1;
+    }
   }
 
   alg_k = s->s3->tmp.new_cipher->algorithm_mkey;
@@ -2182,8 +2188,10 @@ int ssl3_send_client_certificate(SSL *s) {
   }
 
   if (s->state == SSL3_ST_CW_CERT_C) {
-    s->state = SSL3_ST_CW_CERT_D;
-    ssl3_output_cert_chain(s, (s->s3->tmp.cert_req == 2) ? NULL : s->cert->key);
+    CERT_PKEY *cert_pkey = (s->s3->tmp.cert_req == 2) ? NULL : s->cert->key;
+    if (!ssl3_output_cert_chain(s, cert_pkey)) {
+      return -1;
+    }
   }
 
   /* SSL3_ST_CW_CERT_D */
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 3f89558b..c020b3a5 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -2163,6 +2163,9 @@ int ssl3_get_cert_verify(SSL *s) {
 
   /* Filter out unsupported certificate types. */
   pkey = X509_get_pubkey(peer);
+  if (pkey == NULL) {
+    goto err;
+  }
   if (!(X509_certificate_type(peer, pkey) & EVP_PKT_SIGN) ||
       (pkey->type != EVP_PKEY_RSA && pkey->type != EVP_PKEY_EC)) {
     al = SSL_AD_UNSUPPORTED_CERTIFICATE;
@@ -2413,7 +2416,9 @@ int ssl3_send_server_certificate(SSL *s) {
       return 0;
     }
 
-    ssl3_output_cert_chain(s, cpk);
+    if (!ssl3_output_cert_chain(s, cpk)) {
+      return 0;
+    }
     s->state = SSL3_ST_SW_CERT_B;
   }
 
@@ -2683,6 +2688,9 @@ int ssl3_get_channel_id(SSL *s) {
   BN_init(&y);
   sig.r = BN_new();
   sig.s = BN_new();
+  if (sig.r == NULL || sig.s == NULL) {
+    goto err;
+  }
 
   p = CBS_data(&extension);
   if (BN_bin2bn(p + 0, 32, &x) == NULL ||
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 348e2a5d..de573303 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1677,11 +1677,6 @@ void SSL_CTX_set_next_proto_select_cb(
   ctx->next_proto_select_cb_arg = arg;
 }
 
-/* SSL_CTX_set_alpn_protos sets the ALPN protocol list on |ctx| to |protos|.
- * |protos| must be in wire-format (i.e. a series of non-empty, 8-bit
- * length-prefixed strings).
- *
- * Returns 0 on success. */
 int SSL_CTX_set_alpn_protos(SSL_CTX *ctx, const uint8_t *protos,
                             unsigned protos_len) {
   if (ctx->alpn_client_proto_list) {
@@ -1697,11 +1692,6 @@ int SSL_CTX_set_alpn_protos(SSL_CTX *ctx, const uint8_t *protos,
   return 0;
 }
 
-/* SSL_set_alpn_protos sets the ALPN protocol list on |ssl| to |protos|.
- * |protos| must be in wire-format (i.e. a series of non-empty, 8-bit
- * length-prefixed strings).
- *
- * Returns 0 on success. */
 int SSL_set_alpn_protos(SSL *ssl, const uint8_t *protos, unsigned protos_len) {
   if (ssl->alpn_client_proto_list) {
     OPENSSL_free(ssl->alpn_client_proto_list);
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index d63ddda5..c7abe79b 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -758,7 +758,7 @@ int ssl3_final_finish_mac(SSL *s, const char *sender, int slen, uint8_t *p);
 int ssl3_cert_verify_mac(SSL *s, int md_nid, uint8_t *p);
 void ssl3_finish_mac(SSL *s, const uint8_t *buf, int len);
 void ssl3_free_digest_list(SSL *s);
-unsigned long ssl3_output_cert_chain(SSL *s, CERT_PKEY *cpk);
+int ssl3_output_cert_chain(SSL *s, CERT_PKEY *cpk);
 const SSL_CIPHER *ssl3_choose_cipher(
     SSL *ssl, STACK_OF(SSL_CIPHER) * clnt,
     struct ssl_cipher_preference_list_st *srvr);
diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c
index fd0223d2..a3d8925e 100644
--- a/ssl/t1_enc.c
+++ b/ssl/t1_enc.c
@@ -737,7 +737,10 @@ int tls1_cert_verify_mac(SSL *s, int md_nid, uint8_t *out) {
   }
 
   EVP_MD_CTX_init(&ctx);
-  EVP_MD_CTX_copy_ex(&ctx, d);
+  if (!EVP_MD_CTX_copy_ex(&ctx, d)) {
+    EVP_MD_CTX_cleanup(&ctx);
+    return 0;
+  }
   EVP_DigestFinal_ex(&ctx, out, &ret);
   EVP_MD_CTX_cleanup(&ctx);
 
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 19e52a40..967dad75 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2584,7 +2584,10 @@ int tls1_channel_id_hash(EVP_MD_CTX *md, SSL *s) {
     if (s->s3->handshake_dgst[i] == NULL) {
       continue;
     }
-    EVP_MD_CTX_copy_ex(&ctx, s->s3->handshake_dgst[i]);
+    if (!EVP_MD_CTX_copy_ex(&ctx, s->s3->handshake_dgst[i])) {
+      EVP_MD_CTX_cleanup(&ctx);
+      return 0;
+    }
     EVP_DigestFinal_ex(&ctx, temp_digest, &temp_digest_len);
     EVP_DigestUpdate(md, temp_digest, temp_digest_len);
   }
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 90d142a1..e5520651 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -363,10 +363,14 @@ static int RetryAsync(SSL *ssl, int ret, BIO *async,
     case SSL_ERROR_WANT_WRITE:
       AsyncBioAllowWrite(async, 1);
       return 1;
-    case SSL_ERROR_WANT_CHANNEL_ID_LOOKUP:
-      GetTestState(ssl)->channel_id =
-          LoadPrivateKey(GetConfigPtr(ssl)->send_channel_id);
+    case SSL_ERROR_WANT_CHANNEL_ID_LOOKUP: {
+      ScopedEVP_PKEY pkey = LoadPrivateKey(GetConfigPtr(ssl)->send_channel_id);
+      if (!pkey) {
+        return 0;
+      }
+      GetTestState(ssl)->channel_id = std::move(pkey);
       return 1;
+    }
     case SSL_ERROR_WANT_X509_LOOKUP:
       GetTestState(ssl)->cert_ready = true;
       return 1;
@@ -455,12 +459,17 @@ static int DoExchange(ScopedSSL_SESSION *out_session, SSL_CTX *ssl_ctx,
       }
     }
   }
-  if (!config->host_name.empty()) {
-    SSL_set_tlsext_host_name(ssl.get(), config->host_name.c_str());
+  if (!config->host_name.empty() &&
+      !SSL_set_tlsext_host_name(ssl.get(), config->host_name.c_str())) {
+    BIO_print_errors_fp(stdout);
+    return 1;
   }
-  if (!config->advertise_alpn.empty()) {
-    SSL_set_alpn_protos(ssl.get(), (const uint8_t *)config->advertise_alpn.data(),
-                        config->advertise_alpn.size());
+  if (!config->advertise_alpn.empty() &&
+      SSL_set_alpn_protos(ssl.get(),
+                          (const uint8_t *)config->advertise_alpn.data(),
+                          config->advertise_alpn.size()) != 0) {
+    BIO_print_errors_fp(stdout);
+    return 1;
   }
   if (!config->psk.empty()) {
     SSL_set_psk_client_callback(ssl.get(), PskClientCallback);
diff --git a/ssl/test/packeted_bio.cc b/ssl/test/packeted_bio.cc
index e0039858..8f4f3de8 100644
--- a/ssl/test/packeted_bio.cc
+++ b/ssl/test/packeted_bio.cc
@@ -186,6 +186,9 @@ const BIO_METHOD g_packeted_bio_method = {
 
 ScopedBIO PacketedBioCreate(OPENSSL_timeval *out_timeout) {
   ScopedBIO bio(BIO_new(&g_packeted_bio_method));
+  if (!bio) {
+    return nullptr;
+  }
   bio->ptr = out_timeout;
   return bio;
 }