diff --git a/lib/net/imap.rb b/lib/net/imap.rb index d7c2014d..ee3bc672 100644 --- a/lib/net/imap.rb +++ b/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 diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb index 3eb726c2..79e6deaf 100644 --- a/test/net/imap/test_imap.rb +++ b/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