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
This commit is contained in:
Alexandru Croitor 2019-07-17 12:54:40 +03:00
parent 8ae75677a3
commit 5af535d919
14 changed files with 78 additions and 19 deletions

View file

@ -73,6 +73,7 @@ module Issuable
validates :author, presence: true validates :author, presence: true
validates :title, presence: true, length: { maximum: 255 } validates :title, presence: true, length: { maximum: 255 }
validates :description, length: { maximum: Gitlab::Database::MAX_TEXT_SIZE_LIMIT }, allow_blank: true
validate :milestone_is_valid validate :milestone_is_valid
scope :authored, ->(user) { where(author_id: user) } scope :authored, ->(user) { where(author_id: user) }

View file

@ -89,6 +89,7 @@ class Note < ApplicationRecord
delegate :title, to: :noteable, allow_nil: true delegate :title, to: :noteable, allow_nil: true
validates :note, presence: true validates :note, presence: true
validates :note, length: { maximum: Gitlab::Database::MAX_TEXT_SIZE_LIMIT }
validates :project, presence: true, if: :for_project_noteable? validates :project, presence: true, if: :for_project_noteable?
# Attachments are deprecated and are handled by Markdown uploader # Attachments are deprecated and are handled by Markdown uploader

View file

@ -0,0 +1,5 @@
---
title: Speed up regexp in namespace format by failing fast after reaching maximum namespace depth
merge_request:
author:
type: security

View file

@ -0,0 +1,5 @@
---
title: Limit the size of issuable description and comments
merge_request:
author:
type: security

View file

@ -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 | | `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 | | `title` | string | yes | The title of the epic |
| `labels` | string | no | The comma separated list of labels | | `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_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) | | `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) | | `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 | | `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 | | `epic_iid` | integer/string | yes | The internal ID of the epic |
| `title` | string | no | The title of an 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 | | `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_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) | | `start_date_fixed` | string | no | The fixed start date of an epic (since 11.3) |

View file

@ -593,7 +593,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 | | `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) | | `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 | | `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`. | | `confidential` | boolean | no | Set an issue to be confidential. Default is `false`. |
| `assignee_ids` | integer array | no | The ID of a user to assign issue | | `assignee_ids` | integer array | no | The ID of a user to assign issue |
| `milestone_id` | integer | no | The global ID of a milestone to assign issue | | `milestone_id` | integer | no | The global ID of a milestone to assign issue |
@ -694,7 +694,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 | | `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 | | `issue_iid` | integer | yes | The internal ID of a project's issue |
| `title` | string | no | The title of an 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 | | `confidential` | boolean | no | Updates an issue to be confidential |
| `assignee_ids` | integer array | no | The ID of the user(s) to assign the issue to. Set to `0` or provide an empty value to unassign all assignees. | | `assignee_ids` | integer array | 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.| | `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.|

View file

@ -837,7 +837,7 @@ POST /projects/:id/merge_requests
| `title` | string | yes | Title of MR | | `title` | string | yes | Title of MR |
| `assignee_id` | integer | no | Assignee user ID | | `assignee_id` | integer | no | Assignee user ID |
| `assignee_ids` | integer array | no | The ID of the user(s) to assign the MR to. Set to `0` or provide an empty value to unassign all assignees. | | `assignee_ids` | integer array | 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) | | `target_project_id` | integer | no | The target project (numeric id) |
| `labels` | string | no | Labels for MR as a comma-separated list | | `labels` | string | no | Labels for MR as a comma-separated list |
| `milestone_id` | integer | no | The global ID of a milestone | | `milestone_id` | integer | no | The global ID of a milestone |
@ -990,7 +990,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid
| `assignee_ids` | integer array | no | The ID of the user(s) to assign the MR to. Set to `0` or provide an empty value to unassign all assignees. | | `assignee_ids` | integer array | 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.| | `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. | | `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) | | `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 | | `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 | | `squash` | boolean | no | Squash commits into a single commit when merging |

View file

