Merge branch 'security-project-import-bypass' into 'master'

Project visibility restriction bypass

See merge request gitlab/gitlabhq!3306

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

changelogs/unreleased/security-project-import-bypass.yml

+---
+title: Fix project import restricted visibility bypass via API
+merge_request:
+author:
+type: security

lib/gitlab/visibility_level_checker.rb

+# 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

spec/lib/gitlab/visibility_level_checker_spec.rb

+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

spec/services/projects/create_service_spec.rb

@@ -183,27 +183,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