Revert "Don't create Date or Calendar objects for ASN.1 dates unless needed. (#1176)" (#1200)

prbprbprb · web-flow · commit 59de3194ca10 · 2024-03-20T12:04:48.000Z
Reason: Unexpected behaviour change which rejects certificates with
invalid date files using UTCTIME with an offset, of which it turns
out there are many in circulation.

Also added a regression test to ensure we don't accidentally start
rejecting them again.
diff --git a/common/src/jni/main/cpp/conscrypt/native_crypto.cc b/common/src/jni/main/cpp/conscrypt/native_crypto.cc
@@ -4904,20 +4904,6 @@ static jobjectArray NativeCrypto_get_X509_GENERAL_NAME_stack(JNIEnv* env, jclass
     return joa.release();
 }
 
-/*
- * Converts an ASN1_TIME to epoch time in milliseconds.
- */
-static jlong ASN1_TIME_convert_to_posix(JNIEnv* env, const ASN1_TIME* time) {
-    int64_t retval;
-    if (!ASN1_TIME_to_posix(time, &retval)) {
-        JNI_TRACE("ASN1_TIME_convert_to_posix(%p) => Invalid date value", time);
-        conscrypt::jniutil::throwParsingException(env, "Invalid date value");
-        return 0;
-    }
-    // ASN1_TIME_to_posix can only return years from 0000 to 9999, so this won't overflow.
-    return static_cast<jlong>(retval * 1000);
-}
-
 static jlong NativeCrypto_X509_get_notBefore(JNIEnv* env, jclass, jlong x509Ref,
                                              CONSCRYPT_UNUSED jobject holder) {
     CHECK_ERROR_QUEUE_ON_RETURN;
@@ -4932,7 +4918,7 @@ static jlong NativeCrypto_X509_get_notBefore(JNIEnv* env, jclass, jlong x509Ref,
 
     ASN1_TIME* notBefore = X509_get_notBefore(x509);
     JNI_TRACE("X509_get_notBefore(%p) => %p", x509, notBefore);
-    return ASN1_TIME_convert_to_posix(env, notBefore);
+    return reinterpret_cast<uintptr_t>(notBefore);
 }
 
 static jlong NativeCrypto_X509_get_notAfter(JNIEnv* env, jclass, jlong x509Ref,
@@ -4949,7 +4935,7 @@ static jlong NativeCrypto_X509_get_notAfter(JNIEnv* env, jclass, jlong x509Ref,
 
     ASN1_TIME* notAfter = X509_get_notAfter(x509);
     JNI_TRACE("X509_get_notAfter(%p) => %p", x509, notAfter);
-    return ASN1_TIME_convert_to_posix(env, notAfter);
+    return reinterpret_cast<uintptr_t>(notAfter);
 }
 
 // NOLINTNEXTLINE(runtime/int)
@@ -5585,7 +5571,7 @@ static jlong NativeCrypto_get_X509_REVOKED_revocationDate(JNIEnv* env, jclass,
 
     JNI_TRACE("get_X509_REVOKED_revocationDate(%p) => %p", revoked,
               X509_REVOKED_get0_revocationDate(revoked));
-    return ASN1_TIME_convert_to_posix(env, X509_REVOKED_get0_revocationDate(revoked));
+    return reinterpret_cast<uintptr_t>(X509_REVOKED_get0_revocationDate(revoked));
 }
 
 #ifdef __GNUC__
@@ -5679,7 +5665,7 @@ static jlong NativeCrypto_X509_CRL_get_lastUpdate(JNIEnv* env, jclass, jlong x50
 
     ASN1_TIME* lastUpdate = X509_CRL_get_lastUpdate(crl);
     JNI_TRACE("X509_CRL_get_lastUpdate(%p) => %p", crl, lastUpdate);
-    return ASN1_TIME_convert_to_posix(env, lastUpdate);
+    return reinterpret_cast<uintptr_t>(lastUpdate);
 }
 
 static jlong NativeCrypto_X509_CRL_get_nextUpdate(JNIEnv* env, jclass, jlong x509CrlRef,
@@ -5696,7 +5682,7 @@ static jlong NativeCrypto_X509_CRL_get_nextUpdate(JNIEnv* env, jclass, jlong x50
 
     ASN1_TIME* nextUpdate = X509_CRL_get_nextUpdate(crl);
     JNI_TRACE("X509_CRL_get_nextUpdate(%p) => %p", crl, nextUpdate);
-    return ASN1_TIME_convert_to_posix(env, nextUpdate);
+    return reinterpret_cast<uintptr_t>(nextUpdate);
 }
 
 static jbyteArray NativeCrypto_i2d_X509_REVOKED(JNIEnv* env, jclass, jlong x509RevokedRef) {
@@ -5720,6 +5706,63 @@ static jint NativeCrypto_X509_supported_extension(JNIEnv* env, jclass, jlong x50
     return X509_supported_extension(ext);
 }
 
+static inline bool decimal_to_integer(const char* data, size_t len, int* out) {
+    int ret = 0;
+    for (size_t i = 0; i < len; i++) {
+        ret *= 10;
+        if (data[i] < '0' || data[i] > '9') {
+            return false;
+        }
+        ret += data[i] - '0';
+    }
+    *out = ret;
+    return true;
+}
+
+static void NativeCrypto_ASN1_TIME_to_Calendar(JNIEnv* env, jclass, jlong asn1TimeRef,
+                                               jobject calendar) {
+    CHECK_ERROR_QUEUE_ON_RETURN;
+    ASN1_TIME* asn1Time = reinterpret_cast<ASN1_TIME*>(static_cast<uintptr_t>(asn1TimeRef));
+    JNI_TRACE("ASN1_TIME_to_Calendar(%p, %p)", asn1Time, calendar);
+
+    if (asn1Time == nullptr) {
+        conscrypt::jniutil::throwNullPointerException(env, "asn1Time == null");
+        return;
+    }
+
+    if (!ASN1_TIME_check(asn1Time)) {
+        conscrypt::jniutil::throwParsingException(env, "Invalid date format");
+        return;
+    }
+
+    bssl::UniquePtr<ASN1_GENERALIZEDTIME> gen(ASN1_TIME_to_generalizedtime(asn1Time, nullptr));
+    if (gen.get() == nullptr) {
+        conscrypt::jniutil::throwParsingException(env,
+                                                  "ASN1_TIME_to_generalizedtime returned null");
+        return;
+    }
+
+    if (ASN1_STRING_length(gen.get()) < 14 || ASN1_STRING_get0_data(gen.get()) == nullptr) {
+        conscrypt::jniutil::throwNullPointerException(env, "gen->length < 14 || gen->data == null");
+        return;
+    }
+
+    int year, mon, mday, hour, min, sec;
+    const char* data = reinterpret_cast<const char*>(ASN1_STRING_get0_data(gen.get()));
+    if (!decimal_to_integer(data, 4, &year) ||
+        !decimal_to_integer(data + 4, 2, &mon) ||
+        !decimal_to_integer(data + 6, 2, &mday) ||
+        !decimal_to_integer(data + 8, 2, &hour) ||
+        !decimal_to_integer(data + 10, 2, &min) ||
+        !decimal_to_integer(data + 12, 2, &sec)) {
+        conscrypt::jniutil::throwParsingException(env, "Invalid date format");
+        return;
+    }
+
+    env->CallVoidMethod(calendar, conscrypt::jniutil::calendar_setMethod, year, mon - 1, mday, hour,
+                        min, sec);
+}
+
 // A CbsHandle is a structure used to manage resources allocated by asn1_read-*
 // functions so that they can be freed properly when finished.  This struct owns
 // all objects pointed to by its members.
@@ -11219,6 +11262,7 @@ static JNINativeMethod sNativeCryptoMethods[] = {
         CONSCRYPT_NATIVE_METHOD(X509_REVOKED_dup, "(J)J"),
         CONSCRYPT_NATIVE_METHOD(i2d_X509_REVOKED, "(J)[B"),
         CONSCRYPT_NATIVE_METHOD(X509_supported_extension, "(J)I"),
+        CONSCRYPT_NATIVE_METHOD(ASN1_TIME_to_Calendar, "(JLjava/util/Calendar;)V"),
         CONSCRYPT_NATIVE_METHOD(asn1_read_init, "([B)J"),
         CONSCRYPT_NATIVE_METHOD(asn1_read_sequence, "(J)J"),
         CONSCRYPT_NATIVE_METHOD(asn1_read_next_tag_is, "(JI)Z"),
diff --git a/common/src/main/java/org/conscrypt/NativeCrypto.java b/common/src/main/java/org/conscrypt/NativeCrypto.java
@@ -33,6 +33,7 @@
 import java.security.cert.CertificateParsingException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Calendar;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -637,6 +638,10 @@ static native long X509_CRL_get_nextUpdate(long x509CrlCtx, OpenSSLX509CRL holde
 
     static native int X509_supported_extension(long x509ExtensionRef);
 
+    // --- ASN1_TIME -----------------------------------------------------------
+
+    static native void ASN1_TIME_to_Calendar(long asn1TimeCtx, Calendar cal) throws ParsingException;
+
     // --- ASN1 Encoding -------------------------------------------------------
 
     /**
diff --git a/common/src/main/java/org/conscrypt/OpenSSLX509CRL.java b/common/src/main/java/org/conscrypt/OpenSSLX509CRL.java
@@ -34,10 +34,13 @@
 import java.security.cert.X509Certificate;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Calendar;
 import java.util.Date;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.TimeZone;
+
 import javax.crypto.BadPaddingException;
 import javax.crypto.IllegalBlockSizeException;
 import javax.security.auth.x500.X500Principal;
@@ -48,15 +51,23 @@
  */
 final class OpenSSLX509CRL extends X509CRL {
     private volatile long mContext;
-    private final long thisUpdate;
-    private final long nextUpdate;
+    private final Date thisUpdate;
+    private final Date nextUpdate;
 
     private OpenSSLX509CRL(long ctx) throws ParsingException {
         mContext = ctx;
         // The legacy X509 OpenSSL APIs don't validate ASN1_TIME structures until access, so
         // parse them here because this is the only time we're allowed to throw ParsingException
-        thisUpdate = NativeCrypto.X509_CRL_get_lastUpdate(mContext, this);
-        nextUpdate = NativeCrypto.X509_CRL_get_nextUpdate(mContext, this);
+        thisUpdate = toDate(NativeCrypto.X509_CRL_get_lastUpdate(mContext, this));
+        nextUpdate = toDate(NativeCrypto.X509_CRL_get_nextUpdate(mContext, this));
+    }
+
+    // Package-visible because it's also used by OpenSSLX509CRLEntry
+    static Date toDate(long asn1time) throws ParsingException {
+        Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC"));
+        calendar.set(Calendar.MILLISECOND, 0);
+        NativeCrypto.ASN1_TIME_to_Calendar(asn1time, calendar);
+        return calendar.getTime();
     }
 
     static OpenSSLX509CRL fromX509DerInputStream(InputStream is) throws ParsingException {
@@ -268,12 +279,12 @@ public X500Principal getIssuerX500Principal() {
 
     @Override
     public Date getThisUpdate() {
-        return new Date(thisUpdate);
+        return (Date) thisUpdate.clone();
     }
 
     @Override
     public Date getNextUpdate() {
-        return new Date(nextUpdate);
+        return (Date) nextUpdate.clone();
     }
 
     @Override
diff --git a/common/src/main/java/org/conscrypt/OpenSSLX509CRLEntry.java b/common/src/main/java/org/conscrypt/OpenSSLX509CRLEntry.java
@@ -31,13 +31,13 @@
  */
 final class OpenSSLX509CRLEntry extends X509CRLEntry {
     private final long mContext;
-    private final long revocationDate;
+    private final Date revocationDate;
 
     OpenSSLX509CRLEntry(long ctx) throws ParsingException {
         mContext = ctx;
         // The legacy X509 OpenSSL APIs don't validate ASN1_TIME structures until access, so
         // parse them here because this is the only time we're allowed to throw ParsingException
-        revocationDate = NativeCrypto.get_X509_REVOKED_revocationDate(mContext);
+        revocationDate = OpenSSLX509CRL.toDate(NativeCrypto.get_X509_REVOKED_revocationDate(mContext));
     }
 
     @Override
@@ -112,7 +112,7 @@ public BigInteger getSerialNumber() {
 
     @Override
     public Date getRevocationDate() {
-        return new Date(revocationDate);
+        return (Date) revocationDate.clone();
     }
 
     @Override
diff --git a/common/src/main/java/org/conscrypt/OpenSSLX509Certificate.java b/common/src/main/java/org/conscrypt/OpenSSLX509Certificate.java
@@ -39,12 +39,15 @@
 import java.security.spec.X509EncodedKeySpec;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Calendar;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.TimeZone;
+
 import javax.crypto.BadPaddingException;
 import javax.security.auth.x500.X500Principal;
 import org.conscrypt.OpenSSLX509CertificateFactory.ParsingException;
@@ -59,15 +62,29 @@ public final class OpenSSLX509Certificate extends X509Certificate {
     private transient volatile long mContext;
     private transient Integer mHashCode;
 
-    private final long notBefore;
-    private final long notAfter;
+    private final Date notBefore;
+    private final Date notAfter;
 
     OpenSSLX509Certificate(long ctx) throws ParsingException {
         mContext = ctx;
         // The legacy X509 OpenSSL APIs don't validate ASN1_TIME structures until access, so
         // parse them here because this is the only time we're allowed to throw ParsingException
-        notBefore = NativeCrypto.X509_get_notBefore(mContext, this);
-        notAfter = NativeCrypto.X509_get_notAfter(mContext, this);
+        notBefore = toDate(NativeCrypto.X509_get_notBefore(mContext, this));
+        notAfter = toDate(NativeCrypto.X509_get_notAfter(mContext, this));
+    }
+
+    // A non-throwing constructor used when we have already parsed the dates
+    private OpenSSLX509Certificate(long ctx, Date notBefore, Date notAfter) {
+        mContext = ctx;
+        this.notBefore = notBefore;
+        this.notAfter = notAfter;
+    }
+
+    private static Date toDate(long asn1time) throws ParsingException {
+        Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC"));
+        calendar.set(Calendar.MILLISECOND, 0);
+        NativeCrypto.ASN1_TIME_to_Calendar(asn1time, calendar);
+        return calendar.getTime();
     }
 
     public static OpenSSLX509Certificate fromX509DerInputStream(InputStream is)
@@ -244,12 +261,12 @@ public void checkValidity(Date date) throws CertificateExpiredException,
             CertificateNotYetValidException {
         if (getNotBefore().compareTo(date) > 0) {
             throw new CertificateNotYetValidException("Certificate not valid until "
-                    + getNotBefore() + " (compared to " + date + ")");
+                    + getNotBefore().toString() + " (compared to " + date.toString() + ")");
         }
 
         if (getNotAfter().compareTo(date) < 0) {
             throw new CertificateExpiredException("Certificate expired at "
-                    + getNotAfter() + " (compared to " + date + ")");
+                    + getNotAfter().toString() + " (compared to " + date.toString() + ")");
         }
     }
 
@@ -275,12 +292,12 @@ public Principal getSubjectDN() {
 
     @Override
     public Date getNotBefore() {
-        return new Date(notBefore);
+        return (Date) notBefore.clone();
     }
 
     @Override
     public Date getNotAfter() {
-        return new Date(notAfter);
+        return (Date) notAfter.clone();
     }
 
     @Override
diff --git a/common/src/test/java/org/conscrypt/HostnameVerifierTest.java b/common/src/test/java/org/conscrypt/HostnameVerifierTest.java
@@ -598,7 +598,7 @@ public void subjectAltNameWithToplevelWildcard() throws Exception {
         //
         // Certificate generated using:-
         //     openssl req -x509 -nodes -days 36500 -subj "/CN=Google Inc" \
-        //         -addext "subjectAltName=DNS:*.com" -
+        //         -addext "subjectAltName=DNS:*.com" -newkey rsa:512
         SSLSession session = session(""
                 + "-----BEGIN CERTIFICATE-----\n"
                 + "MIIBlTCCAT+gAwIBAgIUe1RB6C61ZW/SEQpKiywSEJOEOUMwDQYJKoZIhvcNAQEL\n"
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
@@ -18,6 +18,7 @@
 
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
@@ -350,6 +351,26 @@ public class X509CertificateTest {
             + "mmi08cueFV7mHzJSYV51yRQ=\n"
             + "-----END CERTIFICATE-----\n";
 
+    private static final String UTCTIME_WITH_OFFSET = "-----BEGIN CERTIFICATE-----\n" +
+            "MIIDPzCCAicCAgERMA0GCSqGSIb3DQEBCwUAMFsxCzAJBgNVBAYTAlVTMRMwEQYD\n" +
+            "VQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1Nb3VudGFpbiBWaWV3MR8wHQYDVQQK\n" +
+            "DBZHb29nbGUgQXV0b21vdGl2ZSBMaW5rMCYXETE0MDcwNDAwMDAwMC0wNzAwFxE0\n" +
+            "ODA4MDExMDIxMjMtMDcwMDBnMQswCQYDVQQGEwJVUzETMBEGA1UECAwKQ2FsaWZv\n" +
+            "cm5pYTEWMBQGA1UEBwwNTW91bnRhaW4gVmlldzEeMBwGA1UECgwVQW5kcm9pZC1B\n" +
+            "dXRvLUludGVybmFsMQswCQYDVQQLDAIwMTCCASIwDQYJKoZIhvcNAQEBBQADggEP\n" +
+            "ADCCAQoCggEBAOWghAac2eJLbi/ijgZGRB6/MuaBVfOImkQddBJUhXbnskTJB/JI\n" +
+            "12Ea22E5GeVN8CkWULAZT28yDWqsKMyq9BzpjpsHc9TKxMYqrIn0HP7mIJcBu5z7\n" +
+            "K8DoXqc86encncJlkGeuQkUA68yyp7RG7eQ6XoBHEjNmyvX13Y8NY5sPUHfLfmp6\n" +
+            "A2n+Jdmecq3L0GS84ctdNtnp2zSopTy0L1Gp6+lrnuOPAYZeV+Ei2jAvhycvuSoB\n" +
+            "yV6rT9wvREvC2TDncurMwR6ws44+ZStqkhnvDLhV04ray5aPplQwwB9GELFCYSRk\n" +
+            "56sm57uYSJj/LlmOMcvyBmUHVJ7MLxgtlykCAwEAATANBgkqhkiG9w0BAQsFAAOC\n" +
+            "AQEA1Bs8v6HuAIiBdhGDGHzZJDwO6lW0LheBqsGLG9KsVvIVrTMPP9lpdTPjStGn\n" +
+            "en1RIce4R4l3YTBwxOadLMkf8rymAE5JNjPsWlBue7eI4TFFw/cvnKxcTQ61bC4i\n" +
+            "2uosyDI5VfrXm38zYcZoK4TFtMhNyx6aYSEClWB9MjHa+n6eR3dLBCg1kMGqGdZ/\n" +
+            "AoK0UEkyI3UFU8sW86iaS4dvPSaQ+z0tmfUzbrc5ZSk4hYCeUYvuyd2ShxjKmxvD\n" +
+            "0K8A7gKLY0jP8Zp+6rYBcpxc7cylWMbdlhFTHAGiKI+XeQ/9u+RPeocZsn5jGlDt\n" +
+            "K3ftMoWFce+baNq/WcMzRj04AA==\n" +
+            "-----END CERTIFICATE-----\n";
     private static Date dateFromUTC(int year, int month, int day, int hour, int minute, int second)
             throws Exception {
         Calendar c = Calendar.getInstance(TimeZone.getTimeZone("UTC"));
@@ -715,4 +736,24 @@ public void test(Provider p, String algorithm) throws Exception {
             }
         });
     }
+
+    // Ensure we don't reject certificates with UTCTIME fields with offsets for now: b/311260068
+    @Test
+    public void utcTimeWithOffset() throws Exception {
+        ServiceTester tester = ServiceTester.test("CertificateFactory").withAlgorithm("X509");
+        tester.skipProvider("SUN") // Sun and BC interpret the offset, Conscrypt just drops it...
+                .skipProvider("BC")
+                .run(new ServiceTester.Test() {
+            @Override
+            public void test(Provider p, String algorithm) throws Exception {
+                X509Certificate c = certificateFromPEM(p, UTCTIME_WITH_OFFSET);
+                assertDatesEqual(
+                        dateFromUTC(2014, Calendar.JULY, 4, 0, 0, 0),
+                        c.getNotBefore());
+                assertDatesEqual(
+                        dateFromUTC(2048, Calendar.AUGUST, 1, 10, 21, 23),
+                        c.getNotAfter());
+            }
+        });
+    }
 }
