diff options
author | George Koltsov <gkoltsov@gitlab.com> | 2019-08-06 16:05:18 +0300 |
---|---|---|
committer | George Koltsov <gkoltsov@gitlab.com> | 2019-08-15 12:30:47 +0300 |
commit | 06eddc3edfa739c0c685d461a58621b43c4ecd5d (patch) | |
tree | 58b052191c121af531c03d9f871e681638a2e201 | |
parent | 1dfbb27f6e8d01023564eededff2a0ba1a04badc (diff) |
Fix project import restricted visibility bypass
Add Gitlab::VisibilityLevelChecker that verifies
selected project visibility level (or overridden param)
is not restricted when creating or importing a project
-rw-r--r-- | app/services/projects/create_service.rb | 27 | ||||
-rw-r--r-- | changelogs/unreleased/security-project-import-bypass.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/visibility_level_checker.rb | 88 | ||||
-rw-r--r-- | spec/lib/gitlab/visibility_level_checker_spec.rb | 82 | ||||
-rw-r--r-- | spec/services/projects/create_service_spec.rb | 68 |
5 files changed, 244 insertions, 26 deletions
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 89dc4375c63..942a45286b2 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -5,9 +5,11 @@ module Projects include ValidatesClassificationLabel def initialize(user, params) - @current_user, @params = user, params.dup - @skip_wiki = @params.delete(:skip_wiki) + @current_user, @params = user, params.dup + @skip_wiki = @params.delete(:skip_wiki) @initialize_with_readme = Gitlab::Utils.to_boolean(@params.delete(:initialize_with_readme)) + @import_data = @params.delete(:import_data) + @relations_block = @params.delete(:relations_block) end def execute @@ -15,14 +17,11 @@ module Projects return ::Projects::CreateFromTemplateService.new(current_user, params).execute end - import_data = params.delete(:import_data) - relations_block = params.delete(:relations_block) - @project = Project.new(params) # Make sure that the user is allowed to use the specified visibility level - unless Gitlab::VisibilityLevel.allowed_for?(current_user, @project.visibility_level) - deny_visibility_level(@project) + if project_visibility.restricted? + deny_visibility_level(@project, project_visibility.visibility_level) return @project end @@ -44,7 +43,7 @@ module Projects @project.namespace_id = current_user.namespace_id end - relations_block&.call(@project) + @relations_block&.call(@project) yield(@project) if block_given? validate_classification_label(@project, :external_authorization_classification_label) @@ -54,7 +53,7 @@ module Projects @project.creator = current_user - save_project_and_import_data(import_data) + save_project_and_import_data after_create_actions if @project.persisted? @@ -129,9 +128,9 @@ module Projects !@project.feature_available?(:wiki, current_user) || @skip_wiki end - def save_project_and_import_data(import_data) + def save_project_and_import_data Project.transaction do - @project.create_or_update_import_data(data: import_data[:data], credentials: import_data[:credentials]) if import_data + @project.create_or_update_import_data(data: @import_data[:data], credentials: @import_data[:credentials]) if @import_data if @project.save unless @project.gitlab_project_import? @@ -192,5 +191,11 @@ module Projects fail(error: @project.errors.full_messages.join(', ')) end end + + def project_visibility + @project_visibility ||= Gitlab::VisibilityLevelChecker + .new(current_user, @project, project_params: { import_data: @import_data }) + .level_restricted? + end end end diff --git a/changelogs/unreleased/security-project-import-bypass.yml b/changelogs/unreleased/security-project-import-bypass.yml new file mode 100644 index 00000000000..fc7b823509c --- /dev/null +++ b/changelogs/unreleased/security-project-import-bypass.yml @@ -0,0 +1,5 @@ +--- +title: Fix project import restricted visibility bypass via API +merge_request: +author: +type: security diff --git a/lib/gitlab/visibility_level_checker.rb b/lib/gitlab/visibility_level_checker.rb new file mode 100644 index 00000000000..f15f1486a4e --- /dev/null +++ b/lib/gitlab/visibility_level_checker.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +# Gitlab::VisibilityLevelChecker verifies that: +# - Current @project.visibility_level is not restricted +# - Override visibility param is not restricted +# - @see https://docs.gitlab.com/ce/api/project_import_export.html#import-a-file +# +# @param current_user [User] Current user object to verify visibility level against +# @param project [Project] Current project that is being created/imported +# @param project_params [Hash] Supplementary project params (e.g. import +# params containing visibility override) +# +# @example +# user = User.find(2) +# project = Project.last +# project_params = {:import_data=>{:data=>{:override_params=>{"visibility"=>"public"}}}} +# level_checker = Gitlab::VisibilityLevelChecker.new(user, project, project_params: project_params) +# +# project_visibility = level_checker.level_restricted? +# => #<Gitlab::VisibilityEvaluationResult:0x00007fbe16ee33c0 @restricted=true, @visibility_level=20> +# +# if project_visibility.restricted? +# deny_visibility_level(project, project_visibility.visibility_level) +# end +# +# @return [VisibilityEvaluationResult] Visibility evaluation result. Responds to: +# #restricted - boolean indicating if level is restricted +# #visibility_level - integer of restricted visibility level +# +module Gitlab + class VisibilityLevelChecker + def initialize(current_user, project, project_params: {}) + @current_user = current_user + @project = project + @project_params = project_params + end + + def level_restricted? + return VisibilityEvaluationResult.new(true, override_visibility_level_value) if override_visibility_restricted? + return VisibilityEvaluationResult.new(true, project.visibility_level) if project_visibility_restricted? + + VisibilityEvaluationResult.new(false, nil) + end + + private + + attr_reader :current_user, :project, :project_params + + def override_visibility_restricted? + return unless import_data + return unless override_visibility_level + return if Gitlab::VisibilityLevel.allowed_for?(current_user, override_visibility_level_value) + + true + end + + def project_visibility_restricted? + return if Gitlab::VisibilityLevel.allowed_for?(current_user, project.visibility_level) + + true + end + + def import_data + @import_data ||= project_params[:import_data] + end + + def override_visibility_level + @override_visibility_level ||= import_data.deep_symbolize_keys.dig(:data, :override_params, :visibility) + end + + def override_visibility_level_value + @override_visibility_level_value ||= Gitlab::VisibilityLevel.level_value(override_visibility_level) + end + end + + class VisibilityEvaluationResult + attr_reader :visibility_level + + def initialize(restricted, visibility_level) + @restricted = restricted + @visibility_level = visibility_level + end + + def restricted? + @restricted + end + end +end diff --git a/spec/lib/gitlab/visibility_level_checker_spec.rb b/spec/lib/gitlab/visibility_level_checker_spec.rb new file mode 100644 index 00000000000..325ac3c6f31 --- /dev/null +++ b/spec/lib/gitlab/visibility_level_checker_spec.rb @@ -0,0 +1,82 @@ +require 'spec_helper' + +describe Gitlab::VisibilityLevelChecker do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:visibility_level_checker) { } + let(:override_params) { {} } + + subject { described_class.new(user, project, project_params: override_params) } + + describe '#level_restricted?' do + context 'when visibility level is allowed' do + it 'returns false with nil for visibility level' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(false) + expect(result.visibility_level).to be_nil + end + end + + context 'when visibility level is restricted' do + before do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) + end + + it 'returns true and visibility name' do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + result = subject.level_restricted? + + expect(result.restricted?).to eq(true) + expect(result.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) + end + + context 'overridden visibility' do + let(:override_params) do + { + import_data: { + data: { + override_params: { + visibility: override_visibility + } + } + } + } + end + + context 'when restricted' do + let(:override_visibility) { 'public' } + + it 'returns true and visibility name' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(true) + expect(result.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) + end + end + + context 'when misspelled' do + let(:override_visibility) { 'publik' } + + it 'returns false with nil for visibility level' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(false) + expect(result.visibility_level).to be_nil + end + end + + context 'when import_data is missing' do + let(:override_params) { {} } + + it 'returns false with nil for visibility level' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(false) + expect(result.visibility_level).to be_nil + end + end + end + end + end +end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index b0b74407812..a87ae7bfd37 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -182,27 +182,65 @@ describe Projects::CreateService, '#execute' do context 'restricted visibility level' do before do stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) + end - opts.merge!( - visibility_level: Gitlab::VisibilityLevel::PUBLIC - ) + shared_examples 'restricted visibility' do + it 'does not allow a restricted visibility level for non-admins' do + project = create_project(user, opts) + + expect(project).to respond_to(:errors) + expect(project.errors.messages).to have_key(:visibility_level) + expect(project.errors.messages[:visibility_level].first).to( + match('restricted by your GitLab administrator') + ) + end + + it 'allows a restricted visibility level for admins' do + admin = create(:admin) + project = create_project(admin, opts) + + expect(project.errors.any?).to be(false) + expect(project.saved?).to be(true) + end end - it 'does not allow a restricted visibility level for non-admins' do - project = create_project(user, opts) - expect(project).to respond_to(:errors) - expect(project.errors.messages).to have_key(:visibility_level) - expect(project.errors.messages[:visibility_level].first).to( - match('restricted by your GitLab administrator') - ) + context 'when visibility is project based' do + before do + opts.merge!( + visibility_level: Gitlab::VisibilityLevel::PUBLIC + ) + end + + include_examples 'restricted visibility' end - it 'allows a restricted visibility level for admins' do - admin = create(:admin) - project = create_project(admin, opts) + context 'when visibility is overridden' do + let(:visibility) { 'public' } - expect(project.errors.any?).to be(false) - expect(project.saved?).to be(true) + before do + opts.merge!( + import_data: { + data: { + override_params: { + visibility: visibility + } + } + } + ) + end + + include_examples 'restricted visibility' + + context 'when visibility is misspelled' do + let(:visibility) { 'publik' } + + it 'does not restrict project creation' do + project = create_project(user, opts) + + expect(project.errors.any?).to be(false) + expect(project.saved?).to be(true) + end + end end end |