Merge branch 'security-dns-rebind-ssrf-in-slack-notifications-ce' into 'master'

Use Gitlab::HTTP for all chat notifications

See merge request gitlab/gitlabhq!3517

app/models/project_services/chat_notification_service.rb

@@ -113,12 +113,9 @@ class ChatNotificationService < Service
 
   private
 
+  # every notifier must implement this independently
   def notify(message, opts)
-    Slack::Notifier.new(webhook, opts).ping(
-      message.pretext,
-      attachments: message.attachments,
-      fallback: message.fallback
-    )
+    raise NotImplementedError
   end
 
   def custom_data(data)

app/models/project_services/mattermost_service.rb

 # frozen_string_literal: true
 
 class MattermostService < ChatNotificationService
+  include ::SlackService::Notifier
+
   def title
     'Mattermost notifications'
   end

app/models/project_services/slack_service.rb

@@ -30,4 +30,28 @@ class SlackService < ChatNotificationService
   def webhook_placeholder
     'https://hooks.slack.com/services/…'
   end
+
+  module Notifier
+    private
+
+    def notify(message, opts)
+      # See https://github.com/stevenosloan/slack-notifier#custom-http-client
+      notifier = Slack::Notifier.new(webhook, opts.merge(http_client: HTTPClient))
+
+      notifier.ping(
+        message.pretext,
+        attachments: message.attachments,
+        fallback: message.fallback
+      )
+    end
+
+    class HTTPClient
+      def self.post(uri, params = {})
+        params.delete(:http_options) # these are internal to the client and we do not want them
+        Gitlab::HTTP.post(uri, body: params)
+      end
+    end
+  end
+
+  include Notifier
 end

changelogs/unreleased/security-dns-rebind-ssrf-in-slack-notifications.yml

+---
+title: Limit potential for DNS rebind SSRF in chat notifications
+merge_request:
+author:
+type: security

config/initializers/hangouts_chat_http_override.rb

+# frozen_string_literal: true
+
+module HangoutsChat
+  class Sender
+    class HTTP
+      module GitlabHTTPOverride
+        extend ::Gitlab::Utils::Override
+
+        attr_reader :uri
+
+        # see https://github.com/enzinia/hangouts-chat/blob/6a509f61a56e757f8f417578b393b94423831ff7/lib/hangouts_chat/http.rb
+        override :post
+        def post(payload)
+          httparty_response = Gitlab::HTTP.post(
+            uri,
+            body: payload.to_json,
+            headers: { 'Content-Type' => 'application/json' },
+            parse: nil # disables automatic response parsing
+          )
+          net_http_response = httparty_response.response
+          # The rest of the integration expects a Net::HTTP response
+          net_http_response
+        end
+      end
+
+      prepend GitlabHTTPOverride
+    end
+  end
+end

lib/microsoft_teams/notifier.rb

@@ -14,7 +14,6 @@ module MicrosoftTeams
         response = Gitlab::HTTP.post(
           @webhook.to_str,
           headers: @header,
-          allow_local_requests: true,
           body: body(options)
         )
 

spec/initializers/hangouts_chat_http_override_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'HangoutsChat::Sender Gitlab::HTTP override' do
+  describe 'HangoutsChat::Sender::HTTP#post' do
+    it 'calls Gitlab::HTTP.post with default protection settings' do
+      webhook_url = 'https://example.gitlab.com'
+      payload = { key: 'value' }
+      http = HangoutsChat::Sender::HTTP.new(webhook_url)
+      mock_response = double(response: 'the response')
+
+      expect(Gitlab::HTTP).to receive(:post)
+        .with(
+          URI.parse(webhook_url),
+          body: payload.to_json,
+          headers: { 'Content-Type' => 'application/json' },
+          parse: nil
+        )
+        .and_return(mock_response)
+
+      expect(http.post(payload)).to eq(mock_response.response)
+    end
+
+    it_behaves_like 'a request using Gitlab::UrlBlocker' do
+      let(:http_method) { :post }
+      let(:url_blocked_error_class) { Gitlab::HTTP::BlockedUrlError }
+
+      def make_request(uri)
+        HangoutsChat::Sender::HTTP.new(uri).post({})
+      end
+    end
+  end
+end

