Skip to content

Commit

Permalink
OpenSslEngine protocol selection must be contiguous
Browse files Browse the repository at this point in the history 
Motivation:
TLS doesn't support a way to advertise non-contiguous versions from the client's perspective, and the client just advertises the max supported version. The TLS protocol also doesn't support all different combinations of discrete protocols, and instead assumes contiguous ranges. OpenSSL has some unexpected behavior (e.g. handshake failures) if non-contiguous protocols are used even where there is a compatible set of protocols and ciphers. For these reasons this method will determine the minimum protocol and the maximum protocol and enabled a contiguous range from [min protocol, max protocol] in OpenSSL.

Modifications:
- ReferenceCountedOpenSslEngine#setEnabledProtocols should determine the min/max protocol versions and enable a contiguous range

Result:
OpenSslEngine is more consistent with the JDK's SslEngineImpl and no more unexpected handshake failures due to protocol selection quirks.
  • Loading branch information
@Scottmitch
Scottmitch committed Jul 13, 2017
1 parent 43ae974 commit 6152990
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 25 deletions.
83 changes: 59 additions & 24 deletions handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,18 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
new SSLException("renegotiation unsupported"), ReferenceCountedOpenSslEngine.class, "beginHandshake()");
private static final ResourceLeakDetector<ReferenceCountedOpenSslEngine> leakDetector =
ResourceLeakDetectorFactory.instance().newResourceLeakDetector(ReferenceCountedOpenSslEngine.class);
private static final int OPENSSL_OP_NO_PROTOCOL_INDEX_SSLV2 = 0;
private static final int OPENSSL_OP_NO_PROTOCOL_INDEX_SSLV3 = 1;
private static final int OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1 = 2;
private static final int OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_1 = 3;
private static final int OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_2 = 4;
private static final int[] OPENSSL_OP_NO_PROTOCOLS = new int[] {
SSL.SSL_OP_NO_SSLv2,
SSL.SSL_OP_NO_SSLv3,
SSL.SSL_OP_NO_TLSv1,
SSL.SSL_OP_NO_TLSv1_1,
SSL.SSL_OP_NO_TLSv1_2
};
/**
* <a href="https://www.openssl.org/docs/man1.0.2/crypto/X509_check_host.html">The flags argument is usually 0</a>.
*/
Expand Down Expand Up @@ -1363,54 +1375,77 @@ private static boolean isProtocolEnabled(int opts, int disableMask, String proto
return (opts & disableMask) == 0 && OpenSsl.SUPPORTED_PROTOCOLS_SET.contains(protocolString);
}

