SMTP security: prevent command injection via To/From addresses

Validate addresses passed as SMTP command arguments to prevent
injection of other SMTP commands. Disallow line breaks and very
long addresses which may cause overflows on some old SMTP servers.

Ruby 2.4 Net::SMTP already disallows addresses that contain newlines.
Enforce this validation in Mail to cover older Ruby versions and
other SMTP implementations that don't validate input.

SMTP injection whitepaper: http://www.mbsd.jp/Whitepaper/smtpi.pdf
Ruby security report: https://hackerone.com/reports/137631
OSVDB entry: https://rubysec.com/advisories/mail-OSVDB-131677

Closes #1098

CHANGELOG.rdoc

@@ -1,3 +1,8 @@
+== Version 2.6.6 - unreleased
+
+Security:
+* #1097 – SMTP security: prevent command injection via To/From addresses. (jeremy)
+
 == Version 2.6.5 - 2017-04-26 Jeremy Daer <[email protected]>
 
 Features:

lib/mail/check_delivery_params.rb

@@ -1,21 +1,58 @@
 # frozen_string_literal: true
 module Mail
-  module CheckDeliveryParams
-    def check_delivery_params(mail)
-      if Utilities.blank?(mail.smtp_envelope_from)
-        raise ArgumentError.new('An SMTP From address is required to send a message. Set the message smtp_envelope_from, return_path, sender, or from address.')
+  module CheckDeliveryParams #:nodoc:
+    class << self
+      def check(mail)
+        [ check_from(mail.smtp_envelope_from),
+          check_to(mail.smtp_envelope_to),
+          check_message(mail) ]
       end
 
-      if Utilities.blank?(mail.smtp_envelope_to)
-        raise ArgumentError.new('An SMTP To address is required to send a message. Set the message smtp_envelope_to, to, cc, or bcc address.')
+      def check_from(addr)
+        if Utilities.blank?(addr)
+          raise ArgumentError, "SMTP From address may not be blank: #{addr.inspect}"
+        end
+
+        check_addr 'From', addr
+      end
+
+      def check_to(addrs)
+        if Utilities.blank?(addrs)
+          raise ArgumentError, "SMTP To address may not be blank: #{addrs.inspect}"
+        end
+
+        Array(addrs).map do |addr|
+          check_addr 'To', addr
+        end
       end
 
-      message = mail.encoded if mail.respond_to?(:encoded)
-      if Utilities.blank?(message)
-        raise ArgumentError.new('An encoded message is required to send an email')
+      def check_addr(addr_name, addr)
+        validate_smtp_addr addr do |error_message|
+          raise ArgumentError, "SMTP #{addr_name} address #{error_message}: #{addr.inspect}"
+        end
       end
 
-      [mail.smtp_envelope_from, mail.smtp_envelope_to, message]
+      def validate_smtp_addr(addr)
+        if addr.bytesize > 2048
+          yield 'may not exceed 2kB'
+        end
+
+        if /[\r\n]/ =~ addr
+          yield 'may not contain CR or LF line breaks'
+        end
+
+        addr
+      end
+
+      def check_message(message)
+        message = message.encoded if message.respond_to?(:encoded)
+
+        if Utilities.blank?(message)
+          raise ArgumentError, 'An encoded message is required to send an email'
+        end
+
+        message
+      end
     end
   end
 end

lib/mail/network/delivery_methods/file_delivery.rb

@@ -2,7 +2,6 @@
 require 'mail/check_delivery_params'
 
 module Mail
-
   # FileDelivery class delivers emails into multiple files based on the destination
   # address.  Each file is appended to if it already exists.
   # 
@@ -14,22 +13,20 @@ module Mail
   # Make sure the path you specify with :location is writable by the Ruby process
   # running Mail.
   class FileDelivery
-    include Mail::CheckDeliveryParams
-
     if RUBY_VERSION >= '1.9.1'
       require 'fileutils'
     else
       require 'ftools'
     end
 
+    attr_accessor :settings
+
     def initialize(values)
       self.settings = { :location => './mails' }.merge!(values)
     end
-
-    attr_accessor :settings
-
+
     def deliver!(mail)
-      check_delivery_params(mail)
+      Mail::CheckDeliveryParams.check(mail)
 
       if ::File.respond_to?(:makedirs)
         ::File.makedirs settings[:location]
@@ -41,6 +38,5 @@ def deliver!(mail)
         ::File.open(::File.join(settings[:location], File.basename(to.to_s)), 'a') { |f| "#{f.write(mail.encoded)}\r\n\r\n" }
       end
     end
-
   end
 end

lib/mail/network/delivery_methods/sendmail.rb

@@ -38,17 +38,15 @@ module Mail
   #
   #   mail.deliver!
   class Sendmail
-    include Mail::CheckDeliveryParams
+    attr_accessor :settings
 
     def initialize(values)
       self.settings = { :location       => '/usr/sbin/sendmail',
                         :arguments      => '-i' }.merge(values)
     end
 