spec/initializers/rest-client-hostname_override_spec.rb

@@ -3,147 +3,12 @@
 require 'spec_helper'
 
 describe 'rest-client dns rebinding protection' do
-  include StubRequests
+  it_behaves_like 'a request using Gitlab::UrlBlocker' do
+    let(:http_method) { :get }
+    let(:url_blocked_error_class) { ArgumentError }
 
-  context 'when local requests are not allowed' do
-    it 'allows an external request with http' do
-      request_stub = stub_full_request('http://example.com', ip_address: '93.184.216.34')
-
-      RestClient.get('http://example.com/')
-
-      expect(request_stub).to have_been_requested
-    end
-
-    it 'allows an external request with https' do
-      request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34')
-
-      RestClient.get('https://example.com/')
-
-      expect(request_stub).to have_been_requested
-    end
-
-    it 'raises error when it is a request that resolves to a local address' do
-      stub_full_request('https://example.com', ip_address: '172.16.0.0')
-
-      expect { RestClient.get('https://example.com') }
-        .to raise_error(ArgumentError,
-                        "URL 'https://example.com' is blocked: Requests to the local network are not allowed")
-    end
-
-    it 'raises error when it is a request that resolves to a localhost address' do
-      stub_full_request('https://example.com', ip_address: '127.0.0.1')
-
-      expect { RestClient.get('https://example.com') }
-        .to raise_error(ArgumentError,
-                        "URL 'https://example.com' is blocked: Requests to localhost are not allowed")
-    end
-
-    it 'raises error when it is a request to local address' do
-      expect { RestClient.get('http://172.16.0.0') }
-        .to raise_error(ArgumentError,
-                        "URL 'http://172.16.0.0' is blocked: Requests to the local network are not allowed")
-    end
-
-    it 'raises error when it is a request to localhost address' do
-      expect { RestClient.get('http://127.0.0.1') }
-        .to raise_error(ArgumentError,
-                        "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed")
-    end
-  end
-
-  context 'when port different from URL scheme is used' do
-    it 'allows the request' do
-      request_stub = stub_full_request('https://example.com:8080', ip_address: '93.184.216.34')
-
-      RestClient.get('https://example.com:8080/')
-
-      expect(request_stub).to have_been_requested
-    end
-
-    it 'raises error when it is a request to local address' do
-      expect { RestClient.get('https://172.16.0.0:8080') }
-        .to raise_error(ArgumentError,
-                        "URL 'https://172.16.0.0:8080' is blocked: Requests to the local network are not allowed")
-    end
-
-    it 'raises error when it is a request to localhost address' do
-      expect { RestClient.get('https://127.0.0.1:8080') }
-        .to raise_error(ArgumentError,
-                        "URL 'https://127.0.0.1:8080' is blocked: Requests to localhost are not allowed")
-    end
-  end
-
-  context 'when DNS rebinding protection is disabled' do
-    before do
-      stub_application_setting(dns_rebinding_protection_enabled: false)
-    end
-
-    it 'allows the request' do
-      request_stub = stub_request(:get, 'https://example.com')
-
-      RestClient.get('https://example.com/')
-
-      expect(request_stub).to have_been_requested
-    end
-  end
-
-  context 'when http(s) proxy environment variable is set' do
-    before do
-      stub_env('https_proxy' => 'https://my.proxy')
-    end
-
-    it 'allows the request' do
-      request_stub = stub_request(:get, 'https://example.com')
-
-      RestClient.get('https://example.com/')
-
-      expect(request_stub).to have_been_requested
-    end
-  end
-
-  context 'when local requests are allowed' do
-    before do
-      stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
-    end
-
-    it 'allows an external request' do
-      request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34')
-
-      RestClient.get('https://example.com/')
-
-      expect(request_stub).to have_been_requested
-    end
-
-    it 'allows an external request that resolves to a local address' do
-      request_stub = stub_full_request('https://example.com', ip_address: '172.16.0.0')
-
-      RestClient.get('https://example.com/')
-
-      expect(request_stub).to have_been_requested
-    end
-
-    it 'allows an external request that resolves to a localhost address' do
-      request_stub = stub_full_request('https://example.com', ip_address: '127.0.0.1')
-
-      RestClient.get('https://example.com/')
-
-      expect(request_stub).to have_been_requested
-    end
-
-    it 'allows a local address request' do
-      request_stub = stub_request(:get, 'http://172.16.0.0')
-
-      RestClient.get('http://172.16.0.0')
-
-      expect(request_stub).to have_been_requested
-    end
-
-    it 'allows a localhost address request' do
-      request_stub = stub_request(:get, 'http://127.0.0.1')
-
-      RestClient.get('http://127.0.0.1')
-
-      expect(request_stub).to have_been_requested
+    def make_request(uri)
+      RestClient.get(uri)
     end
   end
 end

