🔀 Merge pull request #666 from ruby/backport/v0.4/STARTTLS-stripping

🔒 Fix STARTTLS stripping vulnerability (backports #664)

Author: nevans

lib/net/imap.rb

@@ -1312,9 +1312,11 @@ def logout!
     #
     def starttls(**options)
       @ssl_ctx_params, @ssl_ctx = build_ssl_ctx(options)
+      handled = false
       error = nil
       ok = send_command("STARTTLS") do |resp|
         if resp.kind_of?(TaggedResponse) && resp.name == "OK"
+          handled = true
           clear_cached_capabilities
           clear_responses
           start_tls_session
@@ -1326,6 +1328,13 @@ def starttls(**options)
         disconnect
         raise error
       end
+      unless handled
+        disconnect
+        raise InvalidResponseError,
+              "STARTTLS handler was bypassed, although server responded %p" % [
+                ok.raw_data.chomp
+              ]
+      end
       ok
     end
 
@@ -3052,6 +3061,7 @@ def send_command(cmd, *args, &block)
           put_string(" ")
           send_data(i, tag)
         end
+        guard_against_tagged_response_skipping_handler!(tag)
         put_string(CRLF)
         if cmd == "LOGOUT"
           @logout_command_tag = tag
@@ -3067,6 +3077,17 @@ def send_command(cmd, *args, &block)
           end
         end
       end
+    rescue InvalidResponseError
+      disconnect
+      raise
+    end
+
+    def guard_against_tagged_response_skipping_handler!(tag)
+      return unless (resp = @tagged_responses[tag])&.name&.upcase == "OK"
+      raise(InvalidResponseError,
+            "Server sent tagged 'OK' before command was finished: %p. " \
+            "This could indicate a malicious server or client-side " \
+            "command injection. Disconnecting." % [resp.raw_data.chomp])
     end
 
     def generate_tag

test/net/imap/test_imap.rb

@@ -158,6 +158,43 @@ def test_starttls_stripping
       assert_equal(CA_FILE,                   imap.ssl_ctx.ca_file)
       assert_equal(OpenSSL::SSL::VERIFY_PEER, imap.ssl_ctx.verify_mode)
     end
+
+    def test_starttls_stripping_ok_sent_before_response
+      # to coordinate between threads (better than sleep)
+      server_to_client, client_to_server = Queue.new, Queue.new
+      imap = nil
+      server = create_tcp_server
+      port = server.addr[1]
+      start_server do
+        sock = server.accept
+        begin
+          sock.print("* OK test server\r\n")
+          assert_equal :send_malicious_response, client_to_server.pop
+          sock.print("RUBY0001 OK hahaha, fooled you!\r\n")
+          server_to_client << :malicious_response_sent
+          sock.gets
+        ensure
+          sock.close
+          server.close
+        end
+      end
+      begin
+        imap = Net::IMAP.new("localhost", :port => port)
+        client_to_server << :send_malicious_response
+        assert_equal :malicious_response_sent, server_to_client.pop
+        sleep 0.010 # to be sure the network buffers have flushed, etc
+        assert_raise(Net::IMAP::InvalidResponseError) do
+          imap.starttls(:ca_file => CA_FILE)
+        end
+        assert imap.disconnected?
+      ensure
+        imap.disconnect if imap && !imap.disconnected?
+      end
+      assert_equal false, imap.tls_verified?
+      assert_equal({ca_file: CA_FILE},        imap.ssl_ctx_params)
+      assert_equal(CA_FILE,                   imap.ssl_ctx.ca_file)
+      assert_equal(OpenSSL::SSL::VERIFY_PEER, imap.ssl_ctx.verify_mode)
+    end
   end
 
   def start_server