From 8a833c720e91c7b4d764e85c30e3be18ee5221fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Thu, 4 Apr 2019 15:00:56 +0000 Subject: Allow to use untrusted Regexp via feature flag This brings support for untrusted regexp for 'only:refs:' when enabled via feature flag: alllow_unsafe_ruby_regexp. This is by default disabled, and should not be used in production --- .../allow-to-use-untrusted-ruby-syntax.yml | 5 ++ doc/ci/yaml/README.md | 21 +++++++ lib/gitlab/ci/build/policy/refs.rb | 2 +- lib/gitlab/ci/config/entry/policy.rb | 4 +- lib/gitlab/config/entry/validators.rb | 16 ++++- lib/gitlab/untrusted_regexp/ruby_syntax.rb | 39 +++++++++---- spec/lib/gitlab/ci/build/policy/refs_spec.rb | 26 +++++++++ spec/lib/gitlab/ci/config/entry/policy_spec.rb | 67 +++++++++++++++++++++ .../gitlab/untrusted_regexp/ruby_syntax_spec.rb | 68 ++++++++++++++++++---- 9 files changed, 223 insertions(+), 25 deletions(-) create mode 100644 changelogs/unreleased/allow-to-use-untrusted-ruby-syntax.yml diff --git a/changelogs/unreleased/allow-to-use-untrusted-ruby-syntax.yml b/changelogs/unreleased/allow-to-use-untrusted-ruby-syntax.yml new file mode 100644 index 00000000000..731c9c10b00 --- /dev/null +++ b/changelogs/unreleased/allow-to-use-untrusted-ruby-syntax.yml @@ -0,0 +1,5 @@ +--- +title: Allow to use untrusted Regexp via feature flag +merge_request: 26905 +author: +type: deprecated diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index e75f7050a09..686d36c50ee 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -414,6 +414,27 @@ job: only: ['branches', 'tags'] ``` +### Supported `only`/`except` regexp syntax + +CAUTION: **Warning:** +This is a breaking change that was introduced with GitLab 11.9.4. + +In GitLab 11.9.4, GitLab begun internally converting regexp used +in `only` and `except` parameters to [RE2](https://github.com/google/re2/wiki/Syntax). + +This means that only subset of features provided by [Ruby Regexp](https://ruby-doc.org/core/Regexp.html) +is supported. [RE2](https://github.com/google/re2/wiki/Syntax) limits the set of features +provided due to computational complexity, which means some features became unavailable in GitLab 11.9.4. +For example, negative lookaheads. + +For GitLab versions from 11.9.7 and up to GitLab 12.0, GitLab provides a feature flag that can be +enabled by administrators that allows users to use unsafe regexp syntax. This brings compatibility +with previously allowed syntax version and allows users to gracefully migrate to the new syntax. + +```ruby +Feature.enable(:allow_unsafe_ruby_regexp) +``` + ### `only`/`except` (advanced) > - `refs` and `kubernetes` policies introduced in GitLab 10.0. diff --git a/lib/gitlab/ci/build/policy/refs.rb b/lib/gitlab/ci/build/policy/refs.rb index 360424bec11..c3005303fd8 100644 --- a/lib/gitlab/ci/build/policy/refs.rb +++ b/lib/gitlab/ci/build/policy/refs.rb @@ -35,7 +35,7 @@ module Gitlab # patterns can be matched only when branch or tag is used # the pattern matching does not work for merge requests pipelines if pipeline.branch? || pipeline.tag? - if regexp = Gitlab::UntrustedRegexp::RubySyntax.fabricate(pattern) + if regexp = Gitlab::UntrustedRegexp::RubySyntax.fabricate(pattern, fallback: true) regexp.match?(pipeline.ref) else pattern == pipeline.ref diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb index adc3660d950..7b14218d3ea 100644 --- a/lib/gitlab/ci/config/entry/policy.rb +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -17,7 +17,7 @@ module Gitlab include ::Gitlab::Config::Entry::Validatable validations do - validates :config, array_of_strings_or_regexps: true + validates :config, array_of_strings_or_regexps_with_fallback: true end def value @@ -38,7 +38,7 @@ module Gitlab validate :variables_expressions_syntax with_options allow_nil: true do - validates :refs, array_of_strings_or_regexps: true + validates :refs, array_of_strings_or_regexps_with_fallback: true validates :kubernetes, allowed_values: %w[active] validates :variables, array_of_strings: true validates :changes, array_of_strings: true diff --git a/lib/gitlab/config/entry/validators.rb b/lib/gitlab/config/entry/validators.rb index d0ee94370ba..746fe83f90f 100644 --- a/lib/gitlab/config/entry/validators.rb +++ b/lib/gitlab/config/entry/validators.rb @@ -129,6 +129,12 @@ module Gitlab end end + protected + + def fallback + false + end + private def matches_syntax?(value) @@ -137,7 +143,7 @@ module Gitlab def validate_regexp(value) matches_syntax?(value) && - Gitlab::UntrustedRegexp::RubySyntax.valid?(value) + Gitlab::UntrustedRegexp::RubySyntax.valid?(value, fallback: fallback) end end @@ -162,6 +168,14 @@ module Gitlab end end + class ArrayOfStringsOrRegexpsWithFallbackValidator < ArrayOfStringsOrRegexpsValidator + protected + + def fallback + true + end + end + class ArrayOfStringsOrStringValidator < RegexpValidator def validate_each(record, attribute, value) unless validate_array_of_strings_or_string(value) diff --git a/lib/gitlab/untrusted_regexp/ruby_syntax.rb b/lib/gitlab/untrusted_regexp/ruby_syntax.rb index 91f300f97d0..6adf119aa75 100644 --- a/lib/gitlab/untrusted_regexp/ruby_syntax.rb +++ b/lib/gitlab/untrusted_regexp/ruby_syntax.rb @@ -6,7 +6,7 @@ module Gitlab # and converts that to RE2 representation: # // class RubySyntax - PATTERN = %r{^/(?.+)/(?[ismU]*)$}.freeze + PATTERN = %r{^/(?.*)/(?[ismU]*)$}.freeze # Checks if pattern matches a regexp pattern # but does not enforce it's validity @@ -16,28 +16,47 @@ module Gitlab # The regexp can match the pattern `/.../`, but may not be fabricatable: # it can be invalid or incomplete: `/match ( string/` - def self.valid?(pattern) - !!self.fabricate(pattern) + def self.valid?(pattern, fallback: false) + !!self.fabricate(pattern, fallback: fallback) end - def self.fabricate(pattern) - self.fabricate!(pattern) + def self.fabricate(pattern, fallback: false) + self.fabricate!(pattern, fallback: fallback) rescue RegexpError nil end - def self.fabricate!(pattern) + def self.fabricate!(pattern, fallback: false) raise RegexpError, 'Pattern is not string!' unless pattern.is_a?(String) matches = pattern.match(PATTERN) raise RegexpError, 'Invalid regular expression!' if matches.nil? - expression = matches[:regexp] - flags = matches[:flags] - expression.prepend("(?#{flags})") if flags.present? + begin + create_untrusted_regexp(matches[:regexp], matches[:flags]) + rescue RegexpError + raise unless fallback && + Feature.enabled?(:allow_unsafe_ruby_regexp, default_enabled: false) - UntrustedRegexp.new(expression, multiline: false) + create_ruby_regexp(matches[:regexp], matches[:flags]) + end end + + def self.create_untrusted_regexp(pattern, flags) + pattern.prepend("(?#{flags})") if flags.present? + + UntrustedRegexp.new(pattern, multiline: false) + end + private_class_method :create_untrusted_regexp + + def self.create_ruby_regexp(pattern, flags) + options = 0 + options += Regexp::IGNORECASE if flags&.include?('i') + options += Regexp::MULTILINE if flags&.include?('m') + + Regexp.new(pattern, options) + end + private_class_method :create_ruby_regexp end end end diff --git a/spec/lib/gitlab/ci/build/policy/refs_spec.rb b/spec/lib/gitlab/ci/build/policy/refs_spec.rb index ec0450643c3..22ca681cfd3 100644 --- a/spec/lib/gitlab/ci/build/policy/refs_spec.rb +++ b/spec/lib/gitlab/ci/build/policy/refs_spec.rb @@ -101,6 +101,32 @@ describe Gitlab::Ci::Build::Policy::Refs do expect(described_class.new(['/fix-.*/'])) .not_to be_satisfied_by(pipeline) end + + context 'when unsafe regexp is used' do + let(:subject) { described_class.new(['/^(?!master).+/']) } + + context 'when allow_unsafe_ruby_regexp is disabled' do + before do + stub_feature_flags(allow_unsafe_ruby_regexp: false) + end + + it 'ignores invalid regexp' do + expect(subject) + .not_to be_satisfied_by(pipeline) + end + end + + context 'when allow_unsafe_ruby_regexp is enabled' do + before do + stub_feature_flags(allow_unsafe_ruby_regexp: true) + end + + it 'is satisfied by regexp' do + expect(subject) + .to be_satisfied_by(pipeline) + end + end + end end context 'malicious regexp' do diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb index 1c987e13a9a..fba5671594d 100644 --- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb @@ -1,4 +1,5 @@ require 'fast_spec_helper' +require 'support/helpers/stub_feature_flags' require_dependency 'active_model' describe Gitlab::Ci::Config::Entry::Policy do @@ -33,6 +34,44 @@ describe Gitlab::Ci::Config::Entry::Policy do end end + context 'when config is an empty regexp' do + let(:config) { ['//'] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + + context 'when using unsafe regexp' do + include StubFeatureFlags + + let(:config) { ['/^(?!master).+/'] } + + subject { described_class.new([regexp]) } + + context 'when allow_unsafe_ruby_regexp is disabled' do + before do + stub_feature_flags(allow_unsafe_ruby_regexp: false) + end + + it 'is not valid' do + expect(entry).not_to be_valid + end + end + + context 'when allow_unsafe_ruby_regexp is enabled' do + before do + stub_feature_flags(allow_unsafe_ruby_regexp: true) + end + + it 'is valid' do + expect(entry).to be_valid + end + end + end + context 'when config is a special keyword' do let(:config) { %w[tags triggers branches] } @@ -67,6 +106,34 @@ describe Gitlab::Ci::Config::Entry::Policy do end end + context 'when using unsafe regexp' do + include StubFeatureFlags + + let(:config) { { refs: ['/^(?!master).+/'] } } + + subject { described_class.new([regexp]) } + + context 'when allow_unsafe_ruby_regexp is disabled' do + before do + stub_feature_flags(allow_unsafe_ruby_regexp: false) + end + + it 'is not valid' do + expect(entry).not_to be_valid + end + end + + context 'when allow_unsafe_ruby_regexp is enabled' do + before do + stub_feature_flags(allow_unsafe_ruby_regexp: true) + end + + it 'is valid' do + expect(entry).to be_valid + end + end + end + context 'when specifying kubernetes policy' do let(:config) { { kubernetes: 'active' } } diff --git a/spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb b/spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb index 005d41580de..f1882e03581 100644 --- a/spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb +++ b/spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb @@ -1,5 +1,6 @@ require 'fast_spec_helper' require 'support/shared_examples/malicious_regexp_shared_examples' +require 'support/helpers/stub_feature_flags' describe Gitlab::UntrustedRegexp::RubySyntax do describe '.matches_syntax?' do @@ -33,6 +34,12 @@ describe Gitlab::UntrustedRegexp::RubySyntax do end end + context 'when regexp is empty' do + it 'fabricates regexp correctly' do + expect(described_class.fabricate('//')).not_to be_nil + end + end + context 'when regexp is a raw pattern' do it 'returns error' do expect(described_class.fabricate('some .* thing')).to be_nil @@ -41,24 +48,63 @@ describe Gitlab::UntrustedRegexp::RubySyntax do end describe '.fabricate!' do - context 'when regexp is using /regexp/ scheme with flags' do - it 'fabricates regexp with a single flag' do - regexp = described_class.fabricate!('/something/i') + context 'safe regexp is used' do + context 'when regexp is using /regexp/ scheme with flags' do + it 'fabricates regexp with a single flag' do + regexp = described_class.fabricate!('/something/i') + + expect(regexp).to eq Gitlab::UntrustedRegexp.new('(?i)something') + expect(regexp.scan('SOMETHING')).to be_one + end - expect(regexp).to eq Gitlab::UntrustedRegexp.new('(?i)something') - expect(regexp.scan('SOMETHING')).to be_one + it 'fabricates regexp with multiple flags' do + regexp = described_class.fabricate!('/something/im') + + expect(regexp).to eq Gitlab::UntrustedRegexp.new('(?im)something') + end + + it 'fabricates regexp without flags' do + regexp = described_class.fabricate!('/something/') + + expect(regexp).to eq Gitlab::UntrustedRegexp.new('something') + end end + end - it 'fabricates regexp with multiple flags' do - regexp = described_class.fabricate!('/something/im') + context 'when unsafe regexp is used' do + include StubFeatureFlags - expect(regexp).to eq Gitlab::UntrustedRegexp.new('(?im)something') + before do + stub_feature_flags(allow_unsafe_ruby_regexp: true) + + allow(Gitlab::UntrustedRegexp).to receive(:new).and_raise(RegexpError) end - it 'fabricates regexp without flags' do - regexp = described_class.fabricate!('/something/') + context 'when no fallback is enabled' do + it 'raises an exception' do + expect { described_class.fabricate!('/something/') } + .to raise_error(RegexpError) + end + end + + context 'when fallback is used' do + it 'fabricates regexp with a single flag' do + regexp = described_class.fabricate!('/something/i', fallback: true) + + expect(regexp).to eq Regexp.new('something', Regexp::IGNORECASE) + end + + it 'fabricates regexp with multiple flags' do + regexp = described_class.fabricate!('/something/im', fallback: true) + + expect(regexp).to eq Regexp.new('something', Regexp::IGNORECASE | Regexp::MULTILINE) + end + + it 'fabricates regexp without flags' do + regexp = described_class.fabricate!('/something/', fallback: true) - expect(regexp).to eq Gitlab::UntrustedRegexp.new('something') + expect(regexp).to eq Regexp.new('something') + end end end -- cgit v1.2.1