Introduce ossl_ssize_t and use it in ASN1_STRING_set.

We have a number of APIs that cannot migrate to size_t because OpenSSL
used negative numbers as some special indicator. This makes it hard to
become size_t-clean.

However, in reality, the largest buffer size is SSIZE_MAX, or, more
accurately PTRDIFF_MAX. But every platform I've ever seen make ptrdiff_t
and size_t the same size. malloc is just obligated to fail allocations
that don't fit in ssize_t. ssize_t itself is not portable (Windows
doesn't have it), but we can define ossl_ssize_t to be ptrdiff_t.
OpenSSL also has an ossl_ssize_t (though they don't use it much), so
we're also improving compatibility.

Start this out with ASN1_STRING_set. It still internally refuses to
construct a string bigger than INT_MAX; the struct can't hold this and
even if we fix the struct, no other code, inside or outside the library,
can tolerate it. But now code which passes in a size_t (including our
own) can do so without overflow.

Bug: 428, 516
Change-Id: I17aa6971733f34dfda7d971882d0f062e92340e9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54953
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

crypto/asn1/a_bitstr.c

@@ -66,7 +66,8 @@
 #include "internal.h"
 
 
-int ASN1_BIT_STRING_set(ASN1_BIT_STRING *x, const unsigned char *d, int len) {
+int ASN1_BIT_STRING_set(ASN1_BIT_STRING *x, const unsigned char *d,
+                        ossl_ssize_t len) {
   return ASN1_STRING_set(x, d, len);
 }
 

crypto/asn1/asn1_lib.c

@@ -271,19 +271,27 @@ ASN1_STRING *ASN1_STRING_dup(const ASN1_STRING *str) {
   return ret;
 }
 
-int ASN1_STRING_set(ASN1_STRING *str, const void *_data, int len) {
-  unsigned char *c;
+int ASN1_STRING_set(ASN1_STRING *str, const void *_data, ossl_ssize_t len_s) {
   const char *data = _data;
-
-  if (len < 0) {
+  size_t len;
+  if (len_s < 0) {
     if (data == NULL) {
       return 0;
-    } else {
-      len = strlen(data);
     }
+    len = strlen(data);
+  } else {
+    len = (size_t)len_s;
   }
-  if ((str->length <= len) || (str->data == NULL)) {
-    c = str->data;
+
+  // |ASN1_STRING| cannot represent strings that exceed |int|, and we must
+  // reserve space for a trailing NUL below.
+  if (len > INT_MAX || len + 1 < len) {
+    OPENSSL_PUT_ERROR(ASN1, ERR_R_OVERFLOW);
+    return 0;
+  }
+
+  if (str->length <= (int)len || str->data == NULL) {
+    unsigned char *c = str->data;
     if (c == NULL) {
       str->data = OPENSSL_malloc(len + 1);
     } else {
@@ -296,10 +304,13 @@ int ASN1_STRING_set(ASN1_STRING *str, const void *_data, int len) {
       return 0;
     }
   }
-  str->length = len;
+  str->length = (int)len;
   if (data != NULL) {
     OPENSSL_memcpy(str->data, data, len);
-    // an allowance for strings :-)
+    // Historically, OpenSSL would NUL-terminate most (but not all)
+    // |ASN1_STRING|s, in case anyone accidentally passed |str->data| into a
+    // function expecting a C string. We retain this behavior for compatibility,
+    // but code must not rely on this. See CVE-2021-3712.
     str->data[len] = '\0';
   }
   return 1;

crypto/crypto.c

@@ -14,11 +14,16 @@
 
 #include <openssl/crypto.h>
 
+#include <assert.h>
+
 #include "fipsmodule/rand/fork_detect.h"
 #include "fipsmodule/rand/internal.h"
 #include "internal.h"
 
 
+static_assert(sizeof(ossl_ssize_t) == sizeof(size_t),
+              "ossl_ssize_t should be the same size as size_t");
+
 #if !defined(OPENSSL_NO_ASM) && !defined(OPENSSL_STATIC_ARMCAP) && \
     (defined(OPENSSL_X86) || defined(OPENSSL_X86_64) || \
      defined(OPENSSL_ARM) || defined(OPENSSL_AARCH64) || \

include/openssl/asn1.h

@@ -608,7 +608,8 @@ OPENSSL_EXPORT int ASN1_STRING_cmp(const ASN1_STRING *a, const ASN1_STRING *b);
 // |data|. It returns one on success and zero on error. If |data| is NULL, it
 // updates the length and allocates the buffer as needed, but does not
 // initialize the contents.
-OPENSSL_EXPORT int ASN1_STRING_set(ASN1_STRING *str, const void *data, int len);
+OPENSSL_EXPORT int ASN1_STRING_set(ASN1_STRING *str, const void *data,
+                                   ossl_ssize_t len);
 
 // ASN1_STRING_set0 sets the contents of |str| to |len| bytes from |data|. It
 // takes ownership of |data|, which must have been allocated with
@@ -985,7 +986,8 @@ OPENSSL_EXPORT int ASN1_BIT_STRING_num_bytes(const ASN1_BIT_STRING *str,
 // TODO(davidben): Maybe it should? Wrapping a byte string in a bit string is a
 // common use case.
 OPENSSL_EXPORT int ASN1_BIT_STRING_set(ASN1_BIT_STRING *str,
-                                       const unsigned char *d, int length);
+                                       const unsigned char *d,
+                                       ossl_ssize_t length);
 
 // ASN1_BIT_STRING_set_bit sets bit |n| of |str| to one if |value| is non-zero
 // and zero if |value| is zero, resizing |str| as needed. It then truncates

include/openssl/base.h

@@ -326,6 +326,15 @@ enum ssl_verify_result_t BORINGSSL_ENUM_INT;
 #define BORINGSSL_ENUM_INT
 #endif
 
+// ossl_ssize_t is a signed type which is large enough to fit the size of any
+// valid memory allocation. We prefer using |size_t|, but sometimes we need a
+// signed type for OpenSSL API compatibility. This type can be used in such
+// cases to avoid overflow.
+//
+// Not all |size_t| values fit in |ossl_ssize_t|, but all |size_t| values that
+// are sizes of or indices into C objects, can be converted without overflow.
+typedef ptrdiff_t ossl_ssize_t;
+
 // CRYPTO_THREADID is a dummy value.
 typedef int CRYPTO_THREADID;