From 9b7fb2f3620449c8cfafa62fd294cb82600f043a Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Wed, 16 Nov 2016 15:16:19 +0100 Subject: [PATCH] Use the correct alert depending on the CertificateException when using OpenSslEngine Motivation: We tried to detect the correct alert to use depending on the CertificateException that is thrown by the TrustManager. This not worked all the time as depending on the TrustManager implementation it may also wrap a CertPathValidatorException. Modification: - Try to unwrap the CertificateException if needed and detect the right alert via the CertPathValidatorException. - Add unit to verify Result: Send the correct alert depending on the CertificateException when using OpenSslEngine. --- .../ssl/ReferenceCountedOpenSslContext.java | 33 ++- .../io/netty/handler/ssl/SslErrorTest.java | 248 ++++++++++++++++++ pom.xml | 3 + 3 files changed, 282 insertions(+), 2 deletions(-) create mode 100644 handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java 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 a12818df45..a25b0a247c 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java @@ -35,6 +35,7 @@ import org.apache.tomcat.jni.SSLContext; import java.security.AccessController; import java.security.PrivateKey; import java.security.PrivilegedAction; +import java.security.cert.CertPathValidatorException; import java.security.cert.Certificate; import java.security.cert.CertificateExpiredException; import java.security.cert.CertificateNotYetValidException; @@ -605,7 +606,10 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen e.initCause(cause); engine.handshakeException = e; + // Try to extract the correct error code that should be used. if (cause instanceof OpenSslCertificateException) { + // This will never return a negative error code as its validated when constructing the + // OpenSslCertificateException. return ((OpenSslCertificateException) cause).errorCode(); } if (cause instanceof CertificateExpiredException) { @@ -614,9 +618,34 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen if (cause instanceof CertificateNotYetValidException) { return CertificateVerifier.X509_V_ERR_CERT_NOT_YET_VALID; } - if (PlatformDependent.javaVersion() >= 7 && cause instanceof CertificateRevokedException) { - return CertificateVerifier.X509_V_ERR_CERT_REVOKED; + if (PlatformDependent.javaVersion() >= 7) { + if (cause instanceof CertificateRevokedException) { + return CertificateVerifier.X509_V_ERR_CERT_REVOKED; + } + + // The X509TrustManagerImpl uses a Validator which wraps a CertPathValidatorException into + // an CertificateException. So we need to handle the wrapped CertPathValidatorException to be + // able to send the correct alert. + Throwable wrapped = cause.getCause(); + while (wrapped != null) { + if (wrapped instanceof CertPathValidatorException) { + CertPathValidatorException ex = (CertPathValidatorException) wrapped; + CertPathValidatorException.Reason reason = ex.getReason(); + if (reason == CertPathValidatorException.BasicReason.EXPIRED) { + return CertificateVerifier.X509_V_ERR_CERT_HAS_EXPIRED; + } + if (reason == CertPathValidatorException.BasicReason.NOT_YET_VALID) { + return CertificateVerifier.X509_V_ERR_CERT_NOT_YET_VALID; + } + if (reason == CertPathValidatorException.BasicReason.REVOKED) { + return CertificateVerifier.X509_V_ERR_CERT_REVOKED; + } + } + wrapped = wrapped.getCause(); + } } + + // Could not detect a specific error code to use, so fallback to a default code. return CertificateVerifier.X509_V_ERR_UNSPECIFIED; } } diff --git a/handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java b/handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java new file mode 100644 index 0000000000..d3c238b5a9 --- /dev/null +++ b/handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java @@ -0,0 +1,248 @@ +/* + * Copyright 2016 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.bootstrap.Bootstrap; +import io.netty.bootstrap.ServerBootstrap; +import io.netty.channel.Channel; +import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.ChannelInboundHandlerAdapter; +import io.netty.channel.ChannelInitializer; +import io.netty.channel.EventLoopGroup; +import io.netty.channel.nio.NioEventLoopGroup; +import io.netty.channel.socket.nio.NioServerSocketChannel; +import io.netty.channel.socket.nio.NioSocketChannel; +import io.netty.handler.logging.LogLevel; +import io.netty.handler.logging.LoggingHandler; +import io.netty.handler.ssl.util.InsecureTrustManagerFactory; +import io.netty.handler.ssl.util.SelfSignedCertificate; +import io.netty.handler.ssl.util.SimpleTrustManagerFactory; +import io.netty.util.ReferenceCountUtil; +import io.netty.util.concurrent.Promise; +import io.netty.util.internal.EmptyArrays; +import org.junit.Assume; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import sun.security.validator.ValidatorException; + +import javax.net.ssl.ManagerFactoryParameters; +import javax.net.ssl.SSLException; +import javax.net.ssl.TrustManager; +import javax.net.ssl.X509TrustManager; +import javax.security.auth.x500.X500Principal; +import java.io.File; +import java.security.KeyStore; +import java.security.cert.CRLReason; +import java.security.cert.CertPathValidatorException; +import java.security.cert.CertificateException; +import java.security.cert.CertificateExpiredException; +import java.security.cert.CertificateNotYetValidException; +import java.security.cert.CertificateRevokedException; +import java.security.cert.Extension; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.Date; +import java.util.List; + + +@RunWith(Parameterized.class) +public class SslErrorTest { + + @Parameterized.Parameters(name = "{index}: serverProvider = {0}, clientProvider = {1}, exception = {2}") + public static Collection data() { + List serverProviders = new ArrayList(2); + List clientProviders = new ArrayList(3); + + if (OpenSsl.isAvailable()) { + serverProviders.add(SslProvider.OPENSSL); + serverProviders.add(SslProvider.OPENSSL_REFCNT); + clientProviders.add(SslProvider.OPENSSL); + clientProviders.add(SslProvider.OPENSSL_REFCNT); + } + // We not test with SslProvider.JDK on the server side as the JDK implementation currently just send the same + // alert all the time, sigh..... + clientProviders.add(SslProvider.JDK); + + List exceptions = new ArrayList(6); + exceptions.add(new CertificateExpiredException()); + exceptions.add(new CertificateNotYetValidException()); + exceptions.add(new CertificateRevokedException( + new Date(), CRLReason.AA_COMPROMISE, new X500Principal(""), + Collections.emptyMap())); + + // Also use wrapped exceptions as this is what the JDK implementation of X509TrustManagerFactory is doing. + exceptions.add(newCertificateException(CertPathValidatorException.BasicReason.EXPIRED)); + exceptions.add(newCertificateException(CertPathValidatorException.BasicReason.NOT_YET_VALID)); + exceptions.add(newCertificateException(CertPathValidatorException.BasicReason.REVOKED)); + + List params = new ArrayList(); + for (SslProvider serverProvider: serverProviders) { + for (SslProvider clientProvider: clientProviders) { + for (CertificateException exception: exceptions) { + params.add(new Object[] { serverProvider, clientProvider, exception}); + } + } + } + return params; + } + + private static CertificateException newCertificateException(CertPathValidatorException.Reason reason) { + return new ValidatorException("Test exception", + new CertPathValidatorException("x", null, null, -1, reason)); + } + + private final SslProvider serverProvider; + private final SslProvider clientProvider; + private final CertificateException exception; + + public SslErrorTest(SslProvider serverProvider, SslProvider clientProvider, CertificateException exception) { + this.serverProvider = serverProvider; + this.clientProvider = clientProvider; + this.exception = exception; + } + + @Test(timeout = 10000L) + public void testCorrectAlert() throws Exception { + // As this only works correctly at the moment when OpenSslEngine is used on the server-side there is + // no need to run it if there is no openssl is available at all. + 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) + .keyManager(new File(getClass().getResource("test.crt").getFile()), + new File(getClass().getResource("test_unencrypted.pem").getFile())) + .sslProvider(clientProvider).build(); + + Channel serverChannel = null; + Channel clientChannel = null; + EventLoopGroup group = new NioEventLoopGroup(); + 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 { + ch.pipeline().addLast(sslServerCtx.newHandler(ch.alloc())); + ch.pipeline().addLast(new ChannelInboundHandlerAdapter() { + + @Override + public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { + ctx.close(); + } + }); + } + }).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 { + ch.pipeline().addLast(sslClientCtx.newHandler(ch.alloc())); + ch.pipeline().addLast(new ChannelInboundHandlerAdapter() { + @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 ValidatorException) { + CertPathValidatorException.Reason reason = + ((CertPathValidatorException) exception.getCause()).getReason(); + if (reason == CertPathValidatorException.BasicReason.EXPIRED) { + verifyException(unwrappedCause, "expired", promise); + } else if (reason == CertPathValidatorException.BasicReason.NOT_YET_VALID) { + 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) { + verifyException(unwrappedCause, "bad", promise); + } else if (exception instanceof CertificateRevokedException) { + verifyException(unwrappedCause, "revoked", promise); + } + } + } + }); + } + }).connect(serverChannel.localAddress()).syncUninterruptibly().channel(); + // Block until we received the correct exception + promise.syncUninterruptibly(); + } finally { + if (clientChannel != null) { + clientChannel.close().syncUninterruptibly(); + } + if (serverChannel != null) { + serverChannel.close().syncUninterruptibly(); + } + group.shutdownGracefully(); + + ReferenceCountUtil.release(sslServerCtx); + ReferenceCountUtil.release(sslClientCtx); + } + } + + // 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) { + String message = cause.getMessage(); + if (message.contains(messagePart)) { + promise.setSuccess(null); + } else { + promise.setFailure(new AssertionError("message not contains '" + messagePart + "': " + message)); + } + } +} diff --git a/pom.xml b/pom.xml index 7ae527e8e9..06f506dc4d 100644 --- a/pom.xml +++ b/pom.xml @@ -723,6 +723,9 @@ javax.net.ssl.SSLParameters java.security.AlgorithmConstraints java.security.cert.CertificateRevokedException + java.security.cert.CertPathValidatorException + java.security.cert.CertPathValidatorException$Reason + java.security.cert.CertPathValidatorException$BasicReason java.util.concurrent.ConcurrentLinkedDeque