Merge pull request #924 from rhenium/ky/ssl-dh-auto

ssl: use SSL_CTX_set_dh_auto() by default

Author: rhenium

ext/openssl/extconf.rb

@@ -147,7 +147,7 @@ def find_openssl_library
 have_func("EVP_PKEY_check(NULL)", evp_h)
 
 # added in 3.0.0
-have_func("SSL_set0_tmp_dh_pkey(NULL, NULL)", ssl_h)
+have_func("SSL_CTX_set0_tmp_dh_pkey(NULL, NULL)", ssl_h)
 have_func("ERR_get_error_all(NULL, NULL, NULL, NULL, NULL)", "openssl/err.h")
 have_func("SSL_CTX_load_verify_file(NULL, \"\")", ssl_h)
 have_func("BN_check_prime(NULL, NULL, NULL)", "openssl/bn.h")

ext/openssl/ossl_ssl.c

@@ -36,8 +36,7 @@ VALUE cSSLSocket;
 static VALUE eSSLErrorWaitReadable;
 static VALUE eSSLErrorWaitWritable;
 
-static ID id_call, ID_callback_state, id_tmp_dh_callback,
-	  id_npn_protocols_encoded, id_each;
+static ID id_call, ID_callback_state, id_npn_protocols_encoded, id_each;
 static VALUE sym_exception, sym_wait_readable, sym_wait_writable;
 
 static ID id_i_cert_store, id_i_ca_file, id_i_ca_path, id_i_verify_mode,
@@ -47,7 +46,7 @@ static ID id_i_cert_store, id_i_ca_file, id_i_ca_path, id_i_verify_mode,
 	  id_i_session_id_context, id_i_session_get_cb, id_i_session_new_cb,
 	  id_i_session_remove_cb, id_i_npn_select_cb, id_i_npn_protocols,
 	  id_i_alpn_select_cb, id_i_alpn_protocols, id_i_servername_cb,
-	  id_i_verify_hostname, id_i_keylog_cb;
+	  id_i_verify_hostname, id_i_keylog_cb, id_i_tmp_dh_callback;
 static ID id_i_io, id_i_context, id_i_hostname;
 
 static int ossl_ssl_ex_ptr_idx;
@@ -90,6 +89,7 @@ ossl_sslctx_s_alloc(VALUE klass)
         ossl_raise(eSSLError, "SSL_CTX_new");
     }
     SSL_CTX_set_mode(ctx, mode);
+    SSL_CTX_set_dh_auto(ctx, 1);
     RTYPEDDATA_DATA(obj) = ctx;
     SSL_CTX_set_ex_data(ctx, ossl_sslctx_ex_ptr_idx, (void *)obj);
 
@@ -133,8 +133,6 @@ ossl_client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
 #if !defined(OPENSSL_NO_DH)
 struct tmp_dh_callback_args {
     VALUE ssl_obj;
-    ID id;
-    int type;
     int is_export;
     int keylength;
 };
@@ -143,48 +141,36 @@ static VALUE
 ossl_call_tmp_dh_callback(VALUE arg)
 {
     struct tmp_dh_callback_args *args = (struct tmp_dh_callback_args *)arg;
-    VALUE cb, dh;
-    EVP_PKEY *pkey;
+    VALUE ctx_obj, cb, obj;
+    const DH *dh;
 
-    cb = rb_funcall(args->ssl_obj, args->id, 0);
+    ctx_obj = rb_attr_get(args->ssl_obj, id_i_context);
+    cb = rb_attr_get(ctx_obj, id_i_tmp_dh_callback);
     if (NIL_P(cb))
-	return (VALUE)NULL;
-    dh = rb_funcall(cb, id_call, 3, args->ssl_obj, INT2NUM(args->is_export),
-		    INT2NUM(args->keylength));
-    pkey = GetPKeyPtr(dh);
-    if (EVP_PKEY_base_id(pkey) != args->type)
-	return (VALUE)NULL;
+        return (VALUE)NULL;
+
+    obj = rb_funcall(cb, id_call, 3, args->ssl_obj, INT2NUM(args->is_export),
+                     INT2NUM(args->keylength));
+    // TODO: We should riase if obj is not DH
+    dh = EVP_PKEY_get0_DH(GetPKeyPtr(obj));
+    if (!dh)
+        ossl_clear_error();
 
-    return (VALUE)pkey;
+    return (VALUE)dh;
 }
-#endif
 
