Commit 8a833c72 authored by Kamil Trzciński's avatar Kamil Trzciński Committed by Sean McGivern

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
parent 7926384f
---
title: Allow to use untrusted Regexp via feature flag
merge_request: 26905
author:
type: deprecated
......@@ -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.
......
......@@ -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
......
......@@ -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
......
......@@ -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)
......
......@@ -6,7 +6,7 @@ module Gitlab
# and converts that to RE2 representation:
# /<regexp>/<flags>
class RubySyntax
PATTERN = %r{^/(?<regexp>.+)/(?<flags>[ismU]*)$}.freeze
PATTERN = %r{^/(?<regexp>.*)/(?<flags>[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)
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')
UntrustedRegexp.new(expression, multiline: false)
Regexp.new(pattern, options)
end
private_class_method :create_ruby_regexp
end
end
end
......@@ -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
......
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' } }
......
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,6 +48,7 @@ describe Gitlab::UntrustedRegexp::RubySyntax do
end
describe '.fabricate!' do
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')
......@@ -61,6 +69,44 @@ describe Gitlab::UntrustedRegexp::RubySyntax do
expect(regexp).to eq Gitlab::UntrustedRegexp.new('something')
end
end
end
context 'when unsafe regexp is used' do
include StubFeatureFlags
before do
stub_feature_flags(allow_unsafe_ruby_regexp: true)
allow(Gitlab::UntrustedRegexp).to receive(:new).and_raise(RegexpError)
end
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 Regexp.new('something')
end
end
end
context 'when regexp is a raw pattern' do
it 'raises an error' do
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment