Commit 7698d405 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'master' of dev.gitlab.org:gitlab/gitlabhq

parents fa160c26 09095625
Pipeline #79277971 failed with stages
in 37 minutes and 5 seconds
......@@ -2,6 +2,38 @@
documentation](doc/development/changelog.md) for instructions on adding your own
entry.
## 12.2.3
### Security (22 changes)
- Ensure only authorised users can create notes on Merge Requests and Issues.
- Gitaly: ignore git redirects.
- Add :login_recaptcha_protection_enabled setting to prevent bots from brute-force attacks.
- Speed up regexp in namespace format by failing fast after reaching maximum namespace depth.
- Limit the size of issuable description and comments.
- Send TODOs for comments on commits correctly.
- Restrict MergeRequests#test_reports to authenticated users with read-access on Builds.
- Added image proxy to mitigate potential stealing of IP addresses.
- Filter out old system notes for epics in notes api endpoint response.
- Avoid exposing unaccessible repo data upon GFM post processing.
- Fix HTML injection for label description.
- Make sure HTML text is always escaped when replacing label/milestone references.
- Prevent DNS rebind on JIRA service integration.
- Use admin_group authorization in Groups::RunnersController.
- Prevent disclosure of merge request ID via email.
- Show cross-referenced MR-id in issues' activities only to authorized users.
- Enforce max chars and max render time in markdown math.
- Check permissions before responding in MergeController#pipeline_status.
- Remove EXIF from users/personal snippet uploads.
- Fix project import restricted visibility bypass via API.
- Fix weak session management by clearing password reset tokens after login (username/email) are updated.
- Fix SSRF via DNS rebinding in Kubernetes Integration.
## 12.2.2
- Unreleased due to QA failure.
## 12.2.1
### Fixed (3 changes)
......@@ -591,6 +623,34 @@ entry.
- Removes EE differences for app/views/admin/users/show.html.haml.
## 12.0.7
### Security (22 changes)
- Ensure only authorised users can create notes on Merge Requests and Issues.
- Add :login_recaptcha_protection_enabled setting to prevent bots from brute-force attacks.
- Queries for Upload should be scoped by model.
- Speed up regexp in namespace format by failing fast after reaching maximum namespace depth.
- Limit the size of issuable description and comments.
- Send TODOs for comments on commits correctly.
- Restrict MergeRequests#test_reports to authenticated users with read-access on Builds.
- Added image proxy to mitigate potential stealing of IP addresses.
- Filter out old system notes for epics in notes api endpoint response.
- Avoid exposing unaccessible repo data upon GFM post processing.
- Fix HTML injection for label description.
- Make sure HTML text is always escaped when replacing label/milestone references.
- Prevent DNS rebind on JIRA service integration.
- Use admin_group authorization in Groups::RunnersController.
- Prevent disclosure of merge request ID via email.
- Show cross-referenced MR-id in issues' activities only to authorized users.
- Enforce max chars and max render time in markdown math.
- Check permissions before responding in MergeController#pipeline_status.
- Remove EXIF from users/personal snippet uploads.
- Fix project import restricted visibility bypass via API.
- Fix weak session management by clearing password reset tokens after login (username/email) are updated.
- Fix SSRF via DNS rebinding in Kubernetes Integration.
## 12.0.6
- No changes.
......
import $ from 'jquery';
import { __ } from '~/locale';
import flash from '~/flash';
import { s__, sprintf } from '~/locale';
// Renders math using KaTeX in any element with the
// `js-render-math` class
......@@ -10,21 +9,131 @@ import flash from '~/flash';
// <code class="js-render-math"></div>
//
// Loop over all math elements and render math
function renderWithKaTeX(elements, katex) {
elements.each(function katexElementsLoop() {
const mathNode = $('<span></span>');
const $this = $(this);
const display = $this.attr('data-math-style') === 'display';
try {
katex.render($this.text(), mathNode.get(0), { displayMode: display, throwOnError: false });
mathNode.insertAfter($this);
$this.remove();
} catch (err) {
throw err;
const MAX_MATH_CHARS = 1000;
const MAX_RENDER_TIME_MS = 2000;
// These messages might be used with inline errors in the future. Keep them around. For now, we will
// display a single error message using flash().
// const CHAR_LIMIT_EXCEEDED_MSG = sprintf(
// s__(
// 'math|The following math is too long. For performance reasons, math blocks are limited to %{maxChars} characters. Try splitting up this block, or include an image instead.',
// ),
// { maxChars: MAX_MATH_CHARS },
// );
// const RENDER_TIME_EXCEEDED_MSG = s__(
// "math|The math in this entry is taking too long to render. Any math below this point won't be shown. Consider splitting it among multiple entries.",
// );
const RENDER_FLASH_MSG = sprintf(
s__(
'math|The math in this entry is taking too long to render and may not be displayed as expected. For performance reasons, math blocks are also limited to %{maxChars} characters. Consider splitting up large formulae, splitting math blocks among multiple entries, or using an image instead.',
),
{ maxChars: MAX_MATH_CHARS },
);
// Wait for the browser to reflow the layout. Reflowing SVG takes time.
// This has to wrap the inner function, otherwise IE/Edge throw "invalid calling object".
const waitForReflow = fn => {
window.requestAnimationFrame(fn);
};
/**
* Renders math blocks sequentially while protecting against DoS attacks. Math blocks have a maximum character limit of MAX_MATH_CHARS. If rendering math takes longer than MAX_RENDER_TIME_MS, all subsequent math blocks are skipped and an error message is shown.
*/
class SafeMathRenderer {
/*
How this works:
The performance bottleneck in rendering math is in the browser trying to reflow the generated SVG.
During this time, the JS is blocked and the page becomes unresponsive.
We want to render math blocks one by one until a certain time is exceeded, after which we stop
rendering subsequent math blocks, to protect against DoS. However, browsers do reflowing in an
asynchronous task, so we can't time it synchronously.
SafeMathRenderer essentially does the following:
1. Replaces all math blocks with placeholders so that they're not mistakenly rendered twice.
2. Places each placeholder element in a queue.
3. Renders the element at the head of the queue and waits for reflow.
4. After reflow, gets the elapsed time since step 3 and repeats step 3 until the queue is empty.
*/
queue = [];
totalMS = 0;
constructor(elements, katex) {
this.elements = elements;
this.katex = katex;
this.renderElement = this.renderElement.bind(this);
this.render = this.render.bind(this);
}
renderElement() {
if (!this.queue.length) {
return;
}
});
const el = this.queue.shift();
const text = el.textContent;
el.removeAttribute('style');
if (this.totalMS >= MAX_RENDER_TIME_MS || text.length > MAX_MATH_CHARS) {
if (!this.flashShown) {
flash(RENDER_FLASH_MSG);
this.flashShown = true;
}
// Show unrendered math code
const codeElement = document.createElement('pre');
codeElement.className = 'code';
codeElement.textContent = el.textContent;
el.parentNode.replaceChild(codeElement, el);
// Render the next math
this.renderElement();
} else {
this.startTime = Date.now();
try {
el.innerHTML = this.katex.renderToString(text, {
displayMode: el.getAttribute('data-math-style') === 'display',
throwOnError: true,
maxSize: 20,
maxExpand: 20,
});
} catch {
// Don't show a flash for now because it would override an existing flash message
el.textContent = s__('math|There was an error rendering this math block');
// el.style.color = '#d00';
el.className = 'katex-error';
}
// Give the browser time to reflow the svg
waitForReflow(() => {
const deltaTime = Date.now() - this.startTime;
this.totalMS += deltaTime;
this.renderElement();
});
}
}
render() {
// Replace math blocks with a placeholder so they aren't rendered twice
this.elements.forEach(el => {
const placeholder = document.createElement('span');
placeholder.style.display = 'none';
placeholder.setAttribute('data-math-style', el.getAttribute('data-math-style'));
placeholder.textContent = el.textContent;
el.parentNode.replaceChild(placeholder, el);
this.queue.push(placeholder);
});
// If we wait for the browser thread to settle down a bit, math rendering becomes 5-10x faster
// and less prone to timeouts.
setTimeout(this.renderElement, 400);
}
}
export default function renderMath($els) {
......@@ -34,7 +143,8 @@ export default function renderMath($els) {
import(/* webpackChunkName: 'katex' */ 'katex/dist/katex.min.css'),
])
.then(([katex]) => {
renderWithKaTeX($els, katex);
const renderer = new SafeMathRenderer($els.get(), katex);
renderer.render();
})
.catch(() => flash(__('An error occurred while rendering KaTeX')));
.catch(() => {});
}
......@@ -138,7 +138,7 @@ module IssuableActions
end
notes = prepare_notes_for_rendering(notes)
notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
notes = notes.select { |n| n.visible_for?(current_user) }
discussions = Discussion.build_collection(notes, issuable)
......
......@@ -29,7 +29,7 @@ module NotesActions
end
notes = prepare_notes_for_rendering(notes)
notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
notes = notes.select { |n| n.visible_for?(current_user) }
notes_json[:notes] =
if use_note_serializer?
......
......@@ -127,4 +127,8 @@ module UploadsActions
def model
strong_memoize(:model) { find_model }
end
def workhorse_authorize_request?
action_name == 'authorize'
end
end
......@@ -3,7 +3,7 @@
class Groups::RunnersController < Groups::ApplicationController
# Proper policies should be implemented per
# https://gitlab.com/gitlab-org/gitlab-ce/issues/45894
before_action :authorize_admin_pipeline!
before_action :authorize_admin_group!
before_action :runner, only: [:edit, :update, :destroy, :pause, :resume, :show]
......@@ -50,10 +50,6 @@ class Groups::RunnersController < Groups::ApplicationController
@runner ||= @group.runners.find(params[:id])
end
def authorize_admin_pipeline!
return render_404 unless can?(current_user, :admin_pipeline, group)
end
def runner_params
params.require(:runner).permit(Ci::Runner::FORM_EDITABLE)
end
......
......@@ -12,6 +12,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
skip_before_action :merge_request, only: [:index, :bulk_update]
before_action :whitelist_query_limiting, only: [:assign_related_issues, :update]
before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort]
before_action :authorize_test_reports!, only: [:test_reports]
before_action :set_issuables_index, only: [:index]
before_action :authenticate_user!, only: [:assign_related_issues]
before_action :check_user_can_push_to_source_branch!, only: [:rebase]
......@@ -189,7 +190,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
def pipeline_status
render json: PipelineSerializer
.new(project: @project, current_user: @current_user)
.represent_status(@merge_request.head_pipeline)
.represent_status(head_pipeline)
end
def ci_environments_status
......@@ -239,6 +240,13 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
private
def head_pipeline
strong_memoize(:head_pipeline) do
pipeline = @merge_request.head_pipeline
pipeline if can?(current_user, :read_pipeline, pipeline)
end
end
def ci_environments_status_on_merge_result?
params[:environment_target] == 'merge_commit'
end
......@@ -337,4 +345,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
render json: { status_reason: 'Unknown error' }, status: :internal_server_error
end
end
def authorize_test_reports!
# MergeRequest#actual_head_pipeline is the pipeline accessed in MergeRequest#compare_reports.
return render_404 unless can?(current_user, :read_build, merge_request.actual_head_pipeline)
end
end
......@@ -21,10 +21,13 @@ class SessionsController < Devise::SessionsController
prepend_before_action :ensure_password_authentication_enabled!, if: -> { action_name == 'create' && password_based_login? }
before_action :auto_sign_in_with_provider, only: [:new]
before_action :store_unauthenticated_sessions, only: [:new]
before_action :save_failed_login, if: :action_new_and_failed_login?
before_action :load_recaptcha
after_action :log_failed_login, if: -> { action_name == 'new' && failed_login? }
helper_method :captcha_enabled?
after_action :log_failed_login, if: :action_new_and_failed_login?
helper_method :captcha_enabled?, :captcha_on_login_required?
# protect_from_forgery is already prepended in ApplicationController but
# authenticate_with_two_factor which signs in the user is prepended before
......@@ -38,6 +41,7 @@ class SessionsController < Devise::SessionsController
protect_from_forgery with: :exception, prepend: true
CAPTCHA_HEADER = 'X-GitLab-Show-Login-Captcha'.freeze
MAX_FAILED_LOGIN_ATTEMPTS = 5
def new
set_minimum_password_length
......@@ -81,10 +85,14 @@ class SessionsController < Devise::SessionsController
request.headers[CAPTCHA_HEADER] && Gitlab::Recaptcha.enabled?
end
def captcha_on_login_required?
Gitlab::Recaptcha.enabled_on_login? && unverified_anonymous_user?
end
# From https://github.com/plataformatec/devise/wiki/How-To:-Use-Recaptcha-with-Devise#devisepasswordscontroller
def check_captcha
return unless user_params[:password].present?
return unless captcha_enabled?
return unless captcha_enabled? || captcha_on_login_required?
return unless Gitlab::Recaptcha.load_configurations!
if verify_recaptcha
......@@ -126,10 +134,28 @@ class SessionsController < Devise::SessionsController
Gitlab::AppLogger.info("Failed Login: username=#{user_params[:login]} ip=#{request.remote_ip}")
end
def action_new_and_failed_login?
action_name == 'new' && failed_login?
end
def save_failed_login
session[:failed_login_attempts] ||= 0
session[:failed_login_attempts] += 1
end
def failed_login?
(options = request.env["warden.options"]) && options[:action] == "unauthenticated"
end
# storing sessions per IP lets us check if there are associated multiple
# anonymous sessions with one IP and prevent situations when there are
# multiple attempts of logging in
def store_unauthenticated_sessions
return if current_user
Gitlab::AnonymousSession.new(request.remote_ip, session_id: request.session.id).store_session_id_per_ip
end
# Handle an "initial setup" state, where there's only one user, it's an admin,
# and they require a password change.
# rubocop: disable CodeReuse/ActiveRecord
......@@ -240,6 +266,18 @@ class SessionsController < Devise::SessionsController
@ldap_servers ||= Gitlab::Auth::LDAP::Config.available_servers
end
def unverified_anonymous_user?
exceeded_failed_login_attempts? || exceeded_anonymous_sessions?
end
def exceeded_failed_login_attempts?
session.fetch(:failed_login_attempts, 0) > MAX_FAILED_LOGIN_ATTEMPTS
end
def exceeded_anonymous_sessions?
Gitlab::AnonymousSession.new(request.remote_ip).stored_sessions >= MAX_FAILED_LOGIN_ATTEMPTS
end
def authentication_method
if user_params[:otp_attempt]
"two-factor"
......
......@@ -2,6 +2,7 @@
class UploadsController < ApplicationController
include UploadsActions
include WorkhorseRequest
UnknownUploadModelError = Class.new(StandardError)
......@@ -21,7 +22,8 @@ class UploadsController < ApplicationController
before_action :upload_mount_satisfied?
before_action :find_model
before_action :authorize_access!, only: [:show]
before_action :authorize_create_access!, only: [:create]
before_action :authorize_create_access!, only: [:create, :authorize]
before_action :verify_workhorse_api!, only: [:authorize]
def uploader_class
PersonalFileUploader
......@@ -72,7 +74,7 @@ class UploadsController < ApplicationController
end
def render_unauthorized
if current_user
if current_user || workhorse_authorize_request?
render_404
else
authenticate_user!
......
......@@ -164,6 +164,10 @@ module ApplicationSettingsHelper
:allow_local_requests_from_system_hooks,
:dns_rebinding_protection_enabled,
:archive_builds_in_human_readable,
:asset_proxy_enabled,
:asset_proxy_secret_key,
:asset_proxy_url,
:asset_proxy_whitelist,
:authorized_keys_enabled,
:auto_devops_enabled,
:auto_devops_domain,
......@@ -231,6 +235,7 @@ module ApplicationSettingsHelper
:recaptcha_enabled,
:recaptcha_private_key,
:recaptcha_site_key,
:login_recaptcha_protection_enabled,
:receive_max_input_size,
:repository_checks_enabled,
:repository_storages,
......
......@@ -90,6 +90,8 @@ module EmailsHelper
when MergeRequest
merge_request = MergeRequest.find(closed_via[:id]).present
return "" unless Ability.allowed?(@recipient, :read_merge_request, merge_request)
case format
when :html
merge_request_link = link_to(merge_request.to_reference, merge_request.web_url)
......@@ -102,6 +104,8 @@ module EmailsHelper
# Technically speaking this should be Commit but per
# https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/15610#note_163812339
# we can't deserialize Commit without custom serializer for ActiveJob
return "" unless Ability.allowed?(@recipient, :download_code, @project)
_("via %{closed_via}") % { closed_via: closed_via }
else
""
......
......@@ -71,7 +71,7 @@ module LabelsHelper
end
def label_tooltip_title(label)
label.description
Sanitize.clean(label.description)
end
def suggested_colors
......
......@@ -34,6 +34,8 @@ module Emails
setup_issue_mail(issue_id, recipient_id, closed_via: closed_via)
@updated_by = User.find(updated_by_user_id)
@recipient = User.find(recipient_id)
mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason))
end
......
......@@ -18,12 +18,19 @@ class ApplicationSetting < ApplicationRecord
# fix a lot of tests using allow_any_instance_of
include ApplicationSettingImplementation
attr_encrypted :asset_proxy_secret_key,
mode: :per_attribute_iv,
insecure_mode: true,
key: Settings.attr_encrypted_db_key_base_truncated,
algorithm: 'aes-256-cbc'
serialize :restricted_visibility_levels # rubocop:disable Cop/ActiveRecordSerialize
serialize :import_sources # rubocop:disable Cop/ActiveRecordSerialize
serialize :disabled_oauth_sign_in_sources, Array # rubocop:disable Cop/ActiveRecordSerialize
serialize :domain_whitelist, Array # rubocop:disable Cop/ActiveRecordSerialize
serialize :domain_blacklist, Array # rubocop:disable Cop/ActiveRecordSerialize
serialize :repository_storages # rubocop:disable Cop/ActiveRecordSerialize
serialize :asset_proxy_whitelist, Array # rubocop:disable Cop/ActiveRecordSerialize
ignore_column :koding_url
ignore_column :koding_enabled
......@@ -75,11 +82,11 @@ class ApplicationSetting < ApplicationRecord
validates :recaptcha_site_key,
presence: true,
if: :recaptcha_enabled
if: :recaptcha_or_login_protection_enabled
validates :recaptcha_private_key,
presence: true,
if: :recaptcha_enabled
if: :recaptcha_or_login_protection_enabled
validates :akismet_api_key,
presence: true,
......@@ -192,6 +199,17 @@ class ApplicationSetting < ApplicationRecord
allow_nil: true,
numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than: 65536 }
validates :asset_proxy_url,
presence: true,
allow_blank: false,
url: true,
if: :asset_proxy_enabled?
validates :asset_proxy_secret_key,
presence: true,
allow_blank: false,
if: :asset_proxy_enabled?
SUPPORTED_KEY_TYPES.each do |type|
validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type }
end
......@@ -292,4 +310,8 @@ class ApplicationSetting < ApplicationRecord
def self.cache_backend
Gitlab::ThreadMemoryCache.cache_backend
end
def recaptcha_or_login_protection_enabled
recaptcha_enabled || login_recaptcha_protection_enabled
end
end
......@@ -23,8 +23,9 @@ module ApplicationSettingImplementation
akismet_enabled: false,
allow_local_requests_from_web_hooks_and_services: false,
allow_local_requests_from_system_hooks: true,
dns_rebinding_protection_enabled: true,
asset_proxy_enabled: false,
authorized_keys_enabled: true, # TODO default to false if the instance is configured to use AuthorizedKeysCommand
commit_email_hostname: default_commit_email_hostname,
container_registry_token_expire_delay: 5,
default_artifacts_expire_in: '30 days',
default_branch_protection: Settings.gitlab['default_branch_protection'],
......@@ -33,7 +34,9 @@ module ApplicationSettingImplementation
default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'],
default_projects_limit: Settings.gitlab['default_projects_limit'],
default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'],
diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES,
disabled_oauth_sign_in_sources: [],
dns_rebinding_protection_enabled: true,
domain_whitelist: Settings.gitlab['domain_whitelist'],
dsa_key_restriction: 0,
ecdsa_key_restriction: 0,
......@@ -52,9 +55,11 @@ module ApplicationSettingImplementation
housekeeping_gc_period: 200,
housekeeping_incremental_repack_period: 10,
import_sources: Settings.gitlab['import_sources'],
local_markdown_version: 0,
max_artifacts_size: Settings.artifacts['max_size'],
max_attachment_size: Settings.gitlab['max_attachment_size'],
mirror_available: true,
outbound_local_requests_whitelist: [],
password_authentication_enabled_for_git: true,
password_authentication_enabled_for_web: Settings.gitlab['signin_enabled'],
performance_bar_allowed_group_id: nil,
......@@ -63,7 +68,10 @@ module ApplicationSettingImplementation
plantuml_url: nil,
polling_interval_multiplier: 1,
project_export_enabled: true,
protected_ci_variables: false,
raw_blob_request_limit: 300,
recaptcha_enabled: false,
login_recaptcha_protection_enabled: false,
repository_checks_enabled: true,
repository_storages: ['default'],
require_two_factor_authentication: false,
......@@ -95,16 +103,10 @@ module ApplicationSettingImplementation
user_default_internal_regex: nil,
user_show_add_ssh_key_message: true,
usage_stats_set_by_user_id: nil,
diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES,
commit_email_hostname: default_commit_email_hostname,
snowplow_collector_hostname: nil,
snowplow_cookie_domain: nil,
snowplow_enabled: false,
snowplow_site_id: nil,
protected_ci_variables: false,
local_markdown_version: 0,
outbound_local_requests_whitelist: [],
raw_blob_request_limit: 300
snowplow_site_id: nil
}
end
......@@ -198,6 +200,15 @@ module ApplicationSettingImplementation
end
end
def asset_proxy_whitelist=(values)
values = domain_strings_to_array(values) if values.is_a?(String)
# make sure we always whitelist the running host
values << Gitlab.config.gitlab.host unless values.include?(Gitlab.config.gitlab.host)
self[:asset_proxy_whitelist] = values
end
def repository_storages
Array(read_attribute(:repository_storages))
end
......@@ -306,6 +317,7 @@ module ApplicationSettingImplementation
values
.split(DOMAIN_LIST_SEPARATOR)
.map(&:strip)
.reject(&:empty?)
.uniq
end
......
......@@ -203,6 +203,7 @@ module Ci