Merge branch 'security-master-secret-ci-variables-exposed' into 'master'

[master] Secret CI variables can exposed by creating a tag with the same name as an existing protected branch

See merge request gitlab/gitlabhq!2596

app/models/ci/build.rb

@@ -10,6 +10,7 @@ module Ci
     include Importable
     include Gitlab::Utils::StrongMemoize
     include Deployable
+    include HasRef
 
     belongs_to :project, inverse_of: :builds
     belongs_to :runner
@@ -640,11 +641,11 @@ module Ci
     def secret_group_variables
       return [] unless project.group
 
-      project.group.ci_variables_for(ref, project)
+      project.group.ci_variables_for(git_ref, project)
     end
 
     def secret_project_variables(environment: persisted_environment)
-      project.ci_variables_for(ref: ref, environment: environment)
+      project.ci_variables_for(ref: git_ref, environment: environment)
     end
 
     def steps

app/models/ci/pipeline.rb

@@ -11,6 +11,7 @@ module Ci
     include Gitlab::Utils::StrongMemoize
     include AtomicInternalId
     include EnumWithNil
+    include HasRef
 
     belongs_to :project, inverse_of: :all_pipelines
     belongs_to :user
@@ -380,7 +381,7 @@ module Ci
     end
 
     def branch?
-      !tag? && !merge_request?
+      super && !merge_request?
     end
 
     def stuck?
@@ -580,7 +581,7 @@ module Ci
     end
 
     def protected_ref?
-      strong_memoize(:protected_ref) { project.protected_for?(ref) }
+      strong_memoize(:protected_ref) { project.protected_for?(git_ref) }
     end
 
     def legacy_trigger
@@ -712,14 +713,10 @@ module Ci
     end
 
     def git_ref
-      if branch?
+      if merge_request?
         Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s
-      elsif merge_request?
-        Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s
-      elsif tag?
-        Gitlab::Git::TAG_REF_PREFIX + ref.to_s
       else
-        raise ArgumentError, 'Invalid pipeline type!'
+        super
       end
     end
 

app/models/concerns/has_ref.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module HasRef
+  extend ActiveSupport::Concern
+
+  def branch?
+    !tag?
+  end
+
+  def git_ref
+    if branch?
+      Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s
+    elsif tag?
+      Gitlab::Git::TAG_REF_PREFIX + ref.to_s
+    end
+  end
+end

app/models/project.rb

@@ -1733,10 +1733,21 @@ class Project < ActiveRecord::Base
   end
 
   def protected_for?(ref)
-    if repository.branch_exists?(ref)
-      ProtectedBranch.protected?(self, ref)
-    elsif repository.tag_exists?(ref)
-      ProtectedTag.protected?(self, ref)
+    raise Repository::AmbiguousRefError if repository.ambiguous_ref?(ref)
+
+    resolved_ref = repository.expand_ref(ref) || ref
+    return false unless Gitlab::Git.tag_ref?(resolved_ref) || Gitlab::Git.branch_ref?(resolved_ref)
+
+    ref_name = if resolved_ref == ref
+                 Gitlab::Git.ref_name(resolved_ref)
+               else
+                 ref
+               end
+
+    if Gitlab::Git.branch_ref?(resolved_ref)
+      ProtectedBranch.protected?(self, ref_name)
+    elsif Gitlab::Git.tag_ref?(resolved_ref)
+      ProtectedTag.protected?(self, ref_name)
     end
   end
 

app/models/repository.rb

@@ -25,6 +25,7 @@ class Repository
   delegate :bundle_to_disk, to: :raw_repository
 
   CreateTreeError = Class.new(StandardError)
+  AmbiguousRefError = Class.new(StandardError)
 
   # Methods that cache data from the Git repository.
   #
@@ -181,6 +182,18 @@ class Repository
     tags.find { |tag| tag.name == name }
   end
 
