Always set min_version / max_version.

Saves us some mess if they're never zero. This also fixes a bug in
ssl3_get_max_client_version where it didn't account for all versions being
disabled properly.

Change-Id: I4c95ff57cf8953cb4a528263b252379f252f3e01
Reviewed-on: https://boringssl-review.googlesource.com/8512
Reviewed-by: David Benjamin <davidben@google.com>

include/openssl/ssl.h

@@ -3619,12 +3619,10 @@ struct ssl_ctx_st {
   /* lock is used to protect various operations on this object. */
   CRYPTO_MUTEX lock;
 
-  /* max_version is the maximum acceptable protocol version. If zero, the
-   * maximum supported version, currently (D)TLS 1.2, is used. */
+  /* max_version is the maximum acceptable wire protocol version. */
   uint16_t max_version;
 
-  /* min_version is the minimum acceptable protocl version. If zero, the
-   * minimum supported version, currently SSL 3.0 and DTLS 1.0, is used */
+  /* min_version is the minimum acceptable wire protocol version. */
   uint16_t min_version;
 
   struct ssl_cipher_preference_list_st *cipher_list;
@@ -3868,12 +3866,10 @@ struct ssl_st {
   /* version is the protocol version. */
   int version;
 
-  /* max_version is the maximum acceptable protocol version. If zero, the
-   * maximum supported version, currently (D)TLS 1.2, is used. */
+  /* max_version is the maximum acceptable wire protocol version. */
   uint16_t max_version;
 
-  /* min_version is the minimum acceptable protocl version. If zero, the
-   * minimum supported version, currently SSL 3.0 and DTLS 1.0, is used */
+  /* min_version is the minimum acceptable wire protocol version. */
   uint16_t min_version;
 
   /* method is the method table corresponding to the current protocol (DTLS or

ssl/ssl_lib.c

@@ -297,9 +297,14 @@ SSL_CTX *SSL_CTX_new(const SSL_METHOD *method) {
   if (method->version != 0) {
     SSL_CTX_set_max_version(ret, method->version);
     SSL_CTX_set_min_version(ret, method->version);
-  } else if (!method->method->is_dtls) {
+  } else if (method->method->is_dtls) {
+    /* TODO(svaldez): Enable DTLS 1.3 once implemented. */
+    SSL_CTX_set_max_version(ret, DTLS1_2_VERSION);
+    SSL_CTX_set_min_version(ret, DTLS1_VERSION);
+  } else {
     /* TODO(svaldez): Enable TLS 1.3 once implemented. */
     SSL_CTX_set_max_version(ret, TLS1_2_VERSION);
+    SSL_CTX_set_min_version(ret, SSL3_VERSION);
   }
 
   return ret;
@@ -2558,7 +2563,7 @@ uint16_t ssl3_get_mutual_version(SSL *ssl, uint16_t client_version) {
 
   if (SSL_IS_DTLS(ssl)) {
     /* Clamp client_version to max_version. */
-    if (ssl->max_version != 0 && client_version < ssl->max_version) {
+    if (client_version < ssl->max_version) {
       client_version = ssl->max_version;
     }
 
@@ -2571,13 +2576,13 @@ uint16_t ssl3_get_mutual_version(SSL *ssl, uint16_t client_version) {
     }
 
     /* Check against min_version. */
-    if (version != 0 && ssl->min_version != 0 && version > ssl->min_version) {
+    if (version != 0 && version > ssl->min_version) {
       return 0;
     }
     return version;
   } else {
     /* Clamp client_version to max_version. */
-    if (ssl->max_version != 0 && client_version > ssl->max_version) {
+    if (client_version > ssl->max_version) {
       client_version = ssl->max_version;
     }
 
@@ -2599,7 +2604,7 @@ uint16_t ssl3_get_mutual_version(SSL *ssl, uint16_t client_version) {
     }
 
     /* Check against min_version. */
-    if (version != 0 && ssl->min_version != 0 && version < ssl->min_version) {
+    if (version != 0 && version < ssl->min_version) {
       return 0;
     }
     return version;
@@ -2630,7 +2635,7 @@ uint16_t ssl3_get_max_client_version(SSL *ssl) {
     if (!(options & SSL_OP_NO_DTLSv1) && (options & SSL_OP_NO_DTLSv1_2)) {
       version = DTLS1_VERSION;
     }
-    if (ssl->max_version != 0 && version < ssl->max_version) {
+    if (version != 0 && version < ssl->max_version) {
       version = ssl->max_version;
     }
   } else {
@@ -2649,7 +2654,7 @@ uint16_t ssl3_get_max_client_version(SSL *ssl) {
     if (!(options & SSL_OP_NO_SSLv3) && (options & SSL_OP_NO_TLSv1)) {
       version = SSL3_VERSION;
     }
-    if (ssl->max_version != 0 && version > ssl->max_version) {
+    if (version != 0 && version > ssl->max_version) {
       version = ssl->max_version;
     }
   }
@@ -2659,10 +2664,10 @@ uint16_t ssl3_get_max_client_version(SSL *ssl) {
 
 int ssl3_is_version_enabled(SSL *ssl, uint16_t version) {
   if (SSL_IS_DTLS(ssl)) {
-    if (ssl->max_version != 0 && version < ssl->max_version) {
+    if (version < ssl->max_version) {
       return 0;
     }
-    if (ssl->min_version != 0 && version > ssl->min_version) {
+    if (version > ssl->min_version) {
       return 0;
     }
 
@@ -2677,10 +2682,10 @@ int ssl3_is_version_enabled(SSL *ssl, uint16_t version) {
         return 0;
     }
   } else {
-    if (ssl->max_version != 0 && version > ssl->max_version) {
+    if (version > ssl->max_version) {
       return 0;
     }
-    if (ssl->min_version != 0 && version < ssl->min_version) {
+    if (version < ssl->min_version) {
       return 0;
     }
 

ssl/ssl_test.cc

@@ -689,16 +689,13 @@ static bool TestBadSSL_SESSIONEncoding(const char *input_b64) {
   return true;
 }
 
-static bool TestDefaultVersion(uint16_t version,
+static bool TestDefaultVersion(uint16_t min_version, uint16_t max_version,
                                const SSL_METHOD *(*method)(void)) {
   ScopedSSL_CTX ctx(SSL_CTX_new(method()));
   if (!ctx) {
     return false;
   }
-  // TODO(svaldez): Remove TLS1_2_VERSION fallback upon implementing TLS 1.3.
-  return ctx->min_version == version &&
-         (ctx->max_version == version ||
-          (version == 0 && ctx->max_version == TLS1_2_VERSION));
+  return ctx->min_version == min_version && ctx->max_version == max_version;
 }
 
 static bool CipherGetRFCName(std::string *out, uint16_t value) {
@@ -1361,14 +1358,15 @@ int main() {
       !TestBadSSL_SESSIONEncoding(kBadSessionExtraField) ||
       !TestBadSSL_SESSIONEncoding(kBadSessionVersion) ||
       !TestBadSSL_SESSIONEncoding(kBadSessionTrailingData) ||
-      !TestDefaultVersion(0, &TLS_method) ||
-      !TestDefaultVersion(SSL3_VERSION, &SSLv3_method) ||
-      !TestDefaultVersion(TLS1_VERSION, &TLSv1_method) ||
-      !TestDefaultVersion(TLS1_1_VERSION, &TLSv1_1_method) ||
-      !TestDefaultVersion(TLS1_2_VERSION, &TLSv1_2_method) ||
-      !TestDefaultVersion(0, &DTLS_method) ||
-      !TestDefaultVersion(DTLS1_VERSION, &DTLSv1_method) ||
-      !TestDefaultVersion(DTLS1_2_VERSION, &DTLSv1_2_method) ||
+      // TODO(svaldez): Update this when TLS 1.3 is enabled by default.
+      !TestDefaultVersion(SSL3_VERSION, TLS1_2_VERSION, &TLS_method) ||
+      !TestDefaultVersion(SSL3_VERSION, SSL3_VERSION, &SSLv3_method) ||
+      !TestDefaultVersion(TLS1_VERSION, TLS1_VERSION, &TLSv1_method) ||
+      !TestDefaultVersion(TLS1_1_VERSION, TLS1_1_VERSION, &TLSv1_1_method) ||
+      !TestDefaultVersion(TLS1_2_VERSION, TLS1_2_VERSION, &TLSv1_2_method) ||
+      !TestDefaultVersion(DTLS1_VERSION, DTLS1_2_VERSION, &DTLS_method) ||
+      !TestDefaultVersion(DTLS1_VERSION, DTLS1_VERSION, &DTLSv1_method) ||
+      !TestDefaultVersion(DTLS1_2_VERSION, DTLS1_2_VERSION, &DTLSv1_2_method) ||
       !TestCipherGetRFCName() ||
       !TestPaddingExtension() ||
       !TestClientCAList() ||