Don't return invalid versions in version_from_wire.

This is in preparation for using the supported_versions extension to
experiment with draft TLS 1.3 versions, since we don't wish to restore
the fallback. With versions begin opaque values, we will want
version_from_wire to reject unknown values, not attempt to preserve
order in some way.

This means ClientHello.version processing needs to be separate code.
That's just written out fully in negotiate_version now. It also means
SSL_set_{min,max}_version will notice invalid inputs which aligns us
better with upstream's versions of those APIs.

This CL doesn't replace ssl->version with an internal-representation
version, though follow work should do it once a couple of changes land
in consumers.

BUG=90

Change-Id: Id2f5e1fa72847c823ee7f082e9e69f55e51ce9da
Reviewed-on: https://boringssl-review.googlesource.com/11122
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

include/openssl/ssl.h

@@ -565,20 +565,20 @@ OPENSSL_EXPORT int DTLSv1_handle_timeout(SSL *ssl);
 #define TLS1_3_DRAFT_VERSION 14
 
 /* SSL_CTX_set_min_version sets the minimum protocol version for |ctx| to
- * |version|. */
-OPENSSL_EXPORT void SSL_CTX_set_min_version(SSL_CTX *ctx, uint16_t version);
+ * |version|. It returns one on success and zero if |version| is invalid. */
+OPENSSL_EXPORT int SSL_CTX_set_min_version(SSL_CTX *ctx, uint16_t version);
 
 /* SSL_CTX_set_max_version sets the maximum protocol version for |ctx| to
- * |version|. */
-OPENSSL_EXPORT void SSL_CTX_set_max_version(SSL_CTX *ctx, uint16_t version);
+ * |version|. It returns one on success and zero if |version| is invalid. */
+OPENSSL_EXPORT int SSL_CTX_set_max_version(SSL_CTX *ctx, uint16_t version);
 
 /* SSL_set_min_version sets the minimum protocol version for |ssl| to
- * |version|. */
-OPENSSL_EXPORT void SSL_set_min_version(SSL *ssl, uint16_t version);
+ * |version|. It returns one on success and zero if |version| is invalid. */
+OPENSSL_EXPORT int SSL_set_min_version(SSL *ssl, uint16_t version);
 
 /* SSL_set_max_version sets the maximum protocol version for |ssl| to
- * |version|. */
-OPENSSL_EXPORT void SSL_set_max_version(SSL *ssl, uint16_t version);
+ * |version|. It returns one on success and zero if |version| is invalid. */
+OPENSSL_EXPORT int SSL_set_max_version(SSL *ssl, uint16_t version);
 
 /* SSL_version returns the TLS or DTLS protocol version used by |ssl|, which is
  * one of the |*_VERSION| values. (E.g. |TLS1_2_VERSION|.) Before the version

ssl/dtls_method.c

@@ -65,31 +65,33 @@
 #include "internal.h"
 
 
-static uint16_t dtls1_version_from_wire(uint16_t wire_version) {
-  uint16_t tls_version = ~wire_version;
-  uint16_t version = tls_version + 0x0201;
-  /* If either component overflowed, clamp it so comparisons still work. */
-  if ((version >> 8) < (tls_version >> 8)) {
-    version = 0xff00 | (version & 0xff);
+static int dtls1_version_from_wire(uint16_t *out_version,
+                                   uint16_t wire_version) {
+  switch (wire_version) {
+    case DTLS1_VERSION:
+      /* DTLS 1.0 maps to TLS 1.1, not TLS 1.0. */
+      *out_version = TLS1_1_VERSION;
+      return 1;
+    case DTLS1_2_VERSION:
+      *out_version = TLS1_2_VERSION;
+      return 1;
   }
-  if ((version & 0xff) < (tls_version & 0xff)) {
-    version = (version & 0xff00) | 0xff;
-  }
-  /* DTLS 1.0 maps to TLS 1.1, not TLS 1.0. */
-  if (version == TLS1_VERSION) {
-    version = TLS1_1_VERSION;
-  }
-  return version;
+
+  return 0;
 }
 
 static uint16_t dtls1_version_to_wire(uint16_t version) {
-  assert(version >= TLS1_1_VERSION);
-
-  /* DTLS 1.0 maps to TLS 1.1, not TLS 1.0. */
-  if (version == TLS1_1_VERSION) {
-    return DTLS1_VERSION;
+  switch (version) {
+    case TLS1_1_VERSION:
+      /* DTLS 1.0 maps to TLS 1.1, not TLS 1.0. */
+      return DTLS1_VERSION;
+    case TLS1_2_VERSION:
+      return DTLS1_2_VERSION;
   }
-  return ~(version - 0x0201);
+
+  /* It is an error to use this function with an invalid version. */
+  assert(0);
+  return 0;
 }
 
 static int dtls1_set_read_state(SSL *ssl, SSL_AEAD_CTX *aead_ctx) {

ssl/handshake_client.c

@@ -609,9 +609,11 @@ static int ssl_write_client_cipher_list(SSL *ssl, CBB *out,
       return 0;
     }
     /* Add PSK ciphers for TLS 1.3 resumption. */
+    uint16_t session_version;
     if (ssl->session != NULL &&
-        ssl->method->version_from_wire(ssl->session->ssl_version) >=
-            TLS1_3_VERSION) {
+        ssl->method->version_from_wire(&session_version,
+                                       ssl->session->ssl_version) &&
+        session_version >= TLS1_3_VERSION) {
       uint16_t resumption_cipher;
       if (ssl_cipher_get_ecdhe_psk_cipher(cipher, &resumption_cipher) &&
           !CBB_add_u16(&child, resumption_cipher)) {
@@ -716,9 +718,10 @@ static int ssl3_send_client_hello(SSL *ssl) {
   /* If the configured session has expired or was created at a disabled
    * version, drop it. */
   if (ssl->session != NULL) {
-    uint16_t session_version =
-        ssl->method->version_from_wire(ssl->session->ssl_version);
-    if ((session_version < TLS1_3_VERSION &&
+    uint16_t session_version;
+    if (!ssl->method->version_from_wire(&session_version,
+                                        ssl->session->ssl_version) ||
+        (session_version < TLS1_3_VERSION &&
          ssl->session->session_id_length == 0) ||
         ssl->session->not_resumable ||
         !ssl_session_is_time_valid(ssl, ssl->session) ||
@@ -797,7 +800,7 @@ static int ssl3_get_server_hello(SSL *ssl) {
   CERT *ct = ssl->cert;
   int al = SSL_AD_INTERNAL_ERROR;
   CBS server_hello, server_random, session_id;
-  uint16_t server_wire_version, server_version, cipher_suite;
+  uint16_t server_wire_version, cipher_suite;
   uint8_t compression_method;
 
   int ret = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message);
@@ -831,10 +834,9 @@ static int ssl3_get_server_hello(SSL *ssl) {
     goto f_err;
   }
 
-  server_version = ssl->method->version_from_wire(server_wire_version);
-
-  uint16_t min_version, max_version;
+  uint16_t min_version, max_version, server_version;
   if (!ssl_get_version_range(ssl, &min_version, &max_version) ||
+      !ssl->method->version_from_wire(&server_version, server_wire_version) ||
       server_version < min_version || server_version > max_version) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_PROTOCOL);
     al = SSL_AD_PROTOCOL_VERSION;

ssl/handshake_server.c

@@ -564,12 +564,30 @@ static int negotiate_version(
     return 0;
   }
 
-  uint16_t client_version =
-      ssl->method->version_from_wire(client_hello->version);
-  ssl->client_version = client_hello->version;
+  /* For TLS versions which use ClientHello.version, convert it to a version we
+   * are aware of. */
+  uint16_t version = 0;
+  if (SSL_is_dtls(ssl)) {
+    if (client_hello->version <= DTLS1_2_VERSION) {
+      version = TLS1_2_VERSION;
+    } else if (client_hello->version <= DTLS1_VERSION) {
+      version = TLS1_1_VERSION;
+    }
+  } else {
+    if (client_hello->version >= TLS1_3_VERSION) {
+      version = TLS1_3_VERSION;
+    } else if (client_hello->version >= TLS1_2_VERSION) {
+      version = TLS1_2_VERSION;
+    } else if (client_hello->version >= TLS1_1_VERSION) {
+      version = TLS1_1_VERSION;
+    } else if (client_hello->version >= TLS1_VERSION) {
+      version = TLS1_VERSION;
+    } else if (client_hello->version >= SSL3_VERSION) {
+      version = SSL3_VERSION;
+    }
+  }
 
-  /* Select the version to use. */
-  uint16_t version = client_version;
+  /* Apply our minimum and maximum version. */
   if (version > max_version) {
     version = max_version;
   }
@@ -589,6 +607,7 @@ static int negotiate_version(
     return 0;
   }
 
+  ssl->client_version = client_hello->version;
   ssl->version = ssl->method->version_to_wire(version);
   ssl->s3->enc_method = ssl3_get_enc_method(version);
   assert(ssl->s3->enc_method != NULL);

ssl/internal.h

@@ -1090,14 +1090,10 @@ struct ssl_protocol_method_st {
   uint16_t min_version;
   /* max_version is the maximum implemented version. */
   uint16_t max_version;
-  /* version_from_wire maps |wire_version| to a protocol version. For
-   * SSLv3/TLS, the version is returned as-is. For DTLS, the corresponding TLS
-   * version is used. Note that this mapping is not injective but preserves
-   * comparisons.
-   *
-   * TODO(davidben): To normalize some DTLS-specific code, move away from using
-   * the wire version except at API boundaries. */
-  uint16_t (*version_from_wire)(uint16_t wire_version);
+  /* version_from_wire maps |wire_version| to a protocol version. On success, it
+   * sets |*out_version| to the result and returns one. If the version is
+   * unknown, it returns zero. */
+  int (*version_from_wire)(uint16_t *out_version, uint16_t wire_version);
   /* version_to_wire maps |version| to the wire representation. It is an error
    * to call it with an invalid version. */
   uint16_t (*version_to_wire)(uint16_t version);

ssl/ssl_lib.c

@@ -949,20 +949,20 @@ int SSL_get_error(const SSL *ssl, int ret_code) {
   return SSL_ERROR_SYSCALL;
 }
 
-void SSL_CTX_set_min_version(SSL_CTX *ctx, uint16_t version) {
-  ctx->min_version = ctx->method->version_from_wire(version);
+int SSL_CTX_set_min_version(SSL_CTX *ctx, uint16_t version) {
+  return ctx->method->version_from_wire(&ctx->min_version, version);
 }
 
-void SSL_CTX_set_max_version(SSL_CTX *ctx, uint16_t version) {
-  ctx->max_version = ctx->method->version_from_wire(version);
+int SSL_CTX_set_max_version(SSL_CTX *ctx, uint16_t version) {
+  return ctx->method->version_from_wire(&ctx->max_version, version);
 }
 
-void SSL_set_min_version(SSL *ssl, uint16_t version) {
-  ssl->min_version = ssl->method->version_from_wire(version);
+int SSL_set_min_version(SSL *ssl, uint16_t version) {
+  return ssl->method->version_from_wire(&ssl->min_version, version);
 }
 
-void SSL_set_max_version(SSL *ssl, uint16_t version) {
-  ssl->max_version = ssl->method->version_from_wire(version);
+int SSL_set_max_version(SSL *ssl, uint16_t version) {
+  return ssl->method->version_from_wire(&ssl->max_version, version);
 }
 
 uint32_t SSL_CTX_set_options(SSL_CTX *ctx, uint32_t options) {
@@ -2750,7 +2750,15 @@ int ssl_get_version_range(const SSL *ssl, uint16_t *out_min_version,
 
 uint16_t ssl3_protocol_version(const SSL *ssl) {
   assert(ssl->s3->have_version);
-  return ssl->method->version_from_wire(ssl->version);
+  uint16_t version;
+  if (!ssl->method->version_from_wire(&version, ssl->version)) {
+    /* TODO(davidben): Use the internal version representation for ssl->version
+     * and map to the public API representation at API boundaries. */
+    assert(0);
+    return 0;
+  }
+
+  return version;
 }
 
 int SSL_is_server(const SSL *ssl) { return ssl->server; }

ssl/ssl_test.cc

@@ -1523,11 +1523,11 @@ static bool TestGetPeerCertificate() {
     bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
     if (!ctx ||
         !SSL_CTX_use_certificate(ctx.get(), cert.get()) ||
-        !SSL_CTX_use_PrivateKey(ctx.get(), key.get())) {
+        !SSL_CTX_use_PrivateKey(ctx.get(), key.get()) ||
+        !SSL_CTX_set_min_version(ctx.get(), version) ||
+        !SSL_CTX_set_max_version(ctx.get(), version)) {
       return false;
     }
-    SSL_CTX_set_min_version(ctx.get(), version);
-    SSL_CTX_set_max_version(ctx.get(), version);
     SSL_CTX_set_verify(
         ctx.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, nullptr);
     SSL_CTX_set_cert_verify_callback(ctx.get(), VerifySucceed, NULL);
@@ -1590,11 +1590,11 @@ static bool TestRetainOnlySHA256OfCerts() {
     bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
     if (!ctx ||
         !SSL_CTX_use_certificate(ctx.get(), cert.get()) ||
-        !SSL_CTX_use_PrivateKey(ctx.get(), key.get())) {
+        !SSL_CTX_use_PrivateKey(ctx.get(), key.get()) ||
+        !SSL_CTX_set_min_version(ctx.get(), version) ||
+        !SSL_CTX_set_max_version(ctx.get(), version)) {
       return false;
     }
-    SSL_CTX_set_min_version(ctx.get(), version);
-    SSL_CTX_set_max_version(ctx.get(), version);
     SSL_CTX_set_verify(
         ctx.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, nullptr);
     SSL_CTX_set_cert_verify_callback(ctx.get(), VerifySucceed, NULL);
@@ -1631,15 +1631,14 @@ static bool TestRetainOnlySHA256OfCerts() {
 static bool ClientHelloMatches(uint16_t version, const uint8_t *expected,
                                size_t expected_len) {
   bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
-  if (!ctx) {
-    return false;
-  }
-  SSL_CTX_set_max_version(ctx.get(), version);
-  // Our default cipher list varies by CPU capabilities, so manually place the
-  // ChaCha20 ciphers in front.
-  if (!SSL_CTX_set_cipher_list(ctx.get(), "CHACHA20:ALL")) {
+  if (!ctx ||
+      !SSL_CTX_set_max_version(ctx.get(), version) ||
+      // Our default cipher list varies by CPU capabilities, so manually place
+      // the ChaCha20 ciphers in front.
+      !SSL_CTX_set_cipher_list(ctx.get(), "CHACHA20:ALL")) {
     return false;
   }
+
   bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get()));
   if (!ssl) {
     return false;
@@ -1872,16 +1871,15 @@ static bool TestSessionIDContext() {
         !SSL_CTX_use_certificate(server_ctx.get(), cert.get()) ||
         !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get()) ||
         !SSL_CTX_set_session_id_context(server_ctx.get(), kContext1,
-                                        sizeof(kContext1))) {
+                                        sizeof(kContext1)) ||
+        !SSL_CTX_set_min_version(client_ctx.get(), version) ||
+        !SSL_CTX_set_max_version(client_ctx.get(), version) ||
+        !SSL_CTX_set_min_version(server_ctx.get(), version) ||
+        !SSL_CTX_set_max_version(server_ctx.get(), version)) {
       return false;
     }
 
-    SSL_CTX_set_min_version(client_ctx.get(), version);
-    SSL_CTX_set_max_version(client_ctx.get(), version);
     SSL_CTX_set_session_cache_mode(client_ctx.get(), SSL_SESS_CACHE_BOTH);
-
-    SSL_CTX_set_min_version(server_ctx.get(), version);
-    SSL_CTX_set_max_version(server_ctx.get(), version);
     SSL_CTX_set_session_cache_mode(server_ctx.get(), SSL_SESS_CACHE_BOTH);
 
     bssl::UniquePtr<SSL_SESSION> session =
@@ -1933,16 +1931,16 @@ static bool TestSessionTimeout() {
     bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
     if (!server_ctx || !client_ctx ||
         !SSL_CTX_use_certificate(server_ctx.get(), cert.get()) ||
-        !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get())) {
+        !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get()) ||
+        !SSL_CTX_set_min_version(client_ctx.get(), version) ||
+        !SSL_CTX_set_max_version(client_ctx.get(), version) ||
+        !SSL_CTX_set_min_version(server_ctx.get(), version) ||
+        !SSL_CTX_set_max_version(server_ctx.get(), version)) {
       return false;
     }
 
-    SSL_CTX_set_min_version(client_ctx.get(), version);
-    SSL_CTX_set_max_version(client_ctx.get(), version);
     SSL_CTX_set_session_cache_mode(client_ctx.get(), SSL_SESS_CACHE_BOTH);
 
-    SSL_CTX_set_min_version(server_ctx.get(), version);
-    SSL_CTX_set_max_version(server_ctx.get(), version);
     SSL_CTX_set_session_cache_mode(server_ctx.get(), SSL_SESS_CACHE_BOTH);
     SSL_CTX_set_current_time_cb(server_ctx.get(), CurrentTimeCallback);
 
@@ -2012,17 +2010,16 @@ static bool TestSNICallback() {
         // this doesn't happen when |version| is TLS 1.2, configure the private
         // key to only sign SHA-256.
         !SSL_CTX_set_signing_algorithm_prefs(server_ctx2.get(),
-                                             &kECDSAWithSHA256, 1)) {
+                                             &kECDSAWithSHA256, 1) ||
+        !SSL_CTX_set_min_version(client_ctx.get(), version) ||
+        !SSL_CTX_set_max_version(client_ctx.get(), version) ||
+        !SSL_CTX_set_min_version(server_ctx.get(), version) ||
+        !SSL_CTX_set_max_version(server_ctx.get(), version) ||
+        !SSL_CTX_set_min_version(server_ctx2.get(), version) ||
+        !SSL_CTX_set_max_version(server_ctx2.get(), version)) {
       return false;
     }
 
-    SSL_CTX_set_min_version(client_ctx.get(), version);
-    SSL_CTX_set_max_version(client_ctx.get(), version);
-    SSL_CTX_set_min_version(server_ctx.get(), version);
-    SSL_CTX_set_max_version(server_ctx.get(), version);
-    SSL_CTX_set_min_version(server_ctx2.get(), version);
-    SSL_CTX_set_max_version(server_ctx2.get(), version);
-
     SSL_CTX_set_tlsext_servername_callback(server_ctx.get(), SwitchContext);
     SSL_CTX_set_tlsext_servername_arg(server_ctx.get(), server_ctx2.get());
 
@@ -2047,7 +2044,10 @@ static bool TestSNICallback() {
 }
 
 static int SetMaxVersion(const struct ssl_early_callback_ctx *ctx) {
-  SSL_set_max_version(ctx->ssl, TLS1_2_VERSION);
+  if (!SSL_set_max_version(ctx->ssl, TLS1_2_VERSION)) {
+    return -1;
+  }
+
   return 1;
 }
 
@@ -2060,13 +2060,12 @@ static bool TestEarlyCallbackVersionSwitch() {
   bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
   if (!cert || !key || !server_ctx || !client_ctx ||
       !SSL_CTX_use_certificate(server_ctx.get(), cert.get()) ||
-      !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get())) {
+      !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get()) ||
+      !SSL_CTX_set_max_version(client_ctx.get(), TLS1_3_VERSION) ||
+      !SSL_CTX_set_max_version(server_ctx.get(), TLS1_3_VERSION)) {
     return false;
   }
 
-  SSL_CTX_set_max_version(client_ctx.get(), TLS1_3_VERSION);
-  SSL_CTX_set_max_version(server_ctx.get(), TLS1_3_VERSION);
-
   SSL_CTX_set_select_certificate_cb(server_ctx.get(), SetMaxVersion);
 
   bssl::UniquePtr<SSL> client, server;
@@ -2083,6 +2082,58 @@ static bool TestEarlyCallbackVersionSwitch() {
   return true;
 }
 
+static bool TestSetVersion() {
+  bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
+  if (!ctx) {
+    return false;
+  }
+
+  if (!SSL_CTX_set_max_version(ctx.get(), TLS1_VERSION) ||
+      !SSL_CTX_set_max_version(ctx.get(), TLS1_1_VERSION) ||
+      !SSL_CTX_set_min_version(ctx.get(), TLS1_VERSION) ||
+      !SSL_CTX_set_min_version(ctx.get(), TLS1_1_VERSION)) {
+    fprintf(stderr, "Could not set valid TLS version.\n");
+    return false;
+  }
+
+  if (SSL_CTX_set_max_version(ctx.get(), DTLS1_VERSION) ||
+      SSL_CTX_set_max_version(ctx.get(), 0x0200) ||
+      SSL_CTX_set_max_version(ctx.get(), 0x1234) ||
+      SSL_CTX_set_min_version(ctx.get(), DTLS1_VERSION) ||
+      SSL_CTX_set_min_version(ctx.get(), 0x0200) ||
+      SSL_CTX_set_min_version(ctx.get(), 0x1234)) {
+    fprintf(stderr, "Unexpectedly set invalid TLS version.\n");
+    return false;
+  }
+
+  ctx.reset(SSL_CTX_new(DTLS_method()));
+  if (!ctx) {
+    return false;
+  }
+
+  if (!SSL_CTX_set_max_version(ctx.get(), DTLS1_VERSION) ||
+      !SSL_CTX_set_max_version(ctx.get(), DTLS1_2_VERSION) ||
+      !SSL_CTX_set_min_version(ctx.get(), DTLS1_VERSION) ||
+      !SSL_CTX_set_min_version(ctx.get(), DTLS1_2_VERSION)) {
+    fprintf(stderr, "Could not set valid DTLS version.\n");
+    return false;
+  }
+
+  if (SSL_CTX_set_max_version(ctx.get(), TLS1_VERSION) ||
+      SSL_CTX_set_max_version(ctx.get(), 0xfefe /* DTLS 1.1 */) ||
+      SSL_CTX_set_max_version(ctx.get(), 0xfffe /* DTLS 0.1 */) ||
+      SSL_CTX_set_max_version(ctx.get(), 0x1234) ||
+      SSL_CTX_set_min_version(ctx.get(), TLS1_VERSION) ||
+      SSL_CTX_set_min_version(ctx.get(), 0xfefe /* DTLS 1.1 */) ||
+      SSL_CTX_set_min_version(ctx.get(), 0xfffe /* DTLS 0.1 */) ||
+      SSL_CTX_set_min_version(ctx.get(), 0x1234)) {
+    fprintf(stderr, "Unexpectedly set invalid DTLS version.\n");
+    return false;
+  }
+
+  return true;
+}
+
 int main() {
   CRYPTO_library_init();
 
@@ -2118,7 +2169,8 @@ int main() {
       !TestSessionIDContext() ||
       !TestSessionTimeout() ||
       !TestSNICallback() ||
-      !TestEarlyCallbackVersionSwitch()) {
+      !TestEarlyCallbackVersionSwitch() ||
+      !TestSetVersion()) {
     ERR_print_errors_fp(stderr);
     return 1;
   }

ssl/t1_lib.c

@@ -1038,12 +1038,14 @@ static int ext_ticket_add_clienthello(SSL *ssl, CBB *out) {
    * advertise the extension to avoid potentially breaking servers which carry
    * over the state from the previous handshake, such as OpenSSL servers
    * without upstream's 3c3f0259238594d77264a78944d409f2127642c4. */
+  uint16_t session_version;
   if (!ssl->s3->initial_handshake_complete &&
       ssl->session != NULL &&
       ssl->session->tlsext_tick != NULL &&
       /* Don't send TLS 1.3 session tickets in the ticket extension. */
-      ssl->method->version_from_wire(ssl->session->ssl_version) <
-          TLS1_3_VERSION) {
+      ssl->method->version_from_wire(&session_version,
+                                     ssl->session->ssl_version) &&
+      session_version < TLS1_3_VERSION) {
     ticket_data = ssl->session->tlsext_tick;
     ticket_len = ssl->session->tlsext_ticklen;
   }
@@ -1107,7 +1109,12 @@ static int ext_ticket_add_serverhello(SSL *ssl, CBB *out) {
  * https://tools.ietf.org/html/rfc5246#section-7.4.1.4.1 */
 
 static int ext_sigalgs_add_clienthello(SSL *ssl, CBB *out) {
-  if (ssl->method->version_from_wire(ssl->client_version) < TLS1_2_VERSION) {
+  uint16_t min_version, max_version;
+  if (!ssl_get_version_range(ssl, &min_version, &max_version)) {
+    return 0;
+  }
+
+  if (max_version < TLS1_2_VERSION) {
     return 1;
   }
 
@@ -1990,9 +1997,11 @@ static int ext_pre_shared_key_add_clienthello(SSL *ssl, CBB *out) {
     return 0;
   }
 
+  uint16_t session_version;
   if (max_version < TLS1_3_VERSION || ssl->session == NULL ||
-      ssl->method->version_from_wire(ssl->session->ssl_version) <
-          TLS1_3_VERSION) {
+      !ssl->method->version_from_wire(&session_version,
+                                      ssl->session->ssl_version) ||
+      session_version < TLS1_3_VERSION) {
     return 1;
   }
 

ssl/test/bssl_shim.cc

@@ -814,9 +814,10 @@ static bssl::UniquePtr<SSL_CTX> SetupCtx(const TestConfig *config) {
     return nullptr;
   }
 
-  if (!config->is_dtls) {
-    // Enable TLS 1.3 for tests.
-    SSL_CTX_set_max_version(ssl_ctx.get(), TLS1_3_VERSION);
+  // Enable TLS 1.3 for tests.
+  if (!config->is_dtls &&
+      !SSL_CTX_set_max_version(ssl_ctx.get(), TLS1_3_VERSION)) {
+    return nullptr;
   }
 
   std::string cipher_list = "ALL";
@@ -1364,11 +1365,13 @@ static bool DoExchange(bssl::UniquePtr<SSL_SESSION> *out_session,
       !SSL_enable_signed_cert_timestamps(ssl.get())) {
     return false;
   }
-  if (config->min_version != 0) {
-    SSL_set_min_version(ssl.get(), (uint16_t)config->min_version);
+  if (config->min_version != 0 &&
+      !SSL_set_min_version(ssl.get(), (uint16_t)config->min_version)) {
+    return false;
   }
-  if (config->max_version != 0) {
-    SSL_set_max_version(ssl.get(), (uint16_t)config->max_version);
+  if (config->max_version != 0 &&
+      !SSL_set_max_version(ssl.get(), (uint16_t)config->max_version)) {
+    return false;
   }
   if (config->mtu != 0) {
     SSL_set_options(ssl.get(), SSL_OP_NO_QUERY_MTU);

ssl/test/runner/runner.go

@@ -4221,6 +4221,17 @@ func addVersionNegotiationTests() {
 		expectedError: ":UNSUPPORTED_PROTOCOL:",
 	})
 
+	testCases = append(testCases, testCase{
+		name: "ServerBogusVersion",
+		config: Config{
+			Bugs: ProtocolBugs{
+				SendServerHelloVersion: 0x1234,
+			},
+		},
+		shouldFail:    true,
+		expectedError: ":UNSUPPORTED_PROTOCOL:",
+	})
+
 	// Test TLS 1.3's downgrade signal.
 	testCases = append(testCases, testCase{
 		name: "Downgrade-TLS12-Client",

ssl/tls_method.c

@@ -56,6 +56,7 @@
 
 #include <openssl/ssl.h>
 
+#include <assert.h>
 #include <string.h>
 
 #include <openssl/buf.h>
@@ -63,11 +64,35 @@
 #include "internal.h"
 
 
-static uint16_t ssl3_version_from_wire(uint16_t wire_version) {
-  return wire_version;
+static int ssl3_version_from_wire(uint16_t *out_version,
+                                  uint16_t wire_version) {
+  switch (wire_version) {
+    case SSL3_VERSION:
+    case TLS1_VERSION:
+    case TLS1_1_VERSION:
+    case TLS1_2_VERSION:
+    case TLS1_3_VERSION:
+      *out_version = wire_version;
+      return 1;
+  }
+
+  return 0;
 }
 
-static uint16_t ssl3_version_to_wire(uint16_t version) { return version; }
+static uint16_t ssl3_version_to_wire(uint16_t version) {
+  switch (version) {
+    case SSL3_VERSION:
+    case TLS1_VERSION:
+    case TLS1_1_VERSION:
+    case TLS1_2_VERSION:
+    case TLS1_3_VERSION:
+      return version;
+  }
+
+  /* It is an error to use this function with an invalid version. */
+  assert(0);
+  return 0;
+}
 
 static int ssl3_set_read_state(SSL *ssl, SSL_AEAD_CTX *aead_ctx) {
   if (ssl->s3->rrec.length != 0) {

tool/client.cc

@@ -169,7 +169,9 @@ bool Client(const std::vector<std::string> &args) {
               args_map["-max-version"].c_str());
       return false;
     }
-    SSL_CTX_set_max_version(ctx.get(), version);
+    if (!SSL_CTX_set_max_version(ctx.get(), version)) {
+      return false;
+    }
   }
 
   if (args_map.count("-min-version") != 0) {
@@ -179,7 +181,9 @@ bool Client(const std::vector<std::string> &args) {
               args_map["-min-version"].c_str());
       return false;
     }
-    SSL_CTX_set_min_version(ctx.get(), version);
+    if (!SSL_CTX_set_min_version(ctx.get(), version)) {
+      return false;
+    }
   }
 
   if (args_map.count("-select-next-proto") != 0) {

tool/server.cc

@@ -133,7 +133,9 @@ bool Server(const std::vector<std::string> &args) {
               args_map["-max-version"].c_str());
       return false;
     }
-    SSL_CTX_set_max_version(ctx, version);
+    if (!SSL_CTX_set_max_version(ctx, version)) {
+      return false;
+    }
   }
 
   if (args_map.count("-min-version") != 0) {
@@ -143,7 +145,9 @@ bool Server(const std::vector<std::string> &args) {
               args_map["-min-version"].c_str());
       return false;
     }
-    SSL_CTX_set_min_version(ctx, version);
+    if (!SSL_CTX_set_min_version(ctx, version)) {
+      return false;
+    }
   }
 
   if (args_map.count("-ocsp-response") != 0 &&