Merge pull request #1440 from torben-hansen/upstream-merge-2024-02-13

Upstream merge 2024 02 13

crypto/asn1/tasn_enc.c

@@ -448,14 +448,9 @@ static int asn1_set_seq_out(STACK_OF(ASN1_VALUE) *sk, unsigned char **out,
     return 1;
   }
 
-  if (sk_ASN1_VALUE_num(sk) > ((size_t)-1) / sizeof(DER_ENC)) {
-    OPENSSL_PUT_ERROR(ASN1, ERR_R_OVERFLOW);
-    return 0;
-  }
-
   int ret = 0;
   unsigned char *const buf = OPENSSL_malloc(skcontlen);
-  DER_ENC *encoded = OPENSSL_malloc(sk_ASN1_VALUE_num(sk) * sizeof(*encoded));
+  DER_ENC *encoded = OPENSSL_calloc(sk_ASN1_VALUE_num(sk), sizeof(*encoded));
   if (encoded == NULL || buf == NULL) {
     goto err;
   }

crypto/asn1/tasn_new.c

@@ -135,11 +135,10 @@ static int asn1_item_ex_combine_new(ASN1_VALUE **pval, const ASN1_ITEM *it,
         }
       }
       if (!combine) {
-        *pval = OPENSSL_malloc(it->size);
+        *pval = OPENSSL_zalloc(it->size);
         if (!*pval) {
           goto memerr;
         }
-        OPENSSL_memset(*pval, 0, it->size);
       }
       asn1_set_choice_selector(pval, -1, it);
       if (asn1_cb && !asn1_cb(ASN1_OP_NEW_POST, pval, it, NULL)) {
@@ -161,11 +160,10 @@ static int asn1_item_ex_combine_new(ASN1_VALUE **pval, const ASN1_ITEM *it,
         }
       }
       if (!combine) {
-        *pval = OPENSSL_malloc(it->size);
+        *pval = OPENSSL_zalloc(it->size);
         if (!*pval) {
           goto memerr;
         }
-        OPENSSL_memset(*pval, 0, it->size);
         asn1_refcount_set_one(pval, it);
         asn1_enc_init(pval, it);
       }

crypto/base64/base64.c