spec/models/project_services/chat_notification_service_spec.rb

@@ -30,7 +30,8 @@ describe ChatNotificationService do
   end
 
   describe '#execute' do
-    let(:chat_service) { described_class.new }
+    subject(:chat_service) { described_class.new }
+
     let(:user) { create(:user) }
     let(:project) { create(:project, :repository) }
     let(:webhook_url) { 'https://example.gitlab.com/' }
@@ -53,12 +54,7 @@ describe ChatNotificationService do
         subject.project = project
         data = Gitlab::DataBuilder::Push.build_sample(project, user)
 
-        expect(Slack::Notifier).to receive(:new)
-         .with(webhook_url, {})
-         .and_return(
-           double(:slack_service).as_null_object
-         )
-
+        expect(chat_service).to receive(:notify).and_return(true)
         expect(chat_service.execute(data)).to be true
       end
     end
@@ -68,12 +64,7 @@ describe ChatNotificationService do
         subject.project = create(:project, :empty_repo)
         data = Gitlab::DataBuilder::Push.build_sample(subject.project, user)
 
-        expect(Slack::Notifier).to receive(:new)
-         .with(webhook_url, {})
-         .and_return(
-           double(:slack_service).as_null_object
-         )
-
+        expect(chat_service).to receive(:notify).and_return(true)
         expect(chat_service.execute(data)).to be true
       end
     end

spec/models/project_services/slack_service_spec.rb

@@ -3,5 +3,5 @@
 require 'spec_helper'
 
 describe SlackService do
-  it_behaves_like "slack or mattermost notifications", "Slack"
+  it_behaves_like "slack or mattermost notifications", 'Slack'
 end

spec/support/shared_examples/slack_mattermost_notifications_shared_examples.rb

 # frozen_string_literal: true
 
 RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
+  include StubRequests
+
   let(:chat_service) { described_class.new }
-  let(:webhook_url) { 'https://example.gitlab.com/' }
+  let(:webhook_url) { 'https://example.gitlab.com' }
 
   def execute_with_options(options)
-    receive(:new).with(webhook_url, options)
+    receive(:new).with(webhook_url, options.merge(http_client: SlackService::Notifier::HTTPClient))
      .and_return(double(:slack_service).as_null_object)
   end
 
@@ -38,9 +40,13 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
       chat_service.branches_to_be_notified = branches_to_be_notified if branches_to_be_notified
     end
 
+    let!(:stubbed_resolved_hostname) do
+      stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s
+    end
+
     it "notifies about #{event_type} events" do
       chat_service.execute(data)
-      expect(WebMock).to have_requested(:post, webhook_url)
+      expect(WebMock).to have_requested(:post, stubbed_resolved_hostname)
     end
   end
 
@@ -49,9 +55,13 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
       chat_service.branches_to_be_notified = branches_to_be_notified if branches_to_be_notified
     end
 
+    let!(:stubbed_resolved_hostname) do
+      stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s
+    end
+
     it "notifies about #{event_type} events" do
       chat_service.execute(data)
-      expect(WebMock).not_to have_requested(:post, webhook_url)
+      expect(WebMock).not_to have_requested(:post, stubbed_resolved_hostname)
     end
   end
 
@@ -66,6 +76,10 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
       Gitlab::DataBuilder::Push.build_sample(project, user)
     end
 
