Skip to content

Commit

Permalink
Add locking around AbstractSessionContext JNI.
Browse files Browse the repository at this point in the history
We don't have a definitive root cause for google#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.
  • Loading branch information
prbprbprb committed Aug 4, 2023
1 parent 92a8a67 commit 3be111f
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 10 deletions.
73 changes: 66 additions & 7 deletions common/src/main/java/org/conscrypt/AbstractSessionContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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<ByteArray, NativeSslSession> sessions =
new LinkedHashMap<ByteArray, NativeSslSession>() {
Expand Down Expand Up @@ -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<NativeSslSession> i = sessions.values().iterator();
while (i.hasNext()) {
Expand All @@ -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) {
Expand All @@ -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();
}
Expand Down
3 changes: 1 addition & 2 deletions common/src/main/java/org/conscrypt/NativeSsl.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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[] { ' ' });
}

/**
Expand Down

0 comments on commit 3be111f

Please sign in to comment.