Net::IMAP: Command Injection via ID command argument

@@ -3108,10 +3108,11 @@ def enable(*capabilities)
       capabilities = capabilities
         .flatten
         .map {|e| ENABLE_ALIASES[e] || e }
+        .flat_map { _1.is_a?(String) && !_1.empty? ? _1.split(/ /, -1) : [_1] }
         .uniq
-        .join(' ')
+        .map { Atom[_1] }
       synchronize do
-        send_command("ENABLE #{capabilities}")
+        send_command("ENABLE", *capabilities)
         result = clear_responses("ENABLED").last || []
         @utf8_strings ||= result.include? "UTF8=ACCEPT"
         @utf8_strings ||= result.include? "IMAP4REV2"

@@ -27,6 +27,14 @@ def test_enable
       assert_equal %w[CONDSTORE],   result1
       assert_equal %w[UTF8=ACCEPT], result2
       assert_equal [],              result3
+
+      assert_raise(Net::IMAP::DataFormatError) do
+        imap.enable "injection\r\ninjected logout"
+      end
+      assert_empty cmdq
+      assert_raise(Net::IMAP::DataFormatError) do
+        imap.enable "foo", "", "bar"
+      end
     end
   end
 

@@ -150,8 +150,8 @@ def validate
       end
     end
 
-    # Represents IMAP +text+ data, which covers everything in the IMAP grammar,
-    # except for +literal+, +literal8+, and the concluding +CRLF+.
+    # Represents IMAP +text+ or +quoted+ data, which share the same
+    # validations of decoded #data, and differ only in how they are formatted.
     #
     # +data+ may contain any 7-bit ASCII character except +NULL+, +CR+, or +LF+.
     # Any multibyte +UTF-8+ character is also allowed when the connection
@@ -164,7 +164,7 @@ def validate
     #
     # The string's bytes must be valid ASCII or valid UTF-8.  The string's
     # reported encoding is ignored, but the string is _not_ transcoded.
-    class RawText < CommandData # :nodoc:
+    class ValidNonLiteralData < CommandData
       def initialize(data:)
         data = String(data.to_str)
         unless data.encoding in Encoding::ASCII | Encoding::UTF_8
@@ -189,7 +189,17 @@ def validate
 
       def ascii_only? = data.ascii_only?
 
-      def send_data(imap, tag) = imap.__send__(:put_string, data)
+      def send_data(imap, tag = nil) = imap.__send__(:put_string, formatted)
+    end
+
+    # Represents IMAP +text+ data, which covers everything in the IMAP grammar,
+    # except for +literal+, +literal8+, and the concluding +CRLF+.
+    #
+    # NOTE: The current implementation does not verify that the connection
+    # supports UTF-8.  Future versions may validate this.
+    class RawText < ValidNonLiteralData # :nodoc:
+      # raw: no formatting necessary
+      alias formatted data
     end
 
     class RawData < CommandData # :nodoc:
@@ -268,10 +278,13 @@ def send_data(imap, tag)
       end
     end
 
-    class QuotedString < CommandData # :nodoc:
-      def send_data(imap, tag)
-        imap.__send__(:send_quoted_string, data)
-      end
+    # Represents a IMAP +quoted+ string, which can encode any valid ASCII or
+    # UTF-8 string, unless it contains any +CR+, +LF+, or +NULL+ bytes.
+    #
+    # NOTE: The current implementation does not verify that the connection
+    # supports UTF-8.  Future versions may validate this.
+    class QuotedString < ValidNonLiteralData # :nodoc:
+      def formatted = %("#{data.gsub(/["\\]/, "\\\\\\&")}")
     end
 
     class Literal < Data.define(:data, :non_sync) # :nodoc:

@@ -8,6 +8,7 @@ class CommandDataTest < Net::IMAP::TestCase
 
   Atom = Net::IMAP::Atom
   Flag = Net::IMAP::Flag
+  QuotedString = Net::IMAP::QuotedString
   Literal = Net::IMAP::Literal
   Literal8 = Net::IMAP::Literal8
   RawText = Net::IMAP::RawText
@@ -155,6 +156,83 @@ def send_data(*data, tag: TAG)
     ], imap.output
   end
 
