Stop using set_options to set protocol support. (#500)
The *_set_min_protocol_version and *_set_max_protocol_version
functions were introduced to be explicit about setting supported
versions. Use those functions instead of SSL_set_options with
disabling constants.
There aren't any higher-level tests because this is already covered by
tests like SSLSocket.test_SSLSocket_setEnabledProtocols,
SSLSocket.test_SSLSocket_noncontiguousProtocols_useLower, etc.
diff --git a/common/src/jni/main/cpp/conscrypt/native_crypto.cc b/common/src/jni/main/cpp/conscrypt/native_crypto.cc
index 6875f10..07ca573 100644
--- a/common/src/jni/main/cpp/conscrypt/native_crypto.cc
+++ b/common/src/jni/main/cpp/conscrypt/native_crypto.cc
@@ -6475,14 +6475,14 @@
SSL_CTX_set_options(
sslCtx.get(),
SSL_OP_ALL
- // Note: We explicitly do not allow SSLv2 to be used.
- | SSL_OP_NO_SSLv2
// We also disable session tickets for better compatibility b/2682876
| SSL_OP_NO_TICKET
// We also disable compression for better compatibility b/2710492 b/2710497
| SSL_OP_NO_COMPRESSION
// Generate a fresh ECDH keypair for each key exchange.
| SSL_OP_SINGLE_ECDH_USE);
+ SSL_CTX_set_min_proto_version(sslCtx.get(), TLS1_VERSION);
+ SSL_CTX_set_max_proto_version(sslCtx.get(), TLS1_2_VERSION);
uint32_t mode = SSL_CTX_get_mode(sslCtx.get());
/*
@@ -6877,6 +6877,26 @@
return result;
}
+static jint NativeCrypto_SSL_set_protocol_versions(JNIEnv* env, jclass, jlong ssl_address, CONSCRYPT_UNUSED jobject ssl_holder, jint min_version, jint max_version) {
+ CHECK_ERROR_QUEUE_ON_RETURN;
+ SSL* ssl = to_SSL(env, ssl_address, true);
+ JNI_TRACE("ssl=%p NativeCrypto_SSL_set_protocol_versions min=0x%x max=0x%x", ssl, min_version, max_version);
+ if (ssl == nullptr) {
+ return 0;
+ }
+ int min_result = SSL_set_min_proto_version(ssl, static_cast<uint16_t>(min_version));
+ int max_result = SSL_set_max_proto_version(ssl, static_cast<uint16_t>(max_version));
+ // Return failure if either call failed.
+ int result = 1;
+ if (!min_result || !max_result) {
+ result = 0;
+ // The only possible error is an invalid version, so we don't need the details.
+ ERR_clear_error();
+ }
+ JNI_TRACE("ssl=%p NativeCrypto_SSL_set_protocol_versions => (min: %d, max: %d) == %d", ssl, min_result, max_result, result);
+ return result;
+}
+
/**
* public static native void SSL_enable_signed_cert_timestamps(long ssl);
*/
@@ -10128,6 +10148,7 @@
CONSCRYPT_NATIVE_METHOD(SSL_set_mode, "(J" REF_SSL "J)J"),
CONSCRYPT_NATIVE_METHOD(SSL_set_options, "(J" REF_SSL "J)J"),
CONSCRYPT_NATIVE_METHOD(SSL_clear_options, "(J" REF_SSL "J)J"),
+ CONSCRYPT_NATIVE_METHOD(SSL_set_protocol_versions, "(J" REF_SSL "II)I"),
CONSCRYPT_NATIVE_METHOD(SSL_enable_signed_cert_timestamps, "(J" REF_SSL ")V"),
CONSCRYPT_NATIVE_METHOD(SSL_get_signed_cert_timestamp_list, "(J" REF_SSL ")[B"),
CONSCRYPT_NATIVE_METHOD(SSL_set_signed_cert_timestamp_list, "(J" REF_SSL "[B)V"),
diff --git a/common/src/main/java/org/conscrypt/NativeCrypto.java b/common/src/main/java/org/conscrypt/NativeCrypto.java
index b2c8379..d40e25e 100644
--- a/common/src/main/java/org/conscrypt/NativeCrypto.java
+++ b/common/src/main/java/org/conscrypt/NativeCrypto.java
@@ -31,6 +31,7 @@
import java.security.cert.CertificateException;
import java.security.cert.CertificateParsingException;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Calendar;
import java.util.HashSet;
import java.util.List;
@@ -891,6 +892,8 @@
static native long SSL_clear_options(long ssl, NativeSsl ssl_holder, long options);
+ static native int SSL_set_protocol_versions(long ssl, NativeSsl ssl_holder, int min_version, int max_version);
+
static native void SSL_enable_signed_cert_timestamps(long ssl, NativeSsl ssl_holder);
static native byte[] SSL_get_signed_cert_timestamp_list(long ssl, NativeSsl ssl_holder);
@@ -925,55 +928,55 @@
};
/** Protocols to enable by default when "TLSv1.1" is requested. */
- static final String[] TLSV11_PROTOCOLS = new String[] {
- SUPPORTED_PROTOCOL_TLSV1,
- SUPPORTED_PROTOCOL_TLSV1_1,
- SUPPORTED_PROTOCOL_TLSV1_2,
- };
+ static final String[] TLSV11_PROTOCOLS = TLSV12_PROTOCOLS;
/** Protocols to enable by default when "TLSv1" is requested. */
- static final String[] TLSV1_PROTOCOLS = new String[] {
- SUPPORTED_PROTOCOL_TLSV1,
- SUPPORTED_PROTOCOL_TLSV1_1,
- SUPPORTED_PROTOCOL_TLSV1_2,
- };
+ static final String[] TLSV1_PROTOCOLS = TLSV11_PROTOCOLS;
static final String[] DEFAULT_PROTOCOLS = TLSV12_PROTOCOLS;
+ private static final String[] SUPPORTED_PROTOCOLS = DEFAULT_PROTOCOLS;
static String[] getSupportedProtocols() {
- return TLSV12_PROTOCOLS.clone();
+ return SUPPORTED_PROTOCOLS.clone();
}
static void setEnabledProtocols(long ssl, NativeSsl ssl_holder, String[] protocols) {
checkEnabledProtocols(protocols);
- // openssl uses negative logic letting you disable protocols.
- // so first, assume we need to set all (disable all) and clear none (enable none).
- // in the loop, selectively move bits from set to clear (from disable to enable)
- long optionsToSet = (NativeConstants.SSL_OP_NO_SSLv3 | NativeConstants.SSL_OP_NO_TLSv1
- | NativeConstants.SSL_OP_NO_TLSv1_1 | NativeConstants.SSL_OP_NO_TLSv1_2);
- long optionsToClear = 0;
- for (String protocol : protocols) {
- if (protocol.equals(SUPPORTED_PROTOCOL_TLSV1)) {
- optionsToSet &= ~NativeConstants.SSL_OP_NO_TLSv1;
- optionsToClear |= NativeConstants.SSL_OP_NO_TLSv1;
- } else if (protocol.equals(SUPPORTED_PROTOCOL_TLSV1_1)) {
- optionsToSet &= ~NativeConstants.SSL_OP_NO_TLSv1_1;
- optionsToClear |= NativeConstants.SSL_OP_NO_TLSv1_1;
- } else if (protocol.equals(SUPPORTED_PROTOCOL_TLSV1_2)) {
- optionsToSet &= ~NativeConstants.SSL_OP_NO_TLSv1_2;
- optionsToClear |= NativeConstants.SSL_OP_NO_TLSv1_2;
- } else if (protocol.equals(OBSOLETE_PROTOCOL_SSLV3)) {
- // Do nothing since we no longer support this protocol, but
- // allow it in the list of protocols so we can give an error
- // message about it if the handshake fails.
- } else {
- // error checked by checkEnabledProtocols
- throw new IllegalStateException();
+ // TLS protocol negotiation only allows a min and max version
+ // to be set, despite the Java API allowing a sparse set of
+ // protocols to be enabled. Use the lowest contiguous range
+ // of protocols provided by the caller, which is what we've
+ // done historically.
+ List<String> protocolsList = Arrays.asList(protocols);
+ String min = null;
+ String max = null;
+ for (int i = 0; i < SUPPORTED_PROTOCOLS.length; i++) {
+ String protocol = SUPPORTED_PROTOCOLS[i];
+ if (protocolsList.contains(protocol)) {
+ if (min == null) {
+ min = protocol;
+ }
+ max = protocol;
+ } else if (min != null) {
+ break;
}
}
+ if ((min == null) || (max == null)) {
+ throw new IllegalArgumentException("No protocols enabled.");
+ }
+ SSL_set_protocol_versions(ssl, ssl_holder, getProtocolConstant(min), getProtocolConstant(max));
+ }
- SSL_set_options(ssl, ssl_holder, optionsToSet);
- SSL_clear_options(ssl, ssl_holder, optionsToClear);
+ private static int getProtocolConstant(String protocol) {
+ if (protocol.equals(SUPPORTED_PROTOCOL_TLSV1)) {
+ return NativeConstants.TLS1_VERSION;
+ } else if (protocol.equals(SUPPORTED_PROTOCOL_TLSV1_1)) {
+ return NativeConstants.TLS1_1_VERSION;
+ } else if (protocol.equals(SUPPORTED_PROTOCOL_TLSV1_2)) {
+ return NativeConstants.TLS1_2_VERSION;
+ } else {
+ throw new AssertionError("Unknown protocol encountered: " + protocol);
+ }
}
static String[] checkEnabledProtocols(String[] protocols) {
diff --git a/constants/src/gen/cpp/generate_constants.cc b/constants/src/gen/cpp/generate_constants.cc
index 5725abd..b5ea68d 100644
--- a/constants/src/gen/cpp/generate_constants.cc
+++ b/constants/src/gen/cpp/generate_constants.cc
@@ -66,16 +66,16 @@
CONST(SSL_OP_CIPHER_SERVER_PREFERENCE);
CONST(SSL_OP_NO_TICKET);
- CONST(SSL_OP_NO_SSLv3);
- CONST(SSL_OP_NO_TLSv1);
- CONST(SSL_OP_NO_TLSv1_1);
- CONST(SSL_OP_NO_TLSv1_2);
CONST(SSL_ERROR_NONE);
CONST(SSL_ERROR_WANT_READ);
CONST(SSL_ERROR_WANT_WRITE);
CONST(SSL_ERROR_ZERO_RETURN);
+ CONST(TLS1_VERSION);
+ CONST(TLS1_1_VERSION);
+ CONST(TLS1_2_VERSION);
+
CONST(SSL_SENT_SHUTDOWN);
CONST(SSL_RECEIVED_SHUTDOWN);
diff --git a/openjdk/src/test/java/org/conscrypt/NativeCryptoTest.java b/openjdk/src/test/java/org/conscrypt/NativeCryptoTest.java
index 71ffa84..e2738aa 100644
--- a/openjdk/src/test/java/org/conscrypt/NativeCryptoTest.java
+++ b/openjdk/src/test/java/org/conscrypt/NativeCryptoTest.java
@@ -18,13 +18,14 @@
import static org.conscrypt.NativeConstants.SSL_MODE_CBC_RECORD_SPLITTING;
import static org.conscrypt.NativeConstants.SSL_MODE_ENABLE_FALSE_START;
-import static org.conscrypt.NativeConstants.SSL_OP_NO_SSLv3;
-import static org.conscrypt.NativeConstants.SSL_OP_NO_TLSv1;
-import static org.conscrypt.NativeConstants.SSL_OP_NO_TLSv1_1;
-import static org.conscrypt.NativeConstants.SSL_OP_NO_TLSv1_2;
+import static org.conscrypt.NativeConstants.SSL_OP_CIPHER_SERVER_PREFERENCE;
+import static org.conscrypt.NativeConstants.SSL_OP_NO_TICKET;
import static org.conscrypt.NativeConstants.SSL_VERIFY_FAIL_IF_NO_PEER_CERT;
import static org.conscrypt.NativeConstants.SSL_VERIFY_NONE;
import static org.conscrypt.NativeConstants.SSL_VERIFY_PEER;
+import static org.conscrypt.NativeConstants.TLS1_1_VERSION;
+import static org.conscrypt.NativeConstants.TLS1_2_VERSION;
+import static org.conscrypt.NativeConstants.TLS1_VERSION;
import static org.conscrypt.TestUtils.openTestFile;
import static org.conscrypt.TestUtils.readTestFile;
import static org.junit.Assert.assertEquals;
@@ -383,10 +384,7 @@
long s = NativeCrypto.SSL_new(c, null);
assertTrue(s != NULL);
- assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_NO_SSLv3) == 0);
- assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_NO_TLSv1) == 0);
- assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_NO_TLSv1_1) == 0);
- assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_NO_TLSv1_2) == 0);
+ assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_NO_TICKET) != 0);
long s2 = NativeCrypto.SSL_new(c, null);
assertTrue(s != s2);
@@ -535,9 +533,9 @@
public void test_SSL_set_options() throws Exception {
long c = NativeCrypto.SSL_CTX_new();
long s = NativeCrypto.SSL_new(c, null);
- assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_NO_SSLv3) == 0);
- NativeCrypto.SSL_set_options(s, null, SSL_OP_NO_SSLv3);
- assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_NO_SSLv3) != 0);
+ assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_CIPHER_SERVER_PREFERENCE) == 0);
+ NativeCrypto.SSL_set_options(s, null, SSL_OP_CIPHER_SERVER_PREFERENCE);
+ assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_CIPHER_SERVER_PREFERENCE) != 0);
NativeCrypto.SSL_free(s, null);
NativeCrypto.SSL_CTX_free(c, null);
}
@@ -551,11 +549,28 @@
public void test_SSL_clear_options() throws Exception {
long c = NativeCrypto.SSL_CTX_new();
long s = NativeCrypto.SSL_new(c, null);
- assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_NO_SSLv3) == 0);
- NativeCrypto.SSL_set_options(s, null, SSL_OP_NO_SSLv3);
- assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_NO_SSLv3) != 0);
- NativeCrypto.SSL_clear_options(s, null, SSL_OP_NO_SSLv3);
- assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_NO_SSLv3) == 0);
+ assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_CIPHER_SERVER_PREFERENCE) == 0);
+ NativeCrypto.SSL_set_options(s, null, SSL_OP_CIPHER_SERVER_PREFERENCE);
+ assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_CIPHER_SERVER_PREFERENCE) != 0);
+ NativeCrypto.SSL_clear_options(s, null, SSL_OP_CIPHER_SERVER_PREFERENCE);
+ assertTrue((NativeCrypto.SSL_get_options(s, null) & SSL_OP_CIPHER_SERVER_PREFERENCE) == 0);
+ NativeCrypto.SSL_free(s, null);
+ NativeCrypto.SSL_CTX_free(c, null);
+ }
+
+ @Test(expected = NullPointerException.class)
+ public void SSL_set_protocol_versions_withNullShouldThrow() throws Exception {
+ NativeCrypto.SSL_set_protocol_versions(NULL, null, 0, 0);
+ }
+
+ @Test
+ public void SSL_set_protocol_versions() throws Exception {
+ long c = NativeCrypto.SSL_CTX_new();
+ long s = NativeCrypto.SSL_new(c, null);
+ assertEquals(1, NativeCrypto.SSL_set_protocol_versions(s, null, TLS1_VERSION, TLS1_1_VERSION));
+ assertEquals(1, NativeCrypto.SSL_set_protocol_versions(s, null, TLS1_2_VERSION, TLS1_2_VERSION));
+ assertEquals(0, NativeCrypto.SSL_set_protocol_versions(s, null, TLS1_2_VERSION + 413, TLS1_1_VERSION));
+ assertEquals(0, NativeCrypto.SSL_set_protocol_versions(s, null, TLS1_1_VERSION, TLS1_2_VERSION + 413));
NativeCrypto.SSL_free(s, null);
NativeCrypto.SSL_CTX_free(c, null);
}
