🥅 Strictly validate symbol (\flag) arguments

Flags should not allow `atom-specials`.

Previously, no validation was done on symbol data.  Sending atom or flag
args which contain atom specials could lead to various errors.

Although this could theoretically include injection attacks, this is not
considered to be a critical vulnerability in `net-imap`, for the
following reason:  Valid "system flag" inputs are restricted to an
enumerated set of RFC-defined flag types.  User-defined "keyword" flags
are sent as atoms, not flags, which use string inputs (strings which
can't be sent as an atom will be quoted or sent as a literal).  `\Seen`
as a flag (symbol argument) is semantically different from `Seen` as a
keyword (string argument).  So there is no scenario where it is
appropriate to call `#to_sym` on unvetted user input.  Any code which
calls `#to_sym` indiscriminately on user-input is already buggy.

Nevertheless, users should reasonably be able to rely on `net-imap` to
do very basic input validation on its basic input types.

Author: nevans

lib/net/imap/command_data.rb

@@ -27,6 +27,7 @@ def validate_data(data)
         end
       when Time, Date, DateTime
       when Symbol
+        Flag.validate(data)
       else
         data.validate
       end
@@ -47,7 +48,7 @@ def send_data(data, tag = nil)
       when Date
         send_date_data(data)
       when Symbol
-        send_symbol_data(data)
+        Flag[data].send_data(self, tag)
       else
         data.send_data(self, tag)
       end
@@ -138,11 +139,13 @@ 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
 
-    def send_symbol_data(symbol)
-      put_string("\\" + symbol.to_s)
-    end
-
     CommandData = Data.define(:data) do # :nodoc:
+      def self.validate(...)
+        data = new(...)
+        data.validate
+        data
+      end
+
       def send_data(imap, tag)
         raise NoMethodError, "#{self.class} must implement #{__method__}"
       end
@@ -158,8 +161,26 @@ def send_data(imap, tag)
     end
 
     class Atom < CommandData # :nodoc:
+      def initialize(**)
+        super
+        validate
+      end
+
+      def validate
+        data.to_s.ascii_only? \
+          or raise DataFormatError, "#{self.class} must be ASCII only"
+        data.match?(ResponseParser::Patterns::ATOM_SPECIALS) \
+          and raise DataFormatError, "#{self.class} must not contain atom-specials"
+      end
+
       def send_data(imap, tag)
-        imap.__send__(:put_string, data)
+        imap.__send__(:put_string, data.to_s)
+      end
+    end
+
+    class Flag < Atom # :nodoc:
+      def send_data(imap, tag)
+        imap.__send__(:put_string, "\\#{data}")
       end
     end
 

test/net/imap/test_command_data.rb

@@ -6,6 +6,8 @@
 class CommandDataTest < Net::IMAP::TestCase
   DataFormatError = Net::IMAP::DataFormatError
 
+  Atom = Net::IMAP::Atom
+  Flag = Net::IMAP::Flag
   Literal = Net::IMAP::Literal
   Literal8 = Net::IMAP::Literal8
 
@@ -60,6 +62,66 @@ def send_data(*data, tag: TAG)
     @imap = FakeCommandWriter.new
   end
 
+  test "Atom" do
+    imap.send_data(Atom[:INBOX], Atom["INBOX"], Atom["etc"])
+    assert_equal [
+      Output.put_string("INBOX"),
+      Output.put_string("INBOX"),
+      Output.put_string("etc"),
+    ], imap.output
+
+    imap.clear
+    # atom may not contain atom-specials
+    [
+      "with_parens()",
+      "with_list_wildcards*",
+      "with_list_wildcards%",
+      "with_resp_special]",
+      "with\0null",
+      "with\x7fcontrol_char",
+      '"with_quoted_specials"',
+      "with_quoted_specials\\",
+      "with\rCR",
+      "with\nLF",
+    ].each do |symbol|
+      assert_raise_with_message(Net::IMAP::DataFormatError, /\batom\b/i) do
+        imap.send_data Atom[symbol]
+      end
+    end
+    assert_empty imap.output
+  end
+
+  test "Flag" do
+    imap.send_data(Flag[:Seen], Flag[:Flagged],
+                   Flag["Deleted"], Flag["Answered"])
+    assert_equal [
+      Output.put_string("\\Seen"),
+      Output.put_string("\\Flagged"),
+      Output.put_string("\\Deleted"),
+      Output.put_string("\\Answered"),
+    ], imap.output
+
+    imap.clear
+    # symbol may not contain atom-specials
+    [
+      :"with_parens()",
+      :"with_list_wildcards*",
+      :"with_list_wildcards%",
+      :"with_resp_special]",
+      :"with\0null",
+      :"with\x7fcontrol_char",
+      :'"with_quoted_specials"',
+      :"with_quoted_specials\\",
+      :"with\rCR",
+      :"with\nLF",
+    ].each do |symbol|
+      assert_raise_with_message(Net::IMAP::DataFormatError, /\bflag\b/i) do
+        imap.send_data Flag[symbol]
+      end
+    end
+    assert_empty imap.output
+  end
+
   test "Literal" do
     imap.send_data Literal["foo\r\nbar"]
     imap.send_data Literal["foo\r\nbar", false]

test/net/imap/test_imap.rb

@@ -622,6 +622,34 @@ def test_send_invalid_number
     end
   end
 
+  def test_send_symbol_as_flag
+    with_fake_server do |server, imap|
+      server.on "TEST", &:done_ok
+
+      imap.__send__(:send_command, "TEST", :Seen, :Flagged)
+      assert_equal "\\Seen \\Flagged", server.commands.pop.args
+
+      # symbol may not contain atom-specials
+      [
+        :"with_parens()",
+        :"with_list_wildcards*",
+        :"with_list_wildcards%",
+        :"with_resp_special]",
+        :"with\0null",
+        :"with\x7fcontrol_char",
+        :'"with_quoted_specials"',
+        :"with_quoted_specials\\",
+        :"with\rCR",
+        :"with\nLF",
+      ].each do |symbol|
+        assert_raise_with_message(Net::IMAP::DataFormatError, /\bflag\b/i) do
+          imap.__send__(:send_command, "TEST", symbol)
+        end
+        assert_empty server.commands
+      end
+    end
+  end
+
   test("send PartialRange args") do
     with_fake_server do |server, imap|
       server.on "TEST", &:done_ok