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 <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/t_crl.c b/crypto/x509/t_crl.c
index 0e6140d..2aee83d 100644
--- 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(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
index 92ded49..9e3ce26 100644
--- 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 @@
     }
   }
   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
index 5e633a9..e1f280f 100644
--- a/crypto/x509/t_x509.c
+++ b/crypto/x509/t_x509.c
@@ -119,10 +119,8 @@
             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
index 5cbd00d..6a58322 100644
--- a/crypto/x509/x509_set.c
+++ b/crypto/x509/x509_set.c
@@ -74,19 +74,29 @@
 
 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
index 6f2170c..253e898 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -1047,7 +1047,7 @@
 -----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)));
@@ -1055,7 +1055,7 @@
       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)));
@@ -1063,7 +1063,15 @@
       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 @@
 -----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 @@
   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
index de32e30..1671f35 100644
--- 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->crl->version == NULL) {
-        if ((x->crl->version = ASN1_INTEGER_new()) == NULL)
-            return (0);
+    if (x == NULL) {
+        return 0;
     }
-    return (ASN1_INTEGER_set(x->crl->version, version));
+
+    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) {
+        x->crl->version = ASN1_INTEGER_new();
+        if (x->crl->version == NULL) {
+            return 0;
+        }
+    }
+    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
index 5526b23..c69f8cb 100644
--- 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
index ab2a039..588a99f 100644
--- a/crypto/x509/x_crl.c
+++ b/crypto/x509/x_crl.c
@@ -127,12 +127,6 @@
          * 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 @@
         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 @@
                 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
index 590f84d..0893918 100644
--- a/crypto/x509/x_req.c
+++ b/crypto/x509/x_req.c
@@ -83,8 +83,13 @@
             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
index 9d350bd..6586b4c 100644
--- a/crypto/x509/x_x509.c
+++ b/crypto/x509/x_x509.c
@@ -117,27 +117,27 @@
 
     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;
         }
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 314a0e5..d00a19f 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -312,11 +312,8 @@
 #define X509_VERSION_2 1
 #define X509_VERSION_3 2
 
-// X509_get_version returns the numerical value of |x509|'s version. Callers may
-// compare the result to the |X509_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.
+// X509_get_version returns the numerical value of |x509|'s version, which will
+// be one of the |X509_VERSION_*| constants.
 OPENSSL_EXPORT long X509_get_version(const X509 *x509);
 
 // X509_set_version sets |x509|'s version to |version|, which should be one of
@@ -394,15 +391,12 @@
 // |EXFLAG_INVALID| bit.
 OPENSSL_EXPORT long X509_get_pathlen(X509 *x509);
 
-// X509_REQ_VERSION_1 is the version constant for |X509_REQ| objects. Note no
-// other versions are defined.
+// X509_REQ_VERSION_1 is the version constant for |X509_REQ| objects. No other
+// versions are defined.
 #define X509_REQ_VERSION_1 0
 
 // X509_REQ_get_version returns the numerical value of |req|'s version. This
-// will be |X509_REQ_VERSION_1| for valid certificate requests. If |req| is
-// invalid, it may return another value, or -1 on overflow.
-//
-// TODO(davidben): Enforce the version number in the parser.
+// will always be |X509_REQ_VERSION_1|.
 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
@@ -418,11 +412,8 @@
 #define X509_CRL_VERSION_1 0
 #define X509_CRL_VERSION_2 1
 
-// X509_CRL_get_version returns the numerical value of |crl|'s version. Callers
-// may compare the result to |X509_CRL_VERSION_*| constants. If |crl| is
-// invalid, it may return another value, or -1 on overflow.
-//
-// TODO(davidben): Enforce the version number in the parser.
+// X509_CRL_get_version returns the numerical value of |crl|'s version, which
+// will be one of the |X509_CRL_VERSION_*| constants.
 OPENSSL_EXPORT long X509_CRL_get_version(const X509_CRL *crl);
 
 // X509_CRL_get0_lastUpdate returns |crl|'s lastUpdate time.
@@ -1085,7 +1076,8 @@
 // X509_REQ_set_version sets |req|'s version to |version|, which should be
 // |X509_REQ_VERSION_1|. It returns one on success and zero on error.
 //
-// Note no versions other than |X509_REQ_VERSION_1| are defined for CSRs.
+// The only defined CSR version is |X509_REQ_VERSION_1|, so there is no need to
+// call this function.
 OPENSSL_EXPORT int X509_REQ_set_version(X509_REQ *req, long version);
 
 // X509_REQ_set_subject_name sets |req|'s subject to a copy of |name|. It