diff options
author | manojmj <mmj@gitlab.com> | 2019-06-27 14:44:01 +0530 |
---|---|---|
committer | manojmj <mmj@gitlab.com> | 2019-07-05 15:09:04 +0530 |
commit | c93ce836930a875452432ccc0c92733fb8adda29 (patch) | |
tree | a29f7f6461bfd79983cb305d9a7d89ff5ecec3b3 | |
parent | d1154dcd2b3b126cc4d6c3bba87c47b6669e697c (diff) | |
download | gitlab-ce-c93ce836930a875452432ccc0c92733fb8adda29.tar.gz |
Do not allow localhost url redirection in GitHub Integration
-rw-r--r-- | changelogs/unreleased/security-github-ssrf-redirect.yml | 5 | ||||
-rw-r--r-- | config/initializers/octokit.rb | 1 | ||||
-rw-r--r-- | lib/gitlab/github_import/client.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/legacy_github_import/client.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/octokit/middleware.rb | 23 | ||||
-rw-r--r-- | spec/lib/gitlab/octokit/middleware_spec.rb | 68 |
6 files changed, 100 insertions, 3 deletions
diff --git a/changelogs/unreleased/security-github-ssrf-redirect.yml b/changelogs/unreleased/security-github-ssrf-redirect.yml new file mode 100644 index 00000000000..36a36de3eb0 --- /dev/null +++ b/changelogs/unreleased/security-github-ssrf-redirect.yml @@ -0,0 +1,5 @@ +--- +title: Do not allow localhost url redirection in GitHub Integration +merge_request: +author: +type: security diff --git a/config/initializers/octokit.rb b/config/initializers/octokit.rb new file mode 100644 index 00000000000..b3749258ec5 --- /dev/null +++ b/config/initializers/octokit.rb @@ -0,0 +1 @@ +Octokit.middleware.insert_after Octokit::Middleware::FollowRedirects, Gitlab::Octokit::Middleware diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb index a61beafae0d..826b35d685c 100644 --- a/lib/gitlab/github_import/client.rb +++ b/lib/gitlab/github_import/client.rb @@ -40,7 +40,7 @@ module Gitlab # otherwise hitting the rate limit will result in a thread # being blocked in a `sleep()` call for up to an hour. def initialize(token, per_page: 100, parallel: true) - @octokit = Octokit::Client.new( + @octokit = ::Octokit::Client.new( access_token: token, per_page: per_page, api_endpoint: api_endpoint @@ -139,7 +139,7 @@ module Gitlab begin yield - rescue Octokit::TooManyRequests + rescue ::Octokit::TooManyRequests raise_or_wait_for_rate_limit # This retry will only happen when running in sequential mode as we'll diff --git a/lib/gitlab/legacy_github_import/client.rb b/lib/gitlab/legacy_github_import/client.rb index bbdd094e33b..b23efd64dee 100644 --- a/lib/gitlab/legacy_github_import/client.rb +++ b/lib/gitlab/legacy_github_import/client.rb @@ -101,7 +101,7 @@ module Gitlab # GitHub Rate Limit API returns 404 when the rate limit is # disabled. In this case we just want to return gracefully # instead of spitting out an error. - rescue Octokit::NotFound + rescue ::Octokit::NotFound nil end diff --git a/lib/gitlab/octokit/middleware.rb b/lib/gitlab/octokit/middleware.rb new file mode 100644 index 00000000000..2f762957d1b --- /dev/null +++ b/lib/gitlab/octokit/middleware.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Octokit + class Middleware + def initialize(app) + @app = app + end + + def call(env) + Gitlab::UrlBlocker.validate!(env[:url], { allow_localhost: allow_local_requests?, allow_local_network: allow_local_requests? }) + + @app.call(env) + end + + private + + def allow_local_requests? + Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? + end + end + end +end diff --git a/spec/lib/gitlab/octokit/middleware_spec.rb b/spec/lib/gitlab/octokit/middleware_spec.rb new file mode 100644 index 00000000000..7f2b523f5b7 --- /dev/null +++ b/spec/lib/gitlab/octokit/middleware_spec.rb @@ -0,0 +1,68 @@ +require 'spec_helper' + +describe Gitlab::Octokit::Middleware do + let(:app) { double(:app) } + let(:middleware) { described_class.new(app) } + + shared_examples 'Public URL' do + it 'does not raise an error' do + expect(app).to receive(:call).with(env) + + expect { middleware.call(env) }.not_to raise_error + end + end + + shared_examples 'Local URL' do + it 'raises an error' do + expect { middleware.call(env) }.to raise_error(Gitlab::UrlBlocker::BlockedUrlError) + end + end + + describe '#call' do + context 'when the URL is a public URL' do + let(:env) { { url: 'https://public-url.com' } } + + it_behaves_like 'Public URL' + end + + context 'when the URL is a localhost adresss' do + let(:env) { { url: 'http://127.0.0.1' } } + + context 'when localhost requests are not allowed' do + before do + stub_application_setting(allow_local_requests_from_hooks_and_services: false) + end + + it_behaves_like 'Local URL' + end + + context 'when localhost requests are allowed' do + before do + stub_application_setting(allow_local_requests_from_hooks_and_services: true) + end + + it_behaves_like 'Public URL' + end + end + + context 'when the URL is a local network address' do + let(:env) { { url: 'http://172.16.0.0' } } + + context 'when local network requests are not allowed' do + before do + stub_application_setting(allow_local_requests_from_hooks_and_services: false) + end + + it_behaves_like 'Local URL' + end + + context 'when local network requests are allowed' do + before do + stub_application_setting(allow_local_requests_from_hooks_and_services: true) + end + + it_behaves_like 'Public URL' + end + end + end +end |