yggdrasil/ring
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Reimplement X509 parsing without templates

Browse Source 
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>
This commit is contained in:
David Benjamin 2023-03-16 01:01:27 -04:00 committed by Boringssl LUCI CQ
parent 172b291d3d
commit 2a52444f9d
4 changed files with 274 additions and 101 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
crypto/x509/x509_test.cc
View File

@ -4134,6 +4134,34 @@ soBsxWI=
-----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 @@ TEST(X509Test, BER) {
// 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) {

37
crypto/x509/x_all.c
View File

@ -130,22 +130,6 @@ int NETSCAPE_SPKI_verify(NETSCAPE_SPKI *spki, EVP_PKEY *pkey) {
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 @@ int i2d_X509_REQ_bio(BIO *bp, X509_REQ *req) {
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 @@ IMPLEMENT_I2D_FP(RSA, i2d_RSA_PUBKEY_fp, i2d_RSA_PUBKEY_bio)
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 @@ int X509_pubkey_digest(const X509 *data, const EVP_MD *type, unsigned char *md,
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,

301
crypto/x509/x_x509.c
View File

@ -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_enc(X509_CINF, enc, 0) = {
} 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;
}

4
include/openssl/x509.h
View File

@ -118,10 +118,6 @@ extern "C" {
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);