Enforce X.509 version invariants more consistently.

This aligns X509_REQ's and X509_CRL's parsers to the changes already made with X509; we reject invalid versions and check that extensions are only with the corresponding version. For now, we still allow X509v1 CRLs with an explicit version, matching certificates. (The DEFAULT question is moot for X509_REQ because CSRs always encode their version, see RFC 2986.) In addition to rejecting garbage, this allows for a more efficient representation once we stop using the table-based parser: X509 and X509_CRL can just store a small enum. X509_REQ doesn't need to store anything because the single version is information-less. Update-Note: Invalid CRL and CSR versions will no longer be accepted. X509_set_version, etc., no longer allow invalid versions. Fixed: 467 Change-Id: I33f3aec747d8060ab80e0cbb8ddf97672e07642c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52605 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
weijun910909 · May 19, 2022 · 7fd831c · 7fd831c
1 parent 5a79788
commit 7fd831c
Show file tree

Hide file tree

Showing 11 changed files with 189 additions and 68 deletions.
diff --git a/crypto/x509/t_crl.c b/crypto/x509/t_crl.c
@@ -54,6 +54,8 @@
  * copied and put under another distribution licence
  * [including the GNU Public Licence.] */
 
+#include <assert.h>
+
 #include <openssl/asn1.h>
 #include <openssl/err.h>
 #include <openssl/mem.h>
