Merge pull request #2116 from MSP-Greg/ssl-vers

Fix rejecting HTTP requests when TLS1.3 is used by server

History.md

@@ -26,6 +26,7 @@
   * Configuration: `environment` is read from `RAILS_ENV`, if `RACK_ENV` can't be found (#2022)
 
 * Bugfixes
+  * Close client http connections made to an ssl server with TLSv1.3 (#2116)
   * Do not set user_config to quiet by default to allow for file config (#2074)
   * Always close SSL connection in Puma::ControlCLI (#2211)
   * Windows update extconf.rb for use with ssp and varied Ruby/MSYS2 combinations (#2069)

ext/puma_http11/mini_ssl.c

@@ -301,6 +301,7 @@ void raise_error(SSL* ssl, int result) {
   char msg[512];
   const char* err_str;
   int err = errno;
+  int mask = 4095;
   int ssl_err = SSL_get_error(ssl, result);
   int verify_err = (int) SSL_get_verify_result(ssl);
 
@@ -317,8 +318,8 @@ void raise_error(SSL* ssl, int result) {
     } else {
       err = (int) ERR_get_error();
       ERR_error_string_n(err, buf, sizeof(buf));
-      snprintf(msg, sizeof(msg), "OpenSSL error: %s - %d", buf, err);
-
+      int errexp = err & mask;
+      snprintf(msg, sizeof(msg), "OpenSSL error: %s - %d", buf, errexp);
     }
   } else {
     snprintf(msg, sizeof(msg), "Unknown OpenSSL error: %d", ssl_err);
@@ -462,6 +463,13 @@ VALUE engine_peercert(VALUE self) {
   return rb_cert_buf;
 }
 
+static VALUE
+engine_ssl_vers_st(VALUE self) {
+  ms_conn* conn;
+  Data_Get_Struct(self, ms_conn, conn);
+  return rb_ary_new3(2, rb_str_new2(SSL_get_version(conn->ssl)), rb_str_new2(SSL_state_string(conn->ssl)));
+}
+
 VALUE noop(VALUE self) {
   return Qnil;
 }
@@ -533,6 +541,8 @@ void Init_mini_ssl(VALUE puma) {
   rb_define_method(eng, "init?", engine_init, 0);
 
   rb_define_method(eng, "peercert", engine_peercert, 0);
+
+  rb_define_method(eng, "ssl_vers_st", engine_ssl_vers_st, 0);
 }
 
 #else

lib/puma/minissl.rb

@@ -5,8 +5,18 @@
 rescue LoadError
 end
 
+# need for Puma::MiniSSL::OPENSSL constants used in `HAS_TLS1_3`
+require 'puma/puma_http11'
+
 module Puma
   module MiniSSL
+
+    # define constant at runtime, as it's easy to determine at built time,
+    # but Puma could (it shouldn't) be loaded with an older OpenSSL version
+    HAS_TLS1_3 = !IS_JRUBY &&
+      (OPENSSL_VERSION[/ \d+\.\d+\.\d+/].split('.').map(&:to_i) <=> [1,1,1]) != -1 &&
+      (OPENSSL_LIBRARY_VERSION[/ \d+\.\d+\.\d+/].split('.').map(&:to_i) <=> [1,1,1]) !=-1
+
     class Socket
       def initialize(socket, engine)
         @socket = socket
@@ -22,6 +32,24 @@ def closed?
         @socket.closed?
       end
 
+      # returns a two element array
+      # first is protocol version (SSL_get_version)
+      # second is 'handshake' state (SSL_state_string)
+      #
+      # used for dropping tcp connections to ssl
+      # see OpenSSL ssl/ssl_stat.c SSL_state_string for info
+      #
+      def ssl_version_state
+        IS_JRUBY ? [nil, nil] : @engine.ssl_vers_st
+      end
+
+      # used to check the handshake status, in particular when a TCP connection
+      # is made with TLSv1.3 as an available protocol
+      def bad_tlsv1_3?
+        HAS_TLS1_3 && @engine.ssl_vers_st == ['TLSv1.3', 'SSLERR']
+      end
+      private :bad_tlsv1_3?
+
       def readpartial(size)
         while true
           output = @engine.read
@@ -41,6 +69,7 @@ def readpartial(size)
 
       def engine_read_all
         output = @engine.read
+        raise SSLError.exception "HTTP connection?" if bad_tlsv1_3?
         while output and additional_output = @engine.read
           output << additional_output
         end
@@ -167,7 +196,7 @@ def peercert
       end
     end
 
-    if defined?(JRUBY_VERSION)
+    if IS_JRUBY
       class SSLError < StandardError
         # Define this for jruby even though it isn't used.
       end
@@ -184,7 +213,7 @@ def initialize
         @no_tlsv1_1 = false
       end
 
-      if defined?(JRUBY_VERSION)
+      if IS_JRUBY
         # jruby-specific Context properties: java uses a keystore and password pair rather than a cert/key pair
         attr_reader :keystore
         attr_accessor :keystore_pass

test/test_puma_server_ssl.rb

@@ -203,6 +203,44 @@ def test_tls_v1_1_rejection
       assert_match(msg, @events.error.message) if @events.error
     end
   end
+
+  def test_http_rejection
+    body_http  = nil
+    body_https = nil
+
+    start_server
+
+    http = Net::HTTP.new @host, @server.connected_ports[0]
+    http.use_ssl = false
+    http.read_timeout = 6
+
+    tcp = Thread.new do
+      req_http = Net::HTTP::Get.new "/", {}
+      assert_raises(Errno::ECONNREFUSED, EOFError) do
+        http.start.request(req_http) { |rep| body_http = rep.body }
+      end
+    end
+
+    ssl = Thread.new do
+      @http.start do
+        req_https = Net::HTTP::Get.new "/", {}
+        @http.request(req_https) { |rep_https| body_https = rep_https.body }
+      end
+    end
+
+    tcp.join
+    ssl.join
+    http.finish
+    sleep 1.0
+
+    assert_nil body_http
+    assert_equal "https", body_https
+
+    thread_pool = @server.instance_variable_get(:@thread_pool)
+    busy_threads = thread_pool.spawned - thread_pool.waiting
+
+    assert busy_threads.zero?, "Our connection is wasn't dropped"
+  end
 end unless DISABLE_SSL
 
 # client-side TLS authentication tests