summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAlexandru Croitor <acroitor@gitlab.com>2019-07-17 12:54:40 +0300
committerAlexandru Croitor <acroitor@gitlab.com>2019-08-22 10:53:27 +0300
commitf3079ce3f6af91e56b84a2135fcab0a426383b5f (patch)
tree0cf51fda8f21b6c0e281dd3f6ae75c215c70bb8c
parentbef9aef425e5331af54c761d37338226f4d0f813 (diff)
downloadgitlab-ce-f3079ce3f6af91e56b84a2135fcab0a426383b5f.tar.gz
Limit the size of issuable description and comments
Limiting the size of issuable description and comments to 1_000_000, which is close to ~1MB of ASCII characters, which represents 99.9% of all descriptions and comments we have in DB at the moment. This should help prevent DoS attacks when comments contain refference strings. Also this change updates regexp matching the namespaces paths by limiting the namespaces paths to Namespace::NUMBER_OF_ANCESTORS_ALLOWED, as we allow 20 levels deep groups. see https://gitlab.com/gitlab-org/gitlab-ce/issues/61974#note_191274234
-rw-r--r--app/models/concerns/issuable.rb1
-rw-r--r--app/models/note.rb1
-rw-r--r--changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml5
-rw-r--r--changelogs/unreleased/security-61974-limit-issue-comment-size.yml5
-rw-r--r--doc/api/epics.md4
-rw-r--r--doc/api/issues.md4
-rw-r--r--doc/api/merge_requests.md4
-rw-r--r--doc/api/notes.md16
-rw-r--r--lib/gitlab/database.rb4
-rw-r--r--lib/gitlab/path_regex.rb2
-rw-r--r--spec/lib/banzai/filter/project_reference_filter_spec.rb16
-rw-r--r--spec/models/concerns/issuable_spec.rb51
-rw-r--r--spec/models/note_spec.rb1
-rw-r--r--spec/support/shared_examples/models/concern/issuable_shared_examples.rb8
14 files changed, 103 insertions, 19 deletions
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 299e413321d..5692c69b1e9 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -73,6 +73,7 @@ module Issuable
validates :author, presence: true
validates :title, presence: true, length: { maximum: 255 }
+ validates :description, length: { maximum: Gitlab::Database::MAX_TEXT_SIZE_LIMIT }, allow_blank: true
validate :milestone_is_valid
scope :authored, ->(user) { where(author_id: user) }
diff --git a/app/models/note.rb b/app/models/note.rb
index b55af7d9b5e..246536853cb 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -85,6 +85,7 @@ class Note < ApplicationRecord
delegate :title, to: :noteable, allow_nil: true
validates :note, presence: true
+ validates :note, length: { maximum: Gitlab::Database::MAX_TEXT_SIZE_LIMIT }
validates :project, presence: true, if: :for_project_noteable?
# Attachments are deprecated and are handled by Markdown uploader
diff --git a/changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml b/changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml
new file mode 100644
index 00000000000..962171dc6f8
--- /dev/null
+++ b/changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml
@@ -0,0 +1,5 @@
+---
+title: Speed up regexp in namespace format by failing fast after reaching maximum namespace depth
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-61974-limit-issue-comment-size.yml b/changelogs/unreleased/security-61974-limit-issue-comment-size.yml
new file mode 100644
index 00000000000..6d5ef057d83
--- /dev/null
+++ b/changelogs/unreleased/security-61974-limit-issue-comment-size.yml
@@ -0,0 +1,5 @@
+---
+title: Limit the size of issuable description and comments
+merge_request:
+author:
+type: security
diff --git a/doc/api/epics.md b/doc/api/epics.md
index 0541cfaa715..5817174eff5 100644
--- a/doc/api/epics.md
+++ b/doc/api/epics.md
@@ -161,7 +161,7 @@ POST /groups/:id/epics
| `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user |
| `title` | string | yes | The title of the epic |
| `labels` | string | no | The comma separated list of labels |
-| `description` | string | no | The description of the epic |
+| `description` | string | no | The description of the epic. Limited to 1 000 000 characters. |
| `start_date_is_fixed` | boolean | no | Whether start date should be sourced from `start_date_fixed` or from milestones (since 11.3) |
| `start_date_fixed` | string | no | The fixed start date of an epic (since 11.3) |
| `due_date_is_fixed` | boolean | no | Whether due date should be sourced from `due_date_fixed` or from milestones (since 11.3) |
@@ -225,7 +225,7 @@ PUT /groups/:id/epics/:epic_iid
| `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user |
| `epic_iid` | integer/string | yes | The internal ID of the epic |
| `title` | string | no | The title of an epic |
-| `description` | string | no | The description of an epic |
+| `description` | string | no | The description of an epic. Limited to 1 000 000 characters. |
| `labels` | string | no | The comma separated list of labels |
| `start_date_is_fixed` | boolean | no | Whether start date should be sourced from `start_date_fixed` or from milestones (since 11.3) |
| `start_date_fixed` | string | no | The fixed start date of an epic (since 11.3) |
diff --git a/doc/api/issues.md b/doc/api/issues.md
index 0d96cfa1b21..f6309c64e7c 100644
--- a/doc/api/issues.md
+++ b/doc/api/issues.md
@@ -537,7 +537,7 @@ POST /projects/:id/issues
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
| `iid` | integer/string | no | The internal ID of the project's issue (requires admin or project owner rights) |
| `title` | string | yes | The title of an issue |
-| `description` | string | no | The description of an issue |
+| `description` | string | no | The description of an issue. Limited to 1 000 000 characters. |
| `confidential` | boolean | no | Set an issue to be confidential. Default is `false`. |
| `assignee_ids` | Array[integer] | no | The ID of the users to assign issue |
| `milestone_id` | integer | no | The global ID of a milestone to assign issue |
@@ -625,7 +625,7 @@ PUT /projects/:id/issues/:issue_iid
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
| `issue_iid` | integer | yes | The internal ID of a project's issue |
| `title` | string | no | The title of an issue |
-| `description` | string | no | The description of an issue |
+| `description` | string | no | The description of an issue. Limited to 1 000 000 characters. |
| `confidential` | boolean | no | Updates an issue to be confidential |
| `assignee_ids` | Array[integer] | no | The ID of the user(s) to assign the issue to. Set to `0` or provide an empty value to unassign all assignees. |
| `milestone_id` | integer | no | The global ID of a milestone to assign the issue to. Set to `0` or provide an empty value to unassign a milestone.|
diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md
index dd7810c3403..6a9f2c6e5d5 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -779,7 +779,7 @@ POST /projects/:id/merge_requests
| `title` | string | yes | Title of MR |
| `assignee_id` | integer | no | Assignee user ID |
| `assignee_ids` | Array[integer] | no | The ID of the user(s) to assign the MR to. Set to `0` or provide an empty value to unassign all assignees. |
-| `description` | string | no | Description of MR |
+| `description` | string | no | Description of MR. Limited to 1 000 000 characters. |
| `target_project_id` | integer | no | The target project (numeric id) |
| `labels` | string | no | Labels for MR as a comma-separated list |
| `milestone_id` | integer | no | The global ID of a milestone |
@@ -911,7 +911,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid
| `assignee_ids` | Array[integer] | no | The ID of the user(s) to assign the MR to. Set to `0` or provide an empty value to unassign all assignees. |
| `milestone_id` | integer | no | The global ID of a milestone to assign the merge request to. Set to `0` or provide an empty value to unassign a milestone.|
| `labels` | string | no | Comma-separated label names for a merge request. Set to an empty string to unassign all labels. |
-| `description` | string | no | Description of MR |
+| `description` | string | no | Description of MR. Limited to 1 000 000 characters. |
| `state_event` | string | no | New state (close/reopen) |
| `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging |
| `squash` | boolean | no | Squash commits into a single commit when merging |
diff --git a/doc/api/notes.md b/doc/api/notes.md
index c09129c22d4..36aec683994 100644
--- a/doc/api/notes.md
+++ b/doc/api/notes.md
@@ -113,7 +113,7 @@ Parameters:
- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
- `issue_iid` (required) - The IID of an issue
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
- `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights)
```bash
@@ -133,7 +133,7 @@ Parameters:
- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
- `issue_iid` (required) - The IID of an issue
- `note_id` (required) - The ID of a note
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
```bash
curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/issues/11/notes?body=note
@@ -231,7 +231,7 @@ Parameters:
- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
- `snippet_id` (required) - The ID of a snippet
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
- `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z
```bash
@@ -251,7 +251,7 @@ Parameters:
- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
- `snippet_id` (required) - The ID of a snippet
- `note_id` (required) - The ID of a note
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
```bash
curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippets/11/notes?body=note
@@ -354,7 +354,7 @@ Parameters:
- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
- `merge_request_iid` (required) - The IID of a merge request
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
- `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z
### Modify existing merge request note
@@ -370,7 +370,7 @@ Parameters:
- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
- `merge_request_iid` (required) - The IID of a merge request
- `note_id` (required) - The ID of a note
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
```bash
curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/notes?body=note
@@ -472,7 +472,7 @@ Parameters:
| --------- | -------------- | -------- | ----------- |
| `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
| `epic_id` | integer | yes | The ID of an epic |
-| `body` | string | yes | The content of a note |
+| `body` | string | yes | The content of a note. Limited to 1 000 000 characters. |
```bash
curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippet/11/notes?body=note
@@ -493,7 +493,7 @@ Parameters:
| `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
| `epic_id` | integer | yes | The ID of an epic |
| `note_id` | integer | yes | The ID of a note |
-| `body` | string | yes | The content of a note |
+| `body` | string | yes | The content of a note. Limited to 1 000 000 characters. |
```bash
curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippet/11/notes?body=note
diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb
index 8da98cc3909..a44a31d62f4 100644
--- a/lib/gitlab/database.rb
+++ b/lib/gitlab/database.rb
@@ -11,6 +11,10 @@ module Gitlab
# https://dev.mysql.com/doc/refman/5.7/en/datetime.html
MAX_TIMESTAMP_VALUE = Time.at((1 << 31) - 1).freeze
+ # The maximum number of characters for text fields, to avoid DoS attacks via parsing huge text fields
+ # https://gitlab.com/gitlab-org/gitlab-ce/issues/61974
+ MAX_TEXT_SIZE_LIMIT = 1_000_000
+
def self.config
ActiveRecord::Base.configurations[Rails.env]
end
diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb
index a07b1246bee..ecd552b0940 100644
--- a/lib/gitlab/path_regex.rb
+++ b/lib/gitlab/path_regex.rb
@@ -133,7 +133,7 @@ module Gitlab
NO_SUFFIX_REGEX = /(?<!\.git|\.atom)/.freeze
NAMESPACE_FORMAT_REGEX = /(?:#{NAMESPACE_FORMAT_REGEX_JS})#{NO_SUFFIX_REGEX}/.freeze
PROJECT_PATH_FORMAT_REGEX = /(?:#{PATH_REGEX_STR})#{NO_SUFFIX_REGEX}/.freeze
- FULL_NAMESPACE_FORMAT_REGEX = %r{(#{NAMESPACE_FORMAT_REGEX}/)*#{NAMESPACE_FORMAT_REGEX}}.freeze
+ FULL_NAMESPACE_FORMAT_REGEX = %r{(#{NAMESPACE_FORMAT_REGEX}/){,#{Namespace::NUMBER_OF_ANCESTORS_ALLOWED}}#{NAMESPACE_FORMAT_REGEX}}.freeze
def root_namespace_route_regex
@root_namespace_route_regex ||= begin
diff --git a/spec/lib/banzai/filter/project_reference_filter_spec.rb b/spec/lib/banzai/filter/project_reference_filter_spec.rb
index 69f9c1ae829..927d226c400 100644
--- a/spec/lib/banzai/filter/project_reference_filter_spec.rb
+++ b/spec/lib/banzai/filter/project_reference_filter_spec.rb
@@ -26,10 +26,18 @@ describe Banzai::Filter::ProjectReferenceFilter do
expect(reference_filter(act).to_html).to eq(CGI.escapeHTML(exp))
end
- it 'fails fast for long invalid string' do
- expect do
- Timeout.timeout(5.seconds) { reference_filter("A" * 50000).to_html }
- end.not_to raise_error
+ context 'when invalid reference strings are very long' do
+ shared_examples_for 'fails fast' do |ref_string|
+ it 'fails fast for long strings' do
+ # took well under 1 second in CI https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/3267#note_172824
+ expect do
+ Timeout.timeout(3.seconds) { reference_filter(ref_string).to_html }
+ end.not_to raise_error
+ end
+ end
+
+ it_behaves_like 'fails fast', 'A' * 50000
+ it_behaves_like 'fails fast', '/a' * 50000
end
it 'allows references with text after the > character' do
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 64f02978d79..4aa7bca1cb6 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -46,6 +46,7 @@ describe Issuable do
it { is_expected.to validate_presence_of(:author) }
it { is_expected.to validate_presence_of(:title) }
it { is_expected.to validate_length_of(:title).is_at_most(255) }
+ it { is_expected.to validate_length_of(:description).is_at_most(1_000_000) }
end
describe 'milestone' do
@@ -764,4 +765,54 @@ describe Issuable do
end
end
end
+
+ describe '#matches_cross_reference_regex?' do
+ context "issue description with long path string" do
+ let(:mentionable) { build(:issue, description: "/a" * 50000) }
+
+ it_behaves_like 'matches_cross_reference_regex? fails fast'
+ end
+
+ context "note with long path string" do
+ let(:mentionable) { build(:note, note: "/a" * 50000) }
+
+ it_behaves_like 'matches_cross_reference_regex? fails fast'
+ end
+
+ context "note with long path string" do
+ let(:project) { create(:project, :public, :repository) }
+ let(:mentionable) { project.commit }
+
+ before do
+ expect(mentionable.raw).to receive(:message).and_return("/a" * 50000)
+ end
+
+ it_behaves_like 'matches_cross_reference_regex? fails fast'
+ end
+ end
+
+ describe '#matches_cross_reference_regex?' do
+ context "issue description with long path string" do
+ let(:mentionable) { build(:issue, description: "/a" * 50000) }
+
+ it_behaves_like 'matches_cross_reference_regex? fails fast'
+ end
+
+ context "note with long path string" do
+ let(:mentionable) { build(:note, note: "/a" * 50000) }
+
+ it_behaves_like 'matches_cross_reference_regex? fails fast'
+ end
+
+ context "note with long path string" do
+ let(:project) { create(:project, :public, :repository) }
+ let(:mentionable) { project.commit }
+
+ before do
+ expect(mentionable.raw).to receive(:message).and_return("/a" * 50000)
+ end
+
+ it_behaves_like 'matches_cross_reference_regex? fails fast'
+ end
+ end
end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 7a1ab20186a..79dc563b4d5 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -22,6 +22,7 @@ describe Note do
end
describe 'validation' do
+ it { is_expected.to validate_length_of(:note).is_at_most(1_000_000) }
it { is_expected.to validate_presence_of(:note) }
it { is_expected.to validate_presence_of(:project) }
diff --git a/spec/support/shared_examples/models/concern/issuable_shared_examples.rb b/spec/support/shared_examples/models/concern/issuable_shared_examples.rb
new file mode 100644
index 00000000000..9604555c57d
--- /dev/null
+++ b/spec/support/shared_examples/models/concern/issuable_shared_examples.rb
@@ -0,0 +1,8 @@
+shared_examples_for 'matches_cross_reference_regex? fails fast' do
+ it 'fails fast for long strings' do
+ # took well under 1 second in CI https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/3267#note_172823
+ expect do
+ Timeout.timeout(3.seconds) { mentionable.matches_cross_reference_regex? }
+ end.not_to raise_error
+ end
+end