Don't use object reuse in X509_parse_from_buffer.
Instead, move the CRYPTO_BUFFER into ASN1_ENCODING (due to padding,
upgrading the two bitfields do a pointer doesn't increase memory usage),
and instead thread a CRYPTO_BUFFER parameter through tasn_dec.c.
Later, I want to reimplement the X509 and X509_CINF parsers with CBS/CBB
directly (https://crbug.com/boringssl/547), but that will be easier once
the whole crypto/asn1 machinery is rewritten with CBS/CBB
(https://crbug.com/boringssl/548). That, in turn, will be easier with
object reuse gone. But to get rid of object reuse, I need to remove the
one place in the library where we ourselves use it.
Bug: 550
Change-Id: Ia4df3da9280f808b124ac1f4ad58745dfe0f49e2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56646
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h
index 49286ea..a60a037 100644
--- a/crypto/asn1/internal.h
+++ b/crypto/asn1/internal.h
@@ -122,20 +122,16 @@
ASN1_OBJECT *ASN1_OBJECT_new(void);
-// ASN1_ENCODING structure: this is used to save the received
-// encoding of an ASN1 type. This is useful to get round
-// problems with invalid encodings which can break signatures.
+// ASN1_ENCODING is used to save the received encoding of an ASN.1 type. This
+// avoids problems with invalid encodings that break signatures.
typedef struct ASN1_ENCODING_st {
- unsigned char *enc; // DER encoding
- long len; // Length of encoding, or zero if not present.
- // alias_only is zero if |enc| owns the buffer that it points to
- // (although |enc| may still be NULL). If one, |enc| points into a
- // buffer that is owned elsewhere.
- unsigned alias_only : 1;
- // alias_only_on_next_parse is one iff the next parsing operation
- // should avoid taking a copy of the input and rather set
- // |alias_only|.
- unsigned alias_only_on_next_parse : 1;
+ // enc is the saved DER encoding. Its ownership is determined by |buf|.
+ uint8_t *enc;
+ // len is the length of |enc|. If zero, there is no saved encoding.
+ size_t len;
+ // buf, if non-NULL, is the |CRYPTO_BUFFER| that |enc| points into. If NULL,
+ // |enc| must be released with |OPENSSL_free|.
+ CRYPTO_BUFFER *buf;
} ASN1_ENCODING;
OPENSSL_EXPORT int asn1_utctime_to_tm(struct tm *tm, const ASN1_UTCTIME *d,
@@ -147,9 +143,18 @@
void ASN1_item_ex_free(ASN1_VALUE **pval, const ASN1_ITEM *it);
void ASN1_template_free(ASN1_VALUE **pval, const ASN1_TEMPLATE *tt);
+
+// ASN1_item_ex_d2i parses |len| bytes from |*in| as a structure of type |it|
+// and writes the result to |*pval|. If |tag| is non-negative, |it| is
+// implicitly tagged with the tag specified by |tag| and |aclass|. If |opt| is
+// non-zero, the value is optional. If |buf| is non-NULL, |*in| must point into
+// |buf|.
+//
+// This function returns one and advances |*in| if an object was successfully
+// parsed, -1 if an optional value was successfully skipped, and zero on error.
int ASN1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in, long len,
const ASN1_ITEM *it, int tag, int aclass, char opt,
- ASN1_TLC *ctx);
+ CRYPTO_BUFFER *buf);
// ASN1_item_ex_i2d encodes |*pval| as a value of type |it| to |out| under the
// i2d output convention. It returns a non-zero length on success and -1 on
@@ -191,8 +196,11 @@
int asn1_enc_restore(int *len, unsigned char **out, ASN1_VALUE **pval,
const ASN1_ITEM *it);
-int asn1_enc_save(ASN1_VALUE **pval, const unsigned char *in, int inlen,
- const ASN1_ITEM *it);
+// asn1_enc_save saves |inlen| bytes from |in| as |*pval|'s saved encoding. It
+// returns one on success and zero on error. If |buf| is non-NULL, |in| must
+// point into |buf|.
+int asn1_enc_save(ASN1_VALUE **pval, const uint8_t *in, size_t inlen,
+ const ASN1_ITEM *it, CRYPTO_BUFFER *buf);
// asn1_encoding_clear clears the cached encoding in |enc|.
void asn1_encoding_clear(ASN1_ENCODING *enc);
diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c
index 0fb7344..bdde8e3 100644
--- a/crypto/asn1/tasn_dec.c
+++ b/crypto/asn1/tasn_dec.c
@@ -59,6 +59,7 @@
#include <openssl/bytestring.h>
#include <openssl/err.h>
#include <openssl/mem.h>
+#include <openssl/pool.h>
#include <limits.h>
#include <string.h>
@@ -79,10 +80,10 @@
static int asn1_template_ex_d2i(ASN1_VALUE **pval, const unsigned char **in,
long len, const ASN1_TEMPLATE *tt, char opt,
- int depth);
+ CRYPTO_BUFFER *buf, int depth);
static int asn1_template_noexp_d2i(ASN1_VALUE **val, const unsigned char **in,
long len, const ASN1_TEMPLATE *tt, char opt,
- int depth);
+ CRYPTO_BUFFER *buf, int depth);
static int asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len,
int utype, const ASN1_ITEM *it);
static int asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in,
@@ -90,7 +91,7 @@
int aclass, char opt);
static int asn1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in,
long len, const ASN1_ITEM *it, int tag, int aclass,
- char opt, int depth);
+ char opt, CRYPTO_BUFFER *buf, int depth);
// Table to convert tags to bit values, used for MSTRING type
static const unsigned long tag2bit[31] = {
@@ -148,7 +149,8 @@
pval = &ptmpval;
}
- if (asn1_item_ex_d2i(pval, in, len, it, -1, 0, 0, 0) > 0) {
+ if (asn1_item_ex_d2i(pval, in, len, it, /*tag=*/-1, /*aclass=*/0, /*opt=*/0,
+ /*buf=*/NULL, /*depth=*/0) > 0) {
return *pval;
}
return NULL;
@@ -159,7 +161,7 @@
static int asn1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in,
long len, const ASN1_ITEM *it, int tag, int aclass,
- char opt, int depth) {
+ char opt, CRYPTO_BUFFER *buf, int depth) {
const ASN1_TEMPLATE *tt, *errtt = NULL;
const unsigned char *p = NULL, *q;
unsigned char oclass;
@@ -172,6 +174,11 @@
return 0;
}
+ if (buf != NULL) {
+ assert(CRYPTO_BUFFER_data(buf) <= *in &&
+ *in + len <= CRYPTO_BUFFER_data(buf) + CRYPTO_BUFFER_len(buf));
+ }
+
// Bound |len| to comfortably fit in an int. Lengths in this module often
// switch between int and long without overflow checks.
if (len > INT_MAX / 2) {
@@ -194,7 +201,8 @@
OPENSSL_PUT_ERROR(ASN1, ASN1_R_ILLEGAL_OPTIONS_ON_ITEM_TEMPLATE);
goto err;
}
- return asn1_template_ex_d2i(pval, in, len, it->templates, opt, depth);
+ return asn1_template_ex_d2i(pval, in, len, it->templates, opt, buf,
+ depth);
}
return asn1_d2i_ex_primitive(pval, in, len, it, tag, aclass, opt);
break;
@@ -277,7 +285,7 @@
for (i = 0, tt = it->templates; i < it->tcount; i++, tt++) {
pchptr = asn1_get_field_ptr(pval, tt);
// We mark field as OPTIONAL so its absence can be recognised.
- ret = asn1_template_ex_d2i(pchptr, &p, len, tt, 1, depth);
+ ret = asn1_template_ex_d2i(pchptr, &p, len, tt, 1, buf, depth);
// If field not present, try the next one
if (ret == -1) {
continue;
@@ -383,7 +391,7 @@
}
// attempt to read in field, allowing each to be OPTIONAL
- ret = asn1_template_ex_d2i(pseqval, &p, len, seqtt, isopt, depth);
+ ret = asn1_template_ex_d2i(pseqval, &p, len, seqtt, isopt, buf, depth);
if (!ret) {
errtt = seqtt;
goto err;
@@ -422,7 +430,7 @@
}
}
// Save encoding
- if (!asn1_enc_save(pval, *in, p - *in, it)) {
+ if (!asn1_enc_save(pval, *in, p - *in, it, buf)) {
goto auxerr;
}
if (asn1_cb && !asn1_cb(ASN1_OP_D2I_POST, pval, it, NULL)) {
@@ -449,8 +457,9 @@
int ASN1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in, long len,
const ASN1_ITEM *it, int tag, int aclass, char opt,
- ASN1_TLC *ctx) {
- return asn1_item_ex_d2i(pval, in, len, it, tag, aclass, opt, 0);
+ CRYPTO_BUFFER *buf) {
+ return asn1_item_ex_d2i(pval, in, len, it, tag, aclass, opt, buf,
+ /*depth=*/0);
}
// Templates are handled with two separate functions. One handles any
@@ -458,7 +467,7 @@
static int asn1_template_ex_d2i(ASN1_VALUE **val, const unsigned char **in,
long inlen, const ASN1_TEMPLATE *tt, char opt,
- int depth) {
+ CRYPTO_BUFFER *buf, int depth) {
int aclass;
int ret;
long len;
@@ -490,7 +499,7 @@
return 0;
}
// We've found the field so it can't be OPTIONAL now
- ret = asn1_template_noexp_d2i(val, &p, len, tt, 0, depth);
+ ret = asn1_template_noexp_d2i(val, &p, len, tt, /*opt=*/0, buf, depth);
if (!ret) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
return 0;
@@ -503,7 +512,7 @@
goto err;
}
} else {
- return asn1_template_noexp_d2i(val, in, inlen, tt, opt, depth);
+ return asn1_template_noexp_d2i(val, in, inlen, tt, opt, buf, depth);
}
*in = p;
@@ -516,7 +525,7 @@
static int asn1_template_noexp_d2i(ASN1_VALUE **val, const unsigned char **in,
long len, const ASN1_TEMPLATE *tt, char opt,
- int depth) {
+ CRYPTO_BUFFER *buf, int depth) {
int aclass;
int ret;
const unsigned char *p;
@@ -574,8 +583,8 @@
ASN1_VALUE *skfield;
const unsigned char *q = p;
skfield = NULL;
- if (!asn1_item_ex_d2i(&skfield, &p, len, ASN1_ITEM_ptr(tt->item), -1, 0,
- 0, depth)) {
+ if (!asn1_item_ex_d2i(&skfield, &p, len, ASN1_ITEM_ptr(tt->item),
+ /*tag=*/-1, /*aclass=*/0, /*opt=*/0, buf, depth)) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
goto err;
}
@@ -589,7 +598,7 @@
} else if (flags & ASN1_TFLG_IMPTAG) {
// IMPLICIT tagging
ret = asn1_item_ex_d2i(val, &p, len, ASN1_ITEM_ptr(tt->item), tt->tag,
- aclass, opt, depth);
+ aclass, opt, buf, depth);
if (!ret) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
goto err;
@@ -598,8 +607,8 @@
}
} else {
// Nothing special
- ret = asn1_item_ex_d2i(val, &p, len, ASN1_ITEM_ptr(tt->item), -1, 0, opt,
- depth);
+ ret = asn1_item_ex_d2i(val, &p, len, ASN1_ITEM_ptr(tt->item), /*tag=*/-1,
+ /*aclass=*/0, opt, buf, depth);
if (!ret) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
goto err;
diff --git a/crypto/asn1/tasn_utl.c b/crypto/asn1/tasn_utl.c
index 741ce17..488c206 100644
--- a/crypto/asn1/tasn_utl.c
+++ b/crypto/asn1/tasn_utl.c
@@ -63,6 +63,7 @@
#include <openssl/err.h>
#include <openssl/mem.h>
#include <openssl/obj.h>
+#include <openssl/pool.h>
#include <openssl/thread.h>
#include "../internal.h"
@@ -135,8 +136,7 @@
if (enc) {
enc->enc = NULL;
enc->len = 0;
- enc->alias_only = 0;
- enc->alias_only_on_next_parse = 0;
+ enc->buf = NULL;
}
}
@@ -147,42 +147,41 @@
}
}
-int asn1_enc_save(ASN1_VALUE **pval, const unsigned char *in, int inlen,
- const ASN1_ITEM *it) {
+int asn1_enc_save(ASN1_VALUE **pval, const uint8_t *in, size_t in_len,
+ const ASN1_ITEM *it, CRYPTO_BUFFER *buf) {
ASN1_ENCODING *enc;
enc = asn1_get_enc_ptr(pval, it);
if (!enc) {
return 1;
}
- if (!enc->alias_only) {
- OPENSSL_free(enc->enc);
- }
-
- enc->alias_only = enc->alias_only_on_next_parse;
- enc->alias_only_on_next_parse = 0;
-
- if (enc->alias_only) {
+ asn1_encoding_clear(enc);
+ if (buf != NULL) {
+ assert(CRYPTO_BUFFER_data(buf) <= in &&
+ in + in_len <= CRYPTO_BUFFER_data(buf) + CRYPTO_BUFFER_len(buf));
+ CRYPTO_BUFFER_up_ref(buf);
+ enc->buf = buf;
enc->enc = (uint8_t *)in;
} else {
- enc->enc = OPENSSL_memdup(in, inlen);
+ enc->enc = OPENSSL_memdup(in, in_len);
if (!enc->enc) {
return 0;
}
}
- enc->len = inlen;
+ enc->len = in_len;
return 1;
}
void asn1_encoding_clear(ASN1_ENCODING *enc) {
- if (!enc->alias_only) {
+ if (enc->buf != NULL) {
+ CRYPTO_BUFFER_free(enc->buf);
+ } else {
OPENSSL_free(enc->enc);
}
enc->enc = NULL;
enc->len = 0;
- enc->alias_only = 0;
- enc->alias_only_on_next_parse = 0;
+ enc->buf = NULL;
}
int asn1_enc_restore(int *len, unsigned char **out, ASN1_VALUE **pval,
diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h
index 7bf14c9..6c594ec 100644
--- a/crypto/x509/internal.h
+++ b/crypto/x509/internal.h
@@ -159,7 +159,6 @@
NAME_CONSTRAINTS *nc;
unsigned char cert_hash[SHA256_DIGEST_LENGTH];
X509_CERT_AUX *aux;
- CRYPTO_BUFFER *buf;
CRYPTO_MUTEX lock;
} /* X509 */;
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 2e885c4..178e5f8 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -2381,12 +2381,18 @@
size_t data2_len;
bssl::UniquePtr<uint8_t> data2;
ASSERT_TRUE(PEMToDER(&data2, &data2_len, kLeafPEM));
+ EXPECT_TRUE(buffers_alias(root->cert_info->enc.enc, root->cert_info->enc.len,
+ CRYPTO_BUFFER_data(buf.get()),
+ CRYPTO_BUFFER_len(buf.get())));
X509 *x509p = root.get();
const uint8_t *inp = data2.get();
X509 *ret = d2i_X509(&x509p, &inp, data2_len);
ASSERT_EQ(root.get(), ret);
- ASSERT_EQ(nullptr, root->buf);
+ ASSERT_EQ(nullptr, root->cert_info->enc.buf);
+ EXPECT_FALSE(buffers_alias(root->cert_info->enc.enc, root->cert_info->enc.len,
+ CRYPTO_BUFFER_data(buf.get()),
+ CRYPTO_BUFFER_len(buf.get())));
// Free |data2| and ensure that |root| took its own copy. Otherwise the
// following will trigger a use-after-free.
@@ -2401,7 +2407,7 @@
ASSERT_EQ(static_cast<long>(data2_len), i2d_len);
ASSERT_EQ(0, OPENSSL_memcmp(data2.get(), i2d, i2d_len));
- ASSERT_EQ(nullptr, root->buf);
+ ASSERT_EQ(nullptr, root->cert_info->enc.buf);
}
TEST(X509Test, TestFailedParseFromBuffer) {
diff --git a/crypto/x509/x_name.c b/crypto/x509/x_name.c
index 1d6691b..f017423 100644
--- a/crypto/x509/x_name.c
+++ b/crypto/x509/x_name.c
@@ -206,7 +206,7 @@
ASN1_VALUE *intname_val = NULL;
ret = ASN1_item_ex_d2i(&intname_val, &p, len,
ASN1_ITEM_rptr(X509_NAME_INTERNAL), /*tag=*/-1,
- /*aclass=*/0, opt, ctx);
+ /*aclass=*/0, opt, /*buf=*/NULL);
if (ret <= 0) {
return ret;
}
diff --git a/crypto/x509/x_x509.c b/crypto/x509/x_x509.c
index 4f23fd6..5205267 100644
--- a/crypto/x509/x_x509.c
+++ b/crypto/x509/x_x509.c
@@ -101,16 +101,10 @@
ret->akid = NULL;
ret->aux = NULL;
ret->crldp = NULL;
- ret->buf = NULL;
CRYPTO_new_ex_data(&ret->ex_data);
CRYPTO_MUTEX_init(&ret->lock);
break;
- case ASN1_OP_D2I_PRE:
- CRYPTO_BUFFER_free(ret->buf);
- ret->buf = NULL;
- break;
-
case ASN1_OP_D2I_POST: {
// The version must be one of v1(0), v2(1), or v3(2).
long version = X509_VERSION_1;
@@ -150,7 +144,6 @@
CRL_DIST_POINTS_free(ret->crldp);
GENERAL_NAMES_free(ret->altname);
NAME_CONSTRAINTS_free(ret->nc);
- CRYPTO_BUFFER_free(ret->buf);
break;
}
@@ -170,29 +163,20 @@
X509 *X509_parse_from_buffer(CRYPTO_BUFFER *buf) {
if (CRYPTO_BUFFER_len(buf) > LONG_MAX) {
OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW);
- return 0;
- }
-
- X509 *x509 = X509_new();
- if (x509 == NULL) {
return NULL;
}
- x509->cert_info->enc.alias_only_on_next_parse = 1;
-
+ // Pass |buf| into the parser so that the cached encoding in |X509_CINF| can
+ // alias into the original buffer and save some memory.
const uint8_t *inp = CRYPTO_BUFFER_data(buf);
- X509 *x509p = x509;
- X509 *ret = d2i_X509(&x509p, &inp, CRYPTO_BUFFER_len(buf));
- if (ret == NULL ||
- inp - CRYPTO_BUFFER_data(buf) != (ptrdiff_t)CRYPTO_BUFFER_len(buf)) {
- X509_free(x509p);
+ X509 *ret = NULL;
+ if (ASN1_item_ex_d2i((ASN1_VALUE **)&ret, &inp, CRYPTO_BUFFER_len(buf),
+ ASN1_ITEM_rptr(X509), /*tag=*/-1,
+ /*aclass=*/0, /*opt=*/0, buf) <= 0 ||
+ inp != CRYPTO_BUFFER_data(buf) + CRYPTO_BUFFER_len(buf)) {
+ X509_free(ret);
return NULL;
}
- assert(x509p == x509);
- assert(ret == x509);
-
- CRYPTO_BUFFER_up_ref(buf);
- ret->buf = buf;
return ret;
}
