diff --git a/Gemfile b/Gemfile index b0c045662..35c02c170 100644 --- a/Gemfile +++ b/Gemfile @@ -5,5 +5,5 @@ source "https://rubygems.org" gemspec gem "rake" -gem "rdoc" +gem "rdoc", "<7.2" # incompatible with ruby 2.6 gem "test-unit" diff --git a/lib/net/imap.rb b/lib/net/imap.rb index b9f0a4629..6b6a08fad 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -1014,8 +1014,11 @@ def logout # unsolicited untagged response immeditely _after_ #starttls completes. # def starttls(options = {}, verify = true) - send_command("STARTTLS") do |resp| + handled = false + error = nil + ok = send_command("STARTTLS") do |resp| if resp.kind_of?(TaggedResponse) && resp.name == "OK" + handled = true begin # for backward compatibility certs = options.to_str @@ -1024,7 +1027,21 @@ def starttls(options = {}, verify = true) end start_tls_session(options) end + rescue Exception => error + raise # note that the error backtrace is in the receiver_thread end + if error + disconnect + raise error + end + unless handled + disconnect + raise InvalidResponseError, + "STARTTLS handler was bypassed, although server responded %p" % [ + ok.raw_data.chomp + ] + end + ok end # :call-seq: @@ -2294,6 +2311,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 @@ -2309,6 +2327,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 diff --git a/lib/net/imap/errors.rb b/lib/net/imap/errors.rb index 52cb936b4..4a222fef2 100644 --- a/lib/net/imap/errors.rb +++ b/lib/net/imap/errors.rb @@ -81,7 +81,27 @@ class BadResponseError < ResponseError class ByeResponseError < ResponseError end + # Error raised when the server sends an invalid response. + # + # This is different from UnknownResponseError: the response has been + # rejected. Although it may be parsable, the server is forbidden from + # sending it in the current context. The client should automatically + # disconnect, abruptly (without logout). + # + # Note that InvalidResponseError does not inherit from ResponseError: it + # can be raised before the response is fully parsed. A related + # ResponseParseError or ResponseError may be the #cause. + class InvalidResponseError < Error + end + # Error raised upon an unknown response from the server. + # + # This is different from InvalidResponseError: the response may be a + # valid extension response and the server may be allowed to send it in + # this context, but Net::IMAP either does not know how to parse it or + # how to handle it. This could result from enabling unknown or + # unhandled extensions. The connection may still be usable, + # but—depending on context—it may be prudent to disconnect. class UnknownResponseError < ResponseError end diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb index 308c83da4..a299c4520 100644 --- a/test/net/imap/test_imap.rb +++ b/test/net/imap/test_imap.rb @@ -75,6 +75,20 @@ def test_imaps_post_connection_check end if defined?(OpenSSL::SSL) + def test_starttls_unknown_ca + imap = nil + ex = nil + starttls_test do |port| + imap = Net::IMAP.new("localhost", port: port) + begin + imap.starttls + rescue => ex + end + imap + end + assert_kind_of(OpenSSL::SSL::SSLError, ex) + end + def test_starttls imap = nil starttls_test do |port| @@ -99,6 +113,40 @@ def test_starttls_stripping imap end 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 imap.disconnected? + end end def start_server @@ -1004,6 +1052,7 @@ def starttls_test sock.gets sock.print("* BYE terminating connection\r\n") sock.print("RUBY0002 OK LOGOUT completed\r\n") + rescue OpenSSL::SSL::SSLError ensure sock.close server.close