@@ -76,13 +78,11 @@ int X509_CRL_print_fp(FILE *fp, X509_CRL *x)
 int X509_CRL_print(BIO *out, X509_CRL *x)
 {
     long version = X509_CRL_get_version(x);
+    assert(X509_CRL_VERSION_1 <= version && version <= X509_CRL_VERSION_2);
     const X509_ALGOR *sig_alg;
     const ASN1_BIT_STRING *signature;
     X509_CRL_get0_signature(x, &signature, &sig_alg);
     if (BIO_printf(out, "Certificate Revocation List (CRL):\n") <= 0 ||
-        // TODO(https://crbug.com/boringssl/467): This loses information on some
-        // invalid versions, but we should fix this by making invalid versions
-        // impossible.
         BIO_printf(out, "%8sVersion %ld (0x%lx)\n", "", version + 1,
                    (unsigned long)version) <= 0 ||
         // Note this and the other |X509_signature_print| call both print the

diff --git a/crypto/x509/t_req.c b/crypto/x509/t_req.c
@@ -54,6 +54,7 @@
  * copied and put under another distribution licence
  * [including the GNU Public Licence.] */
 
+#include <assert.h>
 #include <stdio.h>
 
 #include <openssl/bn.h>
@@ -103,10 +104,8 @@ int X509_REQ_print_ex(BIO *bio, X509_REQ *x, unsigned long nmflags,
     }
   }
   if (!(cflag & X509_FLAG_NO_VERSION)) {
-    /* TODO(https://crbug.com/boringssl/467): This loses information on some
-     * invalid versions, but we should fix this by making invalid versions
-     * impossible. */
     l = X509_REQ_get_version(x);
+    assert(l == X509_REQ_VERSION_1);
     if (BIO_printf(bio, "%8sVersion: %ld (0x%lx)\n", "", l + 1,
                    (unsigned long)l) <= 0) {
       goto err;

diff --git a/crypto/x509/t_x509.c b/crypto/x509/t_x509.c
@@ -119,10 +119,8 @@ int X509_print_ex(BIO *bp, X509 *x, unsigned long nmflags,
             goto err;
     }
     if (!(cflag & X509_FLAG_NO_VERSION)) {
-        /* TODO(https://crbug.com/boringssl/467): This loses information on some
-         * invalid versions, but we should fix this by making invalid versions
-         * impossible. */
         l = X509_get_version(x);
+        assert(X509_VERSION_1 <= l && l <= X509_VERSION_3);
         if (BIO_printf(bp, "%8sVersion: %ld (0x%lx)\n", "", l + 1,
                        (unsigned long)l) <= 0) {
             goto err;

diff --git a/crypto/x509/x509_set.c b/crypto/x509/x509_set.c
@@ -74,19 +74,29 @@ long X509_get_version(const X509 *x509)
 
 int X509_set_version(X509 *x, long version)
 {
-    // TODO(https://crbug.com/boringssl/467): Reject invalid version numbers.
-    if (x == NULL)
-        return (0);
-    if (version == 0) {
+    if (x == NULL) {
+        return 0;
+    }
+
+    if (version < X509_VERSION_1 || version > X509_VERSION_3) {
+        OPENSSL_PUT_ERROR(X509, X509_R_INVALID_VERSION);
+        return 0;
+    }
+
+    /* v1(0) is default and is represented by omitting the version. */
+    if (version == X509_VERSION_1) {
         ASN1_INTEGER_free(x->cert_info->version);
         x->cert_info->version = NULL;
-        return (1);
+        return 1;
     }
+
     if (x->cert_info->version == NULL) {
-        if ((x->cert_info->version = ASN1_INTEGER_new()) == NULL)
-            return (0);
+        x->cert_info->version = ASN1_INTEGER_new();
+        if (x->cert_info->version == NULL) {
+            return 0;
+        }
     }
-    return (ASN1_INTEGER_set(x->cert_info->version, version));
+    return ASN1_INTEGER_set(x->cert_info->version, version);
 }
 
 int X509_set_serialNumber(X509 *x, const ASN1_INTEGER *serial)

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
@@ -1047,23 +1047,31 @@ TXHOSQQD8Dl4BK0wOet+TP6LBEjHlRFjAqK4bu9xpxV2
 -----END CERTIFICATE-----
 )";
 
-// CertFromPEM parses the given, NUL-terminated pem block and returns an
+// CertFromPEM parses the given, NUL-terminated PEM block and returns an
 // |X509*|.
 static bssl::UniquePtr<X509> CertFromPEM(const char *pem) {
   bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(pem, strlen(pem)));
   return bssl::UniquePtr<X509>(
       PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr));
 }
 
-// CRLFromPEM parses the given, NUL-terminated pem block and returns an
+// CRLFromPEM parses the given, NUL-terminated PEM block and returns an
 // |X509_CRL*|.
 static bssl::UniquePtr<X509_CRL> CRLFromPEM(const char *pem) {
   bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(pem, strlen(pem)));
   return bssl::UniquePtr<X509_CRL>(
       PEM_read_bio_X509_CRL(bio.get(), nullptr, nullptr, nullptr));
 }
 
-// PrivateKeyFromPEM parses the given, NUL-terminated pem block and returns an
+// CSRFromPEM parses the given, NUL-terminated PEM block and returns an
+// |X509_REQ*|.
+static bssl::UniquePtr<X509_REQ> CSRFromPEM(const char *pem) {
+  bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(pem, strlen(pem)));
+  return bssl::UniquePtr<X509_REQ>(
+      PEM_read_bio_X509_REQ(bio.get(), nullptr, nullptr, nullptr));
+}
+
+// PrivateKeyFromPEM parses the given, NUL-terminated PEM block and returns an
 // |EVP_PKEY*|.
 static bssl::UniquePtr<EVP_PKEY> PrivateKeyFromPEM(const char *pem) {
   bssl::UniquePtr<BIO> bio(
@@ -2871,12 +2879,70 @@ MlJhXnXJFA==
 -----END CERTIFICATE-----
 )";
 
-// Test that the X.509 parser enforces versions are valid and match the fields
+// kV1CRLWithExtensionsPEM is a v1 CRL with extensions.
+static const char kV1CRLWithExtensionsPEM[] = R"(
+-----BEGIN X509 CRL-----
+MIIBpDCBjTANBgkqhkiG9w0BAQsFADBOMQswCQYDVQQGEwJVUzETMBEGA1UECAwK
+Q2FsaWZvcm5pYTEWMBQGA1UEBwwNTW91bnRhaW4gVmlldzESMBAGA1UECgwJQm9y
+aW5nU1NMFw0xNjA5MjYxNTEwNTVaFw0xNjEwMjYxNTEwNTVaoA4wDDAKBgNVHRQE
+AwIBATANBgkqhkiG9w0BAQsFAAOCAQEAnrBKKgvd9x9zwK9rtUvVeFeJ7+LNZEAc
++a5oxpPNEsJx6hXoApYEbzXMxuWBQoCs5iEBycSGudct21L+MVf27M38KrWoeOkq
+0a2siqViQZO2Fb/SUFR0k9zb8xl86Zf65lgPplALun0bV/HT7MJcl04Tc4osdsAR
+eBs5nqTGNEd5AlC1iKHvQZkM//MD51DspKnDpsDiUVi54h9C1SpfZmX8H2Vvdiyu
+0fZ/bPAM3VAGawatf/SyWfBMyKpoPXEG39oAzmjjOj8en82psn7m474IGaho/vBb
+hl1ms5qQiLYPjm4YELtnXQoFyC72tBjbdFd/ZE9k4CNKDbxFUXFbkw==
+-----END X509 CRL-----
+)";
+
+// kExplicitDefaultVersionCRLPEM is a v1 CRL with an explicitly-encoded version
+// field.
+static const char kExplicitDefaultVersionCRLPEM[] = R"(
+-----BEGIN X509 CRL-----
+MIIBlzCBgAIBADANBgkqhkiG9w0BAQsFADBOMQswCQYDVQQGEwJVUzETMBEGA1UE
+CAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNTW91bnRhaW4gVmlldzESMBAGA1UECgwJ
+Qm9yaW5nU1NMFw0xNjA5MjYxNTEwNTVaFw0xNjEwMjYxNTEwNTVaMA0GCSqGSIb3
+DQEBCwUAA4IBAQCesEoqC933H3PAr2u1S9V4V4nv4s1kQBz5rmjGk80SwnHqFegC
+lgRvNczG5YFCgKzmIQHJxIa51y3bUv4xV/bszfwqtah46SrRrayKpWJBk7YVv9JQ
+VHST3NvzGXzpl/rmWA+mUAu6fRtX8dPswlyXThNziix2wBF4GzmepMY0R3kCULWI
+oe9BmQz/8wPnUOykqcOmwOJRWLniH0LVKl9mZfwfZW92LK7R9n9s8AzdUAZrBq1/
+9LJZ8EzIqmg9cQbf2gDOaOM6Px6fzamyfubjvggZqGj+8FuGXWazmpCItg+ObhgQ
+u2ddCgXILva0GNt0V39kT2TgI0oNvEVRcVuT
+-----END X509 CRL-----
+)";
+
+// kV3CRLPEM is a v3 CRL. CRL versions only go up to v2.
+static const char kV3CRLPEM[] = R"(
+-----BEGIN X509 CRL-----
+MIIBpzCBkAIBAjANBgkqhkiG9w0BAQsFADBOMQswCQYDVQQGEwJVUzETMBEGA1UE
+CAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNTW91bnRhaW4gVmlldzESMBAGA1UECgwJ
+Qm9yaW5nU1NMFw0xNjA5MjYxNTEwNTVaFw0xNjEwMjYxNTEwNTVaoA4wDDAKBgNV
+HRQEAwIBATANBgkqhkiG9w0BAQsFAAOCAQEAnrBKKgvd9x9zwK9rtUvVeFeJ7+LN
+ZEAc+a5oxpPNEsJx6hXoApYEbzXMxuWBQoCs5iEBycSGudct21L+MVf27M38KrWo
+eOkq0a2siqViQZO2Fb/SUFR0k9zb8xl86Zf65lgPplALun0bV/HT7MJcl04Tc4os
+dsAReBs5nqTGNEd5AlC1iKHvQZkM//MD51DspKnDpsDiUVi54h9C1SpfZmX8H2Vv
+diyu0fZ/bPAM3VAGawatf/SyWfBMyKpoPXEG39oAzmjjOj8en82psn7m474IGaho
+/vBbhl1ms5qQiLYPjm4YELtnXQoFyC72tBjbdFd/ZE9k4CNKDbxFUXFbkw==
+-----END X509 CRL-----
+)";
+
+// kV2CSRPEM is a v2 CSR. CSR versions only go up to v1.
+static const char kV2CSRPEM[] = R"(
+-----BEGIN CERTIFICATE REQUEST-----
+MIHJMHECAQEwDzENMAsGA1UEAwwEVGVzdDBZMBMGByqGSM49AgEGCCqGSM49AwEH
+A0IABJjsayyAQod1J7UJYNT8AH4WWxLdKV0ozhrIz6hCzBAze7AqXWOSH8G+1EWC
+pSfL3oMQNtBdJS0kpXXaUqEAgTSgADAKBggqhkjOPQQDAgNIADBFAiAUXVaEYATg
+4Cc917T73KBImxh6xyhsA5pKuYpq1S4m9wIhAK+G93HR4ur7Ghel6+zUTvIAsj9e
+rsn4lSYsqI4OI4ei
+-----END CERTIFICATE REQUEST-----
+)";
+
+// Test that the library enforces versions are valid and match the fields
 // present.
 TEST(X509Test, InvalidVersion) {
   // kExplicitDefaultVersionPEM is invalid but, for now, we accept it. See
   // https://crbug.com/boringssl/364.
   EXPECT_TRUE(CertFromPEM(kExplicitDefaultVersionPEM));
+  EXPECT_TRUE(CRLFromPEM(kExplicitDefaultVersionCRLPEM));
 
   EXPECT_FALSE(CertFromPEM(kNegativeVersionPEM));
   EXPECT_FALSE(CertFromPEM(kFutureVersionPEM));
@@ -2885,6 +2951,27 @@ TEST(X509Test, InvalidVersion) {
   EXPECT_FALSE(CertFromPEM(kV2WithExtensionsPEM));
   EXPECT_FALSE(CertFromPEM(kV1WithIssuerUniqueIDPEM));
   EXPECT_FALSE(CertFromPEM(kV1WithSubjectUniqueIDPEM));
+  EXPECT_FALSE(CRLFromPEM(kV1CRLWithExtensionsPEM));
+  EXPECT_FALSE(CRLFromPEM(kV3CRLPEM));
+  EXPECT_FALSE(CSRFromPEM(kV2CSRPEM));
+
+  bssl::UniquePtr<X509> x509(X509_new());
+  ASSERT_TRUE(x509);
+  EXPECT_FALSE(X509_set_version(x509.get(), -1));
+  EXPECT_FALSE(X509_set_version(x509.get(), X509_VERSION_3 + 1));
+  EXPECT_FALSE(X509_set_version(x509.get(), 9999));
+
+  bssl::UniquePtr<X509_CRL> crl(X509_CRL_new());
+  ASSERT_TRUE(crl);
+  EXPECT_FALSE(X509_CRL_set_version(crl.get(), -1));
+  EXPECT_FALSE(X509_CRL_set_version(crl.get(), X509_CRL_VERSION_2 + 1));
+  EXPECT_FALSE(X509_CRL_set_version(crl.get(), 9999));
+
+  bssl::UniquePtr<X509_REQ> req(X509_REQ_new());
+  ASSERT_TRUE(req);
+  EXPECT_FALSE(X509_REQ_set_version(req.get(), -1));
+  EXPECT_FALSE(X509_REQ_set_version(req.get(), X509_REQ_VERSION_1 + 1));
+  EXPECT_FALSE(X509_REQ_set_version(req.get(), 9999));
 }
 
 // Unlike upstream OpenSSL, we require a non-null store in

diff --git a/crypto/x509/x509cset.c b/crypto/x509/x509cset.c
@@ -64,16 +64,29 @@
 
 int X509_CRL_set_version(X509_CRL *x, long version)
 {
-    /* TODO(https://crbug.com/boringssl/467): Reject invalid version
-     * numbers. Also correctly handle |X509_CRL_VERSION_1|, which should omit
-     * the encoding. */
-    if (x == NULL)
-        return (0);
+    if (x == NULL) {
+        return 0;
+    }
+
+    if (version < X509_CRL_VERSION_1 || version > X509_CRL_VERSION_2) {
+        OPENSSL_PUT_ERROR(X509, X509_R_INVALID_VERSION);
+        return 0;
+    }
+
+    /* v1(0) is default and is represented by omitting the version. */
+    if (version == X509_CRL_VERSION_1) {
+        ASN1_INTEGER_free(x->crl->version);
+        x->crl->version = NULL;
+        return 1;
+    }
+
     if (x->crl->version == NULL) {
-        if ((x->crl->version = ASN1_INTEGER_new()) == NULL)
-            return (0);
+        x->crl->version = ASN1_INTEGER_new();
+        if (x->crl->version == NULL) {
+            return 0;
+        }
     }
-    return (ASN1_INTEGER_set(x->crl->version, version));
+    return ASN1_INTEGER_set(x->crl->version, version);
 }
 
 int X509_CRL_set_issuer_name(X509_CRL *x, X509_NAME *name)

diff --git a/crypto/x509/x509rset.c b/crypto/x509/x509rset.c
@@ -64,11 +64,14 @@
 
 int X509_REQ_set_version(X509_REQ *x, long version)
 {
-    /* TODO(https://crbug.com/boringssl/467): Reject invalid version
-     * numbers. */
-    if (x == NULL)
-        return (0);
-    return (ASN1_INTEGER_set(x->req_info->version, version));
+    if (x == NULL) {
+        return 0;
+    }
+    if (version != X509_REQ_VERSION_1) {
+        OPENSSL_PUT_ERROR(X509, X509_R_INVALID_VERSION);
+        return 0;
+    }
+    return ASN1_INTEGER_set(x->req_info->version, version);
 }
 
 int X509_REQ_set_subject_name(X509_REQ *x, X509_NAME *name)

diff --git a/crypto/x509/x_crl.c b/crypto/x509/x_crl.c
@@ -127,12 +127,6 @@ static int crl_inf_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
          * affect the output of X509_CRL_print().
          */
     case ASN1_OP_D2I_POST:
-        /* TODO(https://crbug.com/boringssl/467): Reject invalid version
-         * numbers.
-         *
-         * TODO(davidben): Check that default |versions| are never encoded and
-         * that |extensions| is only present in v2. */
-
         (void)sk_X509_REVOKED_set_cmp_func(a->revoked, X509_REVOKED_cmp);
         break;
     }
