Merge branch 'fj-15329-services-callbacks-ssrf' into 'security-10-6'

Server Side Request Forgery in Services and Web Hooks

See merge request gitlab/gitlabhq!2337

8 files changed, 191 insertions, 8 deletions

diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb
new file mode 100644
index 00000000000..b0bc081a3c8
--- /dev/null
+++ b/spec/lib/gitlab/http_spec.rb
@@ -0,0 +1,49 @@
+require 'spec_helper'
+
+describe Gitlab::HTTP do
+  describe 'allow_local_requests_from_hooks_and_services is' do
+    before do
+      WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success')
+    end
+
+    context 'disabled' do
+      before do
+        allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false)
+      end
+
+      it 'deny requests to localhost' do
+        expect { described_class.get('http://localhost:3003') }.to raise_error(URI::InvalidURIError)
+      end
+
+      it 'deny requests to private network' do
+        expect { described_class.get('http://192.168.1.2:3003') }.to raise_error(URI::InvalidURIError)
+      end
+
+      context 'if allow_local_requests set to true' do
+        it 'override the global value and allow requests to localhost or private network' do
+          expect { described_class.get('http://localhost:3003', allow_local_requests: true) }.not_to raise_error
+        end
+      end
+    end
+
+    context 'enabled' do
+      before do
+        allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true)
+      end
+
+      it 'allow requests to localhost' do
+        expect { described_class.get('http://localhost:3003') }.not_to raise_error
+      end
+
+      it 'allow requests to private network' do
+        expect { described_class.get('http://192.168.1.2:3003') }.not_to raise_error
+      end
+
+      context 'if allow_local_requests set to false' do
+        it 'override the global value and ban requests to localhost or private network' do
+          expect { described_class.get('http://localhost:3003', allow_local_requests: false) }.to raise_error(URI::InvalidURIError)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb
index d9b3c2350b1..2d35b026485 100644
--- a/spec/lib/gitlab/url_blocker_spec.rb
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -2,6 +2,8 @@ require 'spec_helper'
 
 describe Gitlab::UrlBlocker do
   describe '#blocked_url?' do
+    let(:valid_ports) { Project::VALID_IMPORT_PORTS }
+
     it 'allows imports from configured web host and port' do
       import_url = "http://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/t.git"
       expect(described_class.blocked_url?(import_url)).to be false
@@ -17,7 +19,7 @@ describe Gitlab::UrlBlocker do
     end
 
     it 'returns true for bad port' do
-      expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git')).to be true
+      expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git', valid_ports: valid_ports)).to be true
     end
 
     it 'returns true for alternative version of 127.0.0.1 (0177.1)' do
@@ -71,6 +73,47 @@ describe Gitlab::UrlBlocker do
     it 'returns false for legitimate URL' do
       expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false
     end
+
+    context 'when allow_private_networks is' do
+      let(:private_networks) { ['192.168.1.2', '10.0.0.2', '172.16.0.2'] }
+      let(:fake_domain) { 'www.fakedomain.fake' }
+
+      context 'true (default)' do
+        it 'does not block urls from private networks' do
+          private_networks.each do |ip|
+            stub_domain_resolv(fake_domain, ip)
+
+            expect(described_class).not_to be_blocked_url("http://#{fake_domain}")
+
+            unstub_domain_resolv
+
+            expect(described_class).not_to be_blocked_url("http://#{ip}")
+          end
+        end
+      end
+
+      context 'false' do
+        it 'blocks urls from private networks' do
+          private_networks.each do |ip|
+            stub_domain_resolv(fake_domain, ip)
+
+            expect(described_class).to be_blocked_url("http://#{fake_domain}", allow_private_networks: false)
+
+            unstub_domain_resolv
+
+            expect(described_class).to be_blocked_url("http://#{ip}", allow_private_networks: false)
+          end
+        end
+      end
+
+      def stub_domain_resolv(domain, ip)
+        allow(Addrinfo).to receive(:getaddrinfo).with(domain, any_args).and_return([double(ip_address: ip, ipv4_private?: true)])
+      end
+
+      def unstub_domain_resolv
+        allow(Addrinfo).to receive(:getaddrinfo).and_call_original
+      end
+    end
   end
 
   # Resolv does not support resolving UTF-8 domain names
diff --git a/spec/lib/mattermost/command_spec.rb b/spec/lib/mattermost/command_spec.rb
index 369e7b181b9..8ba15ae0f38 100644
--- a/spec/lib/mattermost/command_spec.rb
+++ b/spec/lib/mattermost/command_spec.rb
@@ -4,10 +4,11 @@ describe Mattermost::Command do
   let(:params) { { 'token' => 'token', team_id: 'abc' } }
 
   before do
-    Mattermost::Session.base_uri('http://mattermost.example.com')
+    session = Mattermost::Session.new(nil)
+    session.base_uri = 'http://mattermost.example.com'
 
     allow_any_instance_of(Mattermost::Client).to receive(:with_session)
-      .and_yield(Mattermost::Session.new(nil))
+      .and_yield(session)
   end
 
   describe '#create' do