-    attr_accessor :settings
-
     def deliver!(mail)
-      smtp_from, smtp_to, message = check_delivery_params(mail)
+      smtp_from, smtp_to, message = Mail::CheckDeliveryParams.check(mail)
 
       from = "-f #{self.class.shellquote(smtp_from)}"
       to = smtp_to.map { |_to| self.class.shellquote(_to) }.join(' ')

lib/mail/network/delivery_methods/smtp.rb

@@ -75,7 +75,7 @@ module Mail
   # 
   #   mail.deliver!
   class SMTP
-    include Mail::CheckDeliveryParams
+    attr_accessor :settings
 
     def initialize(values)
       self.settings = { :address              => "localhost",
@@ -91,12 +91,10 @@ def initialize(values)
                       }.merge!(values)
     end
 
-    attr_accessor :settings
-
     # Send the message via SMTP.
     # The from and to attributes are optional. If not set, they are retrieve from the Message.
     def deliver!(mail)
-      smtp_from, smtp_to, message = check_delivery_params(mail)
+      smtp_from, smtp_to, message = Mail::CheckDeliveryParams.check(mail)
 
       smtp = Net::SMTP.new(settings[:address], settings[:port])
       if settings[:tls] || settings[:ssl]
@@ -120,7 +118,6 @@ def deliver!(mail)
         self
       end
     end
-
 
     private
 

lib/mail/network/delivery_methods/smtp_connection.rb

@@ -38,25 +38,21 @@ module Mail
   # 
   #   mail.deliver!
   class SMTPConnection
-    include Mail::CheckDeliveryParams
+    attr_accessor :smtp, :settings
 
     def initialize(values)
       raise ArgumentError.new('A Net::SMTP object is required for this delivery method') if values[:connection].nil?
       self.smtp = values[:connection]
       self.settings = values
     end
 
-    attr_accessor :smtp
-    attr_accessor :settings
-
     # Send the message via SMTP.
     # The from and to attributes are optional. If not set, they are retrieve from the Message.
     def deliver!(mail)
-      smtp_from, smtp_to, message = check_delivery_params(mail)
+      smtp_from, smtp_to, message = Mail::CheckDeliveryParams.check(mail)
       response = smtp.sendmail(message, smtp_from, smtp_to)
 
       settings[:return_response] ? response : self
     end
-
   end
 end

lib/mail/network/delivery_methods/test_mailer.rb

@@ -8,10 +8,8 @@ module Mail
   # It also provides a template of the minimum methods you require to implement
   # if you want to make a custom mailer for Mail
   class TestMailer
-    include Mail::CheckDeliveryParams
-
     # Provides a store of all the emails sent with the TestMailer so you can check them.
-    def TestMailer.deliveries
+    def self.deliveries
       @@deliveries ||= []
     end
 
@@ -26,20 +24,19 @@ def TestMailer.deliveries
     # * length
     # * size
     # * and other common Array methods
-    def TestMailer.deliveries=(val)
+    def self.deliveries=(val)
       @@deliveries = val
     end
 
+    attr_accessor :settings
+
     def initialize(values)
       @settings = values.dup
     end
-
-    attr_accessor :settings
 
     def deliver!(mail)
-      check_delivery_params(mail)
+      Mail::CheckDeliveryParams.check(mail)
       Mail::TestMailer.deliveries << mail
     end
-
   end
 end

spec/mail/network/delivery_methods/exim_spec.rb

@@ -187,7 +187,7 @@
         subject "Email with no sender"
         body "body"
       end
-    end.to raise_error('An SMTP From address is required to send a message. Set the message smtp_envelope_from, return_path, sender, or from address.')
+    end.to raise_error('SMTP From address may not be blank: nil')
   end
 
   it "should raise an error if no recipient if defined" do
@@ -200,6 +200,6 @@
         subject "Email with no recipient"
         body "body"
       end
-    end.to raise_error('An SMTP To address is required to send a message. Set the message smtp_envelope_to, to, cc, or bcc address.')
+    end.to raise_error('SMTP To address may not be blank: []')
   end
 end

spec/mail/network/delivery_methods/file_delivery_spec.rb

@@ -101,7 +101,7 @@
           subject "Email with no sender"
           body "body"
         end
-      end.to raise_error('An SMTP From address is required to send a message. Set the message smtp_envelope_from, return_path, sender, or from address.')
+      end.to raise_error('SMTP From address may not be blank: nil')
     end
 
     it "should raise an error if no recipient if defined" do
@@ -115,7 +115,7 @@
           subject "Email with no recipient"
           body "body"
         end
-      end.to raise_error('An SMTP To address is required to send a message. Set the message smtp_envelope_to, to, cc, or bcc address.')
+      end.to raise_error('SMTP To address may not be blank: []')
     end
 
   end

