From 3be111f6f7703200e9213f7806a16b84683b7ea6 Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Fri, 4 Aug 2023 11:45:30 +0100 Subject: [PATCH] Add locking around AbstractSessionContext JNI. We don't have a definitive root cause for #1131 but it seems like either use-after-free (e.g. finalizer ordering) or concurrency issue, so: 1. Make the native pointer private and move all accesses into AbstractSessionContext 2. Zero it out on finalisation 3. Add locking. Note we only need a read lock for the sslNew() path as this is thread safe and doesn't modify the native SSL_CTX aside from atomic refcounts. The above change is broadly equivalent to turning the native pointer into a NativeRef, which would mean its finalizer shouldn't run until after the AbstractSessionContext object is unreachable, but (currently) NativeRefs don't zero out the native address on finalization. --- .../org/conscrypt/AbstractSessionContext.java | 73 +++++++++++++++++-- .../main/java/org/conscrypt/NativeSsl.java | 3 +- .../org/conscrypt/ServerSessionContext.java | 2 +- 3 files changed, 68 insertions(+), 10 deletions(-) diff --git a/common/src/main/java/org/conscrypt/AbstractSessionContext.java b/common/src/main/java/org/conscrypt/AbstractSessionContext.java index c527b5883..180eab97f 100644 --- a/common/src/main/java/org/conscrypt/AbstractSessionContext.java +++ b/common/src/main/java/org/conscrypt/AbstractSessionContext.java @@ -22,6 +22,10 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.NoSuchElementException; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; + +import javax.net.ssl.SSLException; import javax.net.ssl.SSLSession; import javax.net.ssl.SSLSessionContext; @@ -39,7 +43,10 @@ abstract class AbstractSessionContext implements SSLSessionContext { private volatile int maximumSize; private volatile int timeout = DEFAULT_SESSION_TIMEOUT_SECONDS; - final long sslCtxNativePointer = NativeCrypto.SSL_CTX_new(); + private volatile long sslCtxNativePointer = NativeCrypto.SSL_CTX_new(); + + private final ReadWriteLock lock = new ReentrantReadWriteLock(); + private final Map sessions = new LinkedHashMap() { @@ -151,11 +158,7 @@ public final void setSessionTimeout(int seconds) throws IllegalArgumentException // setSessionTimeout(0) is defined to remove the timeout, but passing 0 // to SSL_CTX_set_timeout in BoringSSL sets it to the default timeout instead. // Pass INT_MAX seconds (68 years), since that's equivalent for practical purposes. - if (seconds > 0) { - NativeCrypto.SSL_CTX_set_timeout(sslCtxNativePointer, this, seconds); - } else { - NativeCrypto.SSL_CTX_set_timeout(sslCtxNativePointer, this, Integer.MAX_VALUE); - } + setTimeout(seconds > 0 ? seconds : Integer.MAX_VALUE); Iterator i = sessions.values().iterator(); while (i.hasNext()) { @@ -171,6 +174,17 @@ public final void setSessionTimeout(int seconds) throws IllegalArgumentException } } + private void setTimeout(int seconds) { + lock.writeLock().lock(); + try { + if (isValid()) { + NativeCrypto.SSL_CTX_set_timeout(sslCtxNativePointer, this, seconds); + } + } finally { + lock.writeLock().unlock(); + } + } + @Override public final void setSessionCacheSize(int size) throws IllegalArgumentException { if (size < 0) { @@ -186,11 +200,56 @@ public final void setSessionCacheSize(int size) throws IllegalArgumentException } } + // Should only be called with |lock| held. + private boolean isValid() { + return (sslCtxNativePointer != 0); + } + + /** + * Returns a native pointer to a new SSL object in this SSL_CTX. + */ + long newSsl() throws SSLException { + lock.readLock().lock(); + try { + if (isValid()) { + return NativeCrypto.SSL_new(sslCtxNativePointer, this); + } else { + throw new SSLException("Invalid session context"); + } + } finally { + lock.readLock().unlock(); + } + } + + protected void setSesssionIdContext(byte[] bytes) { + lock.writeLock().lock(); + try { + if (isValid()) { + NativeCrypto.SSL_CTX_set_session_id_context(sslCtxNativePointer, this, bytes); + } + } finally { + lock.writeLock().unlock(); + } + } + + private void freeNative() { + lock.writeLock().lock(); + try { + if (isValid()) { + long toFree = sslCtxNativePointer; + sslCtxNativePointer = 0; + NativeCrypto.SSL_CTX_free(toFree, this); + } + } finally { + lock.writeLock().unlock(); + } + } + @Override @SuppressWarnings("deprecation") protected void finalize() throws Throwable { try { - NativeCrypto.SSL_CTX_free(sslCtxNativePointer, this); + freeNative(); } finally { super.finalize(); } diff --git a/common/src/main/java/org/conscrypt/NativeSsl.java b/common/src/main/java/org/conscrypt/NativeSsl.java index b7b02635f..79369caa8 100644 --- a/common/src/main/java/org/conscrypt/NativeSsl.java +++ b/common/src/main/java/org/conscrypt/NativeSsl.java @@ -76,8 +76,7 @@ private NativeSsl(long ssl, SSLParametersImpl parameters, static NativeSsl newInstance(SSLParametersImpl parameters, SSLHandshakeCallbacks handshakeCallbacks, AliasChooser chooser, PSKCallbacks pskCallbacks) throws SSLException { - AbstractSessionContext ctx = parameters.getSessionContext(); - long ssl = NativeCrypto.SSL_new(ctx.sslCtxNativePointer, ctx); + long ssl = parameters.getSessionContext().newSsl(); return new NativeSsl(ssl, parameters, handshakeCallbacks, chooser, pskCallbacks); } diff --git a/common/src/main/java/org/conscrypt/ServerSessionContext.java b/common/src/main/java/org/conscrypt/ServerSessionContext.java index ce7010d01..64fca9a79 100644 --- a/common/src/main/java/org/conscrypt/ServerSessionContext.java +++ b/common/src/main/java/org/conscrypt/ServerSessionContext.java @@ -42,7 +42,7 @@ public final class ServerSessionContext extends AbstractSessionContext { // sure you don't reuse sessions externalized with i2d_SSL_SESSION // between apps. However our sessions are either in memory or // exported to a app's SSLServerSessionCache. - NativeCrypto.SSL_CTX_set_session_id_context(sslCtxNativePointer, this, new byte[] { ' ' }); + setSesssionIdContext(new byte[] { ' ' }); } /**