@@ -250,7 +244,26 @@ static int crl_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
         crl->base_crl_number = NULL;
         break;
 
-    case ASN1_OP_D2I_POST:
+    case ASN1_OP_D2I_POST: {
+        /* The version must be one of v1(0) or v2(1). */
+        long version = X509_CRL_VERSION_1;
+        if (crl->crl->version != NULL) {
+            version = ASN1_INTEGER_get(crl->crl->version);
+            /* TODO(https://crbug.com/boringssl/364): |X509_CRL_VERSION_1|
+             * should also be rejected. This means an explicitly-encoded X.509v1
+             * version. v1 is DEFAULT, so DER requires it be omitted. */
+            if (version < X509_CRL_VERSION_1 || version > X509_CRL_VERSION_2) {
+                OPENSSL_PUT_ERROR(X509, X509_R_INVALID_VERSION);
+                return 0;
+            }
+        }
+
+        /* Per RFC 5280, section 5.1.2.1, extensions require v2. */
+        if (version != X509_CRL_VERSION_2 && crl->crl->extensions != NULL) {
+            OPENSSL_PUT_ERROR(X509, X509_R_INVALID_FIELD_FOR_VERSION);
+            return 0;
+        }
+
         if (!X509_CRL_digest(crl, EVP_sha256(), crl->crl_hash, NULL)) {
             return 0;
         }
@@ -324,6 +337,7 @@ static int crl_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
                 return 0;
         }
         break;
