From aae7cdca96130a22a3eeb77c69c4846e71facfc6 Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Fri, 12 Oct 2018 09:27:46 +0200
Subject: [PATCH] Prevent NPE when attempting to set client key material with
 no alias (#8378)

Motivation:

It is possible that a client is unable to locate a certificate alias given the list of issuers and key types. In this case the X509KeyManager
will return a null which when past to the OpenSslKeyMaterialProvider implementation may produce a NPE. If no matching alias could be found we should not
call OpenSslKeyMaterialProvider at all which is also consistent what OpenJDK does.

Modifications:

- Add null check before calling OpenSslKeyMaterialProvider
- Add unit test.

Result:

No more NPE caused by passing null as client alias.
---
 .../ssl/OpenSslKeyMaterialManager.java        |  7 +-
 .../ssl/OpenSslKeyMaterialManagerTest.java    | 83 +++++++++++++++++++
 2 files changed, 89 insertions(+), 1 deletion(-)
 create mode 100644 handler/src/test/java/io/netty/handler/ssl/OpenSslKeyMaterialManagerTest.java

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 8b8a41d114..94398d5cf2 100644
--- a/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java
+++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java
@@ -83,7 +83,12 @@ final class OpenSslKeyMaterialManager {
     void setKeyMaterialClientSide(ReferenceCountedOpenSslEngine engine, String[] keyTypes,
                                   X500Principal[] issuer) throws SSLException {
         String alias = chooseClientAlias(engine, keyTypes, issuer);
-        setKeyMaterial(engine, alias);
+        // Only try to set the keymaterial if we have a match. This is also consistent with what OpenJDK does:
+        // http://hg.openjdk.java.net/jdk/jdk11/file/76072a077ee1/
+        // src/java.base/share/classes/sun/security/ssl/CertificateRequest.java#l362
+        if (alias != null) {
+            setKeyMaterial(engine, alias);
+        }
     }
 
     private void setKeyMaterial(ReferenceCountedOpenSslEngine engine, String alias) throws SSLException {
diff --git a/handler/src/test/java/io/netty/handler/ssl/OpenSslKeyMaterialManagerTest.java b/handler/src/test/java/io/netty/handler/ssl/OpenSslKeyMaterialManagerTest.java
new file mode 100644
index 0000000000..ae197a23e9
--- /dev/null
+++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslKeyMaterialManagerTest.java
@@ -0,0 +1,83 @@
+/*
+ * Copyright 2018 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.handler.ssl;
+
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.buffer.UnpooledByteBufAllocator;
+import io.netty.util.internal.EmptyArrays;
+import org.junit.Assert;
+import org.junit.Assume;
+import org.junit.Test;
+
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedKeyManager;
+import java.net.Socket;
+import java.security.Principal;
+import java.security.PrivateKey;
+import java.security.cert.X509Certificate;
+
+public class OpenSslKeyMaterialManagerTest {
+
+    @Test
+    public void testChooseClientAliasReturnsNull() throws SSLException {
+        Assume.assumeTrue(OpenSsl.isAvailable());
+
+        X509ExtendedKeyManager keyManager = new X509ExtendedKeyManager() {
+            @Override
+            public String[] getClientAliases(String s, Principal[] principals) {
+                return EmptyArrays.EMPTY_STRINGS;
+            }
+
+            @Override
+            public String chooseClientAlias(String[] strings, Principal[] principals, Socket socket) {
+                return null;
+            }
+
+            @Override
+            public String[] getServerAliases(String s, Principal[] principals) {
+                return EmptyArrays.EMPTY_STRINGS;
+            }
+
+            @Override
+            public String chooseServerAlias(String s, Principal[] principals, Socket socket) {
+                return null;
+            }
+
+            @Override
+            public X509Certificate[] getCertificateChain(String s) {
+                return EmptyArrays.EMPTY_X509_CERTIFICATES;
+            }
+
+            @Override
+            public PrivateKey getPrivateKey(String s) {
+                return null;
+            }
+        };
+
+        OpenSslKeyMaterialManager manager = new OpenSslKeyMaterialManager(
+                new OpenSslKeyMaterialProvider(keyManager, null) {
+            @Override
+            OpenSslKeyMaterial chooseKeyMaterial(ByteBufAllocator allocator, String alias) throws Exception {
+                Assert.fail("Should not be called when alias is null");
+                return null;
+            }
+        });
+        SslContext context = SslContextBuilder.forClient().sslProvider(SslProvider.OPENSSL).build();
+        OpenSslEngine engine =
+                (OpenSslEngine) context.newEngine(UnpooledByteBufAllocator.DEFAULT);
+        manager.setKeyMaterialClientSide(engine, EmptyArrays.EMPTY_STRINGS, null);
+    }
+}