Add d2i_X509_from_buffer.
d2i_X509_from_buffer parses an |X509| from a |CRYPTO_BUFFER| but ensures
that the |X509_CINF.enc| doesn't make a copy of the encoded
TBSCertificate. Rather the |X509| holds a reference to the given
|CRYPTO_BUFFER|.
Change-Id: I38a4e3d0ca69fc0fd0ef3e15b53181844080fcad
Reviewed-on: https://boringssl-review.googlesource.com/11920
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <alangley@gmail.com>
Commit-Queue: Adam Langley <alangley@gmail.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/crypto/asn1/tasn_utl.c b/crypto/asn1/tasn_utl.c
index d409cfa..3f53072 100644
--- a/crypto/asn1/tasn_utl.c
+++ b/crypto/asn1/tasn_utl.c
@@ -56,6 +56,7 @@
#include <openssl/asn1.h>
+#include <assert.h>
#include <string.h>
#include <openssl/asn1t.h>
@@ -134,6 +135,8 @@
if (enc) {
enc->enc = NULL;
enc->len = 0;
+ enc->alias_only = 0;
+ enc->alias_only_on_next_parse = 0;
enc->modified = 1;
}
}
@@ -142,11 +145,13 @@
ASN1_ENCODING *enc;
enc = asn1_get_enc_ptr(pval, it);
if (enc) {
- if (enc->enc) {
+ if (enc->enc && !enc->alias_only) {
OPENSSL_free(enc->enc);
}
enc->enc = NULL;
enc->len = 0;
+ enc->alias_only = 0;
+ enc->alias_only_on_next_parse = 0;
enc->modified = 1;
}
}
@@ -159,14 +164,23 @@
return 1;
}
- if (enc->enc) {
+ if (!enc->alias_only) {
OPENSSL_free(enc->enc);
}
- enc->enc = OPENSSL_malloc(inlen);
- if (!enc->enc) {
- return 0;
+
+ enc->alias_only = enc->alias_only_on_next_parse;
+ enc->alias_only_on_next_parse = 0;
+
+ if (enc->alias_only) {
+ enc->enc = (uint8_t *) in;
+ } else {
+ enc->enc = OPENSSL_malloc(inlen);
+ if (!enc->enc) {
+ return 0;
+ }
+ memcpy(enc->enc, in, inlen);
}
- memcpy(enc->enc, in, inlen);
+
enc->len = inlen;
enc->modified = 0;
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 7e70162..b6ba2e6 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -17,10 +17,12 @@
#include <assert.h>
#include <string.h>
+#include <openssl/bytestring.h>
#include <openssl/crypto.h>
#include <openssl/digest.h>
#include <openssl/err.h>
#include <openssl/pem.h>
+#include <openssl/pool.h>
#include <openssl/x509.h>
namespace bssl {
@@ -744,6 +746,196 @@
return true;
}
+static bool PEMToDER(bssl::UniquePtr<uint8_t> *out, size_t *out_len,
+ const char *pem) {
+ bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(pem, strlen(pem)));
+ if (!bio) {
+ return false;
+ }
+
+ char *name, *header;
+ uint8_t *data;
+ long data_len;
+ if (!PEM_read_bio(bio.get(), &name, &header, &data, &data_len)) {
+ fprintf(stderr, "failed to read PEM data.\n");
+ return false;
+ }
+ OPENSSL_free(name);
+ OPENSSL_free(header);
+
+ out->reset(data);
+ *out_len = data_len;
+
+ return true;
+}
+
+static bool TestFromBuffer() {
+ size_t data_len;
+ bssl::UniquePtr<uint8_t> data;
+ if (!PEMToDER(&data, &data_len, kRootCAPEM)) {
+ return false;
+ }
+
+ bssl::UniquePtr<CRYPTO_BUFFER> buf(
+ CRYPTO_BUFFER_new(data.get(), data_len, nullptr));
+ if (!buf) {
+ return false;
+ }
+
+ bssl::UniquePtr<X509> root(d2i_X509_from_buffer(buf.get()));
+ if (!root) {
+ return false;
+ }
+
+ const uint8_t *enc_pointer = root->cert_info->enc.enc;
+ const uint8_t *buf_pointer = CRYPTO_BUFFER_data(buf.get());
+ if (enc_pointer < buf_pointer ||
+ enc_pointer >= buf_pointer + CRYPTO_BUFFER_len(buf.get())) {
+ fprintf(stderr, "TestFromBuffer: enc does not alias the buffer.\n");
+ return false;
+ }
+
+ buf.reset();
+
+ /* This ensures the X509 took a reference to |buf|, otherwise this will be a
+ * reference to free memory and ASAN should notice. */
+ if (enc_pointer[0] != CBS_ASN1_SEQUENCE) {
+ fprintf(stderr, "TestFromBuffer: enc data is not a SEQUENCE.\n");
+ return false;
+ }
+
+ return true;
+}
+
+static bool TestFromBufferTrailingData() {
+ size_t data_len;
+ bssl::UniquePtr<uint8_t> data;
+ if (!PEMToDER(&data, &data_len, kRootCAPEM)) {
+ return false;
+ }
+
+ std::unique_ptr<uint8_t[]> trailing_data(new uint8_t[data_len + 1]);
+ memcpy(trailing_data.get(), data.get(), data_len);
+
+ bssl::UniquePtr<CRYPTO_BUFFER> buf_trailing_data(
+ CRYPTO_BUFFER_new(trailing_data.get(), data_len + 1, nullptr));
+ if (!buf_trailing_data) {
+ return false;
+ }
+
+ bssl::UniquePtr<X509> root_trailing_data(
+ d2i_X509_from_buffer(buf_trailing_data.get()));
+ if (root_trailing_data) {
+ fprintf(stderr, "TestFromBuffer: trailing data was not rejected.\n");
+ return false;
+ }
+
+ return true;
+}
+
+static bool TestFromBufferModified() {
+ size_t data_len;
+ bssl::UniquePtr<uint8_t> data;
+ if (!PEMToDER(&data, &data_len, kRootCAPEM)) {
+ return false;
+ }
+
+ bssl::UniquePtr<CRYPTO_BUFFER> buf(
+ CRYPTO_BUFFER_new(data.get(), data_len, nullptr));
+ if (!buf) {
+ return false;
+ }
+
+ bssl::UniquePtr<X509> root(d2i_X509_from_buffer(buf.get()));
+ if (!root) {
+ return false;
+ }
+
+ bssl::UniquePtr<ASN1_INTEGER> fourty_two(ASN1_INTEGER_new());
+ ASN1_INTEGER_set(fourty_two.get(), 42);
+ X509_set_serialNumber(root.get(), fourty_two.get());
+
+ if (i2d_X509(root.get(), nullptr) != static_cast<long>(data_len)) {
+ fprintf(stderr,
+ "TestFromBuffer: i2d_X509 gives different answer before marking as "
+ "modified.\n");
+ return false;
+ }
+
+ X509_CINF_set_modified(root->cert_info);
+
+ if (i2d_X509(root.get(), nullptr) == static_cast<long>(data_len)) {
+ fprintf(stderr,
+ "TestFromBuffer: i2d_X509 gives same answer after marking as "
+ "modified.\n");
+ return false;
+ }
+
+ return true;
+}
+
+static bool TestFromBufferReused() {
+ size_t data_len;
+ bssl::UniquePtr<uint8_t> data;
+ if (!PEMToDER(&data, &data_len, kRootCAPEM)) {
+ return false;
+ }
+
+ bssl::UniquePtr<CRYPTO_BUFFER> buf(
+ CRYPTO_BUFFER_new(data.get(), data_len, nullptr));
+ if (!buf) {
+ return false;
+ }
+
+ bssl::UniquePtr<X509> root(d2i_X509_from_buffer(buf.get()));
+ if (!root) {
+ return false;
+ }
+
+ size_t data2_len;
+ bssl::UniquePtr<uint8_t> data2;
+ if (!PEMToDER(&data2, &data2_len, kLeafPEM)) {
+ return false;
+ }
+
+ X509 *x509p = root.get();
+ const uint8_t *inp = data2.get();
+ X509 *ret = d2i_X509(&x509p, &inp, data2_len);
+ if (ret != root.get()) {
+ fprintf(stderr,
+ "TestFromBuffer: d2i_X509 parsed into a different object.\n");
+ return false;
+ }
+
+ // Free |data2| and ensure that |root| took its own copy. Otherwise the
+ // following will trigger a use-after-free.
+ data2.reset();
+
+ const long i2d_len = i2d_X509(root.get(), nullptr);
+ if (i2d_len < 0) {
+ return false;
+ }
+ std::unique_ptr<uint8_t[]> i2d(new uint8_t[i2d_len]);
+ uint8_t *outp = i2d.get();
+ i2d_X509(root.get(), &outp);
+
+ if (!PEMToDER(&data2, &data2_len, kLeafPEM)) {
+ return false;
+ }
+ if (i2d_len != static_cast<long>(data2_len) ||
+ memcmp(data2.get(), i2d.get(), i2d_len) != 0) {
+ fprintf(stderr, "TestFromBufferReused: i2d gave wrong result.\n");
+ return false;
+ }
+
+ if (root->buf != NULL) {
+ fprintf(stderr, "TestFromBufferReused: X509.buf was not cleared.\n");
+ return false;
+ }
+
+ return true;
+}
+
static int Main() {
CRYPTO_library_init();
@@ -751,7 +943,11 @@
!TestCRL() ||
!TestPSS() ||
!TestBadPSSParameters() ||
- !TestSignCtx()) {
+ !TestSignCtx() ||
+ !TestFromBuffer() ||
+ !TestFromBufferTrailingData() ||
+ !TestFromBufferModified() ||
+ !TestFromBufferReused()) {
return 1;
}
diff --git a/crypto/x509/x_x509.c b/crypto/x509/x_x509.c
index 9fdda82..e2a65ff 100644
--- a/crypto/x509/x_x509.c
+++ b/crypto/x509/x_x509.c
@@ -62,6 +62,7 @@
#include <openssl/evp.h>
#include <openssl/mem.h>
#include <openssl/obj.h>
+#include <openssl/pool.h>
#include <openssl/thread.h>
#include <openssl/x509.h>
#include <openssl/x509v3.h>
@@ -103,9 +104,15 @@
ret->akid = NULL;
ret->aux = NULL;
ret->crldp = NULL;
+ ret->buf = NULL;
CRYPTO_new_ex_data(&ret->ex_data);
break;
+ case ASN1_OP_D2I_PRE:
+ CRYPTO_BUFFER_free(ret->buf);
+ ret->buf = NULL;
+ break;
+
case ASN1_OP_D2I_POST:
if (ret->name != NULL)
OPENSSL_free(ret->name);
@@ -121,6 +128,7 @@
policy_cache_free(ret->policy_cache);
GENERAL_NAMES_free(ret->altname);
NAME_CONSTRAINTS_free(ret->nc);
+ CRYPTO_BUFFER_free(ret->buf);
if (ret->name != NULL)
OPENSSL_free(ret->name);
@@ -142,6 +150,31 @@
IMPLEMENT_ASN1_DUP_FUNCTION(X509)
+X509 *d2i_X509_from_buffer(CRYPTO_BUFFER *buf) {
+ X509 *x509 = X509_new();
+ if (x509 == NULL) {
+ return NULL;
+ }
+
+ x509->cert_info->enc.alias_only_on_next_parse = 1;
+
+ 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(x509);
+ return NULL;
+ }
+ assert(x509p == x509);
+ assert(ret == x509);
+
+ CRYPTO_BUFFER_up_ref(buf);
+ ret->buf = buf;
+
+ return ret;
+}
+
int X509_up_ref(X509 *x)
{
CRYPTO_refcount_inc(&x->references);
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index c8b32e7..42386e0 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -246,7 +246,15 @@
{
unsigned char *enc; /* DER encoding */
long len; /* Length of encoding */
- int modified; /* set to 1 if 'enc' is invalid */
+ int modified; /* set to 1 if 'enc' is invalid */
+ /* 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;
} ASN1_ENCODING;
/* Used with ASN1 LONG type: if a long is set to this it is omitted */
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 9a5eb7d..a457b66 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -77,6 +77,7 @@
#include <openssl/ec.h>
#include <openssl/evp.h>
#include <openssl/obj.h>
+#include <openssl/pool.h>
#include <openssl/rsa.h>
#include <openssl/sha.h>
#include <openssl/stack.h>
@@ -261,6 +262,7 @@
NAME_CONSTRAINTS *nc;
unsigned char sha1_hash[SHA_DIGEST_LENGTH];
X509_CERT_AUX *aux;
+ CRYPTO_BUFFER *buf;
} /* X509 */;
DECLARE_STACK_OF(X509)
@@ -636,6 +638,12 @@
unsigned char *md, unsigned int *len);
#endif
+/* d2i_X509_from_buffer parses an X.509 structure from |buf| and returns a
+ * fresh X509 or NULL on error. There must not be any trailing data in |buf|.
+ * The returned structure (if any) holds a reference to |buf| rather than
+ * copying parts of it as a normal |d2i_X509| call would do. */
+OPENSSL_EXPORT X509 *d2i_X509_from_buffer(CRYPTO_BUFFER *buf);
+
#ifndef OPENSSL_NO_FP_API
OPENSSL_EXPORT X509 *d2i_X509_fp(FILE *fp, X509 **x509);
OPENSSL_EXPORT int i2d_X509_fp(FILE *fp,X509 *x509);