+    let!(:stubbed_resolved_hostname) do
+      stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s
+    end
+
     before do
       allow(chat_service).to receive_messages(
         project: project,
@@ -74,8 +88,6 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
         webhook: webhook_url
       )
 
-      WebMock.stub_request(:post, webhook_url)
-
       issue_service = Issues::CreateService.new(project, user, issue_service_options)
       @issue = issue_service.execute
       @issues_sample_data = issue_service.hook_data(@issue, 'open')
@@ -107,25 +119,25 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
     it "calls #{service_name} API for push events" do
       chat_service.execute(data)
 
-      expect(WebMock).to have_requested(:post, webhook_url).once
+      expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once
     end
 
     it "calls #{service_name} API for issue events" do
       chat_service.execute(@issues_sample_data)
 
-      expect(WebMock).to have_requested(:post, webhook_url).once
+      expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once
     end
 
     it "calls #{service_name} API for merge requests events" do
       chat_service.execute(@merge_sample_data)
 
-      expect(WebMock).to have_requested(:post, webhook_url).once
+      expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once
     end
 
     it "calls #{service_name} API for wiki page events" do
       chat_service.execute(@wiki_page_sample_data)
 
-      expect(WebMock).to have_requested(:post, webhook_url).once
+      expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once
     end
 
     it "calls #{service_name} API for deployment events" do
@@ -133,14 +145,14 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
 
       chat_service.execute(deployment_event_data)
 
-      expect(WebMock).to have_requested(:post, webhook_url).once
+      expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once
     end
 
     it 'uses the username as an option for slack when configured' do
       allow(chat_service).to receive(:username).and_return(username)
 
       expect(Slack::Notifier).to receive(:new)
-       .with(webhook_url, username: username)
+       .with(webhook_url, username: username, http_client: SlackService::Notifier::HTTPClient)
        .and_return(
          double(:slack_service).as_null_object
        )
@@ -151,7 +163,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
     it 'uses the channel as an option when it is configured' do
       allow(chat_service).to receive(:channel).and_return(channel)
       expect(Slack::Notifier).to receive(:new)
-        .with(webhook_url, channel: channel)
+        .with(webhook_url, channel: channel, http_client: SlackService::Notifier::HTTPClient)
         .and_return(
           double(:slack_service).as_null_object
         )
@@ -163,7 +175,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
         chat_service.update(push_channel: "random")
 
         expect(Slack::Notifier).to receive(:new)
-         .with(webhook_url, channel: "random")
+         .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient)
          .and_return(
            double(:slack_service).as_null_object
          )
@@ -175,7 +187,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
         chat_service.update(merge_request_channel: "random")
 
         expect(Slack::Notifier).to receive(:new)
-         .with(webhook_url, channel: "random")
+         .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient)
          .and_return(
            double(:slack_service).as_null_object
          )
@@ -187,7 +199,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
         chat_service.update(issue_channel: "random")
 
         expect(Slack::Notifier).to receive(:new)
-         .with(webhook_url, channel: "random")
+         .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient)
          .and_return(
            double(:slack_service).as_null_object
          )
@@ -219,7 +231,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
         chat_service.update(wiki_page_channel: "random")
 
         expect(Slack::Notifier).to receive(:new)
-         .with(webhook_url, channel: "random")
+         .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient)
          .and_return(
            double(:slack_service).as_null_object
          )
@@ -238,7 +250,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
           note_data = Gitlab::DataBuilder::Note.build(issue_note, user)
 
           expect(Slack::Notifier).to receive(:new)
-           .with(webhook_url, channel: "random")
+           .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient)
            .and_return(
              double(:slack_service).as_null_object
            )
@@ -286,7 +298,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
         webhook: webhook_url
       )
 
-      WebMock.stub_request(:post, webhook_url)
+      stub_full_request(webhook_url, method: :post)
     end
 
     context 'on default branch' do
@@ -461,7 +473,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
         webhook: webhook_url
       )
 
-      WebMock.stub_request(:post, webhook_url)
+      stub_full_request(webhook_url, method: :post)
     end
 
     context 'when commit comment event executed' do
@@ -535,7 +547,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
         webhook: webhook_url
       )
 
