Bring back *_client_method() structs

The method unification broke an API promise of SSL_is_server(). According
to the documentation, calling SSL_is_server() on SSL objects constructed
from generic and server methods would result in 1 even before any call to
SSL_set_accept_state(). This means the information needs to be available
when SSL_new() is called, so must come from the method itself.

Prior to the method unification, s->server would be set to 0 or 1 in
SSL_new() depending on whether the accept method was undefined or not.
Instead, introduce a flag to the internal structs to distinguish client
methods from server and generic methods and copy that flag to s->server in
SSL_new().

This problem was reported to otto due to breakage of DoH in net/dnsdist.
The reason for this is that www/h2o relies on SSL_is_server() to decide
whether to call SSL_accept() or SSL_connect(). Thus, the h2o server would
end up responding to a ClientHello with another ClientHello, which results
in a handshake failure. The bandaid applied to www/h2o can be removed once
this fix has made it into snaps. No other breakage is known.

This commit brings back only about half of the duplication removed in the
method unification, so is preferable to a full revert.

ok jsing

3 files changed, 200 insertions, 11 deletions

diff --git a/src/lib/libssl/ssl_lib.c b/src/lib/libssl/ssl_lib.c
index 58b9dae910..69628b48df 100644
--- a/src/lib/libssl/ssl_lib.c
+++ b/src/lib/libssl/ssl_lib.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_lib.c,v 1.238 2020/11/16 18:55:15 jsing Exp $ */
+/* $OpenBSD: ssl_lib.c,v 1.239 2020/12/01 07:46:01 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -345,7 +345,7 @@ SSL_new(SSL_CTX *ctx)
                 goto err;
 
         s->references = 1;
-        s->server = 0;
+        s->server = ctx->method->internal->server;
 
         SSL_clear(s);
 
diff --git a/src/lib/libssl/ssl_locl.h b/src/lib/libssl/ssl_locl.h
index 46a1ad4884..19d883e3b0 100644
--- a/src/lib/libssl/ssl_locl.h
+++ b/src/lib/libssl/ssl_locl.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_locl.h,v 1.307 2020/11/11 18:14:12 jsing Exp $ */
+/* $OpenBSD: ssl_locl.h,v 1.308 2020/12/01 07:46:02 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -359,6 +359,7 @@ __BEGIN_HIDDEN_DECLS
 
 typedef struct ssl_method_internal_st {
         int dtls;
+        int server;
         int version;
 
         uint16_t min_version;
diff --git a/src/lib/libssl/ssl_methods.c b/src/lib/libssl/ssl_methods.c
index 600aa89095..ea67403d5d 100644
--- a/src/lib/libssl/ssl_methods.c
+++ b/src/lib/libssl/ssl_methods.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_methods.c,v 1.20 2020/10/14 16:44:15 jsing Exp $ */
+/* $OpenBSD: ssl_methods.c,v 1.21 2020/12/01 07:46:02 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -61,6 +61,7 @@
 
 static const SSL_METHOD_INTERNAL DTLSv1_method_internal_data = {
         .dtls = 1,
+        .server = 1,
         .version = DTLS1_VERSION,
         .min_version = DTLS1_VERSION,
         .max_version = DTLS1_VERSION,
@@ -87,10 +88,39 @@ static const SSL_METHOD DTLSv1_method_data = {
         .internal = &DTLSv1_method_internal_data,
 };
 
+static const SSL_METHOD_INTERNAL DTLSv1_client_method_internal_data = {
+        .dtls = 1,
+        .server = 0,
+        .version = DTLS1_VERSION,
+        .min_version = DTLS1_VERSION,
+        .max_version = DTLS1_VERSION,
+        .ssl_new = dtls1_new,
+        .ssl_clear = dtls1_clear,
+        .ssl_free = dtls1_free,
+        .ssl_accept = ssl_undefined_function,
+        .ssl_connect = ssl3_connect,
+        .ssl_shutdown = ssl3_shutdown,
+        .ssl_renegotiate = ssl3_renegotiate,
+        .ssl_renegotiate_check = ssl3_renegotiate_check,
+        .ssl_pending = ssl3_pending,
+        .ssl_read_bytes = dtls1_read_bytes,
+        .ssl_write_bytes = dtls1_write_app_data_bytes,
+        .enc_flags = TLSV1_1_ENC_FLAGS,
+};
+
+static const SSL_METHOD DTLSv1_client_method_data = {
+        .ssl_dispatch_alert = dtls1_dispatch_alert,
+        .num_ciphers = ssl3_num_ciphers,
+        .get_cipher = dtls1_get_cipher,
+        .get_cipher_by_char = ssl3_get_cipher_by_char,
+        .put_cipher_by_char = ssl3_put_cipher_by_char,
+        .internal = &DTLSv1_client_method_internal_data,
+};
+
 const SSL_METHOD *
 DTLSv1_client_method(void)
 {
-        return &DTLSv1_method_data;
+        return &DTLSv1_client_method_data;
 }
 
 const SSL_METHOD *
@@ -108,7 +138,7 @@ DTLSv1_server_method(void)
 const SSL_METHOD *
 DTLS_client_method(void)
 {
-        return DTLSv1_method();
+        return DTLSv1_client_method();
 }
 
 const SSL_METHOD *
@@ -126,6 +156,7 @@ DTLS_server_method(void)
 #if defined(LIBRESSL_HAS_TLS1_3_CLIENT) && defined(LIBRESSL_HAS_TLS1_3_SERVER)
 static const SSL_METHOD_INTERNAL TLS_method_internal_data = {
         .dtls = 0,
+        .server = 1,
         .version = TLS1_3_VERSION,
         .min_version = TLS1_VERSION,
         .max_version = TLS1_3_VERSION,
@@ -155,6 +186,7 @@ static const SSL_METHOD TLS_method_data = {
 
 static const SSL_METHOD_INTERNAL TLS_legacy_method_internal_data = {
         .dtls = 0,
+        .server = 1,
         .version = TLS1_2_VERSION,
         .min_version = TLS1_VERSION,
         .max_version = TLS1_2_VERSION,
@@ -181,8 +213,71 @@ static const SSL_METHOD TLS_legacy_method_data = {
         .internal = &TLS_legacy_method_internal_data,
 };
 
+#if defined(LIBRESSL_HAS_TLS1_3_CLIENT)
+static const SSL_METHOD_INTERNAL TLS_client_method_internal_data = {
+        .dtls = 0,
+        .server = 0,
+        .version = TLS1_3_VERSION,
+        .min_version = TLS1_VERSION,
+        .max_version = TLS1_3_VERSION,
+        .ssl_new = tls1_new,
+        .ssl_clear = tls1_clear,
+        .ssl_free = tls1_free,
+        .ssl_accept = tls13_legacy_accept,
+        .ssl_connect = tls13_legacy_connect,
+        .ssl_shutdown = tls13_legacy_shutdown,
+        .ssl_renegotiate = ssl_undefined_function,
+        .ssl_renegotiate_check = ssl_ok,
+        .ssl_pending = tls13_legacy_pending,
+        .ssl_read_bytes = tls13_legacy_read_bytes,
+        .ssl_write_bytes = tls13_legacy_write_bytes,
+        .enc_flags = TLSV1_3_ENC_FLAGS,
+};
+
+static const SSL_METHOD TLS_client_method_data = {
+        .ssl_dispatch_alert = ssl3_dispatch_alert,
+        .num_ciphers = ssl3_num_ciphers,
+        .get_cipher = ssl3_get_cipher,
+        .get_cipher_by_char = ssl3_get_cipher_by_char,
+        .put_cipher_by_char = ssl3_put_cipher_by_char,
+        .internal = &TLS_client_method_internal_data,
+};
+
+#else
+
+static const SSL_METHOD_INTERNAL TLS_legacy_client_method_internal_data = {
+        .dtls = 0,
+        .server = 0,
+        .version = TLS1_2_VERSION,
+        .min_version = TLS1_VERSION,
+        .max_version = TLS1_2_VERSION,
+        .ssl_new = tls1_new,
+        .ssl_clear = tls1_clear,
+        .ssl_free = tls1_free,
+        .ssl_accept = ssl3_accept,
+        .ssl_connect = ssl3_connect,
+        .ssl_shutdown = ssl3_shutdown,
+        .ssl_renegotiate = ssl_undefined_function,
+        .ssl_renegotiate_check = ssl_ok,
+        .ssl_pending = ssl3_pending,
+        .ssl_read_bytes = ssl3_read_bytes,
+        .ssl_write_bytes = ssl3_write_bytes,
+        .enc_flags = TLSV1_2_ENC_FLAGS,
+};
+
+static const SSL_METHOD TLS_legacy_client_method_data = {
+        .ssl_dispatch_alert = ssl3_dispatch_alert,
+        .num_ciphers = ssl3_num_ciphers,
+        .get_cipher = ssl3_get_cipher,
+        .get_cipher_by_char = ssl3_get_cipher_by_char,
+        .put_cipher_by_char = ssl3_put_cipher_by_char,
+        .internal = &TLS_legacy_client_method_internal_data,
+};
+#endif
+
 static const SSL_METHOD_INTERNAL TLSv1_method_internal_data = {
         .dtls = 0,
+        .server = 1,
         .version = TLS1_VERSION,
         .min_version = TLS1_VERSION,
         .max_version = TLS1_VERSION,
@@ -209,8 +304,38 @@ static const SSL_METHOD TLSv1_method_data = {
         .internal = &TLSv1_method_internal_data,
 };
 
+static const SSL_METHOD_INTERNAL TLSv1_client_method_internal_data = {
+        .dtls = 0,
+        .server = 0,
+        .version = TLS1_VERSION,
+        .min_version = TLS1_VERSION,
+        .max_version = TLS1_VERSION,
+        .ssl_new = tls1_new,
+        .ssl_clear = tls1_clear,
+        .ssl_free = tls1_free,
+        .ssl_accept = ssl_undefined_function,
+        .ssl_connect = ssl3_connect,
+        .ssl_shutdown = ssl3_shutdown,
+        .ssl_renegotiate = ssl3_renegotiate,
+        .ssl_renegotiate_check = ssl3_renegotiate_check,
+        .ssl_pending = ssl3_pending,
+        .ssl_read_bytes = ssl3_read_bytes,
+        .ssl_write_bytes = ssl3_write_bytes,
+        .enc_flags = TLSV1_ENC_FLAGS,
+};
+
+static const SSL_METHOD TLSv1_client_method_data = {
+        .ssl_dispatch_alert = ssl3_dispatch_alert,
+        .num_ciphers = ssl3_num_ciphers,
+        .get_cipher = ssl3_get_cipher,
+        .get_cipher_by_char = ssl3_get_cipher_by_char,
+        .put_cipher_by_char = ssl3_put_cipher_by_char,
+        .internal = &TLSv1_client_method_internal_data,
+};
+
 static const SSL_METHOD_INTERNAL TLSv1_1_method_internal_data = {
         .dtls = 0,
+        .server = 1,
         .version = TLS1_1_VERSION,
         .min_version = TLS1_1_VERSION,
         .max_version = TLS1_1_VERSION,
@@ -237,8 +362,38 @@ static const SSL_METHOD TLSv1_1_method_data = {
         .internal = &TLSv1_1_method_internal_data,
 };
 
+static const SSL_METHOD_INTERNAL TLSv1_1_client_method_internal_data = {
+        .dtls = 0,
+        .server = 0,
+        .version = TLS1_1_VERSION,
+        .min_version = TLS1_1_VERSION,
+        .max_version = TLS1_1_VERSION,
+        .ssl_new = tls1_new,
+        .ssl_clear = tls1_clear,
+        .ssl_free = tls1_free,
+        .ssl_accept = ssl_undefined_function,
+        .ssl_connect = ssl3_connect,
+        .ssl_shutdown = ssl3_shutdown,
+        .ssl_renegotiate = ssl3_renegotiate,
+        .ssl_renegotiate_check = ssl3_renegotiate_check,
+        .ssl_pending = ssl3_pending,
+        .ssl_read_bytes = ssl3_read_bytes,
+        .ssl_write_bytes = ssl3_write_bytes,
+        .enc_flags = TLSV1_1_ENC_FLAGS,
+};
+
+static const SSL_METHOD TLSv1_1_client_method_data = {
+        .ssl_dispatch_alert = ssl3_dispatch_alert,
+        .num_ciphers = ssl3_num_ciphers,
+        .get_cipher = ssl3_get_cipher,
+        .get_cipher_by_char = ssl3_get_cipher_by_char,
+        .put_cipher_by_char = ssl3_put_cipher_by_char,
+        .internal = &TLSv1_1_client_method_internal_data,
+};
+
 static const SSL_METHOD_INTERNAL TLSv1_2_method_internal_data = {
         .dtls = 0,
+        .server = 1,
         .version = TLS1_2_VERSION,
         .min_version = TLS1_2_VERSION,
         .max_version = TLS1_2_VERSION,
@@ -265,10 +420,43 @@ static const SSL_METHOD TLSv1_2_method_data = {
         .internal = &TLSv1_2_method_internal_data,
 };
 
+static const SSL_METHOD_INTERNAL TLSv1_2_client_method_internal_data = {
+        .dtls = 0,
+        .server = 0,
+        .version = TLS1_2_VERSION,
+        .min_version = TLS1_2_VERSION,
+        .max_version = TLS1_2_VERSION,
+        .ssl_new = tls1_new,
+        .ssl_clear = tls1_clear,
+        .ssl_free = tls1_free,
+        .ssl_accept = ssl_undefined_function,
+        .ssl_connect = ssl3_connect,
+        .ssl_shutdown = ssl3_shutdown,
+        .ssl_renegotiate = ssl3_renegotiate,
+        .ssl_renegotiate_check = ssl3_renegotiate_check,
+        .ssl_pending = ssl3_pending,
+        .ssl_read_bytes = ssl3_read_bytes,
+        .ssl_write_bytes = ssl3_write_bytes,
+        .enc_flags = TLSV1_2_ENC_FLAGS,
+};
+
+static const SSL_METHOD TLSv1_2_client_method_data = {
+        .ssl_dispatch_alert = ssl3_dispatch_alert,
+        .num_ciphers = ssl3_num_ciphers,
+        .get_cipher = ssl3_get_cipher,
+        .get_cipher_by_char = ssl3_get_cipher_by_char,
+        .put_cipher_by_char = ssl3_put_cipher_by_char,
+        .internal = &TLSv1_2_client_method_internal_data,
+};
+
 const SSL_METHOD *
 TLS_client_method(void)
 {
-        return TLS_method();
+#if defined(LIBRESSL_HAS_TLS1_3_CLIENT)
+        return (&TLS_client_method_data);
+#else
+        return (&TLS_legacy_client_method_data);
+#endif
 }
 
 const SSL_METHOD *
@@ -296,7 +484,7 @@ tls_legacy_method(void)
 const SSL_METHOD *
 SSLv23_client_method(void)
 {
-        return TLS_method();
+        return TLS_client_method();
 }
 
 const SSL_METHOD *
@@ -314,7 +502,7 @@ SSLv23_server_method(void)
 const SSL_METHOD *
 TLSv1_client_method(void)
 {
-        return (&TLSv1_method_data);
+        return (&TLSv1_client_method_data);
 }
 
 const SSL_METHOD *
@@ -332,7 +520,7 @@ TLSv1_server_method(void)
 const SSL_METHOD *
 TLSv1_1_client_method(void)
 {
-        return (&TLSv1_1_method_data);
+        return (&TLSv1_1_client_method_data);
 }
 
 const SSL_METHOD *
@@ -350,7 +538,7 @@ TLSv1_1_server_method(void)
 const SSL_METHOD *
 TLSv1_2_client_method(void)
 {
-        return (&TLSv1_2_method_data);
+        return (&TLSv1_2_client_method_data);
 }
 
 const SSL_METHOD *