From 21cb040aef7bcf23176537f5766f57bffe3645fc Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Mon, 18 Mar 2019 18:42:11 +0100 Subject: [PATCH] Correctly produce ssl alert when certificate validation fails on the client-side when using native SSL implementation. (#8949) Motivation: When the verification of the server cert fails because of the used TrustManager on the client-side we need to ensure we produce the correct alert and send it to the remote peer before closing the connection. Modifications: - Use the correct verification mode on the client-side by default. - Update tests Result: Fixes https://github.com/netty/netty/issues/8942. --- .../ReferenceCountedOpenSslClientContext.java | 8 +- .../io/netty/handler/ssl/SslErrorTest.java | 190 +++++++++++------- 2 files changed, 122 insertions(+), 76 deletions(-) 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 b562885e5e..686dcfe9ed 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java @@ -131,7 +131,13 @@ public final class ReferenceCountedOpenSslClientContext extends ReferenceCounted throw new SSLException("failed to set certificate and key", e); } - SSLContext.setVerify(ctx, SSL.SSL_CVERIFY_NONE, VERIFY_DEPTH); + // On the client side we always need to use SSL_CVERIFY_OPTIONAL (which will translate to SSL_VERIFY_PEER) + // to ensure that when the TrustManager throws we will produce the correct alert back to the server. + // + // See: + // - https://www.openssl.org/docs/man1.0.2/man3/SSL_CTX_set_verify.html + // - https://github.com/netty/netty/issues/8942 + SSLContext.setVerify(ctx, SSL.SSL_CVERIFY_OPTIONAL, VERIFY_DEPTH); try { if (trustCertCollection != null) { diff --git a/handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java b/handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java index 54324787f1..45e20f8ad3 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java @@ -64,7 +64,8 @@ import java.util.Locale; @RunWith(Parameterized.class) public class SslErrorTest { - @Parameterized.Parameters(name = "{index}: serverProvider = {0}, clientProvider = {1}, exception = {2}") + @Parameterized.Parameters( + name = "{index}: serverProvider = {0}, clientProvider = {1}, exception = {2}, serverProduceError = {3}") public static Collection data() { List serverProviders = new ArrayList<>(2); List clientProviders = new ArrayList<>(3); @@ -95,7 +96,8 @@ public class SslErrorTest { for (SslProvider serverProvider: serverProviders) { for (SslProvider clientProvider: clientProviders) { for (CertificateException exception: exceptions) { - params.add(new Object[] { serverProvider, clientProvider, exception}); + params.add(new Object[] { serverProvider, clientProvider, exception, true }); + params.add(new Object[] { serverProvider, clientProvider, exception, false }); } } } @@ -110,11 +112,14 @@ public class SslErrorTest { private final SslProvider serverProvider; private final SslProvider clientProvider; private final CertificateException exception; + private final boolean serverProduceError; - public SslErrorTest(SslProvider serverProvider, SslProvider clientProvider, CertificateException exception) { + public SslErrorTest(SslProvider serverProvider, SslProvider clientProvider, + CertificateException exception, boolean serverProduceError) { this.serverProvider = serverProvider; this.clientProvider = clientProvider; this.exception = exception; + this.serverProduceError = serverProduceError; } @Test(timeout = 30000) @@ -124,56 +129,41 @@ public class SslErrorTest { Assume.assumeTrue(OpenSsl.isAvailable()); SelfSignedCertificate ssc = new SelfSignedCertificate(); - final SslContext sslServerCtx = - SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey()) - .sslProvider(serverProvider) - .trustManager(new SimpleTrustManagerFactory() { - @Override - protected void engineInit(KeyStore keyStore) { } - @Override - protected void engineInit(ManagerFactoryParameters managerFactoryParameters) { } - @Override - protected TrustManager[] engineGetTrustManagers() { - return new TrustManager[] { new X509TrustManager() { - - @Override - public void checkClientTrusted(X509Certificate[] x509Certificates, String s) - throws CertificateException { - throw exception; - } - - @Override - public void checkServerTrusted(X509Certificate[] x509Certificates, String s) - throws CertificateException { - // NOOP - } - - @Override - public X509Certificate[] getAcceptedIssuers() { - return EmptyArrays.EMPTY_X509_CERTIFICATES; - } - } }; - } - }).clientAuth(ClientAuth.REQUIRE).build(); - - final SslContext sslClientCtx = SslContextBuilder.forClient() - .trustManager(InsecureTrustManagerFactory.INSTANCE) + SslContextBuilder sslServerCtxBuilder = SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey()) + .sslProvider(serverProvider) + .clientAuth(ClientAuth.REQUIRE); + SslContextBuilder sslClientCtxBuilder = SslContextBuilder.forClient() .keyManager(new File(getClass().getResource("test.crt").getFile()), new File(getClass().getResource("test_unencrypted.pem").getFile())) - .sslProvider(clientProvider).build(); + .sslProvider(clientProvider); + + if (serverProduceError) { + sslServerCtxBuilder.trustManager(new ExceptionTrustManagerFactory()); + sslClientCtxBuilder.trustManager(InsecureTrustManagerFactory.INSTANCE); + } else { + sslServerCtxBuilder.trustManager(InsecureTrustManagerFactory.INSTANCE); + sslClientCtxBuilder.trustManager(new ExceptionTrustManagerFactory()); + } + + final SslContext sslServerCtx = sslServerCtxBuilder.build(); + final SslContext sslClientCtx = sslClientCtxBuilder.build(); Channel serverChannel = null; Channel clientChannel = null; EventLoopGroup group = new MultithreadEventLoopGroup(NioHandler.newFactory()); + final Promise promise = group.next().newPromise(); try { serverChannel = new ServerBootstrap().group(group) .channel(NioServerSocketChannel.class) .handler(new LoggingHandler(LogLevel.INFO)) .childHandler(new ChannelInitializer() { @Override - protected void initChannel(Channel ch) throws Exception { + protected void initChannel(Channel ch) { ch.pipeline().addLast(sslServerCtx.newHandler(ch.alloc())); + if (!serverProduceError) { + ch.pipeline().addLast(new AlertValidationHandler(promise)); + } ch.pipeline().addLast(new ChannelInboundHandler() { @Override @@ -184,48 +174,21 @@ public class SslErrorTest { } }).bind(0).sync().channel(); - final Promise promise = group.next().newPromise(); - clientChannel = new Bootstrap().group(group) .channel(NioSocketChannel.class) .handler(new ChannelInitializer() { @Override - protected void initChannel(Channel ch) throws Exception { + protected void initChannel(Channel ch) { ch.pipeline().addLast(sslClientCtx.newHandler(ch.alloc())); + + if (serverProduceError) { + ch.pipeline().addLast(new AlertValidationHandler(promise)); + } ch.pipeline().addLast(new ChannelInboundHandler() { + @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { - // Unwrap as its wrapped by a DecoderException - Throwable unwrappedCause = cause.getCause(); - if (unwrappedCause instanceof SSLException) { - if (exception instanceof TestCertificateException) { - CertPathValidatorException.Reason reason = - ((CertPathValidatorException) exception.getCause()).getReason(); - if (reason == CertPathValidatorException.BasicReason.EXPIRED) { - verifyException(unwrappedCause, "expired", promise); - } else if (reason == CertPathValidatorException.BasicReason.NOT_YET_VALID) { - // BoringSSL uses "expired" in this case while others use "bad" - if (OpenSsl.isBoringSSL()) { - verifyException(unwrappedCause, "expired", promise); - } else { - verifyException(unwrappedCause, "bad", promise); - } - } else if (reason == CertPathValidatorException.BasicReason.REVOKED) { - verifyException(unwrappedCause, "revoked", promise); - } - } else if (exception instanceof CertificateExpiredException) { - verifyException(unwrappedCause, "expired", promise); - } else if (exception instanceof CertificateNotYetValidException) { - // BoringSSL uses "expired" in this case while others use "bad" - if (OpenSsl.isBoringSSL()) { - verifyException(unwrappedCause, "expired", promise); - } else { - verifyException(unwrappedCause, "bad", promise); - } - } else if (exception instanceof CertificateRevokedException) { - verifyException(unwrappedCause, "revoked", promise); - } - } + ctx.close(); } }); } @@ -246,11 +209,88 @@ public class SslErrorTest { } } + private final class ExceptionTrustManagerFactory extends SimpleTrustManagerFactory { + @Override + protected void engineInit(KeyStore keyStore) { } + @Override + protected void engineInit(ManagerFactoryParameters managerFactoryParameters) { } + + @Override + protected TrustManager[] engineGetTrustManagers() { + return new TrustManager[] { new X509TrustManager() { + + @Override + public void checkClientTrusted(X509Certificate[] x509Certificates, String s) + throws CertificateException { + throw exception; + } + + @Override + public void checkServerTrusted(X509Certificate[] x509Certificates, String s) + throws CertificateException { + throw exception; + } + + @Override + public X509Certificate[] getAcceptedIssuers() { + return EmptyArrays.EMPTY_X509_CERTIFICATES; + } + } }; + } + } + + private final class AlertValidationHandler implements ChannelInboundHandler { + private final Promise promise; + + AlertValidationHandler(Promise promise) { + this.promise = promise; + } + + @Override + public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { + // Unwrap as its wrapped by a DecoderException + Throwable unwrappedCause = cause.getCause(); + if (unwrappedCause instanceof SSLException) { + if (exception instanceof TestCertificateException) { + CertPathValidatorException.Reason reason = + ((CertPathValidatorException) exception.getCause()).getReason(); + if (reason == CertPathValidatorException.BasicReason.EXPIRED) { + verifyException(unwrappedCause, "expired", promise); + } else if (reason == CertPathValidatorException.BasicReason.NOT_YET_VALID) { + // BoringSSL uses "expired" in this case while others use "bad" + if (OpenSsl.isBoringSSL()) { + verifyException(unwrappedCause, "expired", promise); + } else { + verifyException(unwrappedCause, "bad", promise); + } + } else if (reason == CertPathValidatorException.BasicReason.REVOKED) { + verifyException(unwrappedCause, "revoked", promise); + } + } else if (exception instanceof CertificateExpiredException) { + verifyException(unwrappedCause, "expired", promise); + } else if (exception instanceof CertificateNotYetValidException) { + // BoringSSL uses "expired" in this case while others use "bad" + if (OpenSsl.isBoringSSL()) { + verifyException(unwrappedCause, "expired", promise); + } else { + verifyException(unwrappedCause, "bad", promise); + } + } else if (exception instanceof CertificateRevokedException) { + verifyException(unwrappedCause, "revoked", promise); + } + } + } + } + // Its a bit hacky to verify against the message that is part of the exception but there is no other way // at the moment as there are no different exceptions for the different alerts. - private static void verifyException(Throwable cause, String messagePart, Promise promise) { + private void verifyException(Throwable cause, String messagePart, Promise promise) { String message = cause.getMessage(); - if (message.toLowerCase(Locale.UK).contains(messagePart.toLowerCase(Locale.UK))) { + if (message.toLowerCase(Locale.UK).contains(messagePart.toLowerCase(Locale.UK)) || + // When the error is produced on the client side and the client side uses JDK as provider it will always + // use "certificate unknown". + !serverProduceError && clientProvider == SslProvider.JDK && + message.toLowerCase(Locale.UK).contains("certificate unknown")) { promise.setSuccess(null); } else { Throwable error = new AssertionError("message not contains '" + messagePart + "': " + message);