spec/mail/network/delivery_methods/sendmail_spec.rb

@@ -206,7 +206,7 @@
         subject "Email with no sender"
         body "body"
       end
-    end.to raise_error('An SMTP From address is required to send a message. Set the message smtp_envelope_from, return_path, sender, or from address.')
+    end.to raise_error('SMTP From address may not be blank: nil')
   end
 
   it "should raise an error if no recipient if defined" do
@@ -219,6 +219,6 @@
         subject "Email with no recipient"
         body "body"
       end
-    end.to raise_error('An SMTP To address is required to send a message. Set the message smtp_envelope_to, to, cc, or bcc address.')
+    end.to raise_error('SMTP To address may not be blank: []')
   end
 end

spec/mail/network/delivery_methods/smtp_connection_spec.rb

@@ -60,7 +60,7 @@
         subject "Email with no sender"
         body "body"
       end
-    end.to raise_error('An SMTP From address is required to send a message. Set the message smtp_envelope_from, return_path, sender, or from address.')
+    end.to raise_error('SMTP From address may not be blank: nil')
   end
 
   it "should raise an error if no recipient if defined" do
@@ -75,6 +75,6 @@
         subject "Email with no recipient"
         body "body"
       end
-    end.to raise_error('An SMTP To address is required to send a message. Set the message smtp_envelope_to, to, cc, or bcc address.')
+    end.to raise_error('SMTP To address may not be blank: []')
   end
 end

spec/mail/network/delivery_methods/smtp_spec.rb

@@ -191,7 +191,7 @@ def redefine_verify_none(new_value)
           subject "Email with no sender"
           body "body"
         end
-      end.to raise_error('An SMTP From address is required to send a message. Set the message smtp_envelope_from, return_path, sender, or from address.')
+      end.to raise_error('SMTP From address may not be blank: nil')
     end
 
     it "should raise an error if no recipient if defined" do
@@ -201,8 +201,63 @@ def redefine_verify_none(new_value)
           subject "Email with no recipient"
           body "body"
         end
-      end.to raise_error('An SMTP To address is required to send a message. Set the message smtp_envelope_to, to, cc, or bcc address.')
+      end.to raise_error('SMTP To address may not be blank: []')
     end
-  end
 
+    it "should raise on SMTP injection via MAIL FROM newlines" do
+      addr = "[email protected]>\r\nDATA"
+
+      mail = Mail.new do
+        from addr
+        to "[email protected]"
+      end
+
+      # Mail 2.6.x header unfolding collapses whitespace, avoiding
+      # SMTP injection as a side effect.
+      expect(mail.smtp_envelope_from).to eq addr.gsub(/[\r\n]+/, ' ')
+    end
+
+    it "should raise on SMTP injection via RCPT TO newlines" do
+      addr = "[email protected]>\r\nDATA"
+
+      mail = Mail.new do
+        from "[email protected]"
+        to addr
+      end
+
+      # Mail 2.6.x header unfolding collapses whitespace, avoiding
+      # SMTP injection as a side effect.
+      expect(mail.smtp_envelope_to).to eq [addr.gsub(/[\r\n]+/, ' ')]
+    end
+
+    it "should raise on SMTP injection via MAIL FROM overflow" do
+      addr = "[email protected]#{'m' * 2025}DATA"
+
+      mail = Mail.new do
+        from addr
+        to "[email protected]"
+      end
+
+      expect(mail.smtp_envelope_from).to eq addr
+
+      expect do
+        mail.deliver
+      end.to raise_error(ArgumentError, "SMTP From address may not exceed 2kB: #{addr.inspect}")
+    end
+
+    it "should raise on SMTP injection via RCPT TO overflow" do
+      addr = "[email protected]#{'m' * 2027}DATA"
+
+      mail = Mail.new do
+        from "[email protected]"
+        to addr
+      end
+
+      expect(mail.smtp_envelope_to).to eq [addr]
+
+      expect do
+        mail.deliver
+      end.to raise_error(ArgumentError, "SMTP To address may not exceed 2kB: #{addr.inspect}")
+    end
+  end
 end

spec/mail/network/delivery_methods/test_mailer_spec.rb

@@ -65,7 +65,7 @@
         subject "Email with no sender"
         body "body"
       end
-    end.to raise_error('An SMTP From address is required to send a message. Set the message smtp_envelope_from, return_path, sender, or from address.')
+    end.to raise_error('SMTP From address may not be blank: nil')
   end
 
   it "should raise an error if no recipient if defined" do
@@ -78,7 +78,7 @@
         subject "Email with no recipient"
         body "body"
       end
-    end.to raise_error('An SMTP To address is required to send a message. Set the message smtp_envelope_to, to, cc, or bcc address.')
+    end.to raise_error('SMTP To address may not be blank: []')
   end
 
   it "should save settings passed to initialize" do