-#if !defined(OPENSSL_NO_DH)
 static DH *
 ossl_tmp_dh_callback(SSL *ssl, int is_export, int keylength)
 {
-    VALUE rb_ssl;
-    EVP_PKEY *pkey;
-    struct tmp_dh_callback_args args;
     int state;
-
-    rb_ssl = (VALUE)SSL_get_ex_data(ssl, ossl_ssl_ex_ptr_idx);
-    args.ssl_obj = rb_ssl;
-    args.id = id_tmp_dh_callback;
-    args.is_export = is_export;
-    args.keylength = keylength;
-    args.type = EVP_PKEY_DH;
-
-    pkey = (EVP_PKEY *)rb_protect(ossl_call_tmp_dh_callback,
-				  (VALUE)&args, &state);
+    VALUE rb_ssl = (VALUE)SSL_get_ex_data(ssl, ossl_ssl_ex_ptr_idx);
+    struct tmp_dh_callback_args args = {rb_ssl, is_export, keylength};
+    VALUE ret = rb_protect(ossl_call_tmp_dh_callback, (VALUE)&args, &state);
     if (state) {
 	rb_ivar_set(rb_ssl, ID_callback_state, INT2NUM(state));
 	return NULL;
     }
-    if (!pkey)
-	return NULL;
-
-    return (DH *)EVP_PKEY_get0_DH(pkey);
+    return (DH *)ret;
 }
 #endif /* OPENSSL_NO_DH */
 
@@ -703,7 +689,10 @@ ossl_sslctx_setup(VALUE self)
     GetSSLCTX(self, ctx);
 
 #if !defined(OPENSSL_NO_DH)
-    SSL_CTX_set_tmp_dh_callback(ctx, ossl_tmp_dh_callback);
+    if (!NIL_P(rb_attr_get(self, id_i_tmp_dh_callback))) {
+        SSL_CTX_set_tmp_dh_callback(ctx, ossl_tmp_dh_callback);
+        SSL_CTX_set_dh_auto(ctx, 0);
+    }
 #endif
 
 #if !defined(OPENSSL_IS_AWSLC) /* AWS-LC has no support for TLS 1.3 PHA. */
@@ -1148,7 +1137,7 @@ ossl_sslctx_set_client_sigalgs(VALUE self, VALUE v)
  * contained in the key object, if any, are ignored. The server will always
  * generate a new key pair for each handshake.
  *
- * Added in version 3.0. See also the man page SSL_set0_tmp_dh_pkey(3).
+ * Added in version 3.0. See also the man page SSL_CTX_set0_tmp_dh_pkey(3).
  *
  * Example:
  *   ctx = OpenSSL::SSL::SSLContext.new
@@ -1169,7 +1158,7 @@ ossl_sslctx_set_tmp_dh(VALUE self, VALUE arg)
     if (EVP_PKEY_base_id(pkey) != EVP_PKEY_DH)
         rb_raise(eSSLError, "invalid pkey type %s (expected DH)",
                  OBJ_nid2sn(EVP_PKEY_base_id(pkey)));
-#ifdef HAVE_SSL_SET0_TMP_DH_PKEY
+#ifdef HAVE_SSL_CTX_SET0_TMP_DH_PKEY
     if (!SSL_CTX_set0_tmp_dh_pkey(ctx, pkey))
         ossl_raise(eSSLError, "SSL_CTX_set0_tmp_dh_pkey");
     EVP_PKEY_up_ref(pkey);
@@ -1178,6 +1167,9 @@ ossl_sslctx_set_tmp_dh(VALUE self, VALUE arg)
         ossl_raise(eSSLError, "SSL_CTX_set_tmp_dh");
 #endif
 
+    // Turn off the "auto" DH parameters set by ossl_sslctx_s_alloc()
+    SSL_CTX_set_dh_auto(ctx, 0);
+
     return arg;
 }
 #endif
@@ -2865,6 +2857,23 @@ Init_ossl_ssl(void)
      */
     rb_attr(cSSLContext, rb_intern_const("client_cert_cb"), 1, 1, Qfalse);
 
