diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-11-19 11:27:35 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-11-19 11:27:35 +0300 |
commit | 7e9c479f7de77702622631cff2628a9c8dcbc627 (patch) | |
tree | c8f718a08e110ad7e1894510980d2155a6549197 /lib/gitlab/middleware | |
parent | e852b0ae16db4052c1c567d9efa4facc81146e88 (diff) |
Add latest changes from gitlab-org/gitlab@13-6-stable-eev13.6.0-rc42
Diffstat (limited to 'lib/gitlab/middleware')
-rw-r--r-- | lib/gitlab/middleware/handle_malformed_strings.rb | 103 | ||||
-rw-r--r-- | lib/gitlab/middleware/handle_null_bytes.rb | 61 | ||||
-rw-r--r-- | lib/gitlab/middleware/read_only/controller.rb | 39 |
3 files changed, 121 insertions, 82 deletions
diff --git a/lib/gitlab/middleware/handle_malformed_strings.rb b/lib/gitlab/middleware/handle_malformed_strings.rb new file mode 100644 index 00000000000..84f7e2e1b14 --- /dev/null +++ b/lib/gitlab/middleware/handle_malformed_strings.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +module Gitlab + module Middleware + # There is no valid reason for a request to contain a malformed string + # so just return HTTP 400 (Bad Request) if we receive one + class HandleMalformedStrings + include ActionController::HttpAuthentication::Basic + + NULL_BYTE_REGEX = Regexp.new(Regexp.escape("\u0000")).freeze + + attr_reader :app + + def initialize(app) + @app = app + end + + def call(env) + return [400, { 'Content-Type' => 'text/plain' }, ['Bad Request']] if request_contains_malformed_string?(env) + + app.call(env) + end + + private + + def request_contains_malformed_string?(env) + return false if ENV['DISABLE_REQUEST_VALIDATION'] == '1' + + # Duplicate the env, so it is not modified when accessing the parameters + # https://github.com/rails/rails/blob/34991a6ae2fc68347c01ea7382fa89004159e019/actionpack/lib/action_dispatch/http/parameters.rb#L59 + # The modification causes problems with our multipart middleware + request = ActionDispatch::Request.new(env.dup) + + return true if malformed_path?(request.path) + return true if credentials_malformed?(request) + + request.params.values.any? do |value| + param_has_null_byte?(value) + end + rescue ActionController::BadRequest + # If we can't build an ActionDispatch::Request something's wrong + # This would also happen if `#params` contains invalid UTF-8 + # in this case we'll return a 400 + # + true + end + + def malformed_path?(path) + string_malformed?(Rack::Utils.unescape(path)) + rescue ArgumentError + # Rack::Utils.unescape raised this, path is malformed. + true + end + + def credentials_malformed?(request) + credentials = if has_basic_credentials?(request) + decode_credentials(request).presence + else + request.authorization.presence + end + + return false unless credentials + + string_malformed?(credentials) + end + + def param_has_null_byte?(value, depth = 0) + # Guard against possible attack sending large amounts of nested params + # Should be safe as deeply nested params are highly uncommon. + return false if depth > 2 + + depth += 1 + + if value.respond_to?(:match) + string_malformed?(value) + elsif value.respond_to?(:values) + value.values.any? do |hash_value| + param_has_null_byte?(hash_value, depth) + end + elsif value.is_a?(Array) + value.any? do |array_value| + param_has_null_byte?(array_value, depth) + end + else + false + end + end + + def string_malformed?(string) + # We're using match rather than include, because that will raise an ArgumentError + # when the string contains invalid UTF8 + # + # We try to encode the string from ASCII-8BIT to UTF8. If we failed to do + # so for certain characters in the string, those chars are probably incomplete + # multibyte characters. + string.encode(Encoding::UTF_8).match?(NULL_BYTE_REGEX) + rescue ArgumentError, Encoding::UndefinedConversionError + # If we're here, we caught a malformed string. Return true + true + end + end + end +end diff --git a/lib/gitlab/middleware/handle_null_bytes.rb b/lib/gitlab/middleware/handle_null_bytes.rb deleted file mode 100644 index c88dfb6ee0b..00000000000 --- a/lib/gitlab/middleware/handle_null_bytes.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Middleware - # There is no valid reason for a request to contain a null byte (U+0000) - # so just return HTTP 400 (Bad Request) if we receive one - class HandleNullBytes - NULL_BYTE_REGEX = Regexp.new(Regexp.escape("\u0000")).freeze - - attr_reader :app - - def initialize(app) - @app = app - end - - def call(env) - return [400, {}, ["Bad Request"]] if request_has_null_byte?(env) - - app.call(env) - end - - private - - def request_has_null_byte?(request) - return false if ENV['REJECT_NULL_BYTES'] == "1" - - request = Rack::Request.new(request) - - request.params.values.any? do |value| - param_has_null_byte?(value) - end - end - - def param_has_null_byte?(value, depth = 0) - # Guard against possible attack sending large amounts of nested params - # Should be safe as deeply nested params are highly uncommon. - return false if depth > 2 - - depth += 1 - - if value.respond_to?(:match) - string_contains_null_byte?(value) - elsif value.respond_to?(:values) - value.values.any? do |hash_value| - param_has_null_byte?(hash_value, depth) - end - elsif value.is_a?(Array) - value.any? do |array_value| - param_has_null_byte?(array_value, depth) - end - else - false - end - end - - def string_contains_null_byte?(string) - string.match?(NULL_BYTE_REGEX) - end - end - end -end diff --git a/lib/gitlab/middleware/read_only/controller.rb b/lib/gitlab/middleware/read_only/controller.rb index cfea4aaddf3..101172cdfcc 100644 --- a/lib/gitlab/middleware/read_only/controller.rb +++ b/lib/gitlab/middleware/read_only/controller.rb @@ -9,20 +9,19 @@ module Gitlab APPLICATION_JSON_TYPES = %W{#{APPLICATION_JSON} application/vnd.git-lfs+json}.freeze ERROR_MESSAGE = 'You cannot perform write operations on a read-only instance' - WHITELISTED_GIT_ROUTES = { - 'repositories/git_http' => %w{git_upload_pack git_receive_pack} + ALLOWLISTED_GIT_ROUTES = { + 'repositories/git_http' => %w{git_upload_pack} }.freeze - WHITELISTED_GIT_LFS_ROUTES = { - 'repositories/lfs_api' => %w{batch}, - 'repositories/lfs_locks_api' => %w{verify create unlock} + ALLOWLISTED_GIT_LFS_BATCH_ROUTES = { + 'repositories/lfs_api' => %w{batch} }.freeze - WHITELISTED_GIT_REVISION_ROUTES = { + ALLOWLISTED_GIT_REVISION_ROUTES = { 'projects/compare' => %w{create} }.freeze - WHITELISTED_SESSION_ROUTES = { + ALLOWLISTED_SESSION_ROUTES = { 'sessions' => %w{destroy}, 'admin/sessions' => %w{create destroy} }.freeze @@ -55,7 +54,7 @@ module Gitlab def disallowed_request? DISALLOWED_METHODS.include?(@env['REQUEST_METHOD']) && - !whitelisted_routes + !allowlisted_routes end def json_request? @@ -87,8 +86,8 @@ module Gitlab end # Overridden in EE module - def whitelisted_routes - workhorse_passthrough_route? || internal_route? || lfs_route? || compare_git_revisions_route? || sidekiq_route? || session_route? || graphql_query? + def allowlisted_routes + workhorse_passthrough_route? || internal_route? || lfs_batch_route? || compare_git_revisions_route? || sidekiq_route? || session_route? || graphql_query? end # URL for requests passed through gitlab-workhorse to rails-web @@ -96,9 +95,9 @@ module Gitlab def workhorse_passthrough_route? # Calling route_hash may be expensive. Only do it if we think there's a possible match return false unless request.post? && - request.path.end_with?('.git/git-upload-pack', '.git/git-receive-pack') + request.path.end_with?('.git/git-upload-pack') - WHITELISTED_GIT_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) + ALLOWLISTED_GIT_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) end def internal_route? @@ -109,18 +108,16 @@ module Gitlab # Calling route_hash may be expensive. Only do it if we think there's a possible match return false unless request.post? && request.path.end_with?('compare') - WHITELISTED_GIT_REVISION_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) + ALLOWLISTED_GIT_REVISION_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) end - def lfs_route? + # Batch upload requests are blocked in: + # https://gitlab.com/gitlab-org/gitlab/blob/master/app/controllers/repositories/lfs_api_controller.rb#L106 + def lfs_batch_route? # Calling route_hash may be expensive. Only do it if we think there's a possible match - unless request.path.end_with?('/info/lfs/objects/batch', - '/info/lfs/locks', '/info/lfs/locks/verify') || - %r{/info/lfs/locks/\d+/unlock\z}.match?(request.path) - return false - end + return unless request.path.end_with?('/info/lfs/objects/batch') - WHITELISTED_GIT_LFS_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) + ALLOWLISTED_GIT_LFS_BATCH_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) end def session_route? @@ -128,7 +125,7 @@ module Gitlab return false unless request.post? && request.path.end_with?('/users/sign_out', '/admin/session', '/admin/session/destroy') - WHITELISTED_SESSION_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) + ALLOWLISTED_SESSION_ROUTES[route_hash[:controller]]&.include?(route_hash[:action]) end def sidekiq_route? |