+  def ambiguous_ref?(ref)
+    tag_exists?(ref) && branch_exists?(ref)
+  end
+
+  def expand_ref(ref)
+    if tag_exists?(ref)
+      Gitlab::Git::TAG_REF_PREFIX + ref
+    elsif branch_exists?(ref)
+      Gitlab::Git::BRANCH_REF_PREFIX + ref
+    end
+  end
+
   def add_branch(user, branch_name, ref)
     branch = raw_repository.add_branch(branch_name, user: user, target: ref)
 

changelogs/unreleased/security-master-secret-ci-variables-exposed.yml

@@ -0,0 +1,5 @@
+---
+title: Prevent leaking protected variables for ambiguous refs.
+merge_request:
+author:
+type: security

lib/gitlab/ci/pipeline/chain/command.rb

@@ -54,7 +54,13 @@ module Gitlab
 
           def protected_ref?
             strong_memoize(:protected_ref) do
-              project.protected_for?(ref)
+              project.protected_for?(origin_ref)
+            end
+          end
+
+          def ambiguous_ref?
+            strong_memoize(:ambiguous_ref) do
+              project.repository.ambiguous_ref?(origin_ref)
             end
           end
         end

lib/gitlab/ci/pipeline/chain/validate/repository.rb

@@ -16,6 +16,10 @@ module Gitlab
               unless @command.sha
                 return error('Commit not found')
               end
+
+              if @command.ambiguous_ref?
+                return error('Ref is ambiguous')
+              end
             end
 
             def break?

lib/gitlab/git.rb

@@ -54,11 +54,11 @@ module Gitlab
       end
 
       def tag_ref?(ref)
-        ref.start_with?(TAG_REF_PREFIX)
+        ref =~ /^#{TAG_REF_PREFIX}.+/
       end
 
       def branch_ref?(ref)
-        ref.start_with?(BRANCH_REF_PREFIX)
+        ref =~ /^#{BRANCH_REF_PREFIX}.+/
       end
 
       def blank_ref?(ref)

spec/lib/gitlab/ci/pipeline/chain/command_spec.rb

@@ -182,4 +182,24 @@ describe Gitlab::Ci::Pipeline::Chain::Command do
       it { is_expected.to eq(false) }
     end
   end
+
+  describe '#ambiguous_ref' do
+    let(:project) { create(:project, :repository) }
+    let(:command) { described_class.new(project: project, origin_ref: 'ref') }
+
+    subject { command.ambiguous_ref? }
+
+    context 'when ref is not ambiguous' do
+      it { is_expected. to eq(false) }
+    end
+
+    context 'when ref is ambiguous' do
+      before do
+        project.repository.add_tag(project.creator, 'ref', 'master')
+        project.repository.add_branch(project.creator, 'ref', 'master')
+      end
+
+      it { is_expected. to eq(true) }
+    end
+  end
 end

spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb

@@ -14,6 +14,7 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do
     Gitlab::Ci::Pipeline::Chain::Command.new(
       project: project,
       current_user: user,
+      origin_ref: 'master',
       seeds_block: nil)
   end
 
@@ -106,6 +107,7 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do
       Gitlab::Ci::Pipeline::Chain::Command.new(
         project: project,
         current_user: user,
+        origin_ref: 'master',
         seeds_block: seeds_block)
     end
 

spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb

@@ -42,6 +42,27 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do
     end
   end
 
