From 611a5e6601151c1aa3af558b73eae63ad0bb29c3 Mon Sep 17 00:00:00 2001 From: dkimitsa Date: Wed, 8 May 2024 14:18:10 +0300 Subject: [PATCH] * SSL session caching/reusing disabled to prevent memory corruption # Context making multiple request to same host/port cause some of them terminated with message > error:14077102:SSL routines:SSL23_GET_SERVER_HELLO:unsupported protocol (/Users/tomski/Coding/asidik/robovm/target/checkout/compiler/vm/rt/android/external/openssl/ssl/s23_clnt.c:714 0x107f58871:0x00000000) (or application crashed random places) # root case Reusing same Session cause same native SSL_Session to be used with each opened OpenSSLSocketImpl. It associates it's native pointer with its SSL. ``` sessionToReuse = this.getCachedClientSession(clientSessionContext); if (sessionToReuse != null) { NativeCrypto.SSL_set_session(this.sslNativePointer, sessionToReuse.sslSessionNativePointer); } ``` As result multiple OpenSSLSocketImpl and its SSL will use same single session. Problem appear once this socked is being closed, as it destroys SSL by calling `NativeCrypto.SSL_free(sslNativePointer);` and SSL under hood destroys all elements it contains, and shared session as result. This cause single object to be multiple times released, released memory is used as valid -- this causes logic errors as described above and SIGABRT crashes. # The "fix" Properly fixing session sharing on Android 4.4.x code base is problematic as things are not implemented this way. In recent version of Libcore its handled completely different way. The way to prevent apps from crashing is to disable the feature. it will introduce longer TLS handshake. RoboVMx experimental port is not affected by this issue. --- .../org/conscrypt/ClientSessionContext.java | 126 ++++++++++-------- 1 file changed, 68 insertions(+), 58 deletions(-) diff --git a/compiler/rt/libcore/crypto/src/main/java/org/conscrypt/ClientSessionContext.java b/compiler/rt/libcore/crypto/src/main/java/org/conscrypt/ClientSessionContext.java index 27a39cfe9..82263a82a 100755 --- a/compiler/rt/libcore/crypto/src/main/java/org/conscrypt/ClientSessionContext.java +++ b/compiler/rt/libcore/crypto/src/main/java/org/conscrypt/ClientSessionContext.java @@ -30,8 +30,9 @@ public class ClientSessionContext extends AbstractSessionContext { * Sessions indexed by host and port. Protect from concurrent * access by holding a lock on sessionsByHostAndPort. */ - final Map sessionsByHostAndPort - = new HashMap(); +// dkimitsa: FIXME: session caching was disabled +// final Map sessionsByHostAndPort +// = new HashMap(); private SSLClientSessionCache persistentCache; @@ -40,7 +41,9 @@ public ClientSessionContext() { } public int size() { - return sessionsByHostAndPort.size(); +// dkimitsa: FIXME: session caching was disabled +// return sessionsByHostAndPort.size(); + return 0; } public void setPersistentCache(SSLClientSessionCache persistentCache) { @@ -48,15 +51,16 @@ public void setPersistentCache(SSLClientSessionCache persistentCache) { } protected void sessionRemoved(SSLSession session) { - String host = session.getPeerHost(); - int port = session.getPeerPort(); - if (host == null) { - return; - } - HostAndPort hostAndPortKey = new HostAndPort(host, port); - synchronized (sessionsByHostAndPort) { - sessionsByHostAndPort.remove(hostAndPortKey); - } +// dkimitsa: FIXME: session caching was disabled +// String host = session.getPeerHost(); +// int port = session.getPeerPort(); +// if (host == null) { +// return; +// } +// HostAndPort hostAndPortKey = new HostAndPort(host, port); +// synchronized (sessionsByHostAndPort) { +// sessionsByHostAndPort.remove(hostAndPortKey); +// } } /** @@ -67,58 +71,64 @@ protected void sessionRemoved(SSLSession session) { * @return cached session or null if none found */ public SSLSession getSession(String host, int port) { - if (host == null) { - return null; - } - SSLSession session; - HostAndPort hostAndPortKey = new HostAndPort(host, port); - synchronized (sessionsByHostAndPort) { - session = sessionsByHostAndPort.get(hostAndPortKey); - } - if (session != null && session.isValid()) { - return session; - } - - // Look in persistent cache. - if (persistentCache != null) { - byte[] data = persistentCache.getSessionData(host, port); - if (data != null) { - session = toSession(data, host, port); - if (session != null && session.isValid()) { - super.putSession(session); - synchronized (sessionsByHostAndPort) { - sessionsByHostAndPort.put(hostAndPortKey, session); - } - return session; - } - } - } +// dkimitsa: FIXME: session caching was disabled as current implementation cause +// multiple usage of SINGLE native session. +// ISSUE: this session was de-allocated multiple times in OpenSSLSocketImpl.free +// as SSL_free also frees session +// +// if (host == null) { +// return null; +// } +// SSLSession session; +// HostAndPort hostAndPortKey = new HostAndPort(host, port); +// synchronized (sessionsByHostAndPort) { +// session = sessionsByHostAndPort.get(hostAndPortKey); +// } +// if (session != null && session.isValid()) { +// return session; +// } +// +// // Look in persistent cache. +// if (persistentCache != null) { +// byte[] data = persistentCache.getSessionData(host, port); +// if (data != null) { +// session = toSession(data, host, port); +// if (session != null && session.isValid()) { +// super.putSession(session); +// synchronized (sessionsByHostAndPort) { +// sessionsByHostAndPort.put(hostAndPortKey, session); +// } +// return session; +// } +// } +// } return null; } @Override public void putSession(SSLSession session) { - super.putSession(session); - - String host = session.getPeerHost(); - int port = session.getPeerPort(); - if (host == null) { - return; - } - - HostAndPort hostAndPortKey = new HostAndPort(host, port); - synchronized (sessionsByHostAndPort) { - sessionsByHostAndPort.put(hostAndPortKey, session); - } - - // TODO: This in a background thread. - if (persistentCache != null) { - byte[] data = toBytes(session); - if (data != null) { - persistentCache.putSessionData(session, data); - } - } +// dkimitsa: FIXME: session caching was disabled +// super.putSession(session); +// +// String host = session.getPeerHost(); +// int port = session.getPeerPort(); +// if (host == null) { +// return; +// } +// +// HostAndPort hostAndPortKey = new HostAndPort(host, port); +// synchronized (sessionsByHostAndPort) { +// sessionsByHostAndPort.put(hostAndPortKey, session); +// } +// +// // TODO: This in a background thread. +// if (persistentCache != null) { +// byte[] data = toBytes(session); +// if (data != null) { +// persistentCache.putSessionData(session, data); +// } +// } } static class HostAndPort {