From 1c74fcf547f8ebff13b0585f46446d19144a8797 Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Tue, 19 Mar 2024 14:30:57 +0000 Subject: [PATCH] Revert "Don't create Date or Calendar objects for ASN.1 dates unless needed. (#1176)" 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. --- .../jni/main/cpp/conscrypt/native_crypto.cc | 82 ++++++++++++++----- .../main/java/org/conscrypt/NativeCrypto.java | 5 ++ .../java/org/conscrypt/OpenSSLX509CRL.java | 23 ++++-- .../org/conscrypt/OpenSSLX509CRLEntry.java | 6 +- .../org/conscrypt/OpenSSLX509Certificate.java | 33 ++++++-- .../org/conscrypt/HostnameVerifierTest.java | 2 +- .../security/cert/X509CertificateTest.java | 41 ++++++++++ 7 files changed, 155 insertions(+), 37 deletions(-) diff --git a/common/src/jni/main/cpp/conscrypt/native_crypto.cc b/common/src/jni/main/cpp/conscrypt/native_crypto.cc index d64298581..d1bd6b089 100644 --- 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(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(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(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(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(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(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(static_cast(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 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(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 index a83278e5f..aa349e786 100644 --- 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 index 6ef72e7c1..ad9749413 100644 --- 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 index 2d4846cf8..9a3db6245 100644 --- 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 index 6a2e2f36b..718d42038 100644 --- 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 index 487a42d1f..a369ecd1e 100644 --- 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 index 26217b00d..04de4410e 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 @@ -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()); + } + }); + } }