/**
* {@inheritDoc}
* TLS doesn't support a way to advertise non-contiguous versions from the client's perspective, and the client
* just advertises the max supported version. The TLS protocol also doesn't support all different combinations of
* discrete protocols, and instead assumes contiguous ranges. OpenSSL has some unexpected behavior
* (e.g. handshake failures) if non-contiguous protocols are used even where there is a compatible set of protocols
* and ciphers. For these reasons this method will determine the minimum protocol and the maximum protocol and
* enabled a contiguous range from [min protocol, max protocol] in OpenSSL.
*/
@Override
public final void setEnabledProtocols(String[] protocols) {
if (protocols == null) {
// This is correct from the API docs
throw new IllegalArgumentException();
}
boolean sslv2 = false;
boolean sslv3 = false;
boolean tlsv1 = false;
boolean tlsv1_1 = false;
boolean tlsv1_2 = false;
int minProtocolIndex = OPENSSL_OP_NO_PROTOCOLS.length;
int maxProtocolIndex = 0;
for (String p: protocols) {
if (!OpenSsl.SUPPORTED_PROTOCOLS_SET.contains(p)) {
throw new IllegalArgumentException("Protocol " + p + " is not supported.");
}
if (p.equals(OpenSsl.PROTOCOL_SSL_V2)) {
sslv2 = true;
if (minProtocolIndex > OPENSSL_OP_NO_PROTOCOL_INDEX_SSLV2) {
minProtocolIndex = OPENSSL_OP_NO_PROTOCOL_INDEX_SSLV2;
}
if (maxProtocolIndex < OPENSSL_OP_NO_PROTOCOL_INDEX_SSLV2) {
maxProtocolIndex = OPENSSL_OP_NO_PROTOCOL_INDEX_SSLV2;
}
} else if (p.equals(OpenSsl.PROTOCOL_SSL_V3)) {
sslv3 = true;
if (minProtocolIndex > OPENSSL_OP_NO_PROTOCOL_INDEX_SSLV3) {
minProtocolIndex = OPENSSL_OP_NO_PROTOCOL_INDEX_SSLV3;
}
if (maxProtocolIndex < OPENSSL_OP_NO_PROTOCOL_INDEX_SSLV3) {
maxProtocolIndex = OPENSSL_OP_NO_PROTOCOL_INDEX_SSLV3;
}
} else if (p.equals(OpenSsl.PROTOCOL_TLS_V1)) {
tlsv1 = true;
if (minProtocolIndex > OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1) {
minProtocolIndex = OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1;
}
if (maxProtocolIndex < OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1) {
maxProtocolIndex = OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1;
}
} else if (p.equals(OpenSsl.PROTOCOL_TLS_V1_1)) {
tlsv1_1 = true;
if (minProtocolIndex > OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_1) {
minProtocolIndex = OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_1;
}
if (maxProtocolIndex < OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_1) {
maxProtocolIndex = OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_1;
}
} else if (p.equals(OpenSsl.PROTOCOL_TLS_V1_2)) {
tlsv1_2 = true;
if (minProtocolIndex > OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_2) {
minProtocolIndex = OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_2;
}
if (maxProtocolIndex < OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_2) {
maxProtocolIndex = OPENSSL_OP_NO_PROTOCOL_INDEX_TLSv1_2;
}
}
}
synchronized (this) {
if (!isDestroyed()) {
// Clear out options which disable protocols
SSL.clearOptions(ssl, SSL.SSL_OP_NO_SSLv2 | SSL.SSL_OP_NO_SSLv3 | SSL.SSL_OP_NO_TLSv1 |
SSL.SSL_OP_NO_TLSv1_1 | SSL.SSL_OP_NO_TLSv1_2);
SSL.SSL_OP_NO_TLSv1_1 | SSL.SSL_OP_NO_TLSv1_2);

int opts = 0;
if (!sslv2) {
opts |= SSL.SSL_OP_NO_SSLv2;
}
if (!sslv3) {
opts |= SSL.SSL_OP_NO_SSLv3;
}
if (!tlsv1) {
opts |= SSL.SSL_OP_NO_TLSv1;
}
if (!tlsv1_1) {
opts |= SSL.SSL_OP_NO_TLSv1_1;
for (int i = 0; i < minProtocolIndex; ++i) {
opts |= OPENSSL_OP_NO_PROTOCOLS[i];
}
if (!tlsv1_2) {
opts |= SSL.SSL_OP_NO_TLSv1_2;
assert maxProtocolIndex != MAX_VALUE;
for (int i = maxProtocolIndex + 1; i < OPENSSL_OP_NO_PROTOCOLS.length; ++i) {
opts |= OPENSSL_OP_NO_PROTOCOLS[i];
}

// Disable protocols we do not want
Expand Down
34 changes: 33 additions & 1 deletion handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,15 @@
import java.nio.ByteBuffer;
import java.nio.channels.ClosedChannelException;
import java.security.KeyStore;
import java.security.Provider;
import java.security.cert.Certificate;
import java.security.cert.CertificateException;
import java.security.Provider;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;

import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLEngineResult;
Expand Down Expand Up @@ -1680,6 +1682,36 @@ private void testProtocol(String[] clientProtocols, String[] serverProtocols) th
}
}

@Test
public void testHandshakeCompletesWithNonContiguousProtocolsTLSv1_2CipherOnly() throws Exception {
SelfSignedCertificate ssc = new SelfSignedCertificate();
// Select a mandatory cipher from the TLSv1.2 RFC https://www.ietf.org/rfc/rfc5246.txt so handshakes won't fail
// due to no shared/supported cipher.
final String sharedCipher = "TLS_RSA_WITH_AES_128_CBC_SHA";
clientSslCtx = SslContextBuilder.forClient()
.trustManager(InsecureTrustManagerFactory.INSTANCE)
.ciphers(Arrays.asList(sharedCipher))
.protocols(OpenSsl.PROTOCOL_TLS_V1_2, OpenSsl.PROTOCOL_TLS_V1)
.sslProvider(sslClientProvider())
.build();

serverSslCtx = SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey())
.ciphers(Arrays.asList(sharedCipher))
.protocols(OpenSsl.PROTOCOL_TLS_V1_2, OpenSsl.PROTOCOL_TLS_V1)
.sslProvider(sslServerProvider())
.build();
SSLEngine clientEngine = null;
SSLEngine serverEngine = null;
try {
clientEngine = clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT);
serverEngine = serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT);
handshake(clientEngine, serverEngine);
} finally {
cleanupClientSslEngine(clientEngine);
cleanupServerSslEngine(serverEngine);
}
}

@Test
public void testPacketBufferSizeLimit() throws Exception {
SelfSignedCertificate cert = new SelfSignedCertificate();
Expand Down

0 comments on commit 6152990

Please sign in to comment.