diff --git a/lib/net/imap.rb b/lib/net/imap.rb index 8d21f400..9ca3d7fa 100644 --- a/lib/net/imap.rb +++ b/lib/net/imap.rb @@ -3201,7 +3201,8 @@ def idle(timeout = nil, &response_handler) synchronize do tag = Thread.current[:net_imap_tag] = generate_tag - guard_against_tagged_response_skipping_handler!(tag, "IDLE") + command = Command[tag:, name: "IDLE"] + finish_sending_command(command) put_string("#{tag} IDLE#{CRLF}") begin @@ -3661,13 +3662,14 @@ def send_command(cmd, *args, &block) validate_data(i) end tag = generate_tag + command = Command[tag:, name: cmd] put_string(tag + " " + cmd) args.each do |i| put_string(" ") send_data(i, tag) end @logout_command_tag = tag if cmd == "LOGOUT" - guard_against_tagged_response_skipping_handler!(tag, cmd) + finish_sending_command(command) add_response_handler(&block) if block begin put_string(CRLF) @@ -3681,14 +3683,12 @@ def send_command(cmd, *args, &block) 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 - ) + # NOTE: This must be synchronized with sending the command's final CRLF and + # adding any command-related response handlers. + def finish_sending_command(command) + if (response = @tagged_responses[command.tag])&.name&.casecmp?("OK") + raise InvalidTaggedResponseError.new(:incomplete, command:, response:) + end end def generate_tag diff --git a/lib/net/imap/command_data.rb b/lib/net/imap/command_data.rb index 9a5749b5..119851fe 100644 --- a/lib/net/imap/command_data.rb +++ b/lib/net/imap/command_data.rb @@ -141,6 +141,9 @@ def send_list_data(list, tag = nil) def send_date_data(date) put_string Net::IMAP.encode_date(date) end def send_time_data(time) put_string Net::IMAP.encode_time(time) end + # NOTE: Currently for internal use only. The API is expected to change. + Command = Data.define(:tag, :name) # :nodoc: + CommandData = Data.define(:data) do # :nodoc: def self.validate(...) data = new(...) diff --git a/lib/net/imap/errors.rb b/lib/net/imap/errors.rb index 0f904a87..a042604e 100644 --- a/lib/net/imap/errors.rb +++ b/lib/net/imap/errors.rb @@ -279,7 +279,11 @@ class ByeResponseError < ResponseError # # 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 + # sending it in the current context. + # + # This could be caused by a bug in the server or in Net::IMAP. Or it + # might indicate a malicious server, a man-in-the-middle attack, or + # client-side command injection. So the client should automatically # disconnect, abruptly (without logout). # # Note that InvalidResponseError does not inherit from ResponseError: it @@ -288,6 +292,67 @@ class ByeResponseError < ResponseError class InvalidResponseError < Error end + # Error raised when the server sends a tagged #response that is invalid for + # the #command #state. + # + # This could be caused by a bug in the server or in Net::IMAP. Or it + # might indicate a malicious server, a man-in-the-middle attack, or + # client-side command injection, so the client should disconnect + # automatically and abruptly (without logout). + class InvalidTaggedResponseError < InvalidResponseError + # A symbol representing the state of the matching tagged command. + # + # +:unknown+:: + # #response does not match any known command (#command will be +nil+). + # +:unstarted+:: + # Any tagged #response is invalid before #command starts sending. + # +:incomplete+:: + # A tagged +OK+ #response is invalid before #command is fully sent. + # +:completed+:: + # Multiple tagged responses were received for the same command. + # + # NOTE: Command state is neither anticipated nor remembered indefinitely. + # +:unknown+ is used before the matching command is called and after the + # it is forgotten. + # + # NOTE: This version of Net::IMAP does not detect or raise an exception + # for all of these states. + attr_reader :state + + # Metadata about the matching IMAP command. + # + # Returns +nil+ when #state is +:unknown+. + # + # NOTE: The non-nil return type is an unstable API, for debug only. It + # may be changed by any release, without warning or deprecation. + attr_reader :command + + # The TaggedResponse which triggered this error + attr_reader :response + + def initialize(state, response:, command: nil) + response => TaggedResponse[tag:, name: status] + case [state, status, command] + in :unknown, _, nil + in :incomplete, "OK", {tag: ^tag, name:} + in :unstarted | :completed, _, {tag: ^tag, name:} + end + @state, @command, @response = state, command, response + cmd_desc = name ? "#{state} #{name}" : state + super "Received tagged #{status} to #{cmd_desc} command (tag=#{tag})" + rescue NoMatchingPatternError => err + raise ArgumentError, err.message + end + + def detailed_message(**) + "#{message}.\n" \ + "Disconnecting: This could indicate a malicious server, a " \ + "man-in-the-middle attack, a client-side command injection, or " \ + "a bug in net-imap.\n" \ + "response.data=#{response.data.inspect}" + end + end + # Error raised upon an unknown response from the server. # # This is different from InvalidResponseError: the response may be a diff --git a/test/net/imap/test_errors.rb b/test/net/imap/test_errors.rb index 29a179c8..6a07a7fb 100644 --- a/test/net/imap/test_errors.rb +++ b/test/net/imap/test_errors.rb @@ -219,4 +219,50 @@ def self.SGR(*attr) = CSI attr.join(?;), ?m "exceeds max_response_size (1200B)", err.message) end + test "InvalidTaggedResponseError" do + assert_raise(ArgumentError) do Net::IMAP::InvalidTaggedResponseError.new end + assert_raise(ArgumentError) do + Net::IMAP::InvalidTaggedResponseError.new("foo", response: nil) + end + assert_raise(ArgumentError) do + Net::IMAP::InvalidTaggedResponseError.new(:unknown, response: nil) + end + + response = Net::IMAP::TaggedResponse[ + "RUBY0001", "NO", Net::IMAP::ResponseText[nil, "cheating"] + ] + assert_raise(ArgumentError) do + Net::IMAP::InvalidTaggedResponseError.new(:unstarted, response:) + end + + err = Net::IMAP::InvalidTaggedResponseError.new(:unknown, response:) + assert_match(/tagged NO to unknown command.*tag=RUBY0001\b/i, err.message) + assert_match(/\bdisconnecting\b/i, err.detailed_message) + assert_match(/response.data=#/i, + err.detailed_message) + + command = Net::IMAP::Command[tag: "mismatch", name: "NOOP"] + assert_raise(ArgumentError) do + Net::IMAP::InvalidTaggedResponseError.new(:unstarted, response:, command:) + end + + command = Net::IMAP::Command[tag: "RUBY0001", name: "NOOP"] + err = Net::IMAP::InvalidTaggedResponseError.new(:unstarted, response:, command:) + assert_match(/tagged NO to unstarted NOOP command.*tag=RUBY0001\b/i, + err.message) + assert_match(/\bdisconnecting\b/i, err.detailed_message) + assert_match(/response.data=#/i, + err.detailed_message) + + response = Net::IMAP::TaggedResponse[ + "RUBY0001", "OK", Net::IMAP::ResponseText[code: nil, text: "cheating"] + ] + command = Net::IMAP::Command[tag: "RUBY0001", name: "STARTTLS"] + err = Net::IMAP::InvalidTaggedResponseError.new(:incomplete, response:, command:) + assert_match(/tagged OK to incomplete STARTTLS command.*tag=RUBY0001\b/i, + err.message) + assert_match(/\bdisconnecting\b/i, err.detailed_message) + assert_match(/response.data=#/i, + err.detailed_message) + end end diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb index f43bdcc7..3c232984 100644 --- a/test/net/imap/test_imap.rb +++ b/test/net/imap/test_imap.rb @@ -195,7 +195,7 @@ def test_starttls_stripping_ok_sent_before_response 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 + assert_raise(Net::IMAP::InvalidTaggedResponseError) do imap.starttls(:ca_file => CA_FILE) end assert imap.disconnected?