diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-02-01 09:01:53 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-02-01 09:01:53 +0000 |
commit | 39b9de3e20e49178a95a040f763209298a684c09 (patch) | |
tree | f21ae72b691c006984d4b0079b181b11756514e3 | |
parent | 25a0a8c7836afeb19c0b500ccaba205a1fc4d4dd (diff) | |
download | gitlab-ce-39b9de3e20e49178a95a040f763209298a684c09.tar.gz |
Add latest changes from gitlab-org/security/gitlab@13-6-stable-ee
-rw-r--r-- | changelogs/unreleased/security-filter-graphql-logs.yml | 5 | ||||
-rw-r--r-- | changelogs/unreleased/security-ssrf-outbound-request.yml | 5 | ||||
-rw-r--r-- | changelogs/unreleased/secutity-404-difference.yml | 5 | ||||
-rw-r--r-- | config/routes.rb | 1 | ||||
-rw-r--r-- | config/routes/unmatched_project.rb | 18 | ||||
-rw-r--r-- | lib/gitlab/graphql/query_analyzers/logger_analyzer.rb | 14 | ||||
-rw-r--r-- | lib/gitlab/url_blocker.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb | 18 | ||||
-rw-r--r-- | spec/lib/gitlab/url_blocker_spec.rb | 15 | ||||
-rw-r--r-- | spec/requests/git_http_spec.rb | 8 | ||||
-rw-r--r-- | spec/routing/project_routing_spec.rb | 69 | ||||
-rw-r--r-- | spec/support/matchers/route_to_route_not_found_matcher.rb | 15 |
12 files changed, 171 insertions, 6 deletions
diff --git a/changelogs/unreleased/security-filter-graphql-logs.yml b/changelogs/unreleased/security-filter-graphql-logs.yml new file mode 100644 index 00000000000..2c70c480289 --- /dev/null +++ b/changelogs/unreleased/security-filter-graphql-logs.yml @@ -0,0 +1,5 @@ +--- +title: Filter sensitive GraphQL variables from logs +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-ssrf-outbound-request.yml b/changelogs/unreleased/security-ssrf-outbound-request.yml new file mode 100644 index 00000000000..e67360fdbbf --- /dev/null +++ b/changelogs/unreleased/security-ssrf-outbound-request.yml @@ -0,0 +1,5 @@ +--- +title: Fix DNS rebinding protection bypass when allowing an IP address in Outbound Requests setting +merge_request: +author: +type: security diff --git a/changelogs/unreleased/secutity-404-difference.yml b/changelogs/unreleased/secutity-404-difference.yml new file mode 100644 index 00000000000..0c09f2da9df --- /dev/null +++ b/changelogs/unreleased/secutity-404-difference.yml @@ -0,0 +1,5 @@ +--- +title: Add routes for unmatched url for not-get requests +merge_request: +author: +type: security diff --git a/config/routes.rb b/config/routes.rb index 867e5c2ec46..254b4b21fba 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -267,6 +267,7 @@ Rails.application.routes.draw do draw :dashboard draw :user draw :project + draw :unmatched_project # Issue https://gitlab.com/gitlab-org/gitlab/-/issues/210024 scope as: 'deprecated' do diff --git a/config/routes/unmatched_project.rb b/config/routes/unmatched_project.rb new file mode 100644 index 00000000000..b4fe243c7b0 --- /dev/null +++ b/config/routes/unmatched_project.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +scope(path: '*namespace_id', + as: :namespace, + namespace_id: Gitlab::PathRegex.full_namespace_route_regex) do + scope(path: ':project_id', + constraints: { project_id: Gitlab::PathRegex.project_route_regex }, + as: :project) do + post '*all', to: 'application#route_not_found' + put '*all', to: 'application#route_not_found' + patch '*all', to: 'application#route_not_found' + delete '*all', to: 'application#route_not_found' + post '/', to: 'application#route_not_found' + put '/', to: 'application#route_not_found' + patch '/', to: 'application#route_not_found' + delete '/', to: 'application#route_not_found' + end +end diff --git a/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb b/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb index 1285365376f..0665ea8b6c9 100644 --- a/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb +++ b/lib/gitlab/graphql/query_analyzers/logger_analyzer.rb @@ -49,13 +49,21 @@ module Gitlab private def process_variables(variables) - if variables.respond_to?(:to_s) - variables.to_s + filtered_variables = filter_sensitive_variables(variables) + + if filtered_variables.respond_to?(:to_s) + filtered_variables.to_s else - variables + filtered_variables end end + def filter_sensitive_variables(variables) + ActiveSupport::ParameterFilter + .new(::Rails.application.config.filter_parameters) + .filter(variables) + end + def duration(time_started) Gitlab::Metrics::System.monotonic_time - time_started end diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index eece2c343d2..10822f943b6 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -49,10 +49,12 @@ module Gitlab return [uri, nil] unless address_info ip_address = ip_address(address_info) - return [uri, nil] if domain_allowed?(uri) || ip_allowed?(ip_address, port: get_port(uri)) + return [uri, nil] if domain_allowed?(uri) protected_uri_with_hostname = enforce_uri_hostname(ip_address, uri, dns_rebind_protection) + return protected_uri_with_hostname if ip_allowed?(ip_address, port: get_port(uri)) + # Allow url from the GitLab instance itself but only for the configured hostname and ports return protected_uri_with_hostname if internal?(uri) diff --git a/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb b/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb index c8432513185..138765afd8a 100644 --- a/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb +++ b/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb @@ -40,4 +40,22 @@ RSpec.describe Gitlab::Graphql::QueryAnalyzers::LoggerAnalyzer do end end end + + describe '#initial_value' do + it 'filters out sensitive variables' do + doc = GraphQL.parse <<-GRAPHQL + mutation createNote($body: String!) { + createNote(input: {noteableId: "1", body: $body}) { + note { + id + } + } + } + GRAPHQL + + query = GraphQL::Query.new(GitlabSchema, document: doc, context: {}, variables: { body: "some note" }) + + expect(subject.initial_value(query)[:variables]).to eq('{:body=>"[FILTERED]"}') + end + end end diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index f466d117851..686382dc262 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -91,6 +91,21 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do end end + context 'DNS rebinding protection with IP allowed' do + let(:import_url) { 'http://a.192.168.0.120.3times.127.0.0.1.1time.repeat.rebind.network:9121/scrape?target=unix:///var/opt/gitlab/redis/redis.socket&check-keys=*' } + + before do + stub_dns(import_url, ip_address: '192.168.0.120') + + allow(Gitlab::UrlBlockers::UrlAllowlist).to receive(:ip_allowed?).and_return(true) + end + + it_behaves_like 'validates URI and hostname' do + let(:expected_uri) { 'http://192.168.0.120:9121/scrape?target=unix:///var/opt/gitlab/redis/redis.socket&check-keys=*' } + let(:expected_hostname) { 'a.192.168.0.120.3times.127.0.0.1.1time.repeat.rebind.network' } + end + end + context 'disabled DNS rebinding protection' do subject { described_class.validate!(import_url, dns_rebind_protection: false) } diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index 32aeeed43b6..3049ca46d7d 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -159,13 +159,17 @@ RSpec.describe 'Git HTTP requests' do context "POST git-upload-pack" do it "fails to find a route" do - expect { clone_post(repository_path) }.to raise_error(ActionController::RoutingError) + clone_post(repository_path) do |response| + expect(response).to have_gitlab_http_status(:not_found) + end end end context "POST git-receive-pack" do it "fails to find a route" do - expect { push_post(repository_path) }.to raise_error(ActionController::RoutingError) + push_post(repository_path) do |response| + expect(response).to have_gitlab_http_status(:not_found) + end end end end diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index a683dc28f4f..b414e8403b6 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -876,4 +876,73 @@ RSpec.describe 'project routing' do ) end end + + context 'with a non-existent project' do + it 'routes to 404 with get request' do + expect(get: "/gitlab/not_exist").to route_to( + 'application#route_not_found', + unmatched_route: 'gitlab/not_exist' + ) + end + + it 'routes to 404 with delete request' do + expect(delete: "/gitlab/not_exist").to route_to( + 'application#route_not_found', + namespace_id: 'gitlab', + project_id: 'not_exist' + ) + end + + it 'routes to 404 with post request' do + expect(post: "/gitlab/not_exist").to route_to( + 'application#route_not_found', + namespace_id: 'gitlab', + project_id: 'not_exist' + ) + end + + it 'routes to 404 with put request' do + expect(put: "/gitlab/not_exist").to route_to( + 'application#route_not_found', + namespace_id: 'gitlab', + project_id: 'not_exist' + ) + end + + context 'with route to some action' do + it 'routes to 404 with get request to' do + expect(get: "/gitlab/not_exist/some_action").to route_to( + 'application#route_not_found', + unmatched_route: 'gitlab/not_exist/some_action' + ) + end + + it 'routes to 404 with delete request' do + expect(delete: "/gitlab/not_exist/some_action").to route_to( + 'application#route_not_found', + namespace_id: 'gitlab', + project_id: 'not_exist', + all: 'some_action' + ) + end + + it 'routes to 404 with post request' do + expect(post: "/gitlab/not_exist/some_action").to route_to( + 'application#route_not_found', + namespace_id: 'gitlab', + project_id: 'not_exist', + all: 'some_action' + ) + end + + it 'routes to 404 with put request' do + expect(put: "/gitlab/not_exist/some_action").to route_to( + 'application#route_not_found', + namespace_id: 'gitlab', + project_id: 'not_exist', + all: 'some_action' + ) + end + end + end end diff --git a/spec/support/matchers/route_to_route_not_found_matcher.rb b/spec/support/matchers/route_to_route_not_found_matcher.rb new file mode 100644 index 00000000000..4105f0f9191 --- /dev/null +++ b/spec/support/matchers/route_to_route_not_found_matcher.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +RSpec::Matchers.define :route_to_route_not_found do + match do |actual| + expect(actual).to route_to(controller: 'application', action: 'route_not_found') + rescue RSpec::Expectations::ExpectationNotMetError => e + # `route_to` matcher requires providing all params for exact match. As we use it in shared examples and we provide different paths, + # this matcher checks if provided route matches controller and action, without checking params. + expect(e.message).to include("-{\"controller\"=>\"application\", \"action\"=>\"route_not_found\"}\n+{\"controller\"=>\"application\", \"action\"=>\"route_not_found\",") + end + + failure_message do |_| + "expected #{actual} to route to route_not_found" + end +end |