Remove d2i_FOO object reuse
The "object reuse" mode in d2i_FOO is extremely problematic. See bug for
details. When we rewrite d2i_RSAPublicKey, etc., with CBS, we switched
dropped this fragile behavior and replaced it with freeing the old value
and replacing it with the new value. Extend this behavior to all functions
generated by crypto/asn1 templates too.
In particular, tasn_dec.c already frees the original object on error,
which means callers must already be prepared for OpenSSL to free their
reused object. The one caller I found that would be incompatible (via
both running tests and auditing callers) was actually broken due to this
error case, and has been fixed.
Update-Note: This slightly changes the calling convention of the d2i_FOO
functions. The change should be compatible with almost all valid calls.
If something goes wrong, it should hopefully be quite obvious. If
affected (or unaffected), prefer to set the output parameter to NULL
and use the return value instead.
Fixed: 550
Change-Id: Ie54cdf17f8f5a4d76fdbcddeaa27e6afd3fa9d8e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56647
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/PORTING.md b/PORTING.md
index 9adaf73..eb951f5 100644
--- a/PORTING.md
+++ b/PORTING.md
@@ -170,14 +170,13 @@
In addition to returning the result, OpenSSL places it in `*out` if `out` is
not `NULL`. On input, if `*out` is not `NULL`, OpenSSL will usually (but not
always) reuse that object rather than allocating a new one. In BoringSSL, these
-functions are compatibility wrappers over a newer ASN.1 stack. Even if `*out`
-is not `NULL`, these wrappers will always allocate a new object and free the
-previous one.
+functions will always allocate a new object and free the previous one.
Ensure that callers do not rely on this object reuse behavior. It is
recommended to avoid the `out` parameter completely and always pass in `NULL`.
-Note that less error-prone APIs are available for BoringSSL-specific code (see
-below).
+In most cases, even in OpenSSL, relying on object reuse is not safe because, on
+parse error, OpenSSL will free the reused object. Note that less error-prone
+APIs are available for BoringSSL-specific code (see below).
### Memory allocation
diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c
index bdde8e3..39329b6 100644
--- a/crypto/asn1/tasn_dec.c
+++ b/crypto/asn1/tasn_dec.c
@@ -61,6 +61,7 @@
#include <openssl/mem.h>
#include <openssl/pool.h>
+#include <assert.h>
#include <limits.h>
#include <string.h>
@@ -144,20 +145,35 @@
ASN1_VALUE *ASN1_item_d2i(ASN1_VALUE **pval, const unsigned char **in, long len,
const ASN1_ITEM *it) {
- ASN1_VALUE *ptmpval = NULL;
- if (!pval) {
- pval = &ptmpval;
+ ASN1_VALUE *ret = NULL;
+ if (asn1_item_ex_d2i(&ret, in, len, it, /*tag=*/-1, /*aclass=*/0, /*opt=*/0,
+ /*buf=*/NULL, /*depth=*/0) <= 0) {
+ // Clean up, in case the caller left a partial object.
+ //
+ // TODO(davidben): I don't think it can leave one, but the codepaths below
+ // are a bit inconsistent. Revisit this when rewriting this function.
+ ASN1_item_ex_free(&ret, it);
}
- if (asn1_item_ex_d2i(pval, in, len, it, /*tag=*/-1, /*aclass=*/0, /*opt=*/0,
- /*buf=*/NULL, /*depth=*/0) > 0) {
- return *pval;
+ // If the caller supplied an output pointer, free the old one and replace it
+ // with |ret|. This differs from OpenSSL slightly in that we don't support
+ // object reuse. We run this on both success and failure. On failure, even
+ // with object reuse, OpenSSL destroys the previous object.
+ if (pval != NULL) {
+ ASN1_item_ex_free(pval, it);
+ *pval = ret;
}
- return NULL;
+ return ret;
}
// Decode an item, taking care of IMPLICIT tagging, if any. If 'opt' set and
// tag mismatch return -1 to handle OPTIONAL
+//
+// TODO(davidben): Historically, all functions in this file had to account for
+// |*pval| containing an arbitrary existing value. This is no longer the case
+// because |ASN1_item_d2i| now always starts from NULL. As part of rewriting
+// this function, take the simplified assumptions into account. Though we must
+// still account for the internal calls to |ASN1_item_ex_new|.
static int asn1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in,
long len, const ASN1_ITEM *it, int tag, int aclass,
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 178e5f8..0e681aa 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -2385,9 +2385,16 @@
CRYPTO_BUFFER_data(buf.get()),
CRYPTO_BUFFER_len(buf.get())));
- X509 *x509p = root.get();
+ // Historically, this function tested the interaction betweeen
+ // |X509_parse_from_buffer| and object reuse. We no longer support object
+ // reuse, so |d2i_X509| will replace |raw| with a new object. However, we
+ // retain this test to verify that releasing objects from |d2i_X509| works
+ // correctly.
+ X509 *raw = root.release();
const uint8_t *inp = data2.get();
- X509 *ret = d2i_X509(&x509p, &inp, data2_len);
+ X509 *ret = d2i_X509(&raw, &inp, data2_len);
+ root.reset(raw);
+
ASSERT_EQ(root.get(), ret);
ASSERT_EQ(nullptr, root->cert_info->enc.buf);
EXPECT_FALSE(buffers_alias(root->cert_info->enc.enc, root->cert_info->enc.len,
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 0370a3e..6c7c1a8 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -213,38 +213,10 @@
//
// Note: If |out| and |*out| are both non-NULL, the object at |*out| is not
// updated in-place. Instead, it is freed, and the pointer is updated to the
-// new object. This differs from OpenSSL, which behaves more like
-// |d2i_SAMPLE_with_reuse|. Callers are recommended to set |out| to NULL and
-// instead use the return value.
+// new object. This differs from OpenSSL. Callers are recommended to set |out|
+// to NULL and instead use the return value.
SAMPLE *d2i_SAMPLE(SAMPLE **out, const uint8_t **inp, long len);
-// d2i_SAMPLE_with_reuse parses a structure from up to |len| bytes at |*inp|. On
-// success, it advances |*inp| by the number of bytes read and returns a
-// non-NULL pointer to an object containing the parsed structure. The object is
-// determined from |out| as follows:
-//
-// If |out| is NULL, the function places the result in a newly-allocated
-// |SAMPLE| object and returns it. This mode is recommended.
-//
-// If |out| is non-NULL, but |*out| is NULL, the function also places the result
-// in a newly-allocated |SAMPLE| object. It sets |*out| to this object and also
-// returns it.
-//
-// If |out| and |*out| are both non-NULL, the function updates the object at
-// |*out| in-place with the result and returns |*out|.
-//
-// If any of the above fail, the function returns NULL.
-//
-// This function does not reject trailing data in the input. This allows the
-// caller to parse a sequence of concatenated structures. Callers parsing only
-// one structure should check for trailing data by comparing the updated |*inp|
-// with the end of the input.
-//
-// WARNING: Callers should not rely on the in-place update mode. It often
-// produces the wrong result or breaks the type's internal invariants. Future
-// revisions of BoringSSL may standardize on the |d2i_SAMPLE| behavior.
-SAMPLE *d2i_SAMPLE_with_reuse(SAMPLE **out, const uint8_t **inp, long len);
-
// i2d_SAMPLE marshals |in|. On error, it returns a negative value. On success,
// it returns the length of the result and outputs it via |outp| as follows:
//
@@ -348,8 +320,8 @@
OPENSSL_EXPORT void ASN1_item_free(ASN1_VALUE *val, const ASN1_ITEM *it);
// ASN1_item_d2i parses the ASN.1 type |it| from up to |len| bytes at |*inp|.
-// It behaves like |d2i_SAMPLE_with_reuse|, except that |out| and the return
-// value are cast to |ASN1_VALUE| pointers.
+// It behaves like |d2i_SAMPLE|, except that |out| and the return value are cast
+// to |ASN1_VALUE| pointers.
//
// TODO(https://crbug.com/boringssl/444): C strict aliasing forbids type-punning
// |T*| and |ASN1_VALUE*| the way this function signature does. When that bug is
@@ -654,7 +626,7 @@
// The following functions parse up to |len| bytes from |*inp| as a
// DER-encoded ASN.1 value of the corresponding type, as described in
-// |d2i_SAMPLE_with_reuse|.
+// |d2i_SAMPLE|.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -846,7 +818,7 @@
OPENSSL_EXPORT void DIRECTORYSTRING_free(ASN1_STRING *str);
// d2i_DIRECTORYSTRING parses up to |len| bytes from |*inp| as a DER-encoded
-// X.509 DirectoryString (RFC 5280), as described in |d2i_SAMPLE_with_reuse|.
+// X.509 DirectoryString (RFC 5280), as described in |d2i_SAMPLE|.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -879,7 +851,7 @@
OPENSSL_EXPORT void DISPLAYTEXT_free(ASN1_STRING *str);
// d2i_DISPLAYTEXT parses up to |len| bytes from |*inp| as a DER-encoded X.509
-// DisplayText (RFC 5280), as described in |d2i_SAMPLE_with_reuse|.
+// DisplayText (RFC 5280), as described in |d2i_SAMPLE|.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -940,7 +912,7 @@
OPENSSL_EXPORT void ASN1_BIT_STRING_free(ASN1_BIT_STRING *str);
// d2i_ASN1_BIT_STRING parses up to |len| bytes from |*inp| as a DER-encoded
-// ASN.1 BIT STRING, as described in |d2i_SAMPLE_with_reuse|.
+// ASN.1 BIT STRING, as described in |d2i_SAMPLE|.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -955,8 +927,7 @@
// c2i_ASN1_BIT_STRING decodes |len| bytes from |*inp| as the contents of a
// DER-encoded BIT STRING, excluding the tag and length. It behaves like
-// |d2i_SAMPLE_with_reuse| except, on success, it always consumes all |len|
-// bytes.
+// |d2i_SAMPLE| except, on success, it always consumes all |len| bytes.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -1051,7 +1022,7 @@
OPENSSL_EXPORT ASN1_INTEGER *ASN1_INTEGER_dup(const ASN1_INTEGER *x);
// d2i_ASN1_INTEGER parses up to |len| bytes from |*inp| as a DER-encoded
-// ASN.1 INTEGER, as described in |d2i_SAMPLE_with_reuse|.
+// ASN.1 INTEGER, as described in |d2i_SAMPLE|.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -1064,8 +1035,7 @@
// c2i_ASN1_INTEGER decodes |len| bytes from |*inp| as the contents of a
// DER-encoded INTEGER, excluding the tag and length. It behaves like
-// |d2i_SAMPLE_with_reuse| except, on success, it always consumes all |len|
-// bytes.
+// |d2i_SAMPLE| except, on success, it always consumes all |len| bytes.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// some invalid inputs, but this will be removed in the future.
@@ -1136,7 +1106,7 @@
OPENSSL_EXPORT void ASN1_ENUMERATED_free(ASN1_ENUMERATED *str);
// d2i_ASN1_ENUMERATED parses up to |len| bytes from |*inp| as a DER-encoded
-// ASN.1 ENUMERATED, as described in |d2i_SAMPLE_with_reuse|.
+// ASN.1 ENUMERATED, as described in |d2i_SAMPLE|.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -1214,7 +1184,7 @@
OPENSSL_EXPORT void ASN1_UTCTIME_free(ASN1_UTCTIME *str);
// d2i_ASN1_UTCTIME parses up to |len| bytes from |*inp| as a DER-encoded
-// ASN.1 UTCTime, as described in |d2i_SAMPLE_with_reuse|.
+// ASN.1 UTCTime, as described in |d2i_SAMPLE|.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -1268,7 +1238,7 @@
OPENSSL_EXPORT void ASN1_GENERALIZEDTIME_free(ASN1_GENERALIZEDTIME *str);
// d2i_ASN1_GENERALIZEDTIME parses up to |len| bytes from |*inp| as a
-// DER-encoded ASN.1 GeneralizedTime, as described in |d2i_SAMPLE_with_reuse|.
+// DER-encoded ASN.1 GeneralizedTime, as described in |d2i_SAMPLE|.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -1326,7 +1296,7 @@
OPENSSL_EXPORT void ASN1_TIME_free(ASN1_TIME *str);
// d2i_ASN1_TIME parses up to |len| bytes from |*inp| as a DER-encoded X.509
-// Time (RFC 5280), as described in |d2i_SAMPLE_with_reuse|.
+// Time (RFC 5280), as described in |d2i_SAMPLE|.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -1464,7 +1434,7 @@
OPENSSL_EXPORT void ASN1_OBJECT_free(ASN1_OBJECT *a);
// d2i_ASN1_OBJECT parses a DER-encoded ASN.1 OBJECT IDENTIFIER from up to |len|
-// bytes at |*inp|, as described in |d2i_SAMPLE_with_reuse|.
+// bytes at |*inp|, as described in |d2i_SAMPLE|.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -1477,8 +1447,7 @@
// c2i_ASN1_OBJECT decodes |len| bytes from |*inp| as the contents of a
// DER-encoded OBJECT IDENTIFIER, excluding the tag and length. It behaves like
-// |d2i_SAMPLE_with_reuse| except, on success, it always consumes all |len|
-// bytes.
+// |d2i_SAMPLE| except, on success, it always consumes all |len| bytes.
OPENSSL_EXPORT ASN1_OBJECT *c2i_ASN1_OBJECT(ASN1_OBJECT **out,
const uint8_t **inp, long len);
@@ -1568,10 +1537,10 @@
OPENSSL_EXPORT void ASN1_TYPE_free(ASN1_TYPE *a);
// d2i_ASN1_TYPE parses up to |len| bytes from |*inp| as an ASN.1 value of any
-// type, as described in |d2i_SAMPLE_with_reuse|. Note this function only
-// validates primitive, universal types supported by this library. Values of
-// type |V_ASN1_SEQUENCE|, |V_ASN1_SET|, |V_ASN1_OTHER|, or an unsupported
-// primitive type must be validated by the caller when interpreting.
+// type, as described in |d2i_SAMPLE|. Note this function only validates
+// primitive, universal types supported by this library. Values of type
+// |V_ASN1_SEQUENCE|, |V_ASN1_SET|, |V_ASN1_OTHER|, or an unsupported primitive
+// type must be validated by the caller when interpreting.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -1615,9 +1584,9 @@
typedef STACK_OF(ASN1_TYPE) ASN1_SEQUENCE_ANY;
// d2i_ASN1_SEQUENCE_ANY parses up to |len| bytes from |*inp| as a DER-encoded
-// ASN.1 SEQUENCE OF ANY structure, as described in |d2i_SAMPLE_with_reuse|. The
-// resulting |ASN1_SEQUENCE_ANY| owns its contents and thus must be released
-// with |sk_ASN1_TYPE_pop_free| and |ASN1_TYPE_free|, not |sk_ASN1_TYPE_free|.
+// ASN.1 SEQUENCE OF ANY structure, as described in |d2i_SAMPLE|. The resulting
+// |ASN1_SEQUENCE_ANY| owns its contents and thus must be released with
+// |sk_ASN1_TYPE_pop_free| and |ASN1_TYPE_free|, not |sk_ASN1_TYPE_free|.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -1631,7 +1600,7 @@
uint8_t **outp);
// d2i_ASN1_SET_ANY parses up to |len| bytes from |*inp| as a DER-encoded ASN.1
-// SET OF ANY structure, as described in |d2i_SAMPLE_with_reuse|. The resulting
+// SET OF ANY structure, as described in |d2i_SAMPLE|. The resulting
// |ASN1_SEQUENCE_ANY| owns its contents and thus must be released with
// |sk_ASN1_TYPE_pop_free| and |ASN1_TYPE_free|, not |sk_ASN1_TYPE_free|.
//
@@ -1943,7 +1912,7 @@
// d2i_ASN1_PRINTABLE parses up to |len| bytes from |*inp| as a DER-encoded
// CHOICE of an ad-hoc subset of string-like types, as described in
-// |d2i_SAMPLE_with_reuse|.
+// |d2i_SAMPLE|.
//
// Do not use this. Despite, the name it has no connection to PrintableString or
// printable characters. See https://crbug.com/boringssl/412.
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index f38574f..32ddcee 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -146,7 +146,7 @@
OPENSSL_EXPORT void X509_free(X509 *x509);
// d2i_X509 parses up to |len| bytes from |*inp| as a DER-encoded X.509
-// Certificate (RFC 5280), as described in |d2i_SAMPLE_with_reuse|.
+// Certificate (RFC 5280), as described in |d2i_SAMPLE|.
OPENSSL_EXPORT X509 *d2i_X509(X509 **out, const uint8_t **inp, long len);
// X509_parse_from_buffer parses an X.509 structure from |buf| and returns a
@@ -398,8 +398,7 @@
// d2i_X509_AUX parses up to |length| bytes from |*inp| as a DER-encoded X.509
// Certificate (RFC 5280), followed optionally by a separate, OpenSSL-specific
-// structure with auxiliary properties. It behaves as described in
-// |d2i_SAMPLE_with_reuse|.
+// structure with auxiliary properties. It behaves as described in |d2i_SAMPLE|.
//
// Some auxiliary properties affect trust decisions, so this function should not
// be used with untrusted input.
@@ -485,7 +484,7 @@
OPENSSL_EXPORT void X509_CRL_free(X509_CRL *crl);
// d2i_X509_CRL parses up to |len| bytes from |*inp| as a DER-encoded X.509
-// CertificateList (RFC 5280), as described in |d2i_SAMPLE_with_reuse|.
+// CertificateList (RFC 5280), as described in |d2i_SAMPLE|.
OPENSSL_EXPORT X509_CRL *d2i_X509_CRL(X509_CRL **out, const uint8_t **inp,
long len);
@@ -699,7 +698,7 @@
OPENSSL_EXPORT void X509_REQ_free(X509_REQ *req);
// d2i_X509_REQ parses up to |len| bytes from |*inp| as a DER-encoded
-// CertificateRequest (RFC 2986), as described in |d2i_SAMPLE_with_reuse|.
+// CertificateRequest (RFC 2986), as described in |d2i_SAMPLE|.
OPENSSL_EXPORT X509_REQ *d2i_X509_REQ(X509_REQ **out, const uint8_t **inp,
long len);
@@ -850,7 +849,7 @@
OPENSSL_EXPORT void X509_NAME_free(X509_NAME *name);
// d2i_X509_NAME parses up to |len| bytes from |*inp| as a DER-encoded X.509
-// Name (RFC 5280), as described in |d2i_SAMPLE_with_reuse|.
+// Name (RFC 5280), as described in |d2i_SAMPLE|.
OPENSSL_EXPORT X509_NAME *d2i_X509_NAME(X509_NAME **out, const uint8_t **inp,
long len);
@@ -970,7 +969,7 @@
OPENSSL_EXPORT void X509_NAME_ENTRY_free(X509_NAME_ENTRY *entry);
// d2i_X509_NAME_ENTRY parses up to |len| bytes from |*inp| as a DER-encoded
-// AttributeTypeAndValue (RFC 5280), as described in |d2i_SAMPLE_with_reuse|.
+// AttributeTypeAndValue (RFC 5280), as described in |d2i_SAMPLE|.
OPENSSL_EXPORT X509_NAME_ENTRY *d2i_X509_NAME_ENTRY(X509_NAME_ENTRY **out,
const uint8_t **inp,
long len);
@@ -1073,7 +1072,7 @@
OPENSSL_EXPORT void X509_EXTENSION_free(X509_EXTENSION *ex);
// d2i_X509_EXTENSION parses up to |len| bytes from |*inp| as a DER-encoded
-// X.509 Extension (RFC 5280), as described in |d2i_SAMPLE_with_reuse|.
+// X.509 Extension (RFC 5280), as described in |d2i_SAMPLE|.
OPENSSL_EXPORT X509_EXTENSION *d2i_X509_EXTENSION(X509_EXTENSION **out,
const uint8_t **inp,
long len);
@@ -1149,7 +1148,7 @@
DECLARE_ASN1_ITEM(X509_EXTENSIONS)
// d2i_X509_EXTENSIONS parses up to |len| bytes from |*inp| as a DER-encoded
-// SEQUENCE OF Extension (RFC 5280), as described in |d2i_SAMPLE_with_reuse|.
+// SEQUENCE OF Extension (RFC 5280), as described in |d2i_SAMPLE|.
OPENSSL_EXPORT X509_EXTENSIONS *d2i_X509_EXTENSIONS(X509_EXTENSIONS **out,
const uint8_t **inp,
long len);
@@ -1235,7 +1234,7 @@
OPENSSL_EXPORT void X509_ALGOR_free(X509_ALGOR *alg);
// d2i_X509_ALGOR parses up to |len| bytes from |*inp| as a DER-encoded
-// AlgorithmIdentifier, as described in |d2i_SAMPLE_with_reuse|.
+// AlgorithmIdentifier, as described in |d2i_SAMPLE|.
OPENSSL_EXPORT X509_ALGOR *d2i_X509_ALGOR(X509_ALGOR **out, const uint8_t **inp,
long len);
