Avoid one malloc indirection in X509
This is very minor in itself but the aim is to start setting up the
patterns to rewrite the X509_CINF parser with CBS/CBB. In doing so, I'd
like to directly embed some of the many child fields in X509. It saves
some tiny mallocs and avoids worrying about whether the field is null.
To support that, I've made the low-level parsing functions for
crypto/asn1 look like "parse into" rather "parse and return new".
The embed + parse-into pattern also avoids a thorny discrepancy between
d2i_FOO and FOO_new: FOO_new currently fills in every required field,
which is nice because it means, e.g., an X509 actually never has a null
issuer. But if the parser first internally calls X509_new to construct
the X509 and then fills it in, the inner X509_NAME parser will then have
to throw it away and make a new one. As a result, we have this weird
internal x509_new_null function. (tasn_dec.cc avoids this because
internally it actually is a parse-into pattern, not a parse-new pattern.
And then it just keeps allocating objects and checking for errors
everywhere.)
On the flip side, it means a bunch of types have to become embeddable
within the library, instead of always allocated. That means adding
internal init/cleanup functions. All this would be made a lot easier
with C++ constructors and destructors, but there's a lot to work through
there (I'm not thrilled about how we stomp ::~ssl_ctx_st() in libssl.)
So, for now, I'm doing all this C-style.
The immediate reason for revisiting that parser now is to thread
EVP_PKEY_ALG parameters into the SPKI parser. The table-based parser is
makes it very hard to add a new parameter. But I've also wanted to do
this for some time anyway.
Bug: 42290417
Change-Id: I628803992d5091333b9213db18736db2b9911cb0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81771
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/asn1/a_bitstr.cc b/crypto/asn1/a_bitstr.cc
index 6bdd726..ba205a7 100644
--- a/crypto/asn1/a_bitstr.cc
+++ b/crypto/asn1/a_bitstr.cc
@@ -20,6 +20,7 @@
#include <openssl/bytestring.h>
#include <openssl/err.h>
#include <openssl/mem.h>
+#include <openssl/span.h>
#include "../internal.h"
#include "internal.h"
@@ -110,76 +111,96 @@
CBB_flush(out);
}
-ASN1_BIT_STRING *c2i_ASN1_BIT_STRING(ASN1_BIT_STRING **a,
- const unsigned char **pp, long len) {
- ASN1_BIT_STRING *ret = NULL;
- const unsigned char *p;
- unsigned char *s;
- int padding;
- uint8_t padding_mask;
-
- if (len < 1) {
+static int asn1_parse_bit_string_contents(bssl::Span<const uint8_t> in,
+ ASN1_BIT_STRING *out) {
+ CBS cbs = in;
+ uint8_t padding;
+ if (!CBS_get_u8(&cbs, &padding)) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_STRING_TOO_SHORT);
- goto err;
+ return 0;
}
- if (len > INT_MAX) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_STRING_TOO_LONG);
- goto err;
- }
-
- if ((a == NULL) || ((*a) == NULL)) {
- if ((ret = ASN1_BIT_STRING_new()) == NULL) {
- return NULL;
- }
- } else {
- ret = (*a);
- }
-
- p = *pp;
- padding = *(p++);
- len--;
if (padding > 7) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_INVALID_BIT_STRING_BITS_LEFT);
- goto err;
+ return 0;
}
// Unused bits in a BIT STRING must be zero.
- padding_mask = (1 << padding) - 1;
- if (padding != 0 && (len < 1 || (p[len - 1] & padding_mask) != 0)) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_INVALID_BIT_STRING_PADDING);
- goto err;
- }
-
- // We do this to preserve the settings. If we modify the settings, via
- // the _set_bit function, we will recalculate on output
- ret->flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07); // clear
- ret->flags |= (ASN1_STRING_FLAG_BITS_LEFT | padding); // set
-
- if (len > 0) {
- s = reinterpret_cast<uint8_t *>(OPENSSL_memdup(p, len));
- if (s == NULL) {
- goto err;
+ uint8_t padding_mask = (1 << padding) - 1;
+ if (padding != 0) {
+ CBS copy = cbs;
+ uint8_t last;
+ if (!CBS_get_last_u8(©, &last) || (last & padding_mask) != 0) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_INVALID_BIT_STRING_PADDING);
+ return 0;
}
- p += len;
- } else {
- s = NULL;
}
- ret->length = (int)len;
- OPENSSL_free(ret->data);
- ret->data = s;
- ret->type = V_ASN1_BIT_STRING;
- if (a != NULL) {
- (*a) = ret;
+ if (!ASN1_STRING_set(out, CBS_data(&cbs), CBS_len(&cbs))) {
+ return 0;
}
- *pp = p;
+
+ out->type = V_ASN1_BIT_STRING;
+ // |ASN1_STRING_FLAG_BITS_LEFT| and the bottom 3 bits encode |padding|.
+ out->flags &= ~0x07;
+ out->flags |= ASN1_STRING_FLAG_BITS_LEFT | padding;
+ return 1;
+}
+
+ASN1_BIT_STRING *c2i_ASN1_BIT_STRING(ASN1_BIT_STRING **a,
+ const unsigned char **pp, long len) {
+ if (len < 0) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_STRING_TOO_SHORT);
+ return nullptr;
+ }
+
+ ASN1_BIT_STRING *ret = nullptr;
+ if (a == nullptr || *a == nullptr) {
+ if ((ret = ASN1_BIT_STRING_new()) == nullptr) {
+ return nullptr;
+ }
+ } else {
+ ret = *a;
+ }
+
+ if (!asn1_parse_bit_string_contents(bssl::Span(*pp, len), ret)) {
+ if (ret != nullptr && (a == nullptr || *a != ret)) {
+ ASN1_BIT_STRING_free(ret);
+ }
+ return nullptr;
+ }
+
+ if (a != nullptr) {
+ *a = ret;
+ }
+ *pp += len;
return ret;
-err:
- if ((ret != NULL) && ((a == NULL) || (*a != ret))) {
- ASN1_BIT_STRING_free(ret);
+}
+
+int asn1_parse_bit_string(CBS *cbs, ASN1_BIT_STRING *out, CBS_ASN1_TAG tag) {
+ tag = tag == 0 ? CBS_ASN1_BITSTRING : tag;
+ CBS child;
+ if (!CBS_get_asn1(cbs, &child, tag)) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
+ return 0;
}
- return NULL;
+ return asn1_parse_bit_string_contents(child, out);
+}
+
+int asn1_parse_bit_string_with_bad_length(CBS *cbs, ASN1_BIT_STRING *out) {
+ CBS child;
+ CBS_ASN1_TAG tag;
+ size_t header_len;
+ int indefinite;
+ if (!CBS_get_any_ber_asn1_element(cbs, &child, &tag, &header_len,
+ /*out_ber_found=*/nullptr,
+ &indefinite) ||
+ tag != CBS_ASN1_BITSTRING || indefinite || //
+ !CBS_skip(&child, header_len)) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
+ return 0;
+ }
+ return asn1_parse_bit_string_contents(child, out);
}
// These next 2 functions from Goetz Babin-Ebell <babinebell@trustcenter.de>
diff --git a/crypto/asn1/asn1_lib.cc b/crypto/asn1/asn1_lib.cc
index 517dda5..ad8c822 100644
--- a/crypto/asn1/asn1_lib.cc
+++ b/crypto/asn1/asn1_lib.cc
@@ -295,11 +295,21 @@
return ret;
}
+void asn1_string_init(ASN1_STRING *str, int type) {
+ OPENSSL_memset(str, 0, sizeof(ASN1_STRING));
+ str->type = type;
+}
+
+void asn1_string_cleanup(ASN1_STRING *str) {
+ OPENSSL_free(str->data);
+ str->data = nullptr;
+}
+
void ASN1_STRING_free(ASN1_STRING *str) {
if (str == NULL) {
return;
}
- OPENSSL_free(str->data);
+ asn1_string_cleanup(str);
OPENSSL_free(str);
}
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h
index 81bb8d0..b64e55e 100644
--- a/crypto/asn1/internal.h
+++ b/crypto/asn1/internal.h
@@ -172,6 +172,14 @@
// ASN.1 PrintableString, and zero otherwise.
int asn1_is_printable(uint32_t value);
+// asn1_string_init initializes |str|, which may be uninitialized, with type
+// |type|.
+void asn1_string_init(ASN1_STRING *str, int type);
+
+// asn1_string_cleanup releases memory associated with |str|'s value, without
+// freeing |str| itself.
+void asn1_string_cleanup(ASN1_STRING *str);
+
// asn1_bit_string_length returns the number of bytes in |str| and sets
// |*out_padding_bits| to the number of padding bits.
//
@@ -180,6 +188,15 @@
int asn1_bit_string_length(const ASN1_BIT_STRING *str,
uint8_t *out_padding_bits);
+// asn1_parse_bit_string parses a DER-encoded ASN.1 BIT STRING and writes the
+// result to |out|. If |tag| is non-zero, the BIT STRING is implicitly tagged
+// with |tag|.
+int asn1_parse_bit_string(CBS *cbs, ASN1_BIT_STRING *out, CBS_ASN1_TAG tag);
+
+// asn1_parse_bit_string_with_bad_length behaves like |asn1_parse_bit_string|
+// but tolerates BER non-minimal, definite lengths.
+int asn1_parse_bit_string_with_bad_length(CBS *cbs, ASN1_BIT_STRING *out);
+
// asn1_marshal_bit_string marshals |in| as a DER-encoded, ASN.1 BIT STRING and
// writes the result to |out|. It returns one on success and zero on error. If
// |tag| is non-zero, the tag is replaced with |tag|.
diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h
index 345b44c..fee3f66 100644
--- a/crypto/x509/internal.h
+++ b/crypto/x509/internal.h
@@ -118,7 +118,7 @@
struct x509_st {
X509_CINF *cert_info;
X509_ALGOR *sig_alg;
- ASN1_BIT_STRING *signature;
+ ASN1_BIT_STRING signature;
CRYPTO_refcount_t references;
CRYPTO_EX_DATA ex_data;
// These contain copies of various extension values
diff --git a/crypto/x509/t_x509.cc b/crypto/x509/t_x509.cc
index fffe63a..765d551 100644
--- a/crypto/x509/t_x509.cc
+++ b/crypto/x509/t_x509.cc
@@ -203,7 +203,7 @@
}
if (!(cflag & X509_FLAG_NO_SIGDUMP)) {
- if (X509_signature_print(bp, x->sig_alg, x->signature) <= 0) {
+ if (X509_signature_print(bp, x->sig_alg, &x->signature) <= 0) {
return 0;
}
}
diff --git a/crypto/x509/x_all.cc b/crypto/x509/x_all.cc
index 4393750..493eb05 100644
--- a/crypto/x509/x_all.cc
+++ b/crypto/x509/x_all.cc
@@ -34,7 +34,7 @@
return 0;
}
return ASN1_item_verify(ASN1_ITEM_rptr(X509_CINF), x509->sig_alg,
- x509->signature, x509->cert_info, pkey);
+ &x509->signature, x509->cert_info, pkey);
}
int X509_REQ_verify(X509_REQ *req, EVP_PKEY *pkey) {
@@ -45,13 +45,13 @@
int X509_sign(X509 *x, EVP_PKEY *pkey, const EVP_MD *md) {
asn1_encoding_clear(&x->cert_info->enc);
return (ASN1_item_sign(ASN1_ITEM_rptr(X509_CINF), x->cert_info->signature,
- x->sig_alg, x->signature, x->cert_info, pkey, md));
+ x->sig_alg, &x->signature, x->cert_info, pkey, md));
}
int X509_sign_ctx(X509 *x, EVP_MD_CTX *ctx) {
asn1_encoding_clear(&x->cert_info->enc);
return ASN1_item_sign_ctx(ASN1_ITEM_rptr(X509_CINF), x->cert_info->signature,
- x->sig_alg, x->signature, x->cert_info, ctx);
+ x->sig_alg, &x->signature, x->cert_info, ctx);
}
int X509_REQ_sign(X509_REQ *x, EVP_PKEY *pkey, const EVP_MD *md) {
diff --git a/crypto/x509/x_x509.cc b/crypto/x509/x_x509.cc
index 3e6c2a0..e2869ac 100644
--- a/crypto/x509/x_x509.cc
+++ b/crypto/x509/x_x509.cc
@@ -57,6 +57,7 @@
ret->references = 1;
ret->ex_pathlen = -1;
+ asn1_string_init(&ret->signature, V_ASN1_BIT_STRING);
CRYPTO_new_ex_data(&ret->ex_data);
CRYPTO_MUTEX_init(&ret->lock);
return ret;
@@ -70,9 +71,7 @@
ret->cert_info = X509_CINF_new();
ret->sig_alg = X509_ALGOR_new();
- ret->signature = ASN1_BIT_STRING_new();
- if (ret->cert_info == NULL || ret->sig_alg == NULL ||
- ret->signature == NULL) {
+ if (ret->cert_info == NULL || ret->sig_alg == NULL) {
X509_free(ret);
return NULL;
}
@@ -89,7 +88,7 @@
X509_CINF_free(x509->cert_info);
X509_ALGOR_free(x509->sig_alg);
- ASN1_BIT_STRING_free(x509->signature);
+ asn1_string_cleanup(&x509->signature);
ASN1_OCTET_STRING_free(x509->skid);
AUTHORITY_KEYID_free(x509->akid);
CRL_DIST_POINTS_free(x509->crldp);
@@ -102,40 +101,29 @@
}
static X509 *x509_parse(CBS *cbs, CRYPTO_BUFFER *buf) {
- CBS cert, tbs, sigalg, sig;
+ bssl::UniquePtr<X509> ret(x509_new_null());
+ if (ret == nullptr) {
+ return nullptr;
+ }
+
+ CBS cert, tbs, sigalg;
if (!CBS_get_asn1(cbs, &cert, CBS_ASN1_SEQUENCE) ||
// Bound the length to comfortably fit in an int. Lengths in this
// module often omit overflow checks.
CBS_len(&cert) > INT_MAX / 2 ||
!CBS_get_asn1_element(&cert, &tbs, CBS_ASN1_SEQUENCE) ||
- !CBS_get_asn1_element(&cert, &sigalg, CBS_ASN1_SEQUENCE)) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
- return nullptr;
- }
-
- // For just the signature field, we accept non-minimal BER lengths, though not
- // indefinite-length encoding. See b/18228011.
- //
- // TODO(crbug.com/boringssl/354): Switch the affected callers to convert the
- // certificate before parsing and then remove this workaround.
- CBS_ASN1_TAG tag;
- size_t header_len;
- int indefinite;
- if (!CBS_get_any_ber_asn1_element(&cert, &sig, &tag, &header_len,
- /*out_ber_found=*/nullptr,
- &indefinite) ||
- tag != CBS_ASN1_BITSTRING || indefinite || //
- !CBS_skip(&sig, header_len) || //
+ !CBS_get_asn1_element(&cert, &sigalg, CBS_ASN1_SEQUENCE) ||
+ // For just the signature field, we accept non-minimal BER lengths, though
+ // not indefinite-length encoding. See b/18228011.
+ //
+ // TODO(crbug.com/boringssl/354): Switch the affected callers to convert
+ // the certificate before parsing and then remove this workaround.
+ !asn1_parse_bit_string_with_bad_length(&cert, &ret->signature) ||
CBS_len(&cert) != 0) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
return nullptr;
}
- bssl::UniquePtr<X509> ret(x509_new_null());
- if (ret == nullptr) {
- return nullptr;
- }
-
// TODO(crbug.com/boringssl/443): When the rest of the library is decoupled
// from the tasn_*.c implementation, replace this with |CBS|-based
// functions.
@@ -153,12 +141,6 @@
return nullptr;
}
- inp = CBS_data(&sig);
- ret->signature = c2i_ASN1_BIT_STRING(nullptr, &inp, CBS_len(&sig));
- if (ret->signature == nullptr || inp != CBS_data(&sig) + CBS_len(&sig)) {
- return nullptr;
- }
-
// The version must be one of v1(0), v2(1), or v3(2).
long version = X509_VERSION_1;
if (ret->cert_info->version != nullptr) {
@@ -233,7 +215,7 @@
!CBB_add_space(&cert, &out, static_cast<size_t>(len)) ||
i2d_X509_CINF(x509->cert_info, &out) != len ||
!x509_marshal_algorithm(&cert, x509->sig_alg) ||
- !asn1_marshal_bit_string(&cert, x509->signature, /*tag=*/0)) {
+ !asn1_marshal_bit_string(&cert, &x509->signature, /*tag=*/0)) {
return -1;
}
@@ -462,18 +444,18 @@
}
int X509_set1_signature_value(X509 *x509, const uint8_t *sig, size_t sig_len) {
- if (!ASN1_STRING_set(x509->signature, sig, sig_len)) {
+ if (!ASN1_STRING_set(&x509->signature, sig, sig_len)) {
return 0;
}
- x509->signature->flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07);
- x509->signature->flags |= ASN1_STRING_FLAG_BITS_LEFT;
+ x509->signature.flags &= ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07);
+ x509->signature.flags |= ASN1_STRING_FLAG_BITS_LEFT;
return 1;
}
void X509_get0_signature(const ASN1_BIT_STRING **psig, const X509_ALGOR **palg,
const X509 *x) {
if (psig) {
- *psig = x->signature;
+ *psig = &x->signature;
}
if (palg) {
*palg = x->sig_alg;