@ -113,7 +113,7 @@ Parameters:
- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
- `issue_iid` (required) - The IID of an issue - `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) - `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights)
```bash ```bash
@ -133,7 +133,7 @@ Parameters:
- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
- `issue_iid` (required) - The IID of an issue - `issue_iid` (required) - The IID of an issue
- `note_id` (required) - The ID of a note - `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 ```bash
curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/issues/11/notes?body=note 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) - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
- `snippet_id` (required) - The ID of a snippet - `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 - `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z
```bash ```bash
@ -251,7 +251,7 @@ Parameters:
- `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
- `snippet_id` (required) - The ID of a snippet - `snippet_id` (required) - The ID of a snippet
- `note_id` (required) - The ID of a note - `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 ```bash
curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippets/11/notes?body=note 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) - `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 - `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 - `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z
### Modify existing merge request note ### 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) - `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 - `merge_request_iid` (required) - The IID of a merge request
- `note_id` (required) - The ID of a note - `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 ```bash
curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/notes?body=note 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) | | `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 | | `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 ```bash
curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippet/11/notes?body=note 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) | | `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 | | `epic_id` | integer | yes | The ID of an epic |
| `note_id` | integer | yes | The ID of a note | | `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 ```bash
curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippet/11/notes?body=note curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippet/11/notes?body=note

View file

@ -13,6 +13,10 @@ module Gitlab
# FIXME: this should just be the max value of timestampz # FIXME: this should just be the max value of timestampz
MAX_TIMESTAMP_VALUE = Time.at((1 << 31) - 1).freeze 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
# Minimum schema version from which migrations are supported # Minimum schema version from which migrations are supported
# Migrations before this version may have been removed # Migrations before this version may have been removed
MIN_SCHEMA_VERSION = 20190506135400 MIN_SCHEMA_VERSION = 20190506135400

View file

@ -132,7 +132,7 @@ module Gitlab
NO_SUFFIX_REGEX = /(?<!\.git|\.atom)/.freeze NO_SUFFIX_REGEX = /(?<!\.git|\.atom)/.freeze
NAMESPACE_FORMAT_REGEX = /(?:#{NAMESPACE_FORMAT_REGEX_JS})#{NO_SUFFIX_REGEX}/.freeze NAMESPACE_FORMAT_REGEX = /(?:#{NAMESPACE_FORMAT_REGEX_JS})#{NO_SUFFIX_REGEX}/.freeze
PROJECT_PATH_FORMAT_REGEX = /(?:#{PATH_REGEX_STR})#{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 def root_namespace_route_regex
@root_namespace_route_regex ||= begin @root_namespace_route_regex ||= begin

View file

@ -26,10 +26,18 @@ describe Banzai::Filter::ProjectReferenceFilter do
expect(reference_filter(act).to_html).to eq(CGI.escapeHTML(exp)) expect(reference_filter(act).to_html).to eq(CGI.escapeHTML(exp))
end end
it 'fails fast for long invalid string' do context 'when invalid reference strings are very long' do
expect do shared_examples_for 'fails fast' do |ref_string|
Timeout.timeout(5.seconds) { reference_filter("A" * 50000).to_html } it 'fails fast for long strings' do
end.not_to raise_error # 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 end
it 'allows references with text after the > character' do it 'allows references with text after the > character' do

View file

@ -46,6 +46,7 @@ describe Issuable do
it { is_expected.to validate_presence_of(:author) } it { is_expected.to validate_presence_of(:author) }
it { is_expected.to validate_presence_of(:title) } 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(:title).is_at_most(255) }
it { is_expected.to validate_length_of(:description).is_at_most(1_000_000) }
end end
describe 'milestone' do describe 'milestone' do
@ -795,4 +796,29 @@ describe Issuable do
end end
end 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
end end

View file

@ -22,6 +22,7 @@ describe Note do
end end
describe 'validation' do 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(:note) }
it { is_expected.to validate_presence_of(:project) } it { is_expected.to validate_presence_of(:project) }

View file

@ -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