+  class QuotedStringTest < CommandDataTest
+    test "quotes ASCII strings (no specials)" do
+      assert_equal '"INBOX"', QuotedString["INBOX"].formatted
+      imap.send_data(
+        QuotedString["INBOX"],
+        QuotedString["etc"]
+      )
+      assert_equal [
+        Output.put_string('"INBOX"'),
+        Output.put_string('"etc"'),
+      ], imap.output
+      imap.clear
+    end
+
+    test "quotes ASCII strings (atom specials)" do
+      [
+        "  with  spaces  in  string  ",
+        "with_parens()",
+        "with_list_wildcards*",
+        "with_list_wildcards%",
+        "with_resp_special]",
+        "with\x7fcontrol_char",
+        %{(){}[]%*'},
+      ].each do |string|
+        imap.send_data QuotedString[string]
+      end
+      assert_equal [
+        Output.put_string('"  with  spaces  in  string  "'),
+        Output.put_string('"with_parens()"'),
+        Output.put_string('"with_list_wildcards*"'),
+        Output.put_string('"with_list_wildcards%"'),
+        Output.put_string('"with_resp_special]"'),
+        Output.put_string(%{"with\x7fcontrol_char"}),
+        Output.put_string(%Q{"(){}[]%*'"}),
+      ], imap.output
+    end
+
+    test "escapes quoted specials" do
+      [
+        '"with" "quoted" "specials"',
+        "\\with\\quoted\\specials\\",
+        %{(){}[]%*"'\\},
+      ].each do |string|
+        imap.send_data QuotedString[string]
+      end
+      assert_equal [
+        Output.put_string('"\"with\" \"quoted\" \"specials\""'),
+        Output.put_string('"\\\\with\\\\quoted\\\\specials\\\\"'),
+        Output.put_string(%q{"(){}[]%*\"'\\\\"}),
+      ], imap.output
+    end
+
+    test "ASCII compatible string with another encodings" do
+      imap.send_data QuotedString.new("foo bar".encode("cp1252"))
+      assert_equal [
+        Output.put_string('"foo bar"'),
+      ], imap.output
+    end
+
+    test "allows ASCII control chars" do
+      text = QuotedString.new("beep\b beep\b escape!\e delete this:\x1f")
+      imap.send_data text
+      assert_equal [
+        Output.put_string(%{"beep\b beep\b escape!\e delete this:\x1f"}),
+      ], imap.output
+    end
+
+    test "quotes valid UTF-8 multibyte chars" do
+      imap.send_data QuotedString.new("föó bär")
+      imap.send_data QuotedString.new("ほげ ふが ぴよ")
+      assert_equal [
+        Output.put_string('"föó bär"'),
+        Output.put_string('"ほげ ふが ぴよ"'),
+      ], imap.output
+    end
+  end
+
   class RawTextTest < CommandDataTest
     test "allows ASCII strings with no specials" do
       imap.send_data(
@@ -229,14 +307,20 @@ class RawTextTest < CommandDataTest
         Output.put_string("ほげ ふが ぴよ"),
       ], imap.output
     end
+  end
 
+  SharedValidNonLiteralDataTests = ->(data_type) do
     data(
       "NULL" => ["with \0 NULL", /NULL\b.+\bbyte/i],
       "CR"   => ["with \r CR",   /CR\b.+\bbyte/i],
       "LF"   => ["with \n LF",   /LF\b.+\bbyte/i],
     )
     test "invalid ASCII byte" do |(text, error_message)|
-      try_multiple_encodings(error_message, text)
+      with_multiple_encodings(text) do |encoded|
+        assert_raise_with_message(DataFormatError, error_message) do
+          data_type[encoded]
+        end
+      end
     end
 
     # See Table 3-7, Well-Formed UTF-8 Byte Sequences, in The Unicode Standard:
@@ -253,7 +337,11 @@ class RawTextTest < CommandDataTest
       "windows-1252"               => "åêïõü".encode("windows-1252"),
     )
     test "invalid UTF-8" do |text|
-      try_multiple_encodings(/invalid UTF-8/i, text)
+      with_multiple_encodings(text) do |encoded|
+        assert_raise_with_message(DataFormatError, /invalid UTF-8/i) do
+          data_type[encoded]
+        end
+      end
     end
 
     def with_multiple_encodings(data)
@@ -262,15 +350,9 @@ def with_multiple_encodings(data)
       yield data.dup.force_encoding("UTF-8")
       yield data.dup.force_encoding("cp1252")
     end
-
-    def try_multiple_encodings(error_message, data)
-      with_multiple_encodings(data) do |encoded|
-        assert_raise_with_message(DataFormatError, error_message) do
-          RawText[encoded]
-        end
-      end
-    end
   end
+  QuotedStringTest.class_exec QuotedString, &SharedValidNonLiteralDataTests
+  RawTextTest     .class_exec RawText,      &SharedValidNonLiteralDataTests
 
   class RawDataTest < CommandDataTest
     test "simple raw text" do

@@ -2209,6 +2209,7 @@ def uid_expunge(uid_set)
     # provided as an array or a string.
     # See {"Argument translation"}[rdoc-ref:#search@Argument+translation]
     # and {"Search criteria"}[rdoc-ref:#search@Search+criteria], below.
+    # <em>Please note</em> the warning for when +criteria+ is a String.
     #
     # +return+ options control what kind of information is returned about
     # messages matching the search +criteria+.  Specifying +return+ should force
@@ -2619,7 +2620,8 @@ def search(...)
     # backward compatibility) but adds SearchResult#modseq when the +CONDSTORE+
     # capability has been enabled.
     #
-    # See #search for documentation of parameters.
+    # See #search for documentation of parameters.  <em>Please note</em> the
+    # warning for when +criteria+ is a String.
     #
     # ==== Capabilities
     #
@@ -2705,7 +2707,8 @@ def fetch(...)
     # {SequenceSet[...]}[rdoc-ref:SequenceSet@Creating+sequence+sets].
     # (For message sequence numbers, use #fetch instead.)
     #
-    # +attr+ behaves the same as with #fetch.
+    # +attr+ behaves the same as with #fetch.  <em>Please note</em> the #fetch
+    # warning on the +attr+ argument.
     # >>>
     #   *Note:* Servers _MUST_ implicitly include the +UID+ message data item as
     #   part of any +FETCH+ response caused by a +UID+ command, regardless of
@@ -2917,8 +2920,10 @@ def uid_move(set, mailbox)
 
     # Sends a {SORT command [RFC5256 §3]}[https://www.rfc-editor.org/rfc/rfc5256#section-3]
     # to search a mailbox for messages that match +search_keys+ and return an
-    # array of message sequence numbers, sorted by +sort_keys+.  +search_keys+
-    # are interpreted the same as for #search.
+    # array of message sequence numbers, sorted by +sort_keys+.
+    #
+    # +search_keys+ are interpreted the same as the +criteria+ argument for
+    # #search.  <em>Please note</em> the #search warning for String +criteria+.
     #
     #--
     # TODO: describe +sort_keys+
@@ -2943,8 +2948,10 @@ def sort(sort_keys, search_keys, charset)
 
     # Sends a {UID SORT command [RFC5256 §3]}[https://www.rfc-editor.org/rfc/rfc5256#section-3]
     # to search a mailbox for messages that match +search_keys+ and return an
-    # array of unique identifiers, sorted by +sort_keys+.  +search_keys+ are
-    # interpreted the same as for #search.
+    # array of unique identifiers, sorted by +sort_keys+.
+    #
+    # +search_keys+ are interpreted the same as the +criteria+ argument for
+    # #search.  <em>Please note</em> the #search warning for String +criteria+.
     #
     # Related: #sort, #search, #uid_search, #thread, #uid_thread
     #
@@ -2958,8 +2965,10 @@ def uid_sort(sort_keys, search_keys, charset)
 
     # Sends a {THREAD command [RFC5256 §3]}[https://www.rfc-editor.org/rfc/rfc5256#section-3]
     # to search a mailbox and return message sequence numbers in threaded
-    # format, as a ThreadMember tree.  +search_keys+ are interpreted the same as
-    # for #search.
+    # format, as a ThreadMember tree.
+    #
+    # +search_keys+ are interpreted the same as the +criteria+ argument for
+    # #search.  <em>Please note</em> the #search warning for String +criteria+.
     #
     # The supported algorithms are:
     #
@@ -2985,6 +2994,9 @@ def thread(algorithm, search_keys, charset)
     # Similar to #thread, but returns unique identifiers instead of
     # message sequence numbers.
     #
+    # +search_keys+ are interpreted the same as the +criteria+ argument for
+    # #search.  <em>Please note</em> the #search warning for String +criteria+.
+    #
     # Related: #thread, #search, #uid_search, #sort, #uid_sort
     #
     # ==== Capabilities

@@ -86,12 +86,19 @@ def send_binary_literal(*a, **kw); send_literal(*a, **kw, binary: true) end
 
     # `non_sync` is an optional tri-state flag:
     # * `true`  -> Force non-synchronizing `LITERAL+`/`LITERAL-` behavior.
-    #   TODO: raise or warn when capabilities don't allow non_sync.
+    #   NOTE: raises DataFormatError when server doesn't support
+    #   non-synchronizing literal, or literal is too large for LITERAL-.
     # * `false` -> Force normal synchronizing literal behavior.
     # * `nil`   -> (default) Currently behaves like `false` (will be dynamic).
     #   TODO: Dynamic, based on capabilities and bytesize.
     def send_literal(str, tag = nil, binary: false, non_sync: nil)
       synchronize do
+        if non_sync && !non_sync_literal_allowed?(str.bytesize)
+          # TODO: check in Printer, so we don't need to close the connection.
+          @sock.close
+          raise DataFormatError, "Connection closed: " \
+            "Cannot send non-synchronizing literal without known server support"
+        end
         prefix = "~" if binary
         plus = "+" if non_sync
         put_string("#{prefix}{#{str.bytesize}#{plus}}\r\n")
@@ -113,6 +120,15 @@ def send_literal(str, tag = nil, binary: false, non_sync: nil)
       end
     end
 
+    def non_sync_literal_allowed?(bytesize)
+      return unless capabilities_cached?
+      return "+" if capable?("LITERAL+")
+      return "-" if capable_literal_minus? && bytesize <= 4096
+      false
+    end
+
+    def capable_literal_minus? = capable?("LITERAL-") || capable?("IMAP4rev2")
+
     # NOTE: +num+ should already be an Integer
     def send_number_data(num)
       put_string(Integer(num).to_s)

@@ -801,7 +801,7 @@ def test_raw_data
   end
 
   test("send literal args") do
-    with_fake_server do |server, imap|
+    with_fake_server(with_extensions: %w[LITERAL-]) do |server, imap|
       server.on "TEST", &:done_ok
       send_args = ->(*args) do
         imap.__send__(:send_command, "TEST", *args)
@@ -850,6 +850,56 @@ def test_raw_data
     end
   end
 
+  test("send non-synchronizing literals with LITERAL+") do
+    with_fake_server(
+      with_extensions: %w[LITERAL+], greeting_capabilities: true,
+    ) do |server, imap|
+      def imap.send_test_args(*args) = send_command("TEST", *args)
+      server.on "TEST", &:done_ok
+
+      # imap.config.max_non_synchronizing_literal = 5_000
+      # NOTE: support for automatic non-synchronizing literals added in v0.6
+      large = "\xff".b * 5_000
+      imap.send_test_args Net::IMAP::Literal[large, nil]
+      assert_equal("{5000}\r\n#{large}".b, server.commands.pop.args)
+
+      large = "\xff".b * 10_000
+      imap.send_test_args Net::IMAP::Literal[large, nil]
+      assert_equal("{10000}\r\n#{large}".b, server.commands.pop.args)
+
+      imap.send_test_args Net::IMAP::Literal[large, true]
+      assert_equal("{10000+}\r\n#{large}".b, server.commands.pop.args)
+    end
+  end
+
+  test("send non-synchronizing literal that's too large for LITERAL-") do
+    with_fake_server(
+      with_extensions: %w[LITERAL-], greeting_capabilities: true,
+      ignore_abrupt_eof: true
+    ) do |server, imap|
+      def imap.send_test_args(*args) = send_command("TEST", *args)
+      server.on "TEST", &:done_ok
+      assert_raise(Net::IMAP::DataFormatError) do
+        imap.send_test_args Net::IMAP::Literal["\xff".b * 5000, true]
+      end
+      assert imap.disconnected?
+    end
+  end
+
+  test("send non-synchronizing literal without known server support") do
+    with_fake_server(
+      with_extensions: %w[LITERAL+], greeting_capabilities: false,
+      ignore_abrupt_eof: true
+    ) do |server, imap|
+      def imap.send_test_args(*args) = send_command("TEST", *args)
+      server.on "TEST", &:done_ok
+      assert_raise(Net::IMAP::DataFormatError) do
+        imap.send_test_args Net::IMAP::Literal["\xff".b * 100, true]
+      end
+      assert imap.disconnected?
+    end
+  end
+
   def test_disconnect
     server = create_tcp_server
     port = server.addr[1]