Merge branch '41293-fix-command-injection-vulnerability-on-system_hook_push-queue-through-web-hook' into 'security-10-3'

Don't allow line breaks on HTTP headers

See merge request gitlab/gitlabhq!2277

(cherry picked from commit 7fc0a6fc096768a5604d6dd24d7d952e53300c82)

073b8f9c Don't allow line breaks on HTTP headers

5 files changed, 28 insertions, 1 deletions

diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb
index 5a70e114f56..27729deeac9 100644
--- a/app/models/hooks/web_hook.rb
+++ b/app/models/hooks/web_hook.rb
@@ -4,6 +4,7 @@ class WebHook < ActiveRecord::Base
   has_many :web_hook_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
   validates :url, presence: true, url: true
+  validates :token, format: { without: /\n/ }
 
   def execute(data, hook_name)
     WebHookService.new(self, data, hook_name).execute
diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb
index 6ebc7c89500..36e589d5aa8 100644
--- a/app/services/web_hook_service.rb
+++ b/app/services/web_hook_service.rb
@@ -113,7 +113,7 @@ class WebHookService
         'Content-Type' => 'application/json',
         'X-Gitlab-Event' => hook_name.singularize.titleize
       }.tap do |hash|
-        hash['X-Gitlab-Token'] = hook.token if hook.token.present?
+        hash['X-Gitlab-Token'] = Gitlab::Utils.remove_line_breaks(hook.token) if hook.token.present?
       end
     end
   end
diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb
index b3baaf036d8..fa22f0e37b2 100644
--- a/lib/gitlab/utils.rb
+++ b/lib/gitlab/utils.rb
@@ -27,6 +27,10 @@ module Gitlab
         .gsub(/(\A-+|-+\z)/, '')
     end
 
+    def remove_line_breaks(str)
+      str.gsub(/\r?\n/, '')
+    end
+
     def to_boolean(value)
       return value if [true, false].include?(value)
       return true if value =~ /^(true|t|yes|y|1|on)$/i
diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb
index e872a5290c5..bda239b7871 100644
--- a/spec/lib/gitlab/utils_spec.rb
+++ b/spec/lib/gitlab/utils_spec.rb
@@ -17,6 +17,22 @@ describe Gitlab::Utils do
     end
   end
 
+  describe '.remove_line_breaks' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:original, :expected) do
+      "foo\nbar\nbaz"     | "foobarbaz"
+      "foo\r\nbar\r\nbaz" | "foobarbaz"
+      "foobar"            | "foobar"
+    end
+
+    with_them do
+      it "replace line breaks with an empty string" do
+        expect(described_class.remove_line_breaks(original)).to eq(expected)
+      end
+    end
+  end
+
   describe '.to_boolean' do
     it 'accepts booleans' do
       expect(to_boolean(true)).to be(true)
diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb
index 388120160ab..ea6d6e53ef5 100644
--- a/spec/models/hooks/web_hook_spec.rb
+++ b/spec/models/hooks/web_hook_spec.rb
@@ -29,6 +29,12 @@ describe WebHook do
         expect(hook.url).to eq('https://example.com')
       end
     end
+
+    describe 'token' do
+      it { is_expected.to allow_value("foobar").for(:token) }
+
+      it { is_expected.not_to allow_values("foo\nbar", "foo\r\nbar").for(:token) }
+    end
   end
 
   describe 'execute' do