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

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

See merge request gitlab/gitlabhq!2270

(cherry picked from commit c32d0c6807dfd41d7838a35742e6d0986871b389)

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

changelogs/unreleased/security-10-3.yml

+---
+title: Fix path traversal in gitlab-ci.yml cache:key
+merge_request:
+author:
+type: security

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.
 ---

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

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