+  context 'when ref is ambiguous' do
+    let(:project) do
+      create(:project, :repository).tap do |proj|
+        proj.repository.add_tag(user, 'master', 'master')
+      end
+    end
+    let(:command) do
+      Gitlab::Ci::Pipeline::Chain::Command.new(
+        project: project, current_user: user, origin_ref: 'master')
+    end
+
+    it 'breaks the chain' do
+      expect(step.break?).to be true
+    end
+
+    it 'adds an error about missing ref' do
+      expect(pipeline.errors.to_a)
+        .to include 'Ref is ambiguous'
+    end
+  end
+
   context 'when does not have existing SHA set' do
     let(:command) do
       Gitlab::Ci::Pipeline::Chain::Command.new(

spec/lib/gitlab/ci/pipeline/seed/build_spec.rb

@@ -1,7 +1,8 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Pipeline::Seed::Build do
-  let(:pipeline) { create(:ci_empty_pipeline) }
+  let(:project) { create(:project, :repository) }
+  let(:pipeline) { create(:ci_empty_pipeline, project: project) }
 
   let(:attributes) do
     { name: 'rspec',

spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb

@@ -1,7 +1,8 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Pipeline::Seed::Stage do
-  let(:pipeline) { create(:ci_empty_pipeline) }
+  let(:project) { create(:project, :repository) }
+  let(:pipeline) { create(:ci_empty_pipeline, project: project) }
 
   let(:attributes) do
     { name: 'test',

spec/models/ci/build_spec.rb

@@ -2386,6 +2386,8 @@ describe Ci::Build do
     end
 
     context 'when protected variable is defined' do
+      let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref }
+
       let(:protected_variable) do
         { key: 'PROTECTED_KEY', value: 'protected_value', public: false }
       end
@@ -2398,7 +2400,7 @@ describe Ci::Build do
 
       context 'when the branch is protected' do
         before do
-          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
+          allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
         end
 
         it { is_expected.to include(protected_variable) }
@@ -2406,7 +2408,7 @@ describe Ci::Build do
 
       context 'when the tag is protected' do
         before do
-          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
+          allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
         end
 
         it { is_expected.to include(protected_variable) }
@@ -2431,6 +2433,8 @@ describe Ci::Build do
     end
 
     context 'when group protected variable is defined' do
+      let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref }
+
       let(:protected_variable) do
         { key: 'PROTECTED_KEY', value: 'protected_value', public: false }
       end
@@ -2443,7 +2447,7 @@ describe Ci::Build do
 
       context 'when the branch is protected' do
         before do
-          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
+          allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
         end
 
         it { is_expected.to include(protected_variable) }
@@ -2451,7 +2455,7 @@ describe Ci::Build do
 
       context 'when the tag is protected' do
         before do
-          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
+          allow(build.project).to receive(:protected_for?).with(ref).and_return(true)
         end
 
         it { is_expected.to include(protected_variable) }

spec/models/ci/pipeline_spec.rb

@@ -397,6 +397,10 @@ describe Ci::Pipeline, :mailer do
   end
 
   describe '#protected_ref?' do
+    before do
+      pipeline.project = create(:project, :repository)
+    end
+
     it 'delegates method to project' do
       expect(pipeline).not_to be_protected_ref
     end

spec/models/concerns/has_ref_spec.rb

@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe HasRef do
+  describe '#branch?' do
+    let(:build) { create(:ci_build) }
+
+    subject { build.branch? }
+
+    context 'is not a tag' do
+      before do
+        build.tag = false
+      end
+
+      it 'return true when tag is set to false' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'is not a tag' do
+      before do
+        build.tag = true
+      end
+
+      it 'return false when tag is set to true' do
+        is_expected.to be_falsey
+      end
+    end
+  end
+
+  describe '#git_ref' do
+    subject { build.git_ref }
+
+    context 'when tag is true' do
+      let(:build) { create(:ci_build, tag: true) }
+
+      it 'returns a tag ref' do
+        is_expected.to start_with(Gitlab::Git::TAG_REF_PREFIX)
+      end
+    end
+
+    context 'when tag is false' do
+      let(:build) { create(:ci_build, tag: false) }
+
+      it 'returns a branch ref' do
+        is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX)
+      end
+    end
+
+    context 'when tag is nil' do
+      let(:build) { create(:ci_build, tag: nil) }
+
+      it 'returns a branch ref' do
+        is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX)
+      end
+    end
+  end
+end

spec/models/project_spec.rb

@@ -2550,6 +2550,10 @@ describe Project do
     end
 
     context 'when the ref is not protected' do
+      before do
+        allow(project).to receive(:protected_for?).with('ref').and_return(false)
+      end
+
       it 'contains only the CI variables' do
         is_expected.to contain_exactly(ci_variable)
       end
@@ -2589,42 +2593,139 @@ describe Project do
   end
 
   describe '#protected_for?' do
-    let(:project) { create(:project) }
+    let(:project) { create(:project, :repository) }
 