+#ifndef OPENSSL_NO_DH
+    /*
+     * A callback invoked when DH parameters are required for ephemeral DH key
+     * exchange.
+     *
+     * The callback is invoked with the SSLSocket, a
+     * flag indicating the use of an export cipher and the keylength
+     * required.
+     *
+     * The callback must return an OpenSSL::PKey::DH instance of the correct
+     * key length.
+     *
+     * <b>Deprecated in version 3.0.</b> Use #tmp_dh= instead.
+     */
+    rb_attr(cSSLContext, rb_intern_const("tmp_dh_callback"), 1, 1, Qfalse);
+#endif
+
     /*
      * Sets the context in which a session can be reused.  This allows
      * sessions for multiple applications to be distinguished, for example, by
@@ -3258,7 +3267,6 @@ Init_ossl_ssl(void)
     sym_wait_readable = ID2SYM(rb_intern_const("wait_readable"));
     sym_wait_writable = ID2SYM(rb_intern_const("wait_writable"));
 
-    id_tmp_dh_callback = rb_intern_const("tmp_dh_callback");
     id_npn_protocols_encoded = rb_intern_const("npn_protocols_encoded");
     id_each = rb_intern_const("each");
 
@@ -3289,6 +3297,7 @@ Init_ossl_ssl(void)
     DefIVarID(servername_cb);
     DefIVarID(verify_hostname);
     DefIVarID(keylog_cb);
+    DefIVarID(tmp_dh_callback);
 
     DefIVarID(io);
     DefIVarID(context);

lib/openssl/ssl.rb

@@ -32,25 +32,6 @@ class SSLContext
         }.call
       }
 
-      if defined?(OpenSSL::PKey::DH)
-        DH_ffdhe2048 = OpenSSL::PKey::DH.new <<-_end_of_pem_
------BEGIN DH PARAMETERS-----
-MIIBCAKCAQEA//////////+t+FRYortKmq/cViAnPTzx2LnFg84tNpWp4TZBFGQz
-+8yTnc4kmz75fS/jY2MMddj2gbICrsRhetPfHtXV/WVhJDP1H18GbtCFY2VVPe0a
-87VXE15/V8k1mE8McODmi3fipona8+/och3xWKE2rec1MKzKT0g6eXq8CrGCsyT7
-YdEIqUuyyOP7uWrat2DX9GgdT0Kj3jlN9K5W7edjcrsZCwenyO4KbXCeAvzhzffi
-7MA0BM0oNC9hkXL+nOmFg/+OTxIy7vKBg8P+OxtMb61zO7X8vC7CIAXFjvGDfRaD
-ssbzSibBsu/6iGtCOGEoXJf//////////wIBAg==
------END DH PARAMETERS-----
-        _end_of_pem_
-        private_constant :DH_ffdhe2048
-
-        DEFAULT_TMP_DH_CALLBACK = lambda { |ctx, is_export, keylen| # :nodoc:
-          warn "using default DH parameters." if $VERBOSE
-          DH_ffdhe2048
-        }
-      end
-
       if !OpenSSL::OPENSSL_VERSION.start_with?("OpenSSL")
         DEFAULT_PARAMS.merge!(
           min_version: OpenSSL::SSL::TLS1_VERSION,
@@ -92,19 +73,6 @@ class SSLContext
       DEFAULT_CERT_STORE = OpenSSL::X509::Store.new # :nodoc:
       DEFAULT_CERT_STORE.set_default_paths
 
-      # A callback invoked when DH parameters are required for ephemeral DH key
-      # exchange.
-      #
-      # The callback is invoked with the SSLSocket, a
-      # flag indicating the use of an export cipher and the keylength
-      # required.
-      #
-      # The callback must return an OpenSSL::PKey::DH instance of the correct
-      # key length.
-      #
-      # <b>Deprecated in version 3.0.</b> Use #tmp_dh= instead.
-      attr_accessor :tmp_dh_callback
-
       # A callback invoked at connect time to distinguish between multiple
       # server names.
       #
@@ -456,10 +424,6 @@ def client_cert_cb
         @context.client_cert_cb
       end
 
-      def tmp_dh_callback
-        @context.tmp_dh_callback || OpenSSL::SSL::SSLContext::DEFAULT_TMP_DH_CALLBACK
-      end
-
       def session_new_cb
         @context.session_new_cb
       end

test/openssl/test_provider.rb

@@ -46,6 +46,7 @@ def test_openssl_legacy_provider
 
     with_openssl(<<-'end;')
       begin
+        OpenSSL::Provider.load("default")
         OpenSSL::Provider.load("legacy")
       rescue OpenSSL::Provider::ProviderError
         omit "Only for OpenSSL with legacy provider"

test/openssl/test_ssl.rb

@@ -2129,11 +2129,7 @@ def test_connect_works_when_setting_dh_callback_to_nil
       ctx.tmp_dh_callback = nil
     }
     start_server(ctx_proc: ctx_proc) do |port|
-      EnvUtil.suppress_warning { # uses default callback
-        assert_nothing_raised {
-          server_connect(port) { }
-        }
-      }
+      assert_nothing_raised { server_connect(port) { } }
     end
   end