Merge branch 'jej/safe-regex-for-push-rules-10-5' into 'security-10-5-ee'

[10.5] Prevent new push rules from using non-RE2 regexes

See merge request gitlab/gitlab-ee!598

(cherry picked from commit 1ff01e35539dd064402cc4631cee5e5eec8c9047)

46052c72 Prevent new push rules from using non-RE2 regexes

app/validators/untrusted_regexp_validator.rb

+class UntrustedRegexpValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    return unless value
+
+    Gitlab::UntrustedRegexp.new(value)
+
+  rescue RegexpError => e
+    record.errors.add(attribute, "not valid RE2 syntax: #{e.message}")
+  end
+end

changelogs/unreleased-ee/jej-safe-regex-for-push-rules.yml

+---
+title: Prevent new push rules from using non-RE2 regexes
+merge_request:
+author:
+type: security

db/migrate/20171222151344_add_regexp_uses_re2_to_push_rules.rb

+class AddRegexpUsesRe2ToPushRules < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    # Default value to true for new values while keeping NULL for existing ones
+    add_column :push_rules, :regexp_uses_re2, :boolean
+    change_column_default :push_rules, :regexp_uses_re2, true
+  end
+
+  def down
+    remove_column :push_rules, :regexp_uses_re2
+  end
+end

db/schema.rb

@@ -2031,6 +2031,7 @@ ActiveRecord::Schema.define(version: 20180301084653) do
     t.string "branch_name_regex"
     t.boolean "reject_unsigned_commits"
     t.boolean "commit_committer_check"
+    t.boolean "regexp_uses_re2", default: true
   end
 
   add_index "push_rules", ["is_sample"], name: "index_push_rules_on_is_sample", where: "is_sample", using: :btree

doc/push_rules/push_rules.md