-    subject { project.protected_for?('ref') }
+    subject { project.protected_for?(ref) }
 
-    context 'when the ref is not protected' do
+    shared_examples 'ref is not protected' do
       before do
         stub_application_setting(
           default_branch_protection: Gitlab::Access::PROTECTION_NONE)
       end
 
       it 'returns false' do
-        is_expected.to be_falsey
+        is_expected.to be false
       end
     end
 
-    context 'when the ref is a protected branch' do
+    shared_examples 'ref is protected branch' do
       before do
-        allow(project).to receive(:repository).and_call_original
-        allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(true)
-        create(:protected_branch, name: 'ref', project: project)
+        create(:protected_branch, name: 'master', project: project)
       end
 
       it 'returns true' do
-        is_expected.to be_truthy
+        is_expected.to be true
       end
     end
 
-    context 'when the ref is a protected tag' do
+    shared_examples 'ref is protected tag' do
       before do
-        allow(project).to receive_message_chain(:repository, :branch_exists?).and_return(false)
-        allow(project).to receive_message_chain(:repository, :tag_exists?).and_return(true)
-        create(:protected_tag, name: 'ref', project: project)
+        create(:protected_tag, name: 'v1.0.0', project: project)
       end
 
       it 'returns true' do
-        is_expected.to be_truthy
+        is_expected.to be true
+      end
+    end
+
+    context 'when ref is nil' do
+      let(:ref) { nil }
+
+      it 'returns false' do
+        is_expected.to be false
+      end
+    end
+
+    context 'when ref is ref name' do
+      context 'when ref is ambiguous' do
+        let(:ref) { 'ref' }
+
+        before do
+          project.repository.add_branch(project.creator, 'ref', 'master')
+          project.repository.add_tag(project.creator, 'ref', 'master')
+        end
+
+        it 'raises an error' do
+          expect { subject }.to raise_error(Repository::AmbiguousRefError)
+        end
+      end
+
+      context 'when the ref is not protected' do
+        let(:ref) { 'master' }
+
+        it_behaves_like 'ref is not protected'
+      end
+
+      context 'when the ref is a protected branch' do
+        let(:ref) { 'master' }
+
+        it_behaves_like 'ref is protected branch'
+      end
+
+      context 'when the ref is a protected tag' do
+        let(:ref) { 'v1.0.0' }
+
+        it_behaves_like 'ref is protected tag'
+      end
+
+      context 'when ref does not exist' do
+        let(:ref) { 'something' }
+
+        it 'returns false' do
+          is_expected.to be false
+        end
+      end
+    end
+
+    context 'when ref is full ref' do
+      context 'when the ref is not protected' do
+        let(:ref) { 'refs/heads/master' }
+
+        it_behaves_like 'ref is not protected'
+      end
+
+      context 'when the ref is a protected branch' do
+        let(:ref) { 'refs/heads/master' }
+
+        it_behaves_like 'ref is protected branch'
+      end
+
+      context 'when the ref is a protected tag' do
+        let(:ref) { 'refs/tags/v1.0.0' }
+
+        it_behaves_like 'ref is protected tag'
+      end
+
+      context 'when branch ref name is a full tag ref' do
+        let(:ref) { 'refs/tags/something' }
+
+        before do
+          project.repository.add_branch(project.creator, ref, 'master')
+        end
+
+        context 'when ref is not protected' do
+          it 'returns false' do
+            is_expected.to be false
+          end
+        end
+
+        context 'when ref is a protected branch' do
+          before do
+            create(:protected_branch, name: 'refs/tags/something', project: project)
+          end
+
+          it 'returns true' do
+            is_expected.to be true
+          end
+        end
+      end
+
+      context 'when ref does not exist' do
+        let(:ref) { 'refs/heads/something' }
+
+        it 'returns false' do
+          is_expected.to be false
+        end
       end
     end
   end
@@ -2844,7 +2945,7 @@ describe Project do
 
     it 'shows full error updating an invalid MR' do
       error_message = 'Failed to replace merge_requests because one or more of the new records could not be saved.'\