-      WebMock.stub_request(:post, webhook_url)
+      stub_full_request(webhook_url, method: :post)
     end
 
     context 'with succeeded pipeline' do

spec/support/shared_examples/uses_gitlab_url_blocker_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'a request using Gitlab::UrlBlocker' do
+  # Written to test internal patches against 3rd party libraries
+  #
+  # Expects the following to be available in the example contexts:
+  #
+  #   make_request(uri): Wraps the request we want to test goes through Gitlab::HTTP
+  #   http_method: :get, :post etc
+  #   url_blocked_error_class: Probably just Gitlab::HTTP::BlockedUrlError
+
+  include StubRequests
+
+  context 'when local requests are not allowed' do
+    it 'allows an external request with http' do
+      request_stub = stub_full_request('http://example.com', method: http_method, ip_address: '93.184.216.34')
+
+      make_request('http://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'allows an external request with https' do
+      request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '93.184.216.34')
+
+      make_request('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'raises error when it is a request that resolves to a local address' do
+      stub_full_request('https://example.com', method: http_method, ip_address: '172.16.0.0')
+
+      expect { make_request('https://example.com') }
+        .to raise_error(url_blocked_error_class,
+                        "URL 'https://example.com' is blocked: Requests to the local network are not allowed")
+    end
+
+    it 'raises error when it is a request that resolves to a localhost address' do
+      stub_full_request('https://example.com', method: http_method, ip_address: '127.0.0.1')
+
+      expect { make_request('https://example.com') }
+        .to raise_error(url_blocked_error_class,
+                        "URL 'https://example.com' is blocked: Requests to localhost are not allowed")
+    end
+
+    it 'raises error when it is a request to local address' do
+      expect { make_request('http://172.16.0.0') }
+        .to raise_error(url_blocked_error_class,
+                        "URL 'http://172.16.0.0' is blocked: Requests to the local network are not allowed")
+    end
+
+    it 'raises error when it is a request to localhost address' do
+      expect { make_request('http://127.0.0.1') }
+        .to raise_error(url_blocked_error_class,
+                        "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed")
+    end
+  end
+
+  context 'when port different from URL scheme is used' do
+    it 'allows the request' do
+      request_stub = stub_full_request('https://example.com:8080', method: http_method, ip_address: '93.184.216.34')
+
+      make_request('https://example.com:8080/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'raises error when it is a request to local address' do
+      expect { make_request('https://172.16.0.0:8080') }
+        .to raise_error(url_blocked_error_class,
+                        "URL 'https://172.16.0.0:8080' is blocked: Requests to the local network are not allowed")
+    end
+
+    it 'raises error when it is a request to localhost address' do
+      expect { make_request('https://127.0.0.1:8080') }
+        .to raise_error(url_blocked_error_class,
+                        "URL 'https://127.0.0.1:8080' is blocked: Requests to localhost are not allowed")
+    end
+  end
+
+  context 'when DNS rebinding protection is disabled' do
+    before do
+      stub_application_setting(dns_rebinding_protection_enabled: false)
+    end
+
+    it 'allows the request' do
+      request_stub = stub_request(http_method, 'https://example.com')
+
+      make_request('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+  end
+
+  context 'when http(s) proxy environment variable is set' do
+    before do
+      stub_env('https_proxy' => 'https://my.proxy')
+    end
+
+    it 'allows the request' do
+      request_stub = stub_request(http_method, 'https://example.com')
+
+      make_request('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+  end
+
+  context 'when local requests are allowed' do
+    before do
+      stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
+    end
+
+    it 'allows an external request' do
+      request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '93.184.216.34')
+
+      make_request('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'allows an external request that resolves to a local address' do
+      request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '172.16.0.0')
+
+      make_request('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'allows an external request that resolves to a localhost address' do
+      request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '127.0.0.1')
+
+      make_request('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'allows a local address request' do
+      request_stub = stub_request(http_method, 'http://172.16.0.0')
+
+      make_request('http://172.16.0.0')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'allows a localhost address request' do
+      request_stub = stub_request(http_method, 'http://127.0.0.1')
+
+      make_request('http://127.0.0.1')
+
+      expect(request_stub).to have_been_requested
+    end
+  end
+end