Correctly synchronize before trying to set key material to fix possib…

…le native crash (netty#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

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<String> aliases = new HashSet<String>(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 @@ void setKeyMaterialClientSide(ReferenceCountedOpenSslEngine engine, String[] key
         }
     }
 
-    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) {

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java

@@ -261,6 +261,10 @@ private static final class OpenSslClientCertificateCallback implements Certifica
         @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<String> keyTypesSet = supportedClientKeyTypes(keyTypeBytes);
                 final String[] keyTypes = keyTypesSet.toArray(new String[0]);

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java

@@ -691,8 +691,12 @@ abstract static class AbstractCertificateVerifier extends CertificateVerifier {
 
         @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;

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

@@ -48,6 +48,7 @@
 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;
@@ -370,9 +371,29 @@ public List<byte[]> getStatusResponses() {
         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");
     }
 
     /**
@@ -389,7 +410,9 @@ public void setOcspResponse(byte[] response) {
         }
 
         synchronized (this) {
-            SSL.setOcspResponse(ssl, response);
+            if (!isDestroyed()) {
+                SSL.setOcspResponse(ssl, response);
+            }
         }
     }
 
@@ -407,6 +430,9 @@ public byte[] getOcspResponse() {
         }
 
         synchronized (this) {
+            if (isDestroyed()) {
+                return EmptyArrays.EMPTY_BYTES;
+            }
             return SSL.getOcspResponse(ssl);
         }
     }

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java

@@ -206,6 +206,10 @@ private static final class OpenSslServerCertificateCallback implements Certifica
         @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.

handler/src/main/java/io/netty/handler/ssl/SslMasterKeyHandler.java

@@ -18,7 +18,6 @@
 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 final void userEventTriggered(ChannelHandlerContext ctx, Object evt) {
                     }
                     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);
                 }
             }