-                      ' Validate fork Source project is not a fork of the target project'
+        ' Validate fork Source project is not a fork of the target project'
 
       expect { project.append_or_update_attribute(:merge_requests, [create(:merge_request)]) }
         .to raise_error(ActiveRecord::RecordNotSaved, error_message)

spec/models/repository_spec.rb

@@ -1005,6 +1005,67 @@ describe Repository do
     end
   end
 
+  describe '#ambiguous_ref?' do
+    let(:ref) { 'ref' }
+
+    subject { repository.ambiguous_ref?(ref) }
+
+    context 'when ref is ambiguous' do
+      before do
+        repository.add_tag(project.creator, ref, 'master')
+        repository.add_branch(project.creator, ref, 'master')
+      end
+
+      it 'should be true' do
+        is_expected.to eq(true)
+      end
+    end
+
+    context 'when ref is not ambiguous' do
+      before do
+        repository.add_tag(project.creator, ref, 'master')
+      end
+
+      it 'should be false' do
+        is_expected.to eq(false)
+      end
+    end
+  end
+
+  describe '#expand_ref' do
+    let(:ref) { 'ref' }
+
+    subject { repository.expand_ref(ref) }
+
+    context 'when ref is not tag or branch name' do
+      let(:ref) { 'refs/heads/master' }
+
+      it 'returns nil' do
+        is_expected.to eq(nil)
+      end
+    end
+
+    context 'when ref is tag name' do
+      before do
+        repository.add_tag(project.creator, ref, 'master')
+      end
+
+      it 'returns the tag ref' do
+        is_expected.to eq("refs/tags/#{ref}")
+      end
+    end
+
+    context 'when ref is branch name' do
+      before do
+        repository.add_branch(project.creator, ref, 'master')
+      end
+
+      it 'returns the branch ref' do
+        is_expected.to eq("refs/heads/#{ref}")
+      end
+    end
+  end
+
   describe '#add_branch' do
     let(:branch_name) { 'new_feature' }
     let(:target) { 'master' }

spec/services/ci/create_pipeline_service_spec.rb

@@ -667,7 +667,7 @@ describe Ci::CreatePipelineService do
         stub_ci_pipeline_yaml_file(YAML.dump(config))
       end
 
-      let(:ref_name) { 'feature' }
+      let(:ref_name) { 'refs/heads/feature' }
 
       context 'when source is merge request' do
         let(:source) { :merge_request }
@@ -696,7 +696,7 @@ describe Ci::CreatePipelineService do
             let(:merge_request) do
               create(:merge_request,
                 source_project: project,
-                source_branch: ref_name,
+                source_branch: Gitlab::Git.ref_name(ref_name),
                 target_project: project,
                 target_branch: 'master')
             end
@@ -709,7 +709,7 @@ describe Ci::CreatePipelineService do
             end
 
             context 'when ref is tag' do
-              let(:ref_name) { 'v1.1.0' }
+              let(:ref_name) { 'refs/tags/v1.1.0' }
 
               it 'does not create a merge request pipeline' do
                 expect(pipeline).not_to be_persisted
@@ -721,7 +721,7 @@ describe Ci::CreatePipelineService do
               let(:merge_request) do
                 create(:merge_request,
                   source_project: project,
-                  source_branch: ref_name,
+                  source_branch: Gitlab::Git.ref_name(ref_name),
                   target_project: target_project,
                   target_branch: 'master')
               end
@@ -786,7 +786,7 @@ describe Ci::CreatePipelineService do
             let(:merge_request) do
               create(:merge_request,
                 source_project: project,
-                source_branch: ref_name,
+                source_branch: Gitlab::Git.ref_name(ref_name),
                 target_project: project,
                 target_branch: 'master')
             end
@@ -928,7 +928,7 @@ describe Ci::CreatePipelineService do
             let(:merge_request) do
               create(:merge_request,
                 source_project: project,
-                source_branch: ref_name,
+                source_branch: Gitlab::Git.ref_name(ref_name),
                 target_project: project,
                 target_branch: 'master')
             end