Skip to content

Commit

Permalink
Don't create Date or Calendar objects for ASN.1 dates unless needed. (#…
Browse files Browse the repository at this point in the history 
…1176)

* Don't create Date or Calendar objects for ASN.1 dates unless needed.

Continues to parse Cert/CRL dates on creation in order to detect
errors, but stores them as longs since the epoch rather than
Dates and lazily creates Dates directly from them rather than
requiring a Calendar.
  • Loading branch information
@prbprbprb
prbprbprb authored Oct 22, 2023
1 parent 23ca210 commit 5daf264
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 107 deletions.
82 changes: 19 additions & 63 deletions common/src/jni/main/cpp/conscrypt/native_crypto.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4861,6 +4861,20 @@ 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;
Expand All @@ -4875,7 +4889,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 reinterpret_cast<uintptr_t>(notBefore);
return ASN1_TIME_convert_to_posix(env, notBefore);
}

static jlong NativeCrypto_X509_get_notAfter(JNIEnv* env, jclass, jlong x509Ref,
Expand All @@ -4892,7 +4906,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 reinterpret_cast<uintptr_t>(notAfter);
return ASN1_TIME_convert_to_posix(env, notAfter);
}

// NOLINTNEXTLINE(runtime/int)
Expand Down Expand Up @@ -5528,7 +5542,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 reinterpret_cast<uintptr_t>(X509_REVOKED_get0_revocationDate(revoked));
return ASN1_TIME_convert_to_posix(env, X509_REVOKED_get0_revocationDate(revoked));
}

#ifdef __GNUC__
Expand Down Expand Up @@ -5622,7 +5636,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 reinterpret_cast<uintptr_t>(lastUpdate);
return ASN1_TIME_convert_to_posix(env, lastUpdate);
}

static jlong NativeCrypto_X509_CRL_get_nextUpdate(JNIEnv* env, jclass, jlong x509CrlRef,
Expand All @@ -5639,7 +5653,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 reinterpret_cast<uintptr_t>(nextUpdate);
return ASN1_TIME_convert_to_posix(env, nextUpdate);
}

static jbyteArray NativeCrypto_i2d_X509_REVOKED(JNIEnv* env, jclass, jlong x509RevokedRef) {
Expand All @@ -5663,63 +5677,6 @@ 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.
Expand Down Expand Up @@ -11218,7 +11175,6 @@ 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"),
Expand Down
4 changes: 0 additions & 4 deletions common/src/main/java/org/conscrypt/NativeCrypto.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -611,10 +611,6 @@ static native void X509_CRL_verify(long x509CrlCtx, OpenSSLX509CRL holder,

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 -------------------------------------------------------

/**
Expand Down
20 changes: 6 additions & 14 deletions common/src/main/java/org/conscrypt/OpenSSLX509CRL.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,23 +50,15 @@
*/
final class OpenSSLX509CRL extends X509CRL {
private volatile long mContext;
private final Date thisUpdate;
private final Date nextUpdate;
private final long thisUpdate;
private final long 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 = 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();
thisUpdate = NativeCrypto.X509_CRL_get_lastUpdate(mContext, this);
nextUpdate = NativeCrypto.X509_CRL_get_nextUpdate(mContext, this);
}

static OpenSSLX509CRL fromX509DerInputStream(InputStream is) throws ParsingException {
Expand Down Expand Up @@ -278,12 +270,12 @@ public X500Principal getIssuerX500Principal() {

@Override
public Date getThisUpdate() {
return (Date) thisUpdate.clone();
return new Date(thisUpdate);
}

@Override
public Date getNextUpdate() {
return (Date) nextUpdate.clone();
return new Date(nextUpdate);
}

@Override
Expand Down
6 changes: 3 additions & 3 deletions common/src/main/java/org/conscrypt/OpenSSLX509CRLEntry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@
*/
final class OpenSSLX509CRLEntry extends X509CRLEntry {
private final long mContext;
private final Date revocationDate;
private final long 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 = OpenSSLX509CRL.toDate(NativeCrypto.get_X509_REVOKED_revocationDate(mContext));
revocationDate = NativeCrypto.get_X509_REVOKED_revocationDate(mContext);
}

@Override
Expand Down Expand Up @@ -112,7 +112,7 @@ public BigInteger getSerialNumber() {

@Override
public Date getRevocationDate() {
return (Date) revocationDate.clone();
return new Date(revocationDate);
}

@Override
Expand Down
30 changes: 8 additions & 22 deletions common/src/main/java/org/conscrypt/OpenSSLX509Certificate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,29 +61,15 @@ public final class OpenSSLX509Certificate extends X509Certificate {
private transient volatile long mContext;
private transient Integer mHashCode;

private final Date notBefore;
private final Date notAfter;
private final long notBefore;
private final long 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 = 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();
notBefore = NativeCrypto.X509_get_notBefore(mContext, this);
notAfter = NativeCrypto.X509_get_notAfter(mContext, this);
}

public static OpenSSLX509Certificate fromX509DerInputStream(InputStream is)
Expand Down Expand Up @@ -260,12 +246,12 @@ public void checkValidity(Date date) throws CertificateExpiredException,
CertificateNotYetValidException {
if (getNotBefore().compareTo(date) > 0) {
throw new CertificateNotYetValidException("Certificate not valid until "
+ getNotBefore().toString() + " (compared to " + date.toString() + ")");
+ getNotBefore() + " (compared to " + date + ")");
}

if (getNotAfter().compareTo(date) < 0) {
throw new CertificateExpiredException("Certificate expired at "
+ getNotAfter().toString() + " (compared to " + date.toString() + ")");
+ getNotAfter() + " (compared to " + date + ")");
}
}

Expand All @@ -291,12 +277,12 @@ public Principal getSubjectDN() {

@Override
public Date getNotBefore() {
return (Date) notBefore.clone();
return new Date(notBefore);
}

@Override
public Date getNotAfter() {
return (Date) notAfter.clone();
return new Date(notAfter);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion common/src/test/java/org/conscrypt/HostnameVerifierTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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" -newkey rsa:512
// -addext "subjectAltName=DNS:*.com" -
SSLSession session = session(""
+ "-----BEGIN CERTIFICATE-----\n"
+ "MIIBlTCCAT+gAwIBAgIUe1RB6C61ZW/SEQpKiywSEJOEOUMwDQYJKoZIhvcNAQEL\n"
Expand Down
1 change: 1 addition & 0 deletions openjdk/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,7 @@ model {

if (toolChain in Clang || toolChain in Gcc) {
cppCompiler.args "-Wall",
"-Werror",
"-fPIC",
"-O3",
"-std=c++17",
Expand Down

0 comments on commit 5daf264

Please sign in to comment.