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>
diff --git a/crypto/asn1/a_bitstr.c b/crypto/asn1/a_bitstr.c
index c67efeb..3d60d3b 100644
--- a/crypto/asn1/a_bitstr.c
+++ b/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);
}
diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c
index 98061db..a0e8f48 100644
--- a/crypto/asn1/asn1_lib.c
+++ b/crypto/asn1/asn1_lib.c
@@ -271,19 +271,27 @@
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 @@
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;
diff --git a/crypto/crypto.c b/crypto/crypto.c
index af7e560..12cbb888 100644
--- a/crypto/crypto.c
+++ b/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) || \
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 693b38e..c7e294b 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -608,7 +608,8 @@
// |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 @@
// 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
diff --git a/include/openssl/base.h b/include/openssl/base.h
index 7afcc4f..1e61e98 100644
--- a/include/openssl/base.h
+++ b/include/openssl/base.h
@@ -326,6 +326,15 @@
#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;