@@ -66,14 +66,14 @@ The following options are available.
 | Check whether committer is the current authenticated user | **Premium** 10.2 | GitLab will reject any commit that was not committed by the current authenticated user |
 | Check whether commit is signed through GPG | **Premium** 10.1 | Reject commit when it is not signed through GPG. Read [signing commits with GPG][signing-commits]. |
 | Prevent committing secrets to Git | **Starter** 8.12 | GitLab will reject any files that are likely to contain secrets. Read [what files are forbidden](#prevent-pushing-secrets-to-the-repository). |
-| Restrict by commit message | **Starter** 7.10 | Only commit messages that match this Ruby regular expression are allowed to be pushed. Leave empty to allow any commit message. |
-| Restrict by branch name | **Starter** 9.3 | Only branch names that match this Ruby regular expression are allowed to be pushed. Leave empty to allow any branch name. |
-| Restrict by commit author's email | **Starter** 7.10 | Only commit author's email that match this Ruby regular expression are allowed to be pushed. Leave empty to allow any email. |
-| Prohibited file names | **Starter** 7.10 | Any committed filenames that match this Ruby regular expression are not allowed to be pushed. Leave empty to allow any filenames. |
+| Restrict by commit message | **Starter** 7.10 | Only commit messages that match this regular expression are allowed to be pushed. Leave empty to allow any commit message. Uses multiline mode, which can be disabled using `(?-m)`. |
+| Restrict by branch name | **Starter** 9.3 | Only branch names that match this regular expression are allowed to be pushed. Leave empty to allow any branch name. |
+| Restrict by commit author's email | **Starter** 7.10 | Only commit author's email that match this regular expression are allowed to be pushed. Leave empty to allow any email. |
+| Prohibited file names | **Starter** 7.10 | Any committed filenames that match this regular expression are not allowed to be pushed. Leave empty to allow any filenames. |
 | Maximum file size | **Starter** 7.12 | Pushes that contain added or updated files that exceed this file size (in MB) are rejected. Set to 0 to allow files of any size. |
 
 >**Tip:**
-You can check your regular expressions at <http://rubular.com>.
+GitLab uses [RE2 syntax](https://github.com/google/re2/wiki/Syntax) for regular expressions in push rules. You can check your regular expressions at <https://regex-golang.appspot.com>.
 
 ## Prevent pushing secrets to the repository
 

ee/app/models/push_rule.rb

 class PushRule < ActiveRecord::Base
   MatchError = Class.new(StandardError)
 
+  REGEX_COLUMNS = %i[
+    force_push_regex
+    delete_branch_regex
+    commit_message_regex
+    author_email_regex
+    file_name_regex
+    branch_name_regex
+  ].freeze
+
   belongs_to :project
 
   validates :project, presence: true, unless: "is_sample?"
   validates :max_file_size, numericality: { greater_than_or_equal_to: 0, only_integer: true }
+  validates(*REGEX_COLUMNS, untrusted_regexp: true)
+
+  before_update :convert_to_re2
 
   FILES_BLACKLIST = YAML.load_file(Rails.root.join('ee/lib/gitlab/checks/files_blacklist.yml'))
   SETTINGS_WITH_GLOBAL_DEFAULT = %i[
@@ -43,7 +55,7 @@ class PushRule < ActiveRecord::Base
   end
 
   def commit_message_allowed?(message)
-    data_match?(message, commit_message_regex)
+    data_match?(message, commit_message_regex, multiline: true)
   end
 
   def branch_name_allowed?(branch)
@@ -94,9 +106,15 @@ class PushRule < ActiveRecord::Base
 
   private
 
-  def data_match?(data, regex)
+  def data_match?(data, regex, multiline: false)
     if regex.present?
-      !!(data =~ Regexp.new(regex))
+      regexp = if allow_regex_fallback?
+                 Gitlab::UntrustedRegexp.with_fallback(regex, multiline: multiline)
+               else
+                 Gitlab::UntrustedRegexp.new(regex, multiline: multiline)
+               end
+
+      regexp === data
     else
       true
     end
@@ -104,6 +122,16 @@ class PushRule < ActiveRecord::Base
     raise MatchError, "Regular expression '#{regex}' is invalid: #{e.message}"
   end
 
+  def convert_to_re2
+    self.regexp_uses_re2 = true
+  end
+
+  # Allow fallback to ruby regex library
+  # Only supported for existing regexes due to denial of service risk
+  def allow_regex_fallback?
+    !regexp_uses_re2?
+  end
+
   def read_setting_with_global_default(setting)
     value = read_attribute(setting)
 

ee/spec/models/push_rule_spec.rb

@@ -13,6 +13,71 @@ describe PushRule do
   describe "Validation" do
     it { is_expected.to validate_presence_of(:project) }
     it { is_expected.to validate_numericality_of(:max_file_size).is_greater_than_or_equal_to(0).only_integer }
+
+    it 'validates RE2 regex syntax' do
+      push_rule = build(:push_rule, branch_name_regex: '(ee|ce).*\1')
+
+      expect(push_rule).not_to be_valid
+      expect(push_rule.errors.full_messages.join).to match /invalid escape sequence/
+    end
+  end
+
+  it 'defaults regexp_uses_re2 to true' do
+    push_rule = create(:push_rule)
+
+    expect(push_rule.regexp_uses_re2).to eq(true)
+  end
+
+  it 'updates regexp_uses_re2 to true on edit' do
+    push_rule = create(:push_rule, regexp_uses_re2: nil)
+
+    expect do
+      push_rule.update!(branch_name_regex: '.*')
+    end.to change(push_rule, :regexp_uses_re2).to true
+  end
+
+  describe '#branch_name_allowed?' do
+    subject(:push_rule) { create(:push_rule, branch_name_regex: '\d+\-.*')}
+
+    it 'checks branch against regex' do
+      expect(subject.branch_name_allowed?('123-feature')).to be true
+      expect(subject.branch_name_allowed?('feature-123')).to be false
+    end
+
+    it 'uses RE2 regex engine' do
+      expect_any_instance_of(Gitlab::UntrustedRegexp).to receive(:===)
+
+      subject.branch_name_allowed?('123-feature')
+    end
+
+    context 'with legacy regex' do
+      before do
+        push_rule.update_column(:regexp_uses_re2, nil)
+      end
+
+      it 'attempts to use safe RE2 regex engine' do
+        expect_any_instance_of(Gitlab::UntrustedRegexp).to receive(:===)
+
+        subject.branch_name_allowed?('ee-feature-ee')
+      end
+
+      it 'falls back to ruby regex engine' do
+        push_rule.update_column(:branch_name_regex, '(ee|ce).*\1')
+
+        expect(subject.branch_name_allowed?('ee-feature-ee')).to be true
+        expect(subject.branch_name_allowed?('ee-feature-ce')).to be false
+      end
+    end
+  end
+
+  describe '#commit_message_allowed?' do
+    subject(:push_rule) { create(:push_rule, commit_message_regex: '^Signed-off-by')}
+
+    it 'uses multiline regex' do
+      commit_message = "Some git commit feature\n\nSigned-off-by: Someone"
+
+      expect(subject.commit_message_allowed?(commit_message)).to be true
+    end
   end
 
   describe '#commit_validation?' do

lib/gitlab/untrusted_regexp.rb

@@ -11,7 +11,11 @@ module Gitlab
   class UntrustedRegexp
     delegate :===, to: :regexp
 
-    def initialize(pattern)
+    def initialize(pattern, multiline: false)
+      if multiline
+        pattern = "(?m)#{pattern}"
+      end
+
       @regexp = RE2::Regexp.new(pattern, log_errors: false)
 
       raise RegexpError.new(regexp.error) unless regexp.ok?
@@ -31,6 +35,19 @@ module Gitlab
       RE2.Replace(text, regexp, rewrite)
     end
 
+    # Handles regular expressions with the preferred RE2 library where possible
+    # via UntustedRegex. Falls back to Ruby's built-in regular expression library
+    # when the syntax would be invalid in RE2.
+    #
+    # One difference between these is `(?m)` multi-line mode. Ruby regex enables
+    # this by default, but also handles `^` and `$` differently.
+    # See: https://www.regular-expressions.info/modifiers.html
+    def self.with_fallback(pattern, multiline: false)
+      UntrustedRegexp.new(pattern, multiline: multiline)
+    rescue RegexpError
+      Regexp.new(pattern)
+    end
+
     private
 
     attr_reader :regexp

spec/lib/gitlab/untrusted_regexp_spec.rb

@@ -39,6 +39,14 @@ describe Gitlab::UntrustedRegexp do
 
       expect(result).to be_falsy
     end
+
+    it 'can handle regular expressions in multiline mode' do
+      regexp = described_class.new('^\d', multiline: true)
+
+      result = regexp === "Header\n\n1. Content"
+
+      expect(result).to be_truthy
+    end
   end
 
   describe '#scan' do