Reimplement X509 parsing without templates
This is a cursory conversion and is, currently, very tedious because it
needs to bridge calling conventions. After tasn_*.c and all the
underlying primitives have CBS/CBB-based calling conventions, this
should be a lot cleaner.
This is to break a dependency cycle:
- We'd like to rewrite d2i_X509 with CBS
- To do that, we need to rewrite its underlying types with CBS
- Those parsers are tied up in tasn_dec.c, so we effectively need to
rewrite tasn_dec.c with CBS.
- CBS is designed for DER, not BER, so such a change would most
naturally switch the TLV parser to require DER.
- We've *almost* done that already except
https://boringssl-review.googlesource.com/c/boringssl/+/51626 had to
stop at non-minimal definite lengths, which are allowed in BER but
forbidden in DER. See b/18228011 for a bunch of certificates which
have a non-minimal definite length at *just* the signature field.
- So, to do that, we'd ideally special case just that field, or BIT
STRINGs in general, to tolerate minimal lengths. That's easiest done
when d2i_X509 is CBS, so we can just do what we want in imperative
code. And thus we're back full circle.
So, detach X509 from the templates now. It's a bit tedious because we
need to switch calling conventions for now, but it breaks the cycle.
Later, we can revisit this and get all the benefits of a fully CBS-based
path.
For now, I haven't added an ASN1_ITEM. If it comes up, we can make an
EXTERN ASN1_ITEM.
Update-Note: The ASN1_ITEM removal means custom ASN.1 templates (which
are discouraged in favor of our much simpler CBS and CBB types) using
X509 will fail to compile. We don't believe anyone is relying on this,
but this can be restored if we find something.
Update-Note: Certificate parsing is slightly stricter: the outermost
TLVs, except for the signature field, no longer accept non-minimal
lengths, as mandated by DER. This strictness was broadly already applied
by the libssl parser.
Bug: 547
Change-Id: Ie5ad8ba4bb39f54fdd3dd45c53965b72a3850709
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58185
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 737cb10..8adba6c 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -4134,6 +4134,34 @@
-----END CERTIFICATE-----
)";
+// kNonMinimalLengthOuter is an X.509 certificate where the outermost SEQUENCE
+// has a non-minimal length.
+static const char kNonMinimalLengthOuter[] = R"(
+-----BEGIN CERTIFICATE-----
+MIMAASAwgcagAwIBAgICBNIwCgYIKoZIzj0EAwIwDzENMAsGA1UEAxMEVGVzdDAg
+Fw0wMDAxMDEwMDAwMDBaGA8yMTAwMDEwMTAwMDAwMFowDzENMAsGA1UEAxMEVGVz
+dDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABOYraeK/ZZ+Xvi8eDZSKTNWXa7ep
+Hg1G+92pqR6d3LpaAefWl6gKGPnDxKMeVuJ8g0jbFhoc9R1+8ZQtS89yIsGjEDAO
+MAwGA1UdEwQFMAMBAf8wCgYIKoZIzj0EAwIDSQAwRgIhAKnSIhfmzfQpeOKFHiAq
+cml3ex6oaVVGoJWCsPQoZjVAAiEAqTHS9HzZBTQ20cMPXUpf8u5AXZP7adeh4qnk
+soBsxWI=
+-----END CERTIFICATE-----
+)";
+
+// kNonMinimalLengthSignature is an X.509 certificate where the signature
+// has a non-minimal length.
+static const char kNonMinimalLengthSignature[] = R"(
+-----BEGIN CERTIFICATE-----
+MIIBITCBxqADAgECAgIE0jAKBggqhkjOPQQDAjAPMQ0wCwYDVQQDEwRUZXN0MCAX
+DTAwMDEwMTAwMDAwMFoYDzIxMDAwMTAxMDAwMDAwWjAPMQ0wCwYDVQQDEwRUZXN0
+MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE5itp4r9ln5e+Lx4NlIpM1Zdrt6ke
+DUb73ampHp3culoB59aXqAoY+cPEox5W4nyDSNsWGhz1HX7xlC1Lz3IiwaMQMA4w
+DAYDVR0TBAUwAwEB/zAKBggqhkjOPQQDAgOBSQAwRgIhAKnSIhfmzfQpeOKFHiAq
+cml3ex6oaVVGoJWCsPQoZjVAAiEAqTHS9HzZBTQ20cMPXUpf8u5AXZP7adeh4qnk
+soBsxWI=
+-----END CERTIFICATE-----
+)";
+
TEST(X509Test, BER) {
// Constructed strings are forbidden in DER.
EXPECT_FALSE(CertFromPEM(kConstructedBitString));
@@ -4145,6 +4173,11 @@
// Tags must be minimal in both BER and DER, though many BER decoders
// incorrectly support non-minimal tags.
EXPECT_FALSE(CertFromPEM(kHighTagNumber));
+ // Lengths must be minimal in DER.
+ EXPECT_FALSE(CertFromPEM(kNonMinimalLengthOuter));
+ // We, for now, accept a non-minimal length in the signature field. See
+ // b/18228011.
+ EXPECT_TRUE(CertFromPEM(kNonMinimalLengthSignature));
}
TEST(X509Test, Names) {
diff --git a/crypto/x509/x_all.c b/crypto/x509/x_all.c
index 23508c0..6808ab7 100644
--- a/crypto/x509/x_all.c
+++ b/crypto/x509/x_all.c
@@ -130,22 +130,6 @@
spki->signature, spki->spkac, pkey));
}
-X509 *d2i_X509_fp(FILE *fp, X509 **x509) {
- return ASN1_item_d2i_fp(ASN1_ITEM_rptr(X509), fp, x509);
-}
-
-int i2d_X509_fp(FILE *fp, X509 *x509) {
- return ASN1_item_i2d_fp(ASN1_ITEM_rptr(X509), fp, x509);
-}
-
-X509 *d2i_X509_bio(BIO *bp, X509 **x509) {
- return ASN1_item_d2i_bio(ASN1_ITEM_rptr(X509), bp, x509);
-}
-
-int i2d_X509_bio(BIO *bp, X509 *x509) {
- return ASN1_item_i2d_bio(ASN1_ITEM_rptr(X509), bp, x509);
-}
-
X509_CRL *d2i_X509_CRL_fp(FILE *fp, X509_CRL **crl) {
return ASN1_item_d2i_fp(ASN1_ITEM_rptr(X509_CRL), fp, crl);
}
@@ -201,6 +185,9 @@
return ret; \
}
+IMPLEMENT_D2I_FP(X509, d2i_X509_fp, d2i_X509_bio);
+IMPLEMENT_I2D_FP(X509, i2d_X509_fp, i2d_X509_bio);
+
IMPLEMENT_D2I_FP(RSA, d2i_RSAPrivateKey_fp, d2i_RSAPrivateKey_bio)
IMPLEMENT_I2D_FP(RSA, i2d_RSAPrivateKey_fp, i2d_RSAPrivateKey_bio)
@@ -235,6 +222,9 @@
return ret; \
}
+IMPLEMENT_D2I_BIO(X509, d2i_X509_bio, d2i_X509)
+IMPLEMENT_I2D_BIO(X509, i2d_X509_bio, i2d_X509)
+
IMPLEMENT_D2I_BIO(RSA, d2i_RSAPrivateKey_bio, d2i_RSAPrivateKey)
IMPLEMENT_I2D_BIO(RSA, i2d_RSAPrivateKey_bio, i2d_RSAPrivateKey)
@@ -278,9 +268,18 @@
return EVP_Digest(key->data, key->length, md, len, type, NULL);
}
-int X509_digest(const X509 *data, const EVP_MD *type, unsigned char *md,
- unsigned int *len) {
- return (ASN1_item_digest(ASN1_ITEM_rptr(X509), type, (char *)data, md, len));
+int X509_digest(const X509 *x509, const EVP_MD *md, uint8_t *out,
+ unsigned *out_len) {
+ uint8_t *der = NULL;
+ // TODO(https://crbug.com/boringssl/407): This function is not const-correct.
+ int der_len = i2d_X509((X509 *)x509, &der);
+ if (der_len < 0) {
+ return 0;
+ }
+
+ int ret = EVP_Digest(der, der_len, out, out_len, md, NULL);
+ OPENSSL_free(der);
+ return ret;
}
int X509_CRL_digest(const X509_CRL *data, const EVP_MD *type, unsigned char *md,
diff --git a/crypto/x509/x_x509.c b/crypto/x509/x_x509.c
index 5205267..1aa2d2e 100644
--- a/crypto/x509/x_x509.c
+++ b/crypto/x509/x_x509.c
@@ -68,6 +68,7 @@
#include <openssl/x509v3.h>
#include "../asn1/internal.h"
+#include "../bytestring/internal.h"
#include "../internal.h"
#include "internal.h"
@@ -87,93 +88,237 @@
} ASN1_SEQUENCE_END_enc(X509_CINF, X509_CINF)
IMPLEMENT_ASN1_FUNCTIONS(X509_CINF)
-// X509 top level structure needs a bit of customisation
-static int x509_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
- void *exarg) {
- X509 *ret = (X509 *)*pval;
-
- switch (operation) {
- case ASN1_OP_NEW_POST:
- ret->ex_flags = 0;
- ret->ex_pathlen = -1;
- ret->skid = NULL;
- ret->akid = NULL;
- ret->aux = NULL;
- ret->crldp = NULL;
- CRYPTO_new_ex_data(&ret->ex_data);
- CRYPTO_MUTEX_init(&ret->lock);
- break;
-
- case ASN1_OP_D2I_POST: {
- // The version must be one of v1(0), v2(1), or v3(2).
- long version = X509_VERSION_1;
- if (ret->cert_info->version != NULL) {
- version = ASN1_INTEGER_get(ret->cert_info->version);
- // TODO(https://crbug.com/boringssl/364): |X509_VERSION_1| should
- // also be rejected here. This means an explicitly-encoded X.509v1
- // version. v1 is DEFAULT, so DER requires it be omitted.
- if (version < X509_VERSION_1 || version > X509_VERSION_3) {
- OPENSSL_PUT_ERROR(X509, X509_R_INVALID_VERSION);
- return 0;
- }
- }
-
- // Per RFC 5280, section 4.1.2.8, these fields require v2 or v3.
- if (version == X509_VERSION_1 && (ret->cert_info->issuerUID != NULL ||
- ret->cert_info->subjectUID != NULL)) {
- OPENSSL_PUT_ERROR(X509, X509_R_INVALID_FIELD_FOR_VERSION);
- return 0;
- }
-
- // Per RFC 5280, section 4.1.2.9, extensions require v3.
- if (version != X509_VERSION_3 && ret->cert_info->extensions != NULL) {
- OPENSSL_PUT_ERROR(X509, X509_R_INVALID_FIELD_FOR_VERSION);
- return 0;
- }
-
- break;
- }
-
- case ASN1_OP_FREE_POST:
- CRYPTO_MUTEX_cleanup(&ret->lock);
- CRYPTO_free_ex_data(&g_ex_data_class, ret, &ret->ex_data);
- X509_CERT_AUX_free(ret->aux);
- ASN1_OCTET_STRING_free(ret->skid);
- AUTHORITY_KEYID_free(ret->akid);
- CRL_DIST_POINTS_free(ret->crldp);
- GENERAL_NAMES_free(ret->altname);
- NAME_CONSTRAINTS_free(ret->nc);
- break;
+// x509_new_null returns a new |X509| object where the |cert_info|, |sig_alg|,
+// and |signature| fields are not yet filled in.
+static X509 *x509_new_null(void) {
+ X509 *ret = OPENSSL_malloc(sizeof(X509));
+ if (ret == NULL) {
+ return NULL;
}
+ OPENSSL_memset(ret, 0, sizeof(X509));
- return 1;
+ ret->references = 1;
+ ret->ex_pathlen = -1;
+ CRYPTO_new_ex_data(&ret->ex_data);
+ CRYPTO_MUTEX_init(&ret->lock);
+ return ret;
}
-ASN1_SEQUENCE_ref(X509, x509_cb) = {
- ASN1_SIMPLE(X509, cert_info, X509_CINF),
- ASN1_SIMPLE(X509, sig_alg, X509_ALGOR),
- ASN1_SIMPLE(X509, signature, ASN1_BIT_STRING),
-} ASN1_SEQUENCE_END_ref(X509, X509)
-
-IMPLEMENT_ASN1_FUNCTIONS(X509)
-
-IMPLEMENT_ASN1_DUP_FUNCTION(X509)
-
-X509 *X509_parse_from_buffer(CRYPTO_BUFFER *buf) {
- if (CRYPTO_BUFFER_len(buf) > LONG_MAX) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW);
+X509 *X509_new(void) {
+ X509 *ret = x509_new_null();
+ if (ret == NULL) {
return NULL;
}
- // 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 *ret = NULL;
- if (ASN1_item_ex_d2i((ASN1_VALUE **)&ret, &inp, CRYPTO_BUFFER_len(buf),
- ASN1_ITEM_rptr(X509), /*tag=*/-1,
+ 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) {
+ X509_free(ret);
+ return NULL;
+ }
+
+ return ret;
+}
+
+void X509_free(X509 *x509) {
+ if (x509 == NULL || !CRYPTO_refcount_dec_and_test_zero(&x509->references)) {
+ return;
+ }
+
+ CRYPTO_free_ex_data(&g_ex_data_class, x509, &x509->ex_data);
+
+ X509_CINF_free(x509->cert_info);
+ X509_ALGOR_free(x509->sig_alg);
+ ASN1_BIT_STRING_free(x509->signature);
+ ASN1_OCTET_STRING_free(x509->skid);
+ AUTHORITY_KEYID_free(x509->akid);
+ CRL_DIST_POINTS_free(x509->crldp);
+ GENERAL_NAMES_free(x509->altname);
+ NAME_CONSTRAINTS_free(x509->nc);
+ X509_CERT_AUX_free(x509->aux);
+ CRYPTO_MUTEX_cleanup(&x509->lock);
+
+ OPENSSL_free(x509);
+}
+
+static X509 *x509_parse(CBS *cbs, CRYPTO_BUFFER *buf) {
+ CBS cert, tbs, sigalg, sig;
+ 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 NULL;
+ }
+
+ // 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=*/NULL,
+ &indefinite) ||
+ tag != CBS_ASN1_BITSTRING || indefinite || //
+ !CBS_skip(&sig, header_len) || //
+ CBS_len(&cert) != 0) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
+ return NULL;
+ }
+
+ X509 *ret = x509_new_null();
+ if (ret == NULL) {
+ return NULL;
+ }
+
+ // TODO(crbug.com/boringssl/443): When the rest of the library is decoupled
+ // from the tasn_*.c implementation, replace this with |CBS|-based functions.
+ const uint8_t *inp = CBS_data(&tbs);
+ if (ASN1_item_ex_d2i((ASN1_VALUE **)&ret->cert_info, &inp, CBS_len(&tbs),
+ ASN1_ITEM_rptr(X509_CINF), /*tag=*/-1,
/*aclass=*/0, /*opt=*/0, buf) <= 0 ||
- inp != CRYPTO_BUFFER_data(buf) + CRYPTO_BUFFER_len(buf)) {
+ inp != CBS_data(&tbs) + CBS_len(&tbs)) {
+ goto err;
+ }
+
+ inp = CBS_data(&sigalg);
+ ret->sig_alg = d2i_X509_ALGOR(NULL, &inp, CBS_len(&sigalg));
+ if (ret->sig_alg == NULL || inp != CBS_data(&sigalg) + CBS_len(&sigalg)) {
+ goto err;
+ }
+
+ inp = CBS_data(&sig);
+ ret->signature = c2i_ASN1_BIT_STRING(NULL, &inp, CBS_len(&sig));
+ if (ret->signature == NULL || inp != CBS_data(&sig) + CBS_len(&sig)) {
+ goto err;
+ }
+
+ // The version must be one of v1(0), v2(1), or v3(2).
+ long version = X509_VERSION_1;
+ if (ret->cert_info->version != NULL) {
+ version = ASN1_INTEGER_get(ret->cert_info->version);
+ // TODO(https://crbug.com/boringssl/364): |X509_VERSION_1| should
+ // also be rejected here. This means an explicitly-encoded X.509v1
+ // version. v1 is DEFAULT, so DER requires it be omitted.
+ if (version < X509_VERSION_1 || version > X509_VERSION_3) {
+ OPENSSL_PUT_ERROR(X509, X509_R_INVALID_VERSION);
+ goto err;
+ }
+ }
+
+ // Per RFC 5280, section 4.1.2.8, these fields require v2 or v3.
+ if (version == X509_VERSION_1 && (ret->cert_info->issuerUID != NULL ||
+ ret->cert_info->subjectUID != NULL)) {
+ OPENSSL_PUT_ERROR(X509, X509_R_INVALID_FIELD_FOR_VERSION);
+ goto err;
+ }
+
+ // Per RFC 5280, section 4.1.2.9, extensions require v3.
+ if (version != X509_VERSION_3 && ret->cert_info->extensions != NULL) {
+ OPENSSL_PUT_ERROR(X509, X509_R_INVALID_FIELD_FOR_VERSION);
+ goto err;
+ }
+
+ return ret;
+
+err:
+ X509_free(ret);
+ return NULL;
+}
+
+X509 *d2i_X509(X509 **out, const uint8_t **inp, long len) {
+ X509 *ret = NULL;
+ if (len < 0) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_BUFFER_TOO_SMALL);
+ goto err;
+ }
+
+ CBS cbs;
+ CBS_init(&cbs, *inp, (size_t)len);
+ ret = x509_parse(&cbs, NULL);
+ if (ret == NULL) {
+ goto err;
+ }
+
+ *inp = CBS_data(&cbs);
+
+err:
+ if (out != NULL) {
+ X509_free(*out);
+ *out = ret;
+ }
+ return ret;
+}
+
+int i2d_X509(X509 *x509, uint8_t **outp) {
+ if (x509 == NULL) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_MISSING_VALUE);
+ return -1;
+ }
+
+ CBB cbb, cert;
+ if (!CBB_init(&cbb, 64) || //
+ !CBB_add_asn1(&cbb, &cert, CBS_ASN1_SEQUENCE)) {
+ goto err;
+ }
+
+ // TODO(crbug.com/boringssl/443): When the rest of the library is decoupled
+ // from the tasn_*.c implementation, replace this with |CBS|-based functions.
+ uint8_t *out;
+ int len = i2d_X509_CINF(x509->cert_info, NULL);
+ if (len < 0 || //
+ !CBB_add_space(&cert, &out, (size_t)len) ||
+ i2d_X509_CINF(x509->cert_info, &out) != len) {
+ goto err;
+ }
+
+ len = i2d_X509_ALGOR(x509->sig_alg, NULL);
+ if (len < 0 || //
+ !CBB_add_space(&cert, &out, (size_t)len) ||
+ i2d_X509_ALGOR(x509->sig_alg, &out) != len) {
+ goto err;
+ }
+
+ len = i2d_ASN1_BIT_STRING(x509->signature, NULL);
+ if (len < 0 || //
+ !CBB_add_space(&cert, &out, (size_t)len) ||
+ i2d_ASN1_BIT_STRING(x509->signature, &out) != len) {
+ goto err;
+ }
+
+ return CBB_finish_i2d(&cbb, outp);
+
+err:
+ CBB_cleanup(&cbb);
+ return -1;
+}
+
+X509 *X509_dup(X509 *x509) {
+ uint8_t *der = NULL;
+ int len = i2d_X509(x509, &der);
+ if (len < 0) {
+ return NULL;
+ }
+
+ const uint8_t *inp = der;
+ X509 *ret = d2i_X509(NULL, &inp, len);
+ OPENSSL_free(der);
+ return ret;
+}
+
+X509 *X509_parse_from_buffer(CRYPTO_BUFFER *buf) {
+ CBS cbs;
+ CBS_init(&cbs, CRYPTO_BUFFER_data(buf), CRYPTO_BUFFER_len(buf));
+ X509 *ret = x509_parse(&cbs, buf);
+ if (ret == NULL || CBS_len(&cbs) != 0) {
X509_free(ret);
return NULL;
}
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 4141007..9a49b40 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -118,10 +118,6 @@
DEFINE_STACK_OF(X509)
-// X509 is an |ASN1_ITEM| whose ASN.1 type is X.509 Certificate (RFC 5280) and C
-// type is |X509*|.
-DECLARE_ASN1_ITEM(X509)
-
// X509_up_ref adds one to the reference count of |x509| and returns one.
OPENSSL_EXPORT int X509_up_ref(X509 *x509);
