Better hash algorithm in FingerprintTrustManagerFactory (#10683)

Motivation:

FingerprintTrustManagerFactory can only use SHA-1 that is considered
insecure.

Modifications:

- Updated FingerprintTrustManagerFactory to accept a stronger hash algorithm.
- Deprecated the constructors that still use SHA-1.
- Added a test for FingerprintTrustManagerFactory.

Result:

A user can now configure FingerprintTrustManagerFactory to use a
stronger hash algorithm.
The compiler shows a warning if the code still uses one of the
unsafe constructors.

Co-authored-by: Norman Maurer <norman_maurer@apple.com>

handler/src/main/java/io/netty/handler/ssl/util/FingerprintTrustManagerFactory.java

@@ -39,7 +39,7 @@ import java.util.List;
 import java.util.regex.Pattern;
 
 /**
- * An {@link TrustManagerFactory} that trusts an X.509 certificate whose SHA1 checksum matches.
+ * An {@link TrustManagerFactory} that trusts an X.509 certificate whose hash matches.
  * <p>
  * <strong>NOTE:</strong> It is recommended to verify certificates and their chain to prevent
  * <a href="https://en.wikipedia.org/wiki/Man-in-the-middle_attack">Man-in-the-middle attacks</a>.
@@ -51,22 +51,30 @@ import java.util.regex.Pattern;
  * actually perform Man-in-the-middle attacks and thus present a different certificate fingerprint.
  * </p>
  * <p>
- * The SHA1 checksum of an X.509 certificate is calculated from its DER encoded format.  You can get the fingerprint of
+ * The hash of an X.509 certificate is calculated from its DER encoded format.  You can get the fingerprint of
  * an X.509 certificate using the {@code openssl} command.  For example:
  *
  * <pre>
- * $ openssl x509 -fingerprint -sha1 -in my_certificate.crt
+ * $ openssl x509 -fingerprint -sha256 -in my_certificate.crt
- * SHA1 Fingerprint=4E:85:10:55:BC:7B:12:08:D1:EA:0A:12:C9:72:EE:F3:AA:B2:C7:CB
+ * SHA256 Fingerprint=1C:53:0E:6B:FF:93:F0:DE:C2:E6:E7:9D:10:53:58:FF:DD:8E:68:CD:82:D9:C9:36:9B:43:EE:B3:DC:13:68:FB
  * -----BEGIN CERTIFICATE-----
- * MIIBqjCCAROgAwIBAgIJALiT3Nvp0kvmMA0GCSqGSIb3DQEBBQUAMBYxFDASBgNV
+ * MIIC/jCCAeagAwIBAgIIIMONxElm0AIwDQYJKoZIhvcNAQELBQAwPjE8MDoGA1UE
- * BAMTC2V4YW1wbGUuY29tMCAXDTcwMDEwMTAwMDAwMFoYDzk5OTkxMjMxMjM1OTU5
+ * AwwzZThhYzAyZmEwZDY1YTg0MjE5MDE2MDQ1ZGI4YjA1YzQ4NWI0ZWNkZi5uZXR0
- * WjAWMRQwEgYDVQQDEwtleGFtcGxlLmNvbTCBnzANBgkqhkiG9w0BAQEFAAOBjQAw
+ * eS50ZXN0MCAXDTEzMDgwMjA3NTEzNloYDzk5OTkxMjMxMjM1OTU5WjA+MTwwOgYD
- * gYkCgYEAnadvODG0QCiHhaFZlLHtr5gLIkDQS8ErZ//KfqeCHTC/KJsl3xYFk0zG
+ * VQQDDDNlOGFjMDJmYTBkNjVhODQyMTkwMTYwNDVkYjhiMDVjNDg1YjRlY2RmLm5l
- * aCv2FcmkOlokm77qV8qOW2DZdND7WuYzX6nLVuLb+GYxZ7b45iMAbAajvGh8jc9U
+ * dHR5LnRlc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDb+HBO3C0U
- * o07fUIahGqTDAIAGCWsoLUOQ9nMzO/8GRHcXJAeQ2MGY2VpCcv0CAwEAATANBgkq
+ * RBKvDUgJHbhIlBye8X/cbNH3lDq3XOOFBz7L4XZKLDIXS+FeQqSAUMo2otmU+Vkj
- * hkiG9w0BAQUFAAOBgQBpRCnmjmNM0D7yrpkUJpBTNiqinhKLbeOvPWm+YmdInUUs
+ * 0KorshMjbUXfE1KkTijTMJlaga2M2xVVt21fRIkJNWbIL0dWFLWyRq7OXdygyFkI
- * LoMu0mZ1IANemLwqbwJJ76fknngeB+YuVAj46SurvVCV6ekwHcbgpW1u063IRwKk
+ * iW9b2/LYaePBgET22kbtHSCAEj+BlSf265+1rNxyAXBGGGccCKzEbcqASBKHOgVp
- * tQhOBO0HQxldUS4+4MYv/kuvnKkbjfgh5qfWw89Kx4kD+cycpP4yPtgDGk8ZMA==
+ * 6pLqlQAfuSy6g/OzGzces3zXRrGu1N3pBIzAIwCW429n52ZlYfYR0nr+REKDnRrP
+ * IIDsWASmEHhBezTD+v0qCJRyLz2usFgWY+7agUJE2yHHI2mTu2RAFngBilJXlMCt
+ * VwT0xGuQxkbHAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAEv8N7Xm8qaY2FgrOc6P
+ * a1GTgA+AOb3aU33TGwAR86f+nLf6BSPaohcQfOeJid7FkFuYInuXl+oqs+RqM/j8
+ * R0E5BuGYY2wOKpL/PbFi1yf/Kyvft7KVh8e1IUUec/i1DdYTDB0lNWvXXxjfMKGL
+ * ct3GMbEHKvLfHx42Iwz/+fva6LUrO4u2TDfv0ycHuR7UZEuC1DJ4xtFhbpq/QRAj
+ * CyfNx3cDc7L2EtJWnCmivTFA9l8MF1ZPMDSVd4ecQ7B0xZIFQ5cSSFt7WGaJCsGM
+ * zYkU4Fp4IykQcWxdlNX7wJZRwQ2TZJFFglpTiFZdeq6I6Ad9An1Encpz5W8UJ4tv
+ * hmw=
  * -----END CERTIFICATE-----
  * </pre>
  * </p>
@@ -75,20 +83,18 @@ public final class FingerprintTrustManagerFactory extends SimpleTrustManagerFact
 
     private static final Pattern FINGERPRINT_PATTERN = Pattern.compile("^[0-9a-fA-F:]+$");
     private static final Pattern FINGERPRINT_STRIP_PATTERN = Pattern.compile(":");
-    private static final int SHA1_BYTE_LEN = 20;
-    private static final int SHA1_HEX_LEN = SHA1_BYTE_LEN * 2;
 
-    private static final FastThreadLocal<MessageDigest> tlmd = new FastThreadLocal<MessageDigest>() {
+    /**
-        @Override
+     * Creates a builder for {@link FingerprintTrustManagerFactory}.
-        protected MessageDigest initialValue() {
+     *
-            try {
+     * @param algorithm a hash algorithm
-                return MessageDigest.getInstance("SHA1");
+     * @return a builder
-            } catch (NoSuchAlgorithmException e) {
+     */
-                // All Java implementation must have SHA1 digest algorithm.
+    public static FingerprintTrustManagerFactoryBuilder builder(String algorithm) {
-                throw new Error(e);
+        return new FingerprintTrustManagerFactoryBuilder(algorithm);
-            }
+    }
-        }
+
-    };
+    private final FastThreadLocal<MessageDigest> tlmd;
 
     private final TrustManager tm = new X509TrustManager() {
 
@@ -136,45 +142,99 @@ public final class FingerprintTrustManagerFactory extends SimpleTrustManagerFact
     /**
      * Creates a new instance.
      *
+     * @deprecated This deprecated constructor uses SHA-1 that is considered insecure.
+     *      It is recommended to specify a stronger hash algorithm, such as SHA-256,
+     *      by calling {@link FingerprintTrustManagerFactory#builder(String)} method.
+     *
      * @param fingerprints a list of SHA1 fingerprints in hexadecimal form
      */
+    @Deprecated
     public FingerprintTrustManagerFactory(Iterable<String> fingerprints) {
-        this(toFingerprintArray(fingerprints));
+        this("SHA1", toFingerprintArray(fingerprints));
     }
 
     /**
      * Creates a new instance.
      *
+     * @deprecated This deprecated constructor uses SHA-1 that is considered insecure.
+     *      It is recommended to specify a stronger hash algorithm, such as SHA-256,
+     *      by calling {@link FingerprintTrustManagerFactory#builder(String)} method.
+     *
      * @param fingerprints a list of SHA1 fingerprints in hexadecimal form
      */
+    @Deprecated
     public FingerprintTrustManagerFactory(String... fingerprints) {
-        this(toFingerprintArray(Arrays.asList(fingerprints)));
+        this("SHA1", toFingerprintArray(Arrays.asList(fingerprints)));
     }
 
     /**
      * Creates a new instance.
      *
+     * @deprecated This deprecated constructor uses SHA-1 that is considered insecure.
+     *      It is recommended to specify a stronger hash algorithm, such as SHA-256,
+     *      by calling {@link FingerprintTrustManagerFactory#builder(String)} method.
+     *
      * @param fingerprints a list of SHA1 fingerprints
      */
+    @Deprecated
     public FingerprintTrustManagerFactory(byte[]... fingerprints) {
+        this("SHA1", fingerprints);
+    }
+
+    /**
+     * Creates a new instance.
+     *
+     * @param algorithm a hash algorithm
+     * @param fingerprints a list of fingerprints
+     */
+    FingerprintTrustManagerFactory(final String algorithm, byte[][] fingerprints) {
+        ObjectUtil.checkNotNull(algorithm, "algorithm");
         ObjectUtil.checkNotNull(fingerprints, "fingerprints");
 
+        if (fingerprints.length == 0) {
+            throw new IllegalArgumentException("No fingerprints provided");
+        }
+
+        // check early if the hash algorithm is available
+        final MessageDigest md;
+        try {
+            md = MessageDigest.getInstance(algorithm);
+        } catch (NoSuchAlgorithmException e) {
+            throw new IllegalArgumentException(
+                    String.format("Unsupported hash algorithm: %s", algorithm), e);
+        }
+
+        int hashLength = md.getDigestLength();
         List<byte[]> list = new ArrayList<byte[]>(fingerprints.length);
         for (byte[] f: fingerprints) {
             if (f == null) {
                 break;
             }
-            if (f.length != SHA1_BYTE_LEN) {
+            if (f.length != hashLength) {
-                throw new IllegalArgumentException("malformed fingerprint: " +
+                throw new IllegalArgumentException(
-                        ByteBufUtil.hexDump(Unpooled.wrappedBuffer(f)) + " (expected: SHA1)");
+                        String.format("malformed fingerprint (length is %d but expected %d): %s",
+                                      f.length, hashLength, ByteBufUtil.hexDump(Unpooled.wrappedBuffer(f))));
             }
             list.add(f.clone());
         }
 
+        this.tlmd = new FastThreadLocal<MessageDigest>() {
+
+            @Override
+            protected MessageDigest initialValue() {
+                try {
+                    return MessageDigest.getInstance(algorithm);
+                } catch (NoSuchAlgorithmException e) {
+                    throw new IllegalArgumentException(
+                            String.format("Unsupported hash algorithm: %s", algorithm), e);
+                }
+            }
+        };
+
         this.fingerprints = list.toArray(new byte[0][]);
     }
 
-    private static byte[][] toFingerprintArray(Iterable<String> fingerprints) {
+    static byte[][] toFingerprintArray(Iterable<String> fingerprints) {
         ObjectUtil.checkNotNull(fingerprints, "fingerprints");
 
         List<byte[]> list = new ArrayList<byte[]>();
@@ -187,9 +247,6 @@ public final class FingerprintTrustManagerFactory extends SimpleTrustManagerFact
                 throw new IllegalArgumentException("malformed fingerprint: " + f);
             }
             f = FINGERPRINT_STRIP_PATTERN.matcher(f).replaceAll("");
-            if (f.length() != SHA1_HEX_LEN) {
-                throw new IllegalArgumentException("malformed fingerprint: " + f + " (expected: SHA1)");
-            }
 
             list.add(StringUtil.decodeHexDump(f));
         }

handler/src/main/java/io/netty/handler/ssl/util/FingerprintTrustManagerFactoryBuilder.java

@@ -0,0 +1,89 @@
+/*
+ * Copyright 2020 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:
+ *
+ *   https://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.util;
+
+import io.netty.util.internal.ObjectUtil;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * A builder for creating {@link FingerprintTrustManagerFactory}.
+ */
+public final class FingerprintTrustManagerFactoryBuilder {
+
+    /**
+     * A hash algorithm for fingerprints.
+     */
+    private final String algorithm;
+
+    /**
+     * A list of fingerprints.
+     */
+    private final List<String> fingerprints = new ArrayList<String>();
+
+    /**
+     * Creates a builder.
+     *
+     * @param algorithm a hash algorithm
+     */
+    FingerprintTrustManagerFactoryBuilder(String algorithm) {
+        this.algorithm = ObjectUtil.checkNotNull(algorithm, "algorithm");
+    }
+
+    /**
+     * Adds fingerprints.
+     *
+     * @param fingerprints a number of fingerprints
+     * @return the same builder
+     */
+    public FingerprintTrustManagerFactoryBuilder fingerprints(CharSequence... fingerprints) {
+        ObjectUtil.checkNotNull(fingerprints, "fingerprints");
+        return fingerprints(Arrays.asList(fingerprints));
+    }
+
+    /**
+     * Adds fingerprints.
+     *
+     * @param fingerprints a number of fingerprints
+     * @return the same builder
+     */
+    public FingerprintTrustManagerFactoryBuilder fingerprints(Iterable<? extends CharSequence> fingerprints) {
+        ObjectUtil.checkNotNull(fingerprints, "fingerprints");
+        for (CharSequence fingerprint : fingerprints) {
+            if (fingerprint == null) {
+                throw new IllegalArgumentException("One of the fingerprints is null");
+            }
+            this.fingerprints.add(fingerprint.toString());
+        }
+        return this;
+    }
+
+    /**
+     * Creates a {@link FingerprintTrustManagerFactory}.
+     *
+     * @return a new {@link FingerprintTrustManagerFactory}
+     */
+    public FingerprintTrustManagerFactory build() {
+        if (fingerprints.isEmpty()) {
+            throw new IllegalStateException("No fingerprints provided");
+        }
+        return new FingerprintTrustManagerFactory(this.algorithm,
+                                                  FingerprintTrustManagerFactory.toFingerprintArray(this.fingerprints));
+    }
+}

handler/src/test/java/io/netty/handler/ssl/Java8SslTestUtils.java

@@ -22,11 +22,16 @@ import javax.net.ssl.SNIMatcher;
 import javax.net.ssl.SNIServerName;
 import javax.net.ssl.SSLEngine;
 import javax.net.ssl.SSLParameters;
+import java.io.InputStream;
 import java.security.Provider;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509Certificate;
 import java.util.Arrays;
 import java.util.Collections;
 
-final class Java8SslTestUtils {
+import static org.junit.Assert.assertNotNull;
+
+public final class Java8SslTestUtils {
 
     private Java8SslTestUtils() { }
 
@@ -53,4 +58,27 @@ final class Java8SslTestUtils {
         }
         return engine;
     }
+
+    public static X509Certificate[] loadCertCollection(String... resourceNames)
+            throws Exception {
+        CertificateFactory certFactory = CertificateFactory
+                .getInstance("X.509");
+
+        X509Certificate[] certCollection = new X509Certificate[resourceNames.length];
+        for (int i = 0; i < resourceNames.length; i++) {
+            String resourceName = resourceNames[i];
+            InputStream is = null;
+            try {
+                is = SslContextTest.class.getResourceAsStream(resourceName);
+                assertNotNull("Cannot find " + resourceName, is);
+                certCollection[i] = (X509Certificate) certFactory
+                        .generateCertificate(is);
+            } finally {
+                if (is != null) {
+                    is.close();
+                }
+            }
+        }
+        return certCollection;
+    }
 }

handler/src/test/java/io/netty/handler/ssl/SslContextTrustManagerTest.java

@@ -15,20 +15,18 @@
  */
 package io.netty.handler.ssl;
 
-import static org.junit.Assert.fail;
+import org.junit.Test;
-import static org.junit.Assert.assertNotNull;
-
-import java.io.InputStream;
-import java.security.cert.CertificateException;
-import java.security.cert.CertificateFactory;
-import java.security.cert.X509Certificate;
-import java.util.Arrays;
 
 import javax.net.ssl.TrustManager;
 import javax.net.ssl.TrustManagerFactory;
 import javax.net.ssl.X509TrustManager;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.Arrays;
 
-import org.junit.Test;
+import static io.netty.handler.ssl.Java8SslTestUtils.loadCertCollection;
+import static org.junit.Assert.fail;
+import static org.junit.Assert.assertNotNull;
 
 public class SslContextTrustManagerTest {
     @Test
@@ -121,27 +119,4 @@ public class SslContextTrustManagerTest {
         throw new Exception(
                 "Unable to find any X509TrustManager from this factory.");
     }
-
-    private static X509Certificate[] loadCertCollection(String[] resourceNames)
-            throws Exception {
-        CertificateFactory certFactory = CertificateFactory
-                .getInstance("X.509");
-
-        X509Certificate[] certCollection = new X509Certificate[resourceNames.length];
-        for (int i = 0; i < resourceNames.length; i++) {
-            String resourceName = resourceNames[i];
-            InputStream is = null;
-            try {
-                is = SslContextTest.class.getResourceAsStream(resourceName);
-                assertNotNull("Cannot find " + resourceName, is);
-                certCollection[i] = (X509Certificate) certFactory
-                        .generateCertificate(is);
-            } finally {
-                if (is != null) {
-                    is.close();
-                }
-            }
-        }
-        return certCollection;
-    }
 }

handler/src/test/java/io/netty/handler/ssl/util/FingerprintTrustManagerFactoryTest.java

@@ -0,0 +1,112 @@
+/*
+ * Copyright 2020 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:
+ *
+ *   https://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.util;
+
+import org.junit.Test;
+
+import javax.net.ssl.X509TrustManager;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+import static io.netty.handler.ssl.Java8SslTestUtils.loadCertCollection;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class FingerprintTrustManagerFactoryTest {
+
+    private static final String FIRST_CERT_SHA1_FINGERPRINT
+            = "18:C7:C2:76:1F:DF:72:3B:2A:A7:BB:2C:B0:30:D4:C0:C0:72:AD:84";
+
+    private static final String FIRST_CERT_SHA256_FINGERPRINT
+            = "1C:53:0E:6B:FF:93:F0:DE:C2:E6:E7:9D:10:53:58:FF:" +
+              "DD:8E:68:CD:82:D9:C9:36:9B:43:EE:B3:DC:13:68:FB";
+
+    private static final X509Certificate[] FIRST_CHAIN;
+
+    private static final X509Certificate[] SECOND_CHAIN;
+
+    static {
+        try {
+            FIRST_CHAIN = loadCertCollection("test.crt");
+            SECOND_CHAIN = loadCertCollection("test2.crt");
+        } catch (Exception e) {
+            throw new Error(e);
+        }
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testFingerprintWithInvalidLength() {
+        FingerprintTrustManagerFactory.builder("SHA-256").fingerprints("00:00:00").build();
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testFingerprintWithUnexpectedCharacters() {
+        FingerprintTrustManagerFactory.builder("SHA-256").fingerprints("00:00:00\n").build();
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void testWithNoFingerprints() {
+        FingerprintTrustManagerFactory.builder("SHA-256").build();
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testWithNullFingerprint() {
+        FingerprintTrustManagerFactory
+                .builder("SHA-256")
+                .fingerprints(FIRST_CERT_SHA256_FINGERPRINT, null)
+                .build();
+    }
+
+    @Test
+    public void testValidSHA1Fingerprint() throws Exception {
+        FingerprintTrustManagerFactory factory = new FingerprintTrustManagerFactory(FIRST_CERT_SHA1_FINGERPRINT);
+
+        assertTrue(factory.engineGetTrustManagers().length > 0);
+        assertTrue(factory.engineGetTrustManagers()[0] instanceof X509TrustManager);
+        X509TrustManager tm = (X509TrustManager) factory.engineGetTrustManagers()[0];
+        tm.checkClientTrusted(FIRST_CHAIN, "test");
+    }
+
+    @Test
+    public void testTrustedCertificateWithSHA256Fingerprint() throws Exception {
+        FingerprintTrustManagerFactory factory = FingerprintTrustManagerFactory
+                .builder("SHA-256")
+                .fingerprints(FIRST_CERT_SHA256_FINGERPRINT)
+                .build();
+
+        X509Certificate[] keyCertChain = loadCertCollection("test.crt");
+        assertNotNull(keyCertChain);
+        assertTrue(factory.engineGetTrustManagers().length > 0);
+        assertTrue(factory.engineGetTrustManagers()[0] instanceof X509TrustManager);
+        X509TrustManager tm = (X509TrustManager) factory.engineGetTrustManagers()[0];
+        tm.checkClientTrusted(keyCertChain, "test");
+    }
+
+    @Test(expected = CertificateException.class)
+    public void testUntrustedCertificateWithSHA256Fingerprint() throws Exception {
+        FingerprintTrustManagerFactory factory = FingerprintTrustManagerFactory
+                .builder("SHA-256")
+                .fingerprints(FIRST_CERT_SHA256_FINGERPRINT)
+                .build();
+
+        assertTrue(factory.engineGetTrustManagers().length > 0);
+        assertTrue(factory.engineGetTrustManagers()[0] instanceof X509TrustManager);
+        X509TrustManager tm = (X509TrustManager) factory.engineGetTrustManagers()[0];
+        tm.checkClientTrusted(SECOND_CHAIN, "test");
+    }
+
+}