Merge branch 'security-ac/fix-path-traversal-10-2' into 'security-10-2'

[10.2] Fix path traversal in gitlab-ci.yml cache:key

See merge request gitlab/gitlabhq!2271

(cherry picked from commit 9184cd7968665137a18c4823ece239a4a1ca0e46)

1050945a Fix path traversal in gitlab-ci.yml cache:key

4 files changed, 83 insertions, 2 deletions

diff --git a/changelogs/unreleased/security-10-3.yml b/changelogs/unreleased/security-10-3.yml
new file mode 100644
index 00000000000..5c718d976cd
--- /dev/null
+++ b/changelogs/unreleased/security-10-3.yml
@@ -0,0 +1,5 @@
+---
+title: Fix path traversal in gitlab-ci.yml cache:key
+merge_request:
+author:
+type: security
diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index 6ad70707594..bb5b9cb1616 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -258,7 +258,7 @@ The `cache:key` variable can use any of the [predefined variables](../variables/
 The default key is **default** across the project, therefore everything is
 shared between each pipelines and jobs by default, starting from GitLab 9.0.
 
->**Note:** The `cache:key` variable cannot contain the `/` character.
+>**Note:** The `cache:key` variable cannot contain the `/` character, or the equivalent URI encoded `%2F`; a value made only of dots (`.`, `%2E`) is also forbidden.
 
 ---
 
diff --git a/lib/gitlab/ci/config/entry/validators.rb b/lib/gitlab/ci/config/entry/validators.rb
index 0159179f0a9..bfc1d75cc70 100644
--- a/lib/gitlab/ci/config/entry/validators.rb
+++ b/lib/gitlab/ci/config/entry/validators.rb
@@ -64,10 +64,24 @@ module Gitlab
             include LegacyValidationHelpers
 
             def validate_each(record, attribute, value)
-              unless validate_string(value)
+              if validate_string(value)
+                validate_path(record, attribute, value)
+              else
                 record.errors.add(attribute, 'should be a string or symbol')
               end
             end
+
+            private
+
+            def validate_path(record, attribute, value)
+              path = CGI.unescape(value.to_s)
+
+              if path.include?('/')
+                record.errors.add(attribute, 'cannot contain the "/" character')
+              elsif path == '.' || path == '..'
+                record.errors.add(attribute, 'cannot be "." or ".."')
+              end
+            end
           end
 
           class RegexpValidator < ActiveModel::EachValidator
diff --git a/spec/lib/gitlab/ci/config/entry/key_spec.rb b/spec/lib/gitlab/ci/config/entry/key_spec.rb
index 5d4de60bc8a..846f5f44470 100644
--- a/spec/lib/gitlab/ci/config/entry/key_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/key_spec.rb
@@ -4,6 +4,26 @@ describe Gitlab::Ci::Config::Entry::Key do
   let(:entry) { described_class.new(config) }
 
   describe 'validations' do
+    shared_examples 'key with slash' do
+      it 'is invalid' do
+        expect(entry).not_to be_valid
+      end
+
+      it 'reports errors with config value' do
+        expect(entry.errors).to  include 'key config cannot contain the "/" character'
+      end
+    end
+
+    shared_examples 'key with only dots' do
+      it 'is invalid' do
+        expect(entry).not_to be_valid
+      end
+
+      it 'reports errors with config value' do
+        expect(entry.errors).to  include 'key config cannot be "." or ".."'
+      end
+    end
+
     context 'when entry config value is correct' do
       let(:config) { 'test' }
 
@@ -30,6 +50,48 @@ describe Gitlab::Ci::Config::Entry::Key do
         end
       end
     end
+
+    context 'when entry value contains slash' do
+      let(:config) { 'key/with/some/slashes' }
+
+      it_behaves_like 'key with slash'
+    end
+
+    context 'when entry value contains URI encoded slash (%2F)' do
+      let(:config) { 'key%2Fwith%2Fsome%2Fslashes' }
+
+      it_behaves_like 'key with slash'
+    end
+
+    context 'when entry value is a dot' do
+      let(:config) { '.' }
+
+      it_behaves_like 'key with only dots'
+    end
+
+    context 'when entry value is two dots' do
+      let(:config) { '..' }
+
+      it_behaves_like 'key with only dots'
+    end
+
+    context 'when entry value is a URI encoded dot (%2E)' do
+      let(:config) { '%2e' }
+
+      it_behaves_like 'key with only dots'
+    end
+
+    context 'when entry value is two URI encoded dots (%2E)' do
+      let(:config) { '%2E%2e' }
+
+      it_behaves_like 'key with only dots'
+    end
+
+    context 'when entry value is one dot and one URI encoded dot' do
+      let(:config) { '.%2e' }
+
+      it_behaves_like 'key with only dots'
+    end
   end
 
   describe '.default' do