From cda8ee95b3868b4990be5304a4dec650a26ce00a Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Mon, 16 Sep 2019 11:14:08 +0200 Subject: [PATCH] Correctly synchronize before trying to set key material to fix possible native crash (#9566) Motivation: When using io.netty.handler.ssl.openssl.useTasks=true we may call ReferenceCountedOpenSslEngine.setKeyMaterial(...) from another thread and so need to synchronize and also check if the engine was destroyed in the meantime to eliminate of the possibility of a native crash. The same is try when trying to access the authentication methods. Modification: - Add synchronized and isDestroyed() checks where missing - Add null checks for the case when a callback is executed by another thread after the engine was destroyed already - Move code for master key extraction to ReferenceCountedOpenSslEngine to ensure there can be no races. Result: No native crash possible anymore when using io.netty.handler.ssl.openssl.useTasks=true --- .../ssl/OpenSslKeyMaterialManager.java | 18 +++++------ .../ReferenceCountedOpenSslClientContext.java | 4 +++ .../ssl/ReferenceCountedOpenSslContext.java | 6 +++- .../ssl/ReferenceCountedOpenSslEngine.java | 32 +++++++++++++++++-- .../ReferenceCountedOpenSslServerContext.java | 4 +++ .../handler/ssl/SslMasterKeyHandler.java | 4 +-- 6 files changed, 52 insertions(+), 16 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java index 0e2e2c3fd2..8872c3466f 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java @@ -15,8 +15,6 @@ */ package io.netty.handler.ssl; -import io.netty.internal.tcnative.SSL; - import javax.net.ssl.SSLException; import javax.net.ssl.X509ExtendedKeyManager; import javax.net.ssl.X509KeyManager; @@ -66,15 +64,19 @@ final class OpenSslKeyMaterialManager { } void setKeyMaterialServerSide(ReferenceCountedOpenSslEngine engine) throws SSLException { - long ssl = engine.sslPointer(); - String[] authMethods = SSL.authenticationMethods(ssl); + String[] authMethods = engine.authMethods(); + if (authMethods.length == 0) { + return; + } Set aliases = new HashSet<>(authMethods.length); for (String authMethod : authMethods) { String type = KEY_TYPES.get(authMethod); if (type != null) { String alias = chooseServerAlias(engine, type); if (alias != null && aliases.add(alias)) { - setKeyMaterial(engine, alias); + if (!setKeyMaterial(engine, alias)) { + return; + } } } } @@ -91,13 +93,11 @@ final class OpenSslKeyMaterialManager { } } - private void setKeyMaterial(ReferenceCountedOpenSslEngine engine, String alias) throws SSLException { + private boolean setKeyMaterial(ReferenceCountedOpenSslEngine engine, String alias) throws SSLException { OpenSslKeyMaterial keyMaterial = null; try { keyMaterial = provider.chooseKeyMaterial(engine.alloc, alias); - if (keyMaterial != null) { - engine.setKeyMaterial(keyMaterial); - } + return keyMaterial == null || engine.setKeyMaterial(keyMaterial); } catch (SSLException e) { throw e; } catch (Exception e) { diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java index 11ee5d7605..094976f111 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java @@ -261,6 +261,10 @@ public final class ReferenceCountedOpenSslClientContext extends ReferenceCounted @Override public void handle(long ssl, byte[] keyTypeBytes, byte[][] asn1DerEncodedPrincipals) throws Exception { final ReferenceCountedOpenSslEngine engine = engineMap.get(ssl); + // May be null if it was destroyed in the meantime. + if (engine == null) { + return; + } try { final Set keyTypesSet = supportedClientKeyTypes(keyTypeBytes); final String[] keyTypes = keyTypesSet.toArray(new String[0]); diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java index 5209daaa5e..a83b380926 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java @@ -679,8 +679,12 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen @Override public final int verify(long ssl, byte[][] chain, String auth) { - X509Certificate[] peerCerts = certificates(chain); final ReferenceCountedOpenSslEngine engine = engineMap.get(ssl); + if (engine == null) { + // May be null if it was destroyed in the meantime. + return CertificateVerifier.X509_V_ERR_UNSPECIFIED; + } + X509Certificate[] peerCerts = certificates(chain); try { verify(engine, peerCerts, auth); return CertificateVerifier.X509_V_OK; diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java index 32c99725e8..e77037504d 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -48,6 +48,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicIntegerFieldUpdater; import java.util.concurrent.locks.Lock; +import javax.crypto.spec.SecretKeySpec; import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLEngineResult; import javax.net.ssl.SSLException; @@ -366,9 +367,29 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc leak = leakDetection ? leakDetector.track(this) : null; } - final void setKeyMaterial(OpenSslKeyMaterial keyMaterial) throws Exception { - SSL.setKeyMaterial(ssl, keyMaterial.certificateChainAddress(), keyMaterial.privateKeyAddress()); + final synchronized String[] authMethods() { + if (isDestroyed()) { + return EmptyArrays.EMPTY_STRINGS; + } + return SSL.authenticationMethods(ssl); + } + + final boolean setKeyMaterial(OpenSslKeyMaterial keyMaterial) throws Exception { + synchronized (this) { + if (isDestroyed()) { + return false; + } + SSL.setKeyMaterial(ssl, keyMaterial.certificateChainAddress(), keyMaterial.privateKeyAddress()); + } localCertificateChain = keyMaterial.certificateChain(); + return true; + } + + final synchronized SecretKeySpec masterKey() { + if (isDestroyed()) { + return null; + } + return new SecretKeySpec(SSL.getMasterKey(ssl), "AES"); } /** @@ -385,7 +406,9 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } synchronized (this) { - SSL.setOcspResponse(ssl, response); + if (!isDestroyed()) { + SSL.setOcspResponse(ssl, response); + } } } @@ -403,6 +426,9 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } synchronized (this) { + if (isDestroyed()) { + return EmptyArrays.EMPTY_BYTES; + } return SSL.getOcspResponse(ssl); } } diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java index 6bb099ca40..53f7fab23f 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java @@ -202,6 +202,10 @@ public final class ReferenceCountedOpenSslServerContext extends ReferenceCounted @Override public void handle(long ssl, byte[] keyTypeBytes, byte[][] asn1DerEncodedPrincipals) throws Exception { final ReferenceCountedOpenSslEngine engine = engineMap.get(ssl); + if (engine == null) { + // Maybe null if destroyed in the meantime. + return; + } try { // For now we just ignore the asn1DerEncodedPrincipals as this is kind of inline with what the // OpenJDK SSLEngineImpl does. diff --git a/handler/src/main/java/io/netty/handler/ssl/SslMasterKeyHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslMasterKeyHandler.java index ef1ef9bbe2..b1c710c841 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslMasterKeyHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslMasterKeyHandler.java @@ -18,7 +18,6 @@ package io.netty.handler.ssl; import io.netty.buffer.ByteBufUtil; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInboundHandlerAdapter; -import io.netty.internal.tcnative.SSL; import io.netty.util.internal.ReflectionUtil; import io.netty.util.internal.SystemPropertyUtil; import io.netty.util.internal.logging.InternalLogger; @@ -140,8 +139,7 @@ public abstract class SslMasterKeyHandler extends ChannelInboundHandlerAdapter { } accept(secretKey, sslSession); } else if (OpenSsl.isAvailable() && engine instanceof ReferenceCountedOpenSslEngine) { - SecretKeySpec secretKey = new SecretKeySpec( - SSL.getMasterKey(((ReferenceCountedOpenSslEngine) engine).sslPointer()), "AES"); + SecretKeySpec secretKey = ((ReferenceCountedOpenSslEngine) engine).masterKey(); accept(secretKey, sslSession); } }