Commit 06eddc3e authored by George Koltsov's avatar George Koltsov

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
parent 1dfbb27f
......@@ -8,6 +8,8 @@ module Projects
@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
---
title: Fix project import restricted visibility bypass via API
merge_request:
author:
type: security
# 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
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
......@@ -182,14 +182,12 @@ describe Projects::CreateService, '#execute' do
context 'restricted visibility level' do
before do
stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
opts.merge!(
visibility_level: Gitlab::VisibilityLevel::PUBLIC
)
end
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(
......@@ -206,6 +204,46 @@ describe Projects::CreateService, '#execute' do
end
end
context 'when visibility is project based' do
before do
opts.merge!(
visibility_level: Gitlab::VisibilityLevel::PUBLIC
)
end
include_examples 'restricted visibility'
end
context 'when visibility is overridden' do
let(:visibility) { 'public' }
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
context 'repository creation' do
it 'synchronously creates the repository' do
expect_any_instance_of(Project).to receive(:create_repository)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment