diff --git a/lib/net/imap.rb b/lib/net/imap.rb index 0d7e4db3..e2e17ae6 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -1440,9 +1440,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 @@ -1454,6 +1456,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 @@ -3165,6 +3174,7 @@ def idle(timeout = nil, &response_handler) synchronize do tag = Thread.current[:net_imap_tag] = generate_tag + guard_against_tagged_response_skipping_handler!(tag, "IDLE") put_string("#{tag} IDLE#{CRLF}") begin @@ -3629,21 +3639,29 @@ def send_command(cmd, *args, &block) put_string(" ") send_data(i, tag) end - put_string(CRLF) - if cmd == "LOGOUT" - @logout_command_tag = tag - end - if block - add_response_handler(&block) - end + @logout_command_tag = tag if cmd == "LOGOUT" + guard_against_tagged_response_skipping_handler!(tag, cmd) + add_response_handler(&block) if block begin - return get_tagged_response(tag, cmd) + put_string(CRLF) + get_tagged_response(tag, cmd) ensure - if block - remove_response_handler(block) - end + remove_response_handler(block) if block end end + rescue InvalidResponseError + disconnect + raise + end + + def guard_against_tagged_response_skipping_handler!(tag, cmd) + return unless (resp = @tagged_responses[tag])&.name&.upcase == "OK" + raise InvalidResponseError, format( + "Received tagged 'OK' to incomplete %s command (tag=%s). " \ + "This could indicate a malicious server, a man-in-the-middle, or " \ + "client-side command injection. Disconnecting.", + cmd, tag + ) end def generate_tag diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb index e82d08cc..a9745fc3 100644 --- a/test/net/imap/test_imap.rb +++ b/test/net/imap/test_imap.rb @@ -138,15 +138,67 @@ def test_starttls end end - def test_starttls_stripping + def test_starttls_stripping_not_ok imap = nil - starttls_stripping_test do |port| + server = create_tcp_server + port = server.addr[1] + start_server do + sock = server.accept + begin + sock.print("* OK test server\r\n") + sock.gets + sock.print("RUBY0001 BUG unhandled command\r\n") + ensure + sock.close + server.close + end + end + begin imap = Net::IMAP.new("localhost", :port => port) assert_raise(Net::IMAP::InvalidResponseError) do imap.starttls(:ca_file => CA_FILE) end assert imap.disconnected? - imap + 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 + + 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) @@ -963,27 +1015,6 @@ def starttls_test end end - def starttls_stripping_test - server = create_tcp_server - port = server.addr[1] - start_server do - sock = server.accept - begin - sock.print("* OK test server\r\n") - sock.gets - sock.print("RUBY0001 BUG unhandled command\r\n") - ensure - sock.close - server.close - end - end - begin - imap = yield(port) - ensure - imap.disconnect if imap && !imap.disconnected? - end - end - def create_tcp_server return TCPServer.new(server_addr, 0) end