andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Correctly produce ssl alert when certificate validation fails on the client-side when using native SSL implementation. (#8949)

Browse Source 
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.
This commit is contained in:
Norman Maurer 2019-03-18 18:42:11 +01:00 committed by GitHub
parent d0fb41e529
commit e9ce5048df
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 121 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java
View File

@ -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) {

189
handler/src/test/java/io/netty/handler/ssl/SslErrorTest.java
View File

@ -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<Object[]> data() {
List<SslProvider> serverProviders = new ArrayList<SslProvider>(2);
List<SslProvider> clientProviders = new ArrayList<SslProvider>(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 NioEventLoopGroup();
final Promise<Void> promise = group.next().newPromise();
try {
serverChannel = new ServerBootstrap().group(group)
.channel(NioServerSocketChannel.class)
.handler(new LoggingHandler(LogLevel.INFO))
.childHandler(new ChannelInitializer<Channel>() {
@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 ChannelInboundHandlerAdapter() {
@Override
@ -184,48 +174,20 @@ public class SslErrorTest {
}
}).bind(0).sync().channel();
final Promise<Void> promise = group.next().newPromise();
clientChannel = new Bootstrap().group(group)
.channel(NioSocketChannel.class)
.handler(new ChannelInitializer<Channel>() {
@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 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 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 +208,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 extends ChannelInboundHandlerAdapter {
private final Promise<Void> promise;
AlertValidationHandler(Promise<Void> 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<Void> promise) {
private void verifyException(Throwable cause, String messagePart, Promise<Void> 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);