diff --git a/spec/lib/mattermost/session_spec.rb b/spec/lib/mattermost/session_spec.rb
index 3db19d06305..c855643c4d8 100644
--- a/spec/lib/mattermost/session_spec.rb
+++ b/spec/lib/mattermost/session_spec.rb
@@ -15,7 +15,7 @@ describe Mattermost::Session, type: :request do
   it { is_expected.to respond_to(:strategy) }
 
   before do
-    described_class.base_uri(mattermost_url)
+    subject.base_uri = mattermost_url
   end
 
   describe '#with session' do
diff --git a/spec/lib/mattermost/team_spec.rb b/spec/lib/mattermost/team_spec.rb
index 3c8206031cf..2cfa6802612 100644
--- a/spec/lib/mattermost/team_spec.rb
+++ b/spec/lib/mattermost/team_spec.rb
@@ -2,10 +2,11 @@ require 'spec_helper'
 
 describe Mattermost::Team do
   before do
-    Mattermost::Session.base_uri('http://mattermost.example.com')
+    session = Mattermost::Session.new(nil)
+    session.base_uri = 'http://mattermost.example.com'
 
     allow_any_instance_of(Mattermost::Client).to receive(:with_session)
-      .and_yield(Mattermost::Session.new(nil))
+      .and_yield(session)
   end
 
   describe '#all' do
diff --git a/spec/models/project_services/mattermost_slash_commands_service_spec.rb b/spec/models/project_services/mattermost_slash_commands_service_spec.rb
index a5bdf9a9337..05d33cd3874 100644
--- a/spec/models/project_services/mattermost_slash_commands_service_spec.rb
+++ b/spec/models/project_services/mattermost_slash_commands_service_spec.rb
@@ -9,10 +9,11 @@ describe MattermostSlashCommandsService do
     let(:user) { create(:user) }
 
     before do
-      Mattermost::Session.base_uri("http://mattermost.example.com")
+      session = Mattermost::Session.new(nil)
+      session.base_uri = 'http://mattermost.example.com'
 
       allow_any_instance_of(Mattermost::Client).to receive(:with_session)
-        .and_yield(Mattermost::Session.new(nil))
+        .and_yield(session)
     end
 
     describe '#configure' do
diff --git a/spec/rubocop/cop/gitlab/httparty_spec.rb b/spec/rubocop/cop/gitlab/httparty_spec.rb
new file mode 100644
index 00000000000..510839a21d7
--- /dev/null
+++ b/spec/rubocop/cop/gitlab/httparty_spec.rb
@@ -0,0 +1,74 @@
+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../../rubocop/cop/gitlab/httparty'
+
+describe RuboCop::Cop::Gitlab::HTTParty do # rubocop:disable RSpec/FilePath
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  shared_examples('registering include offense') do |options|
+    let(:offending_lines) { options[:offending_lines] }
+
+    it 'registers an offense when the class includes HTTParty' do
+      inspect_source(source)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(offending_lines.size)
+        expect(cop.offenses.map(&:line)).to eq(offending_lines)
+      end
+    end
+  end
+
+  shared_examples('registering call offense') do |options|
+    let(:offending_lines) { options[:offending_lines] }
+
+    it 'registers an offense when the class calls HTTParty' do
+      inspect_source(source)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(offending_lines.size)
+        expect(cop.offenses.map(&:line)).to eq(offending_lines)
+      end
+    end
+  end
+
+  context 'when source is a regular module' do
+    it_behaves_like 'registering include offense', offending_lines: [2] do
+      let(:source) do
+        <<~RUBY
+          module M
+            include HTTParty
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is a regular class' do
+    it_behaves_like 'registering include offense', offending_lines: [2] do
+      let(:source) do
+        <<~RUBY
+          class Foo
+            include HTTParty
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when HTTParty is called' do
+    it_behaves_like 'registering call offense', offending_lines: [3] do
+      let(:source) do
+        <<~RUBY
+          class Foo
+            def bar
+              HTTParty.get('http://example.com')
+            end
+          end
+        RUBY
+      end
+    end
+  end
+end
diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb
index 21910e69d2e..2ef2e61babc 100644
--- a/spec/services/web_hook_service_spec.rb
+++ b/spec/services/web_hook_service_spec.rb
@@ -14,6 +14,20 @@ describe WebHookService do
   end
   let(:service_instance) { described_class.new(project_hook, data, :push_hooks) }
 
+  describe '#initialize' do
+    it 'allow_local_requests is true if hook is a SystemHook' do
+      instance = described_class.new(build(:system_hook), data, :system_hook)
+      expect(instance.request_options[:allow_local_requests]).to be_truthy
+    end
+
+    it 'allow_local_requests is false if hook is not a SystemHook' do
+      %i(project_hook service_hook web_hook_log).each do |hook|
+        instance = described_class.new(build(hook), data, hook)
+        expect(instance.request_options[:allow_local_requests]).to be_falsey
+      end
+    end
+  end
+
   describe '#execute' do
     before do
       project.hooks << [project_hook]