Skip to content

Commit

Permalink
* SSL session caching/reusing disabled to prevent memory corruption
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
dkimitsa committed May 8, 2024
1 parent 1d11deb commit 611a5e6
Showing 1 changed file with 68 additions and 58 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<HostAndPort, SSLSession> sessionsByHostAndPort
= new HashMap<HostAndPort, SSLSession>();
// dkimitsa: FIXME: session caching was disabled
// final Map<HostAndPort, SSLSession> sessionsByHostAndPort
// = new HashMap<HostAndPort, SSLSession>();

private SSLClientSessionCache persistentCache;

Expand All @@ -40,23 +41,26 @@ public ClientSessionContext() {
}

public int size() {
return sessionsByHostAndPort.size();
// dkimitsa: FIXME: session caching was disabled
// return sessionsByHostAndPort.size();
return 0;
}

public void setPersistentCache(SSLClientSessionCache persistentCache) {
this.persistentCache = 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);
// }
}

/**
Expand All @@ -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 {
Expand Down

0 comments on commit 611a5e6

Please sign in to comment.