+    }
 
     case ASN1_OP_FREE_POST:
         /* |crl->meth| may be NULL if constructing the object failed before

diff --git a/crypto/x509/x_req.c b/crypto/x509/x_req.c
@@ -83,8 +83,13 @@ static int rinf_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
             return 0;
     }
 
-    /* TODO(https://crbug.com/boringssl/467): Add an |ASN1_OP_D2I_POST| callback
-     * and check the version. */
+    if (operation == ASN1_OP_D2I_POST) {
+        if (ASN1_INTEGER_get(rinf->version) != X509_REQ_VERSION_1) {
+            OPENSSL_PUT_ERROR(X509, X509_R_INVALID_VERSION);
+            return 0;
+        }
+    }
+
     return 1;
 }
 

diff --git a/crypto/x509/x_x509.c b/crypto/x509/x_x509.c
@@ -117,27 +117,27 @@ static int x509_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it,
 
     case ASN1_OP_D2I_POST: {
         /* The version must be one of v1(0), v2(1), or v3(2). */
-        long version = 0;
+        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): |version| = 0 should also
-             * be rejected. This means an explicitly-encoded X.509v1 version.
-             * v1 is DEFAULT, so DER requires it be omitted. */
-            if (version < 0 || version > 2) {
+            /* 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 == 0 && (ret->cert_info->issuerUID != NULL ||
-                             ret->cert_info->subjectUID != NULL)) {
+        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 != 2 && ret->cert_info->extensions != NULL) {
+        if (version != X509_VERSION_3 && ret->cert_info->extensions != NULL) {
             OPENSSL_PUT_ERROR(X509, X509_R_INVALID_FIELD_FOR_VERSION);
             return 0;
         }