Merge branch 'security-11-5-secret-ci-variables-exposed' into 'security-11-5'

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

See merge request gitlab/gitlabhq!2682

19 files changed, 364 insertions, 46 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index c89a841d7fc..4b943486aff 100644
--- a/app/models/ci/build.rb
+++ b/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
@@ -642,11 +643,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
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 56010e899a4..9941baa323b 100644
--- a/app/models/ci/pipeline.rb
+++ b/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: :pipelines
     belongs_to :user
@@ -374,10 +375,6 @@ module Ci
       @commit ||= Commit.lazy(project, sha)
     end
 
-    def branch?
-      !tag?
-    end
-
     def stuck?
       pending_builds.any?(&:stuck?)
     end
@@ -577,7 +574,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
@@ -697,16 +694,6 @@ module Ci
       end
     end
 
-    def git_ref
-      if branch?
-        Gitlab::Git::BRANCH_REF_PREFIX + ref.to_s
-      elsif tag?
-        Gitlab::Git::TAG_REF_PREFIX + ref.to_s
-      else
-        raise ArgumentError, 'Invalid pipeline type!'
-      end
-    end
-
     def latest_builds_status
       return 'failed' unless yaml_errors.blank?
 
diff --git a/app/models/concerns/has_ref.rb b/app/models/concerns/has_ref.rb
new file mode 100644
index 00000000000..d7089294efc
--- /dev/null
+++ b/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
diff --git a/app/models/project.rb b/app/models/project.rb
index 8030204d0c6..8b060f9fa1f 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1817,10 +1817,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
 
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 6e179f61a7b..cf04308983b 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -26,6 +26,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.
   #
@@ -176,6 +177,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)
 
diff --git a/changelogs/unreleased/security-11-5-secret-ci-variables-exposed.yml b/changelogs/unreleased/security-11-5-secret-ci-variables-exposed.yml
new file mode 100644
index 00000000000..702181065f5
--- /dev/null
+++ b/changelogs/unreleased/security-11-5-secret-ci-variables-exposed.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent leaking protected variables for ambiguous refs.
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb
index 05978804d92..0635afed114 100644
--- a/lib/gitlab/ci/pipeline/chain/command.rb
+++ b/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
diff --git a/lib/gitlab/ci/pipeline/chain/validate/repository.rb b/lib/gitlab/ci/pipeline/chain/validate/repository.rb
index d88851d8245..9c6c2bc8e25 100644
--- a/lib/gitlab/ci/pipeline/chain/validate/repository.rb
+++ b/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?
diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb
index c4aac228b2f..44a62586a23 100644
--- a/lib/gitlab/git.rb
+++ b/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)
diff --git a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb
index 75a177d2d1f..6aa802ce6fd 100644
--- a/spec/lib/gitlab/ci/pipeline/chain/command_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb
index 284aed91e29..1b014ecfaa4 100644
--- a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb
+++ b/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
 
diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb
index fb1b53fc55c..a7cad423d09 100644
--- a/spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb
+++ b/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(
diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb
index fffa727c2ed..2cf812b26dc 100644
--- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb
+++ b/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',
diff --git a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb
index 05ce3412fd8..82f741845db 100644
--- a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb
+++ b/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',
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 6849bc6db7a..1874ff17820 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -2275,6 +2275,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
@@ -2287,7 +2289,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) }
@@ -2295,7 +2297,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) }
@@ -2320,6 +2322,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
@@ -2332,7 +2336,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) }
@@ -2340,7 +2344,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) }
@@ -2615,7 +2619,7 @@ describe Ci::Build do
 
           allow_any_instance_of(Project)
             .to receive(:ci_variables_for)
-            .with(ref: 'master', environment: nil) do
+            .with(ref: 'refs/heads/master', environment: nil) do
             [create(:ci_variable, key: 'secret', value: 'value')]
           end
 
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 9e6146b8a44..4f8b5b76821 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -227,6 +227,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
diff --git a/spec/models/concerns/has_ref_spec.rb b/spec/models/concerns/has_ref_spec.rb
new file mode 100644
index 00000000000..8aed72d77a4
--- /dev/null
+++ b/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
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 4b71e211903..b6ea32ec42e 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2482,6 +2482,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
@@ -2521,42 +2525,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
@@ -2765,7 +2866,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)
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 799a60ac62f..fbf8f330d6d 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1066,6 +1066,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' }