@@ -123,12 +123,7 @@ int EVP_EncodedLength(size_t *out_len, size_t len) {
 }
 
 EVP_ENCODE_CTX *EVP_ENCODE_CTX_new(void) {
-  EVP_ENCODE_CTX *ret = OPENSSL_malloc(sizeof(EVP_ENCODE_CTX));
-  if (ret == NULL) {
-    return NULL;
-  }
-  OPENSSL_memset(ret, 0, sizeof(EVP_ENCODE_CTX));
-  return ret;
+  return OPENSSL_zalloc(sizeof(EVP_ENCODE_CTX));
 }
 
 void EVP_ENCODE_CTX_free(EVP_ENCODE_CTX *ctx) {

crypto/bio/bio.c

@@ -98,12 +98,11 @@ static CRYPTO_EX_DATA_CLASS g_ex_data_class =
     CRYPTO_EX_DATA_CLASS_INIT_WITH_APP_DATA;
 
 BIO *BIO_new(const BIO_METHOD *method) {
-  BIO *ret = OPENSSL_malloc(sizeof(BIO));
+  BIO *ret = OPENSSL_zalloc(sizeof(BIO));
   if (ret == NULL) {
     return NULL;
   }
 
-  OPENSSL_memset(ret, 0, sizeof(BIO));
   ret->method = method;
   ret->shutdown = 1;
   ret->references = 1;
@@ -702,11 +701,10 @@ int BIO_get_new_index(void) {
 }
 
 BIO_METHOD *BIO_meth_new(int type, const char *name) {
-  BIO_METHOD *method = OPENSSL_malloc(sizeof(BIO_METHOD));
+  BIO_METHOD *method = OPENSSL_zalloc(sizeof(BIO_METHOD));
   if (method == NULL) {
     return NULL;
   }
-  OPENSSL_memset(method, 0, sizeof(BIO_METHOD));
   method->type = type;
   method->name = name;
   return method;

crypto/bio/connect.c

@@ -296,13 +296,10 @@ static int conn_state(BIO *bio, BIO_CONNECT *c) {
 }
 
 static BIO_CONNECT *BIO_CONNECT_new(void) {
-  BIO_CONNECT *ret = OPENSSL_malloc(sizeof(BIO_CONNECT));
-
+  BIO_CONNECT *ret = OPENSSL_zalloc(sizeof(BIO_CONNECT));
   if (ret == NULL) {
     return NULL;
   }
-  OPENSSL_memset(ret, 0, sizeof(BIO_CONNECT));
-
   ret->state = BIO_CONN_S_BEFORE;
   return ret;
 }

crypto/bio/pair.c

@@ -81,13 +81,10 @@ struct bio_bio_st {
 };
 
 static int bio_new(BIO *bio) {
-  struct bio_bio_st *b;
-
-  b = OPENSSL_malloc(sizeof *b);
+  struct bio_bio_st *b = OPENSSL_zalloc(sizeof *b);
   if (b == NULL) {
     return 0;
   }
-  OPENSSL_memset(b, 0, sizeof(struct bio_bio_st));
 
   b->size = 17 * 1024;  // enough for one TLS record (just a default)
   bio->ptr = b;

crypto/buf/buf.c

@@ -64,17 +64,7 @@
 #include "../internal.h"
 
 
-BUF_MEM *BUF_MEM_new(void) {
-  BUF_MEM *ret;
-
-  ret = OPENSSL_malloc(sizeof(BUF_MEM));
-  if (ret == NULL) {
-    return NULL;
-  }
-
-  OPENSSL_memset(ret, 0, sizeof(BUF_MEM));
-  return ret;
-}
+BUF_MEM *BUF_MEM_new(void) { return OPENSSL_zalloc(sizeof(BUF_MEM)); }
 
 void BUF_MEM_free(BUF_MEM *buf) {
   if (buf == NULL) {

crypto/bytestring/cbb.c

@@ -155,6 +155,29 @@ static struct cbb_buffer_st *cbb_get_base(CBB *cbb) {
   return &cbb->u.base;
 }
 
+static void cbb_on_error(CBB *cbb) {
+  // Due to C's lack of destructors and |CBB|'s auto-flushing API, a failing
+  // |CBB|-taking function may leave a dangling pointer to a child |CBB|. As a
+  // result, the convention is callers may not write to |CBB|s that have failed.
+  // But, as a safety measure, we lock the |CBB| into an error state. Once the
+  // error bit is set, |cbb->child| will not be read.
+  //
+  // TODO(davidben): This still isn't quite ideal. A |CBB| function *outside*
+  // this file may originate an error while the |CBB| points to a local child.
+  // In that case we don't set the error bit and are reliant on the error
+  // convention. Perhaps we allow |CBB_cleanup| on child |CBB|s and make every
+  // child's |CBB_cleanup| set the error bit if unflushed. That will be
+  // convenient for C++ callers, but very tedious for C callers. So C callers
+  // perhaps should get a |CBB_on_error| function that can be, less tediously,
+  // stuck in a |goto err| block.
+  cbb_get_base(cbb)->error = 1;
+
+  // Clearing the pointer is not strictly necessary, but GCC's dangling pointer
+  // warning does not know |cbb->child| will not be read once |error| is set
+  // above.
+  cbb->child = NULL;
+}
+
 // CBB_flush recurses and then writes out any pending length prefix. The
 // current length of the underlying base is taken to be the length of the
 // length-prefixed data.
@@ -244,7 +267,7 @@ int CBB_flush(CBB *cbb) {
   return 1;
 
 err:
-  base->error = 1;
+  cbb_on_error(cbb);
   return 0;
 }
 
@@ -420,7 +443,7 @@ static int cbb_add_u(CBB *cbb, uint64_t v, size_t len_len) {
 
   // |v| must fit in |len_len| bytes.
   if (v != 0) {
-    cbb_get_base(cbb)->error = 1;
+    cbb_on_error(cbb);
     return 0;
   }
 
@@ -479,7 +502,7 @@ int CBB_add_asn1_uint64(CBB *cbb, uint64_t value) {
 int CBB_add_asn1_uint64_with_tag(CBB *cbb, uint64_t value, CBS_ASN1_TAG tag) {
   CBB child;
   if (!CBB_add_asn1(cbb, &child, tag)) {
-    return 0;
+    goto err;
   }
 
   int started = 0;
@@ -493,21 +516,25 @@ int CBB_add_asn1_uint64_with_tag(CBB *cbb, uint64_t value, CBS_ASN1_TAG tag) {
       // If the high bit is set, add a padding byte to make it
       // unsigned.
       if ((byte & 0x80) && !CBB_add_u8(&child, 0)) {
-        return 0;
+        goto err;
       }
       started = 1;
     }
     if (!CBB_add_u8(&child, byte)) {
-      return 0;
+      goto err;
     }
   }
 
   // 0 is encoded as a single 0, not the empty string.
   if (!started && !CBB_add_u8(&child, 0)) {
-    return 0;
+    goto err;
   }
 
   return CBB_flush(cbb);
+
+err:
+  cbb_on_error(cbb);
+  return 0;
 }
 
 int CBB_add_asn1_int64(CBB *cbb, int64_t value) {
@@ -535,25 +562,30 @@ int CBB_add_asn1_int64_with_tag(CBB *cbb, int64_t value, CBS_ASN1_TAG tag) {
 #endif
   CBB child;
   if (!CBB_add_asn1(cbb, &child, tag)) {
-    return 0;
+    goto err;
   }
 #ifdef OPENSSL_BIG_ENDIAN
   for (int i = start; i <= 7; i++) {
 #else
   for (int i = start; i >= 0; i--) {
 #endif
     if (!CBB_add_u8(&child, bytes[i])) {
-      return 0;
+      goto err;
     }
   }
   return CBB_flush(cbb);
+
+err:
+  cbb_on_error(cbb);
+  return 0;
 }
 
 int CBB_add_asn1_octet_string(CBB *cbb, const uint8_t *data, size_t data_len) {
   CBB child;
   if (!CBB_add_asn1(cbb, &child, CBS_ASN1_OCTETSTRING) ||
       !CBB_add_bytes(&child, data, data_len) ||
       !CBB_flush(cbb)) {
+    cbb_on_error(cbb);
     return 0;
   }
 
@@ -565,6 +597,7 @@ int CBB_add_asn1_bool(CBB *cbb, int value) {
   if (!CBB_add_asn1(cbb, &child, CBS_ASN1_BOOLEAN) ||
       !CBB_add_u8(&child, value != 0 ? 0xff : 0) ||
       !CBB_flush(cbb)) {
+    cbb_on_error(cbb);
     return 0;
   }
 
@@ -659,16 +692,13 @@ int CBB_flush_asn1_set_of(CBB *cbb) {
   if (num_children < 2) {
     return 1;  // Nothing to do. This is the common case for X.509.
   }
-  if (num_children > ((size_t)-1) / sizeof(CBS)) {
-    return 0;  // Overflow.
-  }
 
   // Parse out the children and sort. We alias them into a copy of so they
   // remain valid as we rewrite |cbb|.
   int ret = 0;
   size_t buf_len = CBB_len(cbb);
   uint8_t *buf = OPENSSL_memdup(CBB_data(cbb), buf_len);
-  CBS *children = OPENSSL_malloc(num_children * sizeof(CBS));
+  CBS *children = OPENSSL_calloc(num_children, sizeof(CBS));
   if (buf == NULL || children == NULL) {
     goto err;
   }

crypto/chacha/asm/chacha-armv4.pl

@@ -213,7 +213,7 @@ sub ROUND {
 .LChaCha20_ctr32:
 	ldr	r12,[sp,#0]		@ pull pointer to counter and nonce
 	stmdb	sp!,{r0-r2,r4-r11,lr}
-#if __ARM_ARCH__<7 && !defined(__thumb2__)
+#if __ARM_ARCH<7 && !defined(__thumb2__)
 	sub	r14,pc,#16		@ ChaCha20_ctr32
 #else
 	adr	r14,.LChaCha20_ctr32
@@ -295,8 +295,8 @@ sub ROUND {
 	ldr	@t[0],[sp,#4*(0)]	@ load key material
 	ldr	@t[1],[sp,#4*(1)]
 
-#if __ARM_ARCH__>=6 || !defined(__ARMEB__)
-# if __ARM_ARCH__<7
+#if __ARM_ARCH>=6 || !defined(__ARMEB__)
+# if __ARM_ARCH<7
 	orr	@t[2],r12,r14
 	tst	@t[2],#3		@ are input and output aligned?
 	ldr	@t[2],[sp,#4*(2)]
@@ -322,7 +322,7 @@ sub ROUND {
 # endif
 	ldrhs	@t[2],[r12,#-8]
 	ldrhs	@t[3],[r12,#-4]
-# if __ARM_ARCH__>=6 && defined(__ARMEB__)
+# if __ARM_ARCH>=6 && defined(__ARMEB__)
 	rev	@x[0],@x[0]
 	rev	@x[1],@x[1]
 	rev	@x[2],@x[2]
@@ -359,7 +359,7 @@ sub ROUND {
 # endif
 	ldrhs	@t[2],[r12,#-8]
 	ldrhs	@t[3],[r12,#-4]
-# if __ARM_ARCH__>=6 && defined(__ARMEB__)
+# if __ARM_ARCH>=6 && defined(__ARMEB__)
 	rev	@x[4],@x[4]
 	rev	@x[5],@x[5]
 	rev	@x[6],@x[6]
@@ -404,7 +404,7 @@ sub ROUND {
 # endif
 	ldrhs	@t[2],[r12,#-8]
 	ldrhs	@t[3],[r12,#-4]
-# if __ARM_ARCH__>=6 && defined(__ARMEB__)
+# if __ARM_ARCH>=6 && defined(__ARMEB__)
 	rev	@x[0],@x[0]
 	rev	@x[1],@x[1]
 	rev	@x[2],@x[2]
@@ -446,7 +446,7 @@ sub ROUND {
 # endif
 	ldrhs	@t[2],[r12,#-8]
 	ldrhs	@t[3],[r12,#-4]
-# if __ARM_ARCH__>=6 && defined(__ARMEB__)
+# if __ARM_ARCH>=6 && defined(__ARMEB__)
 	rev	@x[4],@x[4]
 	rev	@x[5],@x[5]
 	rev	@x[6],@x[6]
@@ -477,15 +477,15 @@ sub ROUND {
 	bhi	.Loop_outer
 
 	beq	.Ldone
-# if __ARM_ARCH__<7
+# if __ARM_ARCH<7
 	b	.Ltail
 
 .align	4
 .Lunaligned:				@ unaligned endian-neutral path
 	cmp	@t[3],#64		@ restore flags
 # endif
 #endif
-#if __ARM_ARCH__<7
+#if __ARM_ARCH<7
 	ldr	@t[3],[sp,#4*(3)]
 ___
 for ($i=0;$i<16;$i+=4) {

crypto/compiler_test.cc

@@ -23,6 +23,26 @@
 #include "test/test_util.h"
 
 
+// C and C++ have two forms of unspecified behavior: undefined behavior and
+// implementation-defined behavior.
+//
+// Programs that exhibit undefined behavior are invalid. Compilers are
+// permitted to, and often do, arbitrarily miscompile them. BoringSSL thus aims
+// to avoid undefined behavior.
+//
+// Implementation-defined behavior is left up to the compiler to define (or
+// leave undefined). These are often platform-specific details, such as how big
+// |int| is or how |uintN_t| is implemented. Programs that depend on
+// implementation-defined behavior are not necessarily invalid, merely less
+// portable. A compiler that provides some implementation-defined behavior is
+// not permitted to miscompile code that depends on it.
+//
+// C allows a much wider range of platform behaviors than would be practical
+// for us to support, so we make some assumptions on implementation-defined
+// behavior. Platforms that violate those assumptions are not supported. This
+// file aims to document and test these assumptions, so that platforms outside
+// our scope are flagged.
+
 template <typename T>
 static void CheckRepresentation(T value) {
   SCOPED_TRACE(value);

crypto/conf/conf.c

@@ -118,14 +118,7 @@ CONF *NCONF_new(void *method) {
   return conf;
 }
 
-CONF_VALUE *CONF_VALUE_new(void) {
-  CONF_VALUE *v = OPENSSL_malloc(sizeof(CONF_VALUE));
-  if (!v) {
-    return NULL;
-  }
-  OPENSSL_memset(v, 0, sizeof(CONF_VALUE));
-  return v;
-}
+CONF_VALUE *CONF_VALUE_new(void) { return OPENSSL_zalloc(sizeof(CONF_VALUE)); }
 
 static void value_free_contents(CONF_VALUE *value) {
   OPENSSL_free(value->section);