Fix error-handling for X.509 extensions
`d2i_X509` in OpenSSL and BoringSSL currently only parse the core Certificate ASN.1 structure. They leave syntax errors in extensions themselves to be noticed when you parse them. (In the general case, this is unavoidable since we don't know about all extensions.)

Alas, the OpenSSL API is atrocious. I have attempted to document the behavior in https://boringssl-review.googlesource.com/c/boringssl/+/43804/. Roughly we need to fix up all the `X509_get_ext_d2i` calls to check for errors as described in that CL. From there, we need to report it through the X509Certificate API. If the accessor can throw CertificateParsingException, that is the simplest. Some accessors, notably getBasicConstraints, cannot throw. Those we should probably check earlier. (I.e. prevent creating OpenSSLX509Certificates with invalid basicConstraints at all.)

(Separately, I'm considering making BoringSSL report a lot of these parse errors sooner, but we'll figure out what we want for Conscrypt when we get to it.)
diff --git a/common/src/jni/main/cpp/conscrypt/native_crypto.cc b/common/src/jni/main/cpp/conscrypt/native_crypto.cc
index 101ffa89..425eb703 100644
--- a/common/src/jni/main/cpp/conscrypt/native_crypto.cc
+++ b/common/src/jni/main/cpp/conscrypt/native_crypto.cc
@@ -4049,6 +4049,13 @@ static jobjectArray NativeCrypto_get_X509_GENERAL_NAME_stack(JNIEnv* env, jclass
         JNI_TRACE("get_X509_GENERAL_NAME_stack(%p, %d) => unknown type", x509, type);
         return nullptr;
     }
+    // TODO(https://github.com/google/conscrypt/issues/916): Handle errors, remove
+    // |ERR_clear_error|, and throw CertificateParsingException.
+    if (gn_stack == nullptr) {
+        JNI_TRACE("get_X509_GENERAL_NAME_stack(%p, %d) => null (no extension or error)", x509, type);
+        ERR_clear_error();
+        return nullptr;
+    }
 
     int count = static_cast<int>(sk_GENERAL_NAME_num(gn_stack.get()));
     if (count <= 0) {
@@ -4257,7 +4264,16 @@ static jint NativeCrypto_get_X509_ex_flags(JNIEnv* env, jclass, jlong x509Ref,
         return 0;
     }
 
-    return X509_get_extension_flags(x509);
+    uint32_t flags = X509_get_extension_flags(x509);
+    // X509_get_extension_flags sometimes leaves values in the error queue. See
+    // https://crbug.com/boringssl/382.
+    //
+    // TODO(https://github.com/google/conscrypt/issues/916): This function is used to check
+    // EXFLAG_CA, but does not check EXFLAG_INVALID. Fold the two JNI calls in getBasicConstraints()
+    // together and handle errors. (See also NativeCrypto_get_X509_ex_pathlen.) From there, limit
+    // this JNI call to EXFLAG_CRITICAL.
+    ERR_clear_error();
+    return flags;
 }
 
 static jint NativeCrypto_X509_check_issued(JNIEnv* env, jclass, jlong x509Ref1,
@@ -5834,10 +5850,14 @@ static jbooleanArray NativeCrypto_get_X509_ex_kusage(JNIEnv* env, jclass, jlong
         return nullptr;
     }
 
+    // TODO(https://github.com/google/conscrypt/issues/916): Handle errors and remove
+    // |ERR_clear_error|. Note X509Certificate.getKeyUsage() cannot throw
+    // CertificateParsingException, so this needs to be checked earlier, e.g. in the constructor.
     bssl::UniquePtr<ASN1_BIT_STRING> bitStr(
             static_cast<ASN1_BIT_STRING*>(X509_get_ext_d2i(x509, NID_key_usage, nullptr, nullptr)));
     if (bitStr.get() == nullptr) {
         JNI_TRACE("get_X509_ex_kusage(%p) => null", x509);
+        ERR_clear_error();
         return nullptr;
     }
 
@@ -5856,10 +5876,13 @@ static jobjectArray NativeCrypto_get_X509_ex_xkusage(JNIEnv* env, jclass, jlong
         return nullptr;
     }
 
+    // TODO(https://github.com/google/conscrypt/issues/916): Handle errors, remove
+    // |ERR_clear_error|, and throw CertificateParsingException.
     bssl::UniquePtr<STACK_OF(ASN1_OBJECT)> objArray(static_cast<STACK_OF(ASN1_OBJECT)*>(
             X509_get_ext_d2i(x509, NID_ext_key_usage, nullptr, nullptr)));
     if (objArray.get() == nullptr) {
         JNI_TRACE("get_X509_ex_xkusage(%p) => null", x509);
+        ERR_clear_error();
         return nullptr;
     }
 
@@ -5893,12 +5916,51 @@ static jint NativeCrypto_get_X509_ex_pathlen(JNIEnv* env, jclass, jlong x509Ref,
         return 0;
     }
 
-    // TODO(davidben): It is possible for |x509| to have an invalid basicConstraints extension, in
-    // which case |X509_get_pathlen| will report the extension is missing. The |EXFLAG_INVALID| can
-    // be checked for error, but Java's X509Certificate API does not allow for getBasicConstraints()
-    // to throw an exception, so we'd have to check this extension at parse time.
-    long pathlen = X509_get_pathlen(x509);
-    JNI_TRACE("get_X509_ex_pathlen(%p) => %ld", x509, pathlen);
+    // Use |X509_get_ext_d2i| instead of |X509_get_pathlen| because the latter treats
+    // |EXFLAG_INVALID| as the error case. |EXFLAG_INVALID| is set if any built-in extension is
+    // invalid. For now, we preserve Conscrypt's historical behavior in accepting certificates in
+    // the constructor even if |EXFLAG_INVALID| is set.
+    //
+    // TODO(https://github.com/google/conscrypt/issues/916): Handle errors and remove
+    // |ERR_clear_error|. Note X509Certificate.getBasicConstraints() cannot throw
+    // CertificateParsingException, so this needs to be checked earlier, e.g. in the constructor.
+    bssl::UniquePtr<BASIC_CONSTRAINTS> basic_constraints(static_cast<BASIC_CONSTRAINTS*>(
+            X509_get_ext_d2i(x509, NID_basic_constraints, nullptr, nullptr)));
+    if (basic_constraints == nullptr) {
+        JNI_TRACE("get_X509_ex_path(%p) => -1 (no extension or error)", x509);
+        ERR_clear_error();
+        return -1;
+    }
+
+    if (basic_constraints->pathlen == nullptr) {
+        JNI_TRACE("get_X509_ex_path(%p) => -1 (no pathLenConstraint)", x509);
+        return -1;
+    }
+
+    if (!basic_constraints->ca) {
+        // Path length constraints are only valid for CA certificates.
+        // TODO(https://github.com/google/conscrypt/issues/916): Treat this as an error condition.
+        JNI_TRACE("get_X509_ex_path(%p) => -1 (not a CA)", x509);
+        return -1;
+    }
+
+    if (basic_constraints->pathlen->type == V_ASN1_NEG_INTEGER) {
+        // Path length constraints may not be negative.
+        // TODO(https://github.com/google/conscrypt/issues/916): Treat this as an error condition.
+        JNI_TRACE("get_X509_ex_path(%p) => -1 (negative)", x509);
+        return -1;
+    }
+
+    long pathlen = ASN1_INTEGER_get(basic_constraints->pathlen);
+    if (pathlen == -1 || pathlen > INT_MAX) {
+        // TODO(https://github.com/google/conscrypt/issues/916): Treat this as an error condition.
+        // If the integer overflows, the certificate is effectively unconstrained. Reporting no
+        // constraint is plausible, but Chromium rejects all values above 255.
+        JNI_TRACE("get_X509_ex_path(%p) => -1 (overflow)", x509);
+        return -1;
+    }
+
+    JNI_TRACE("get_X509_ex_path(%p) => %ld", x509, pathlen);
     return pathlen;
 }
 
diff --git a/common/src/test/java/org/conscrypt/java/security/cert/X509CertificateTest.java b/common/src/test/java/org/conscrypt/java/security/cert/X509CertificateTest.java
index 26a130c9..c36ccdea 100644
--- a/common/src/test/java/org/conscrypt/java/security/cert/X509CertificateTest.java
+++ b/common/src/test/java/org/conscrypt/java/security/cert/X509CertificateTest.java
@@ -243,6 +243,21 @@ public class X509CertificateTest {
             + "XxSZmmi08cueFV7mHzJSYV51yRQ=\n"
             + "-----END CERTIFICATE-----\n";
 
+    /**
+     * This is a certificate with a pathlen constraint of 10, but there is an unrelated invalid
+     * subjectAltNames extension.
+     */
+    private static final String BASIC_CONSTRAINTS_PATHLEN_10_BAD_SAN =
+            "-----BEGIN CERTIFICATE-----\n"
+            + "MIIBRjCB7aADAgECAgkA2UwE2kl9v+swCgYIKoZIzj0EAwIwFjEUMBIGA1UEAwwL\n"
+            + "VGVzdCBJc3N1ZXIwHhcNMTQwNDIzMjMyMTU3WhcNMTQwNTIzMjMyMTU3WjAXMRUw\n"
+            + "EwYDVQQDDAxUZXN0IFN1YmplY3QwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATm\n"
+            + "K2niv2Wfl74vHg2UikzVl2u3qR4NRvvdqakendy6WgHn1peoChj5w8SjHlbifINI\n"
+            + "2xYaHPUdfvGULUvPciLBoyMwITAPBgNVHRMECDAGAQH/AgEKMA4GA1UdEQQHSU5W\n"
+            + "QUxJRDAKBggqhkjOPQQDAgNIADBFAiEA8qA1XlE6NsOCeZvuJ1CFjnAGdJVX0il0\n"
+            + "APS+FYddxAcCIHweeRRqIYPwenRoeV8UmZpotPHLnhVe5h8yUmFedckU\n"
+            + "-----END CERTIFICATE-----\n";
+
     /**
      * This is a certificate whose keyUsage extension has more than nine bits. The getKeyUsage()
      * method internally rounds up to nine bits, so this tests what happens when it does not need to
@@ -568,6 +583,21 @@ public class X509CertificateTest {
                 // basicConstraints extension and a leaf one.
                 c = certificateFromPEM(p, BASIC_CONSTRAINTS_LEAF);
                 assertEquals(-1, c.getBasicConstraints());
+
+                // If some unrelated extension has a syntax error, and that syntax error does not
+                // fail when constructing the certificate, it should not interfere with
+                // getBasicConstraints().
+                try {
+                    c = certificateFromPEM(p, BASIC_CONSTRAINTS_PATHLEN_10_BAD_SAN);
+                } catch (CertificateParsingException e) {
+                    // The certificate has a syntax error, so it would also be valid for the
+                    // provider to reject the certificate at construction. X.509 is an extensible
+                    // format, so different implementations may notice errors at different points.
+                    c = null;
+                }
+                if (c != null) {
+                    assertEquals(10, c.getBasicConstraints());
+                }
             }
         });
     }
['common/src/jni/main/cpp/conscrypt/native_crypto.cc', 'common/src/test/java/org/conscrypt/java/security/cert/X509CertificateTest.java']