summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2021-03-30 22:40:10 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2021-03-30 22:40:10 +0000
commitb8cacd68a6297f2c6cdd454a3d82a487367f2e70 (patch)
tree093014d689cb2c662f8f3f112791d952263a3b1a
parentb2ce3643e27db4cc0ad30cc09d651c00ec799887 (diff)
downloadgitlab-ce-b8cacd68a6297f2c6cdd454a3d82a487367f2e70.tar.gz
Add latest changes from gitlab-org/security/gitlab@13-10-stable-ee
-rw-r--r--app/models/merge_request.rb4
-rw-r--r--app/models/project.rb2
-rw-r--r--changelogs/unreleased/security-kroki-arbitraryfile-read-write.yml5
-rw-r--r--changelogs/unreleased/security-projects-branch-collaboration-loop.yml5
-rw-r--r--config/initializers/asciidoctor_patch.rb20
-rw-r--r--lib/gitlab/user_access.rb7
-rw-r--r--spec/lib/gitlab/asciidoc_spec.rb43
-rw-r--r--spec/lib/gitlab/user_access_spec.rb9
-rw-r--r--spec/models/project_spec.rb58
-rw-r--r--[-rwxr-xr-x]vendor/gitignore/C++.gitignore0
-rw-r--r--[-rwxr-xr-x]vendor/gitignore/Java.gitignore0
11 files changed, 149 insertions, 4 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index ac50e5c5107..7efdd79ae1c 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1337,8 +1337,8 @@ class MergeRequest < ApplicationRecord
has_no_commits? || branch_missing? || cannot_be_merged?
end
- def can_be_merged_by?(user)
- access = ::Gitlab::UserAccess.new(user, container: project)
+ def can_be_merged_by?(user, skip_collaboration_check: false)
+ access = ::Gitlab::UserAccess.new(user, container: project, skip_collaboration_check: skip_collaboration_check)
access.can_update_branch?(target_branch)
end
diff --git a/app/models/project.rb b/app/models/project.rb
index 274dae8fd65..c52eb95bde8 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -2704,7 +2704,7 @@ class Project < ApplicationRecord
# Issue for N+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/49322
Gitlab::GitalyClient.allow_n_plus_1_calls do
merge_requests_allowing_collaboration(branch_name).any? do |merge_request|
- merge_request.can_be_merged_by?(user)
+ merge_request.can_be_merged_by?(user, skip_collaboration_check: true)
end
end
end
diff --git a/changelogs/unreleased/security-kroki-arbitraryfile-read-write.yml b/changelogs/unreleased/security-kroki-arbitraryfile-read-write.yml
new file mode 100644
index 00000000000..acefc5e6fac
--- /dev/null
+++ b/changelogs/unreleased/security-kroki-arbitraryfile-read-write.yml
@@ -0,0 +1,5 @@
+---
+title: Fix arbitrary read/write in AsciiDoctor and Kroki gems
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-projects-branch-collaboration-loop.yml b/changelogs/unreleased/security-projects-branch-collaboration-loop.yml
new file mode 100644
index 00000000000..607bd37d2f6
--- /dev/null
+++ b/changelogs/unreleased/security-projects-branch-collaboration-loop.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent infinite loop when checking if collaboration is allowed
+merge_request:
+author:
+type: security
diff --git a/config/initializers/asciidoctor_patch.rb b/config/initializers/asciidoctor_patch.rb
new file mode 100644
index 00000000000..b7da50db77c
--- /dev/null
+++ b/config/initializers/asciidoctor_patch.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+# Ensure that locked attributes can not be changed using a counter.
+# TODO: this can be removed once `asciidoctor` gem is > 2.0.12
+# and https://github.com/asciidoctor/asciidoctor/issues/3939 is merged
+module Asciidoctor
+ module DocumentPatch
+ def counter(name, seed = nil)
+ return @parent_document.counter(name, seed) if @parent_document # rubocop: disable Gitlab/ModuleWithInstanceVariables
+
+ unless attribute_locked? name
+ super
+ end
+ end
+ end
+end
+
+class Asciidoctor::Document
+ prepend Asciidoctor::DocumentPatch
+end
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index 0af7ad6ec17..a4a1cccf9d5 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -11,10 +11,11 @@ module Gitlab
attr_reader :user, :push_ability
attr_accessor :container
- def initialize(user, container: nil, push_ability: :push_code)
+ def initialize(user, container: nil, push_ability: :push_code, skip_collaboration_check: false)
@user = user
@container = container
@push_ability = push_ability
+ @skip_collaboration_check = skip_collaboration_check
end
def can_do_action?(action)
@@ -87,6 +88,8 @@ module Gitlab
private
+ attr_reader :skip_collaboration_check
+
def can_push?
user.can?(push_ability, container)
end
@@ -98,6 +101,8 @@ module Gitlab
end
def branch_allows_collaboration_for?(ref)
+ return false if skip_collaboration_check
+
# Checking for an internal project or group to prevent an infinite loop:
# https://gitlab.com/gitlab-org/gitlab/issues/36805
(!project.internal? && project.branch_allows_collaboration?(user, ref))
diff --git a/spec/lib/gitlab/asciidoc_spec.rb b/spec/lib/gitlab/asciidoc_spec.rb
index 08510d4652b..3eb015a5a22 100644
--- a/spec/lib/gitlab/asciidoc_spec.rb
+++ b/spec/lib/gitlab/asciidoc_spec.rb
@@ -92,6 +92,15 @@ module Gitlab
expect(render(data[:input], context)).to include(data[:output])
end
end
+
+ it 'does not allow locked attributes to be overridden' do
+ input = <<~ADOC
+ {counter:max-include-depth:1234}
+ <|-- {max-include-depth}
+ ADOC
+
+ expect(render(input, {})).not_to include('1234')
+ end
end
context "images" do
@@ -543,6 +552,40 @@ module Gitlab
expect(render(input, context)).to include(output.strip)
end
+
+ it 'does not allow kroki-plantuml-include to be overridden' do
+ input = <<~ADOC
+ [plantuml, test="{counter:kroki-plantuml-include:/etc/passwd}", format="png"]
+ ....
+ class BlockProcessor
+
+ BlockProcessor <|-- {counter:kroki-plantuml-include}
+ ....
+ ADOC
+
+ output = <<~HTML
+ <div>
+ <div>
+ <a class=\"no-attachment-icon\" href=\"https://kroki.io/plantuml/png/eNpLzkksLlZwyslPzg4oyk9OLS7OL-LiQuUr2NTo6ipUJ-eX5pWkFlllF-VnZ-oW5CTmlZTm5uhm5iXnlKak1gIABQEb8A==\" target=\"_blank\" rel=\"noopener noreferrer\"><img src=\"data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==\" alt=\"Diagram\" class=\"lazy\" data-src=\"https://kroki.io/plantuml/png/eNpLzkksLlZwyslPzg4oyk9OLS7OL-LiQuUr2NTo6ipUJ-eX5pWkFlllF-VnZ-oW5CTmlZTm5uhm5iXnlKak1gIABQEb8A==\"></a>
+ </div>
+ </div>
+ HTML
+
+ expect(render(input, {})).to include(output.strip)
+ end
+
+ it 'does not allow kroki-server-url to be overridden' do
+ input = <<~ADOC
+ [plantuml, test="{counter:kroki-server-url:evilsite}", format="png"]
+ ....
+ class BlockProcessor
+
+ BlockProcessor
+ ....
+ ADOC
+
+ expect(render(input, {})).not_to include('evilsite')
+ end
end
context 'with Kroki and BlockDiag (additional format) enabled' do
diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb
index 97fff030906..01890305df4 100644
--- a/spec/lib/gitlab/user_access_spec.rb
+++ b/spec/lib/gitlab/user_access_spec.rb
@@ -216,6 +216,15 @@ RSpec.describe Gitlab::UserAccess do
expect(access.can_merge_to_branch?(@branch.name)).to be_falsey
end
end
+
+ context 'when skip_collaboration_check is true' do
+ let(:access) { described_class.new(user, container: project, skip_collaboration_check: true) }
+
+ it 'does not call Project#branch_allows_collaboration?' do
+ expect(project).not_to receive(:branch_allows_collaboration?)
+ expect(access.can_push_to_branch?('master')).to be_falsey
+ end
+ end
end
describe '#can_create_tag?' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 1cee494989d..49d9fd56d70 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -5291,6 +5291,64 @@ RSpec.describe Project, factory_default: :keep do
end
end
+ describe '#branch_allows_collaboration?' do
+ context 'when there are open merge requests that have their source/target branches point to each other' do
+ let_it_be(:project) { create(:project, :repository) }
+ let_it_be(:developer) { create(:user) }
+ let_it_be(:reporter) { create(:user) }
+ let_it_be(:guest) { create(:user) }
+
+ before_all do
+ create(
+ :merge_request,
+ target_project: project,
+ target_branch: 'master',
+ source_project: project,
+ source_branch: 'merge-test',
+ allow_collaboration: true
+ )
+
+ create(
+ :merge_request,
+ target_project: project,
+ target_branch: 'merge-test',
+ source_project: project,
+ source_branch: 'master',
+ allow_collaboration: true
+ )
+
+ project.add_developer(developer)
+ project.add_reporter(reporter)
+ project.add_guest(guest)
+ end
+
+ shared_examples_for 'successful check' do
+ it 'does not go into an infinite loop' do
+ expect { project.branch_allows_collaboration?(user, 'master') }
+ .not_to raise_error
+ end
+ end
+
+ context 'when user is a developer' do
+ let(:user) { developer }
+
+ it_behaves_like 'successful check'
+ end
+
+ context 'when user is a reporter' do
+ let(:user) { reporter }
+
+ it_behaves_like 'successful check'
+ end
+
+ context 'when user is a guest' do
+ let(:user) { guest }
+
+ it_behaves_like 'successful check'
+ end
+ end
+ end
+
context 'with cross project merge requests' do
let(:user) { create(:user) }
let(:target_project) { create(:project, :repository) }
diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore
index 259148fa18f..259148fa18f 100755..100644
--- a/vendor/gitignore/C++.gitignore
+++ b/vendor/gitignore/C++.gitignore
diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore
index a1c2a238a96..a1c2a238a96 100755..100644
--- a/vendor/gitignore/Java.gitignore
+++ b/vendor/gitignore/Java.gitignore