Check for unsafe characters in email addresses before sending

Merge branch 'security-check-unsafe-characters-in-email-addresses-14-6' into '14-6-stable-ee'

See merge request gitlab-org/security/gitlab!2208

Changelog: security

config/initializers/action_mailer_hooks.rb

@@ -8,6 +8,7 @@ end
 ActionMailer::Base.register_interceptors(
   ::Gitlab::Email::Hook::AdditionalHeadersInterceptor,
   ::Gitlab::Email::Hook::EmailTemplateInterceptor,
+  ::Gitlab::Email::Hook::ValidateAddressesInterceptor,
   ::Gitlab::Email::Hook::DeliveryMetricsObserver
 )

lib/gitlab/email/hook/validate_addresses_interceptor.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Email
+    module Hook
+      # Check for unsafe characters in the envelope-from and -to addresses.
+      # These are passed directly as arguments to sendmail and are liable to shell injection attacks:
+      # https://github.com/mikel/mail/blob/2.7.1/lib/mail/network/delivery_methods/sendmail.rb#L53-L58
+      class ValidateAddressesInterceptor
+        UNSAFE_CHARACTERS = /(\\|[^[:print:]])/.freeze
+
+        def self.delivering_email(message)
+          addresses = Array(message.smtp_envelope_from) + Array(message.smtp_envelope_to)
+
+          addresses.each do |address|
+            next unless address.match?(UNSAFE_CHARACTERS)
+
+            Gitlab::AuthLogger.info(
+              message: 'Skipping email with unsafe characters in address',
+              address: address,
+              subject: message.subject
+            )
+
+            message.perform_deliveries = false
+
+            break
+          end
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/email/hook/validate_addresses_interceptor_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Email::Hook::ValidateAddressesInterceptor do
+  describe 'UNSAFE_CHARACTERS' do
+    subject { described_class::UNSAFE_CHARACTERS }
+
+    it { is_expected.to match('\\') }
+    it { is_expected.to match("\x00") }
+    it { is_expected.to match("\x01") }
+    it { is_expected.not_to match('') }
+    it { is_expected.not_to match('user@example.com') }
+    it { is_expected.not_to match('foo-123+bar_456@example.com') }
+  end
+
+  describe '.delivering_email' do
+    let(:mail) do
+      ActionMailer::Base.mail(to: 'test@mail.com', from: 'info@mail.com', subject: 'title', body: 'hello')
+    end
+
+    let(:unsafe_email) { "evil+\x01$HOME@example.com" }
+
+    it 'sends emails to normal addresses' do
+      expect(Gitlab::AuthLogger).not_to receive(:info)
+      expect { mail.deliver_now }.to change(ActionMailer::Base.deliveries, :count)
+    end
+
+    [:from, :to, :cc, :bcc].each do |header|
+      it "does not send emails if the #{header.inspect} header contains unsafe characters" do
+        mail[header] = unsafe_email
+
+        expect(Gitlab::AuthLogger).to receive(:info).with(
+          message: 'Skipping email with unsafe characters in address',
+          address: unsafe_email,
+          subject: mail.subject
+        )
+
+        expect { mail.deliver_now }.not_to change(ActionMailer::Base.deliveries, :count)
+      end
+    end
+
+    [:reply_to].each do |header|
+      it "sends emails if the #{header.inspect} header contains unsafe characters" do
+        mail[header] = unsafe_email
+
+        expect(Gitlab::AuthLogger).not_to receive(:info)
+        expect { mail.deliver_now }.to change(ActionMailer::Base.deliveries, :count)
+      end
+    end
+  end
+end