From 89d1241487fb6b715f6516a1f107e846dd760a85 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 25 Feb 2021 22:20:25 -0500 Subject: [PATCH] Define X509V*_VERSION constants. The X509 version APIs confuse everyone, including our own tests (v3name_test.cc was using the wrong value). Introduce constants. Also document which version to use for each type. It is extra confusing that, although certificates and CRLs use the same Version enum, RFC5280 defines X.509 v3 certificates with X.509 v2 CRLs. (There are no v3 CRLs.) I'll send a similar patch to OpenSSL to keep upstream aligned. Change-Id: If096138eb004156d43df87a6d74f695b04f67480 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45944 Reviewed-by: Adam Langley Commit-Queue: David Benjamin --- crypto/x509/x509_cmp.c | 4 +-- crypto/x509/x509_set.c | 5 ++-- crypto/x509/x509_test.cc | 2 +- crypto/x509/x509_vfy.c | 2 +- crypto/x509v3/v3_purp.c | 2 +- crypto/x509v3/v3name_test.cc | 2 +- include/openssl/x509.h | 56 ++++++++++++++++++++++++++---------- 7 files changed, 50 insertions(+), 23 deletions(-) diff --git a/crypto/x509/x509_cmp.c b/crypto/x509/x509_cmp.c index c9060ddf7..a4f07281a 100644 --- a/crypto/x509/x509_cmp.c +++ b/crypto/x509/x509_cmp.c @@ -383,7 +383,7 @@ int X509_chain_check_suiteb(int *perror_depth, X509 *x, STACK_OF(X509) *chain, } else i = 0; - if (X509_get_version(x) != 2) { + if (X509_get_version(x) != X509V3_VERSION) { rv = X509_V_ERR_SUITE_B_INVALID_VERSION; /* Correct error depth */ i = 0; @@ -401,7 +401,7 @@ int X509_chain_check_suiteb(int *perror_depth, X509 *x, STACK_OF(X509) *chain, for (; i < sk_X509_num(chain); i++) { sign_nid = X509_get_signature_nid(x); x = sk_X509_value(chain, i); - if (X509_get_version(x) != 2) { + if (X509_get_version(x) != X509V3_VERSION) { rv = X509_V_ERR_SUITE_B_INVALID_VERSION; goto end; } diff --git a/crypto/x509/x509_set.c b/crypto/x509/x509_set.c index 9193b2084..4a57fb6f4 100644 --- a/crypto/x509/x509_set.c +++ b/crypto/x509/x509_set.c @@ -64,7 +64,7 @@ long X509_get_version(const X509 *x509) { // The default version is v1(0). if (x509->cert_info->version == NULL) { - return 0; + return X509V1_VERSION; } return ASN1_INTEGER_get(x509->cert_info->version); } @@ -76,6 +76,7 @@ X509_CINF *X509_get_cert_info(const X509 *x509) int X509_set_version(X509 *x, long version) { + // TODO(davidben): Reject invalid version numbers. if (x == NULL) return (0); if (version == 0) { @@ -90,7 +91,7 @@ int X509_set_version(X509 *x, long version) return (ASN1_INTEGER_set(x->cert_info->version, version)); } -int X509_set_serialNumber(X509 *x, ASN1_INTEGER *serial) +int X509_set_serialNumber(X509 *x, const ASN1_INTEGER *serial) { ASN1_INTEGER *in; diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 3a6740fcc..918f615bd 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -1599,7 +1599,7 @@ TEST(X509Test, RSASignManual) { if (new_cert) { cert.reset(X509_new()); // Fill in some fields for the certificate arbitrarily. - EXPECT_TRUE(X509_set_version(cert.get(), 2 /* X.509v3 */)); + EXPECT_TRUE(X509_set_version(cert.get(), X509V3_VERSION)); EXPECT_TRUE(ASN1_INTEGER_set(X509_get_serialNumber(cert.get()), 1)); EXPECT_TRUE(X509_gmtime_adj(X509_getm_notBefore(cert.get()), 0)); EXPECT_TRUE( diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index a997202e8..1b533bfcd 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -2052,7 +2052,7 @@ X509_CRL *X509_CRL_diff(X509_CRL *base, X509_CRL *newer, } /* Create new CRL */ crl = X509_CRL_new(); - if (!crl || !X509_CRL_set_version(crl, 1)) + if (!crl || !X509_CRL_set_version(crl, X509V2_VERSION)) goto memerr; /* Set issuer name */ if (!X509_CRL_set_issuer_name(crl, X509_CRL_get_issuer(newer))) diff --git a/crypto/x509v3/v3_purp.c b/crypto/x509v3/v3_purp.c index acb760250..66bf8daab 100644 --- a/crypto/x509v3/v3_purp.c +++ b/crypto/x509v3/v3_purp.c @@ -440,7 +440,7 @@ int x509v3_cache_extensions(X509 *x) if (!X509_digest(x, EVP_sha1(), x->sha1_hash, NULL)) x->ex_flags |= EXFLAG_INVALID; /* V1 should mean no extensions ... */ - if (!X509_get_version(x)) + if (X509_get_version(x) == X509V1_VERSION) x->ex_flags |= EXFLAG_V1; /* Handle basic constraints */ if ((bs = X509_get_ext_d2i(x, NID_basic_constraints, &j, NULL))) { diff --git a/crypto/x509v3/v3name_test.cc b/crypto/x509v3/v3name_test.cc index 2dcdd87c4..ca918a9c6 100644 --- a/crypto/x509v3/v3name_test.cc +++ b/crypto/x509v3/v3name_test.cc @@ -305,7 +305,7 @@ static X509 *make_cert(void) crt = X509_new(); if (crt == NULL) goto out; - if (!X509_set_version(crt, 3)) + if (!X509_set_version(crt, X509V3_VERSION)) goto out; ret = crt; crt = NULL; diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 49de43353..01842e189 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -475,16 +475,34 @@ extern "C" { // it is safe to call mutating functions is a little tricky due to various // internal caches. -// X509_get_version returns the numerical value of |x509|'s version. That is, -// it returns zero for X.509v1, one for X.509v2, and two for X.509v3. Unknown -// versions are rejected by the parser, but a manually-created |X509| object may -// encode invalid versions. In that case, the function will return the invalid -// version, or -1 on overflow. +// The following constants are version numbers of X.509-related structures. Note +// APIs typically return the numerical value of X.509 versions, which are one +// less than the named version. +#define X509V1_VERSION 0 +#define X509V2_VERSION 1 +#define X509V3_VERSION 2 + +// X509_get_version returns the numerical value of |x509|'s version. Callers may +// compare the result to the |X509V*_VERSION| constants. Unknown versions are +// rejected by the parser, but a manually-created |X509| object may encode +// invalid versions. In that case, the function will return the invalid version, +// or -1 on overflow. OPENSSL_EXPORT long X509_get_version(const X509 *x509); +// X509_set_version sets |x509|'s version to |version|, which should be one of +// the |X509V*_VERSION| constants. It returns one on success and zero on error. +// +// If unsure, use |X509V3_VERSION|. +OPENSSL_EXPORT int X509_set_version(X509 *x509, long version); + // X509_get0_serialNumber returns |x509|'s serial number. OPENSSL_EXPORT const ASN1_INTEGER *X509_get0_serialNumber(const X509 *x509); +// X509_set_serialNumber sets |x509|'s serial number to |serial|. It returns one +// on success and zero on error. +OPENSSL_EXPORT int X509_set_serialNumber(X509 *x509, + const ASN1_INTEGER *serial); + // X509_get0_notBefore returns |x509|'s notBefore time. OPENSSL_EXPORT const ASN1_TIME *X509_get0_notBefore(const X509 *x509); @@ -550,9 +568,9 @@ OPENSSL_EXPORT X509_CINF *X509_get_cert_info(const X509 *x509); // |EXFLAG_INVALID| bit. OPENSSL_EXPORT long X509_get_pathlen(X509 *x509); -// X509_REQ_get_version returns the numerical value of |req|'s version. That is, -// it returns zero for a v1 request. If |req| is invalid, it may return another -// value, or -1 on overflow. +// X509_REQ_get_version returns the numerical value of |req|'s version. Callers +// may compare the result to |X509V*_VERSION| constants. If |req| is invalid, it +// may return another value, or -1 on overflow. OPENSSL_EXPORT long X509_REQ_get_version(const X509_REQ *req); // X509_REQ_get_subject_name returns |req|'s subject name. Note this function is @@ -565,9 +583,9 @@ OPENSSL_EXPORT X509_NAME *X509_REQ_get_subject_name(const X509_REQ *req); // X509_name_cmp is a legacy alias for |X509_NAME_cmp|. #define X509_name_cmp(a, b) X509_NAME_cmp((a), (b)) -// X509_REQ_get_version returns the numerical value of |crl|'s version. That is, -// it returns zero for a v1 CRL and one for a v2 CRL. If |crl| is invalid, it -// may return another value, or -1 on overflow. +// X509_CRL_get_version returns the numerical value of |crl|'s version. Callers +// may compare the result to |X509V*_VERSION| constants. If |crl| is invalid, +// it may return another value, or -1 on overflow. OPENSSL_EXPORT long X509_CRL_get_version(const X509_CRL *crl); // X509_CRL_get0_lastUpdate returns |crl|'s lastUpdate time. @@ -1071,8 +1089,6 @@ OPENSSL_EXPORT int ASN1_item_sign_ctx(const ASN1_ITEM *it, X509_ALGOR *algor1, ASN1_BIT_STRING *signature, void *asn, EVP_MD_CTX *ctx); -OPENSSL_EXPORT int X509_set_version(X509 *x, long version); -OPENSSL_EXPORT int X509_set_serialNumber(X509 *x, ASN1_INTEGER *serial); OPENSSL_EXPORT ASN1_INTEGER *X509_get_serialNumber(X509 *x); OPENSSL_EXPORT int X509_set_issuer_name(X509 *x, X509_NAME *name); OPENSSL_EXPORT X509_NAME *X509_get_issuer_name(const X509 *a); @@ -1085,7 +1101,11 @@ OPENSSL_EXPORT const STACK_OF(X509_EXTENSION) *X509_get0_extensions( const X509 *x); OPENSSL_EXPORT const X509_ALGOR *X509_get0_tbs_sigalg(const X509 *x); -OPENSSL_EXPORT int X509_REQ_set_version(X509_REQ *x, long version); +// X509_REQ_set_version sets |req|'s version to |version|, which should be +// |X509V1_VERSION|. It returns one on success and zero on error. +// +// Note no versions other than |X509V1_VERSION| are defined for CSRs. +OPENSSL_EXPORT int X509_REQ_set_version(X509_REQ *req, long version); OPENSSL_EXPORT int X509_REQ_set_subject_name(X509_REQ *req, X509_NAME *name); OPENSSL_EXPORT void X509_REQ_get0_signature(const X509_REQ *req, const ASN1_BIT_STRING **psig, @@ -1123,7 +1143,13 @@ OPENSSL_EXPORT int X509_REQ_add1_attr_by_txt(X509_REQ *req, const unsigned char *bytes, int len); -OPENSSL_EXPORT int X509_CRL_set_version(X509_CRL *x, long version); +// X509_CRL_set_version sets |crl|'s version to |version|, which should be one +// of the |X509V*_VERSION| constants. It returns one on success and zero on +// error. +// +// If unsure, use |X509V2_VERSION|. Note |X509V3_VERSION| is not defined for +// CRLs. +OPENSSL_EXPORT int X509_CRL_set_version(X509_CRL *crl, long version); OPENSSL_EXPORT int X509_CRL_set_issuer_name(X509_CRL *x, X509_NAME *name); OPENSSL_EXPORT int X509_CRL_sort(X509_CRL *crl); OPENSSL_EXPORT int X509_CRL_up_ref(X509_CRL *crl);