From d8bddb16624f34600069bb5d3540960b25176381 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Wed, 10 Apr 2019 11:39:45 +0800 Subject: Validate MR branch names Prevents refspec as branch name, which would bypass branch protection when used in conjunction with rebase. HEAD seems to be a special case with lots of occurrence, so it is considered valid for now. Another special case is `refs/head/*`, which can be imported. --- lib/gitlab/git_ref_validator.rb | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git_ref_validator.rb b/lib/gitlab/git_ref_validator.rb index 3f13ebeb9d0..dfff6823689 100644 --- a/lib/gitlab/git_ref_validator.rb +++ b/lib/gitlab/git_ref_validator.rb @@ -5,12 +5,15 @@ module Gitlab module GitRefValidator extend self + + EXPANDED_PREFIXES = %w[refs/heads/ refs/remotes/].freeze + DISALLOWED_PREFIXES = %w[-].freeze + # Validates a given name against the git reference specification # # Returns true for a valid reference name, false otherwise def validate(ref_name) - not_allowed_prefixes = %w(refs/heads/ refs/remotes/ -) - return false if ref_name.start_with?(*not_allowed_prefixes) + return false if ref_name.start_with?(*(EXPANDED_PREFIXES + DISALLOWED_PREFIXES)) return false if ref_name == 'HEAD' begin @@ -19,5 +22,21 @@ module Gitlab return false end end + + def validate_merge_request_branch(ref_name) + return false if ref_name.start_with?(*DISALLOWED_PREFIXES) + + expanded_name = if ref_name.start_with?(*EXPANDED_PREFIXES) + ref_name + else + "refs/heads/#{ref_name}" + end + + begin + Rugged::Reference.valid_name?(expanded_name) + rescue ArgumentError + return false + end + end end end -- cgit v1.2.1 From b0fbf001dab134b6638411f0be209bc0d1460519 Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Fri, 3 May 2019 15:09:20 +0200 Subject: Fix url redaction for issue links Add changelog entry Add missing href to all redactor specs and removed href assignment Remove obsolete spec If original_content is given, it should be used for link content --- lib/banzai/redactor.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'lib') diff --git a/lib/banzai/redactor.rb b/lib/banzai/redactor.rb index 7db5f5e1f7d..c2da7fec7cc 100644 --- a/lib/banzai/redactor.rb +++ b/lib/banzai/redactor.rb @@ -70,8 +70,11 @@ module Banzai # Build the raw tag just with a link as href and content if # it's originally a link pattern. We shouldn't return a plain text href. original_link = - if link_reference == 'true' && href = original_content - %(#{href}) + if link_reference == 'true' + href = node.attr('href') + content = original_content + + %(#{content}) end # The reference should be replaced by the original link's content, -- cgit v1.2.1 From b70b43d07ec27c6410e4a8d7ad417662a8823f8f Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 20 May 2019 11:08:31 -0300 Subject: Resolve: Milestones leaked via search API Fix milestone titles being leaked using search API when users cannot read milestones --- lib/gitlab/project_search_results.rb | 6 ++++++ lib/gitlab/search_results.rb | 28 +++++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb index 78337518988..0f3b97e2317 100644 --- a/lib/gitlab/project_search_results.rb +++ b/lib/gitlab/project_search_results.rb @@ -138,6 +138,12 @@ module Gitlab project end + def filter_milestones_by_project(milestones) + return Milestone.none unless Ability.allowed?(@current_user, :read_milestone, @project) + + milestones.where(project_id: project.id) # rubocop: disable CodeReuse/ActiveRecord + end + def repository_project_ref @repository_project_ref ||= repository_ref || project.default_branch end diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb index 4a097a00101..7c1e6b1baff 100644 --- a/lib/gitlab/search_results.rb +++ b/lib/gitlab/search_results.rb @@ -103,9 +103,11 @@ module Gitlab # rubocop: disable CodeReuse/ActiveRecord def milestones - milestones = Milestone.where(project_id: project_ids_relation) - milestones = milestones.search(query) - milestones.reorder('milestones.updated_at DESC') + milestones = Milestone.search(query) + + milestones = filter_milestones_by_project(milestones) + + milestones.reorder('updated_at DESC') end # rubocop: enable CodeReuse/ActiveRecord @@ -123,6 +125,26 @@ module Gitlab 'projects' end + # Filter milestones by authorized projects. + # For performance reasons project_id is being plucked + # to be used on a smaller query. + # + # rubocop: disable CodeReuse/ActiveRecord + def filter_milestones_by_project(milestones) + project_ids = + milestones.where(project_id: project_ids_relation) + .select(:project_id).distinct + .pluck(:project_id) + + return Milestone.none if project_ids.nil? + + authorized_project_ids_relation = + Project.where(id: project_ids).ids_with_milestone_available_for(current_user) + + milestones.where(project_id: authorized_project_ids_relation) + end + # rubocop: enable CodeReuse/ActiveRecord + # rubocop: disable CodeReuse/ActiveRecord def project_ids_relation limit_projects.select(:id).reorder(nil) -- cgit v1.2.1 From 3d4821a8e76d49b388b218824714d3bcb8c54dbf Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Thu, 11 Apr 2019 18:26:16 +0300 Subject: Hide password on import by url form --- lib/gitlab/url_sanitizer.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'lib') diff --git a/lib/gitlab/url_sanitizer.rb b/lib/gitlab/url_sanitizer.rb index 880712de5fe..215454fe63c 100644 --- a/lib/gitlab/url_sanitizer.rb +++ b/lib/gitlab/url_sanitizer.rb @@ -47,6 +47,10 @@ module Gitlab @credentials ||= { user: @url.user.presence, password: @url.password.presence } end + def user + credentials[:user] + end + def full_url @full_url ||= generate_full_url.to_s end -- cgit v1.2.1 From a9bcddee4c2653cbf2254d893299393e3778e7df Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sun, 21 Apr 2019 12:03:26 +0200 Subject: Protect Gitlab::HTTP against DNS rebinding attack Gitlab::HTTP now resolves the hostname only once, verifies the IP is not blocked, and then uses the same IP to perform the actual request, while passing the original hostname in the `Host` header and SSL SNI field. --- lib/gitlab/http.rb | 2 +- lib/gitlab/http_connection_adapter.rb | 37 +++++++++++++++++ lib/gitlab/proxy_http_connection_adapter.rb | 36 ----------------- lib/gitlab/url_blocker.rb | 61 +++++++++++++++++++++++------ 4 files changed, 86 insertions(+), 50 deletions(-) create mode 100644 lib/gitlab/http_connection_adapter.rb delete mode 100644 lib/gitlab/proxy_http_connection_adapter.rb (limited to 'lib') diff --git a/lib/gitlab/http.rb b/lib/gitlab/http.rb index bcd9e2be35f..55ed7e7e837 100644 --- a/lib/gitlab/http.rb +++ b/lib/gitlab/http.rb @@ -11,7 +11,7 @@ module Gitlab include HTTParty # rubocop:disable Gitlab/HTTParty - connection_adapter ProxyHTTPConnectionAdapter + connection_adapter HTTPConnectionAdapter def self.perform_request(http_method, path, options, &block) super diff --git a/lib/gitlab/http_connection_adapter.rb b/lib/gitlab/http_connection_adapter.rb new file mode 100644 index 00000000000..9ccf0653903 --- /dev/null +++ b/lib/gitlab/http_connection_adapter.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +# This class is part of the Gitlab::HTTP wrapper. Depending on the value +# of the global setting allow_local_requests_from_hooks_and_services this adapter +# will allow/block connection to internal IPs and/or urls. +# +# This functionality can be overridden by providing the setting the option +# allow_local_requests = true in the request. For example: +# Gitlab::HTTP.get('http://www.gitlab.com', allow_local_requests: true) +# +# This option will take precedence over the global setting. +module Gitlab + class HTTPConnectionAdapter < HTTParty::ConnectionAdapter + def connection + begin + @uri, hostname = Gitlab::UrlBlocker.validate!(uri, allow_local_network: allow_local_requests?, + allow_localhost: allow_local_requests?) + rescue Gitlab::UrlBlocker::BlockedUrlError => e + raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}" + end + + super.tap do |http| + http.hostname_override = hostname if hostname + end + end + + private + + def allow_local_requests? + options.fetch(:allow_local_requests, allow_settings_local_requests?) + end + + def allow_settings_local_requests? + Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? + end + end +end diff --git a/lib/gitlab/proxy_http_connection_adapter.rb b/lib/gitlab/proxy_http_connection_adapter.rb deleted file mode 100644 index a64cb47e77e..00000000000 --- a/lib/gitlab/proxy_http_connection_adapter.rb +++ /dev/null @@ -1,36 +0,0 @@ -# frozen_string_literal: true - -# This class is part of the Gitlab::HTTP wrapper. Depending on the value -# of the global setting allow_local_requests_from_hooks_and_services this adapter -# will allow/block connection to internal IPs and/or urls. -# -# This functionality can be overridden by providing the setting the option -# allow_local_requests = true in the request. For example: -# Gitlab::HTTP.get('http://www.gitlab.com', allow_local_requests: true) -# -# This option will take precedence over the global setting. -module Gitlab - class ProxyHTTPConnectionAdapter < HTTParty::ConnectionAdapter - def connection - unless allow_local_requests? - begin - Gitlab::UrlBlocker.validate!(uri, allow_local_network: false) - rescue Gitlab::UrlBlocker::BlockedUrlError => e - raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}" - end - end - - super - end - - private - - def allow_local_requests? - options.fetch(:allow_local_requests, allow_settings_local_requests?) - end - - def allow_settings_local_requests? - Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? - end - end -end diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 641ba70ef83..81935f2b052 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -8,38 +8,52 @@ module Gitlab BlockedUrlError = Class.new(StandardError) class << self - def validate!(url, ports: [], schemes: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false) - return true if url.nil? + # Validates the given url according to the constraints specified by arguments. + # + # ports - Raises error if the given URL port does is not between given ports. + # allow_localhost - Raises error if URL resolves to a localhost IP address and argument is true. + # allow_local_network - Raises error if URL resolves to a link-local address and argument is true. + # ascii_only - Raises error if URL has unicode characters and argument is true. + # enforce_user - Raises error if URL user doesn't start with alphanumeric characters and argument is true. + # enforce_sanitization - Raises error if URL includes any HTML/CSS/JS tags and argument is true. + # + # Returns an array with [, ]. + def validate!(url, ports: [], schemes: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false) # rubocop:disable Metrics/CyclomaticComplexity + return [nil, nil] if url.nil? # Param url can be a string, URI or Addressable::URI uri = parse_url(url) validate_html_tags!(uri) if enforce_sanitization - # Allow imports from the GitLab instance itself but only from the configured ports - return true if internal?(uri) - + hostname = uri.hostname port = get_port(uri) - validate_scheme!(uri.scheme, schemes) - validate_port!(port, ports) if ports.any? - validate_user!(uri.user) if enforce_user - validate_hostname!(uri.hostname) - validate_unicode_restriction!(uri) if ascii_only + + unless internal?(uri) + validate_scheme!(uri.scheme, schemes) + validate_port!(port, ports) if ports.any? + validate_user!(uri.user) if enforce_user + validate_hostname!(hostname) + validate_unicode_restriction!(uri) if ascii_only + end begin - addrs_info = Addrinfo.getaddrinfo(uri.hostname, port, nil, :STREAM).map do |addr| + addrs_info = Addrinfo.getaddrinfo(hostname, port, nil, :STREAM).map do |addr| addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr end rescue SocketError - return true + return [uri, nil] end + # Allow url from the GitLab instance itself but only for the configured hostname and ports + return enforce_uri_hostname(addrs_info, uri, hostname) if internal?(uri) + validate_localhost!(addrs_info) unless allow_localhost validate_loopback!(addrs_info) unless allow_localhost validate_local_network!(addrs_info) unless allow_local_network validate_link_local!(addrs_info) unless allow_local_network - true + enforce_uri_hostname(addrs_info, uri, hostname) end def blocked_url?(*args) @@ -52,6 +66,27 @@ module Gitlab private + # Returns the given URI with IP address as hostname and the original hostname respectively + # in an Array. + # + # It checks whether the resolved IP address matches with the hostname. If not, it changes + # the hostname to the resolved IP address. + # + # The original hostname is used to validate the SSL, given in that scenario + # we'll be making the request to the IP address, instead of using the hostname. + def enforce_uri_hostname(addrs_info, uri, hostname) + address = addrs_info.first + ip_address = address&.ip_address + + if ip_address && ip_address != hostname + uri = uri.dup + uri.hostname = ip_address + return [uri, hostname] + end + + [uri, nil] + end + def get_port(uri) uri.port || uri.default_port end -- cgit v1.2.1 From a1a0f8e6b017f57060bc94d14fd4d37d8756e47d Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Wed, 29 May 2019 13:43:07 -0300 Subject: Add DNS rebinding protection settings --- lib/gitlab.rb | 5 +++++ lib/gitlab/http_connection_adapter.rb | 9 ++++++++- lib/gitlab/url_blocker.rb | 34 ++++++++++++++++++++++++---------- 3 files changed, 37 insertions(+), 11 deletions(-) (limited to 'lib') diff --git a/lib/gitlab.rb b/lib/gitlab.rb index 3f107fbbf3b..ccaf06c5d6a 100644 --- a/lib/gitlab.rb +++ b/lib/gitlab.rb @@ -40,6 +40,7 @@ module Gitlab SUBDOMAIN_REGEX = %r{\Ahttps://[a-z0-9]+\.gitlab\.com\z}.freeze VERSION = File.read(root.join("VERSION")).strip.freeze INSTALLATION_TYPE = File.read(root.join("INSTALLATION_TYPE")).strip.freeze + HTTP_PROXY_ENV_VARS = %w(http_proxy https_proxy HTTP_PROXY HTTPS_PROXY).freeze def self.com? # Check `gl_subdomain?` as well to keep parity with gitlab.com @@ -66,6 +67,10 @@ module Gitlab end end + def self.http_proxy_env? + HTTP_PROXY_ENV_VARS.any? { |name| ENV[name] } + end + def self.process_name return 'sidekiq' if Sidekiq.server? return 'console' if defined?(Rails::Console) diff --git a/lib/gitlab/http_connection_adapter.rb b/lib/gitlab/http_connection_adapter.rb index 9ccf0653903..41eab3658bc 100644 --- a/lib/gitlab/http_connection_adapter.rb +++ b/lib/gitlab/http_connection_adapter.rb @@ -14,7 +14,8 @@ module Gitlab def connection begin @uri, hostname = Gitlab::UrlBlocker.validate!(uri, allow_local_network: allow_local_requests?, - allow_localhost: allow_local_requests?) + allow_localhost: allow_local_requests?, + dns_rebind_protection: dns_rebind_protection?) rescue Gitlab::UrlBlocker::BlockedUrlError => e raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}" end @@ -30,6 +31,12 @@ module Gitlab options.fetch(:allow_local_requests, allow_settings_local_requests?) end + def dns_rebind_protection? + return false if Gitlab.http_proxy_env? + + Gitlab::CurrentSettings.dns_rebinding_protection_enabled? + end + def allow_settings_local_requests? Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? end diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 81935f2b052..9a8df719827 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -18,7 +18,21 @@ module Gitlab # enforce_sanitization - Raises error if URL includes any HTML/CSS/JS tags and argument is true. # # Returns an array with [, ]. - def validate!(url, ports: [], schemes: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false) # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/ParameterLists + def validate!( + url, + ports: [], + schemes: [], + allow_localhost: false, + allow_local_network: true, + ascii_only: false, + enforce_user: false, + enforce_sanitization: false, + dns_rebind_protection: true) + # rubocop:enable Metrics/CyclomaticComplexity + # rubocop:enable Metrics/ParameterLists + return [nil, nil] if url.nil? # Param url can be a string, URI or Addressable::URI @@ -45,15 +59,17 @@ module Gitlab return [uri, nil] end + protected_uri_with_hostname = enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection) + # Allow url from the GitLab instance itself but only for the configured hostname and ports - return enforce_uri_hostname(addrs_info, uri, hostname) if internal?(uri) + return protected_uri_with_hostname if internal?(uri) validate_localhost!(addrs_info) unless allow_localhost validate_loopback!(addrs_info) unless allow_localhost validate_local_network!(addrs_info) unless allow_local_network validate_link_local!(addrs_info) unless allow_local_network - enforce_uri_hostname(addrs_info, uri, hostname) + protected_uri_with_hostname end def blocked_url?(*args) @@ -74,17 +90,15 @@ module Gitlab # # The original hostname is used to validate the SSL, given in that scenario # we'll be making the request to the IP address, instead of using the hostname. - def enforce_uri_hostname(addrs_info, uri, hostname) + def enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection) address = addrs_info.first ip_address = address&.ip_address - if ip_address && ip_address != hostname - uri = uri.dup - uri.hostname = ip_address - return [uri, hostname] - end + return [uri, nil] unless dns_rebind_protection && ip_address && ip_address != hostname - [uri, nil] + uri = uri.dup + uri.hostname = ip_address + [uri, hostname] end def get_port(uri) -- cgit v1.2.1