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
…785)

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

* * made it conditional, to allow enabling if required
  • Loading branch information
dkimitsa authored May 8, 2024
1 parent 1d11deb commit 833f6d3
Showing 1 changed file with 17 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@
*/
public class ClientSessionContext extends AbstractSessionContext {

// 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
// can be enabled by declaring SSL_FAULTY_RFC4507_ON property.
// it known to cause connection issues and application crashes.
// check #601 #585 #557 #306
final boolean sessionCachingEnabled = System.getProperty("SSL_FAULTY_RFC4507_ON") != null;

/**
* Sessions indexed by host and port. Protect from concurrent
* access by holding a lock on sessionsByHostAndPort.
Expand All @@ -40,6 +49,8 @@ public ClientSessionContext() {
}

public int size() {
if (!sessionCachingEnabled) return 0;

return sessionsByHostAndPort.size();
}

Expand All @@ -48,6 +59,8 @@ public void setPersistentCache(SSLClientSessionCache persistentCache) {
}

protected void sessionRemoved(SSLSession session) {
if (!sessionCachingEnabled) return;

String host = session.getPeerHost();
int port = session.getPeerPort();
if (host == null) {
Expand All @@ -67,6 +80,8 @@ protected void sessionRemoved(SSLSession session) {
* @return cached session or null if none found
*/
public SSLSession getSession(String host, int port) {
if (!sessionCachingEnabled) return null;

if (host == null) {
return null;
}
Expand Down Expand Up @@ -99,6 +114,8 @@ public SSLSession getSession(String host, int port) {

@Override
public void putSession(SSLSession session) {
if (!sessionCachingEnabled) return;

super.putSession(session);

String host = session.getPeerHost();
Expand Down

0 comments on commit 833f6d3

Please sign in to comment.