diff options
author | Robert Speicher <robert@gitlab.com> | 2018-01-08 17:51:00 +0000 |
---|---|---|
committer | Tiago Botelho <tiago@gitlab.com> | 2018-01-08 20:25:18 +0000 |
commit | fd2813650b3e9b8b85596b8640fbf2bdc7496dfa (patch) | |
tree | 95d6e5dc4d9d61c57b40abba755b3f706a5939b3 | |
parent | 62d41f9229f0e2b729274af11020634d45dc90a0 (diff) | |
download | gitlab-ce-fd2813650b3e9b8b85596b8640fbf2bdc7496dfa.tar.gz |
Merge branch '41293-fix-command-injection-vulnerability-on-system_hook_push-queue-through-web-hook-10-1' into 'security-10-1'
[10.1] Don't allow line breaks on HTTP headers
See merge request gitlab/gitlabhq!2286
(cherry picked from commit 271ef222fa964481379a14a9c07805621a7d52a6)
a30812d3 Don't allow line breaks on HTTP headers
-rw-r--r-- | app/models/hooks/web_hook.rb | 1 | ||||
-rw-r--r-- | app/services/web_hook_service.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/utils.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/utils_spec.rb | 16 | ||||
-rw-r--r-- | spec/models/hooks/web_hook_spec.rb | 6 |
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 cd99e0b90f9..a3c90e4f908 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 abb3d3a02c3..3cb113373d2 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 3137a72fdc4..d6eea28d53e 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 |