diff --git a/changelogs/unreleased/bvl-handle-invalid-headers.yml b/changelogs/unreleased/bvl-handle-invalid-headers.yml deleted file mode 100644 index 74f6b5700f19e15a841f22c91a9c12532acf55fb..0000000000000000000000000000000000000000 --- a/changelogs/unreleased/bvl-handle-invalid-headers.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Handle nullbytes in auth headers -merge_request: 46985 -author: -type: fixed diff --git a/lib/gitlab/middleware/handle_malformed_strings.rb b/lib/gitlab/middleware/handle_malformed_strings.rb index 9baa639caeab255d3c8e645f70184ad1686ed13b..bb2a8ead52514bc8c52b830edc077927800d5fde 100644 --- a/lib/gitlab/middleware/handle_malformed_strings.rb +++ b/lib/gitlab/middleware/handle_malformed_strings.rb @@ -5,8 +5,6 @@ module Gitlab # 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 @@ -23,26 +21,16 @@ module Gitlab private - def request_contains_malformed_string?(env) + def request_contains_malformed_string?(request) 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) + request = Rack::Request.new(request) 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) @@ -52,13 +40,6 @@ module Gitlab true end - def credentials_malformed?(request) - credentials = decode_credentials(request).presence - 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. diff --git a/spec/features/file_uploads/multipart_invalid_uploads_spec.rb b/spec/features/file_uploads/multipart_invalid_uploads_spec.rb index b3ace2e30ff033eb4e25cc3b897671a3f22948f1..e9e24c12af1dd228d4db293e814c38f495af0d09 100644 --- a/spec/features/file_uploads/multipart_invalid_uploads_spec.rb +++ b/spec/features/file_uploads/multipart_invalid_uploads_spec.rb @@ -22,13 +22,13 @@ RSpec.describe 'Invalid uploads that must be rejected', :api, :js do ) end - RSpec.shared_examples 'rejecting invalid keys' do |key_name:, message: nil, status: 500| + RSpec.shared_examples 'rejecting invalid keys' do |key_name:, message: nil| context "with invalid key #{key_name}" do let(:body) { { key_name => file, 'package[test][name]' => 'test' } } it { expect { subject }.not_to change { Packages::Package.nuget.count } } - it { expect(subject.code).to eq(status) } + it { expect(subject.code).to eq(500) } it { expect(subject.body).to include(message.presence || "invalid field: \"#{key_name}\"") } end @@ -45,7 +45,7 @@ RSpec.describe 'Invalid uploads that must be rejected', :api, :js do # These keys are rejected directly by rack itself. # The request will not be received by multipart.rb (can't use the 'handling file uploads' shared example) it_behaves_like 'rejecting invalid keys', key_name: 'x' * 11000, message: 'Puma caught this error: exceeded available parameter key space (RangeError)' - it_behaves_like 'rejecting invalid keys', key_name: 'package[]test', status: 400, message: 'Bad Request' + it_behaves_like 'rejecting invalid keys', key_name: 'package[]test', message: 'Puma caught this error: expected Hash (got Array)' it_behaves_like 'handling file uploads', 'by rejecting uploads with an invalid key' end diff --git a/spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb b/spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb index fec273ecafdad0f5428d234ce5995c3965c7de18..5ed1580fa8db7f38e310653d66f0066a00665774 100644 --- a/spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb +++ b/spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb @@ -4,8 +4,6 @@ require 'spec_helper' require "rack/test" RSpec.describe Gitlab::Middleware::HandleMalformedStrings do - include GitHttpHelpers - let(:null_byte) { "\u0000" } let(:escaped_null_byte) { "%00" } let(:invalid_string) { "mal\xC0formed" } @@ -59,22 +57,6 @@ RSpec.describe Gitlab::Middleware::HandleMalformedStrings do end end - context 'in authorization headers' do - let(:problematic_input) { null_byte } - - it 'rejects problematic input in the password' do - env = env_for.merge(auth_env("username", "password#{problematic_input}encoded", nil)) - - expect(subject.call(env)).to eq error_400 - end - - it 'rejects problematic input in the password' do - env = env_for.merge(auth_env("username#{problematic_input}", "password#{problematic_input}encoded", nil)) - - expect(subject.call(env)).to eq error_400 - end - end - context 'in params' do shared_examples_for 'checks params' do it 'rejects bad params in a top level param' do @@ -104,24 +86,24 @@ RSpec.describe Gitlab::Middleware::HandleMalformedStrings do expect(subject.call(env)).to eq error_400 end - end - - context 'with null byte' do - let(:problematic_input) { null_byte } - - it_behaves_like 'checks params' it "gives up and does not reject too deeply nested params" do env = env_for(name: [ - { - inner_key: { deeper_key: [{ hash_inside_array_key: "I am #{problematic_input} bad" }] } - } - ]) + { + inner_key: { deeper_key: [{ hash_inside_array_key: "I am #{problematic_input} bad" }] } + } + ]) expect(subject.call(env)).not_to eq error_400 end end + context 'with null byte' do + it_behaves_like 'checks params' do + let(:problematic_input) { null_byte } + end + end + context 'with malformed strings' do it_behaves_like 'checks params' do let(:problematic_input) { invalid_string } @@ -142,10 +124,4 @@ RSpec.describe Gitlab::Middleware::HandleMalformedStrings do expect(subject.call(env)).not_to eq error_400 end end - - it 'does not modify the env' do - env = env_for - - expect { subject.call(env) }.not_to change { env } - end end diff --git a/spec/requests/user_sends_malformed_strings_spec.rb b/spec/requests/user_sends_malformed_strings_spec.rb index da533606be5fd125d73ef5804e0fe1a27e07671d..b6eda9159bc00b3675e1a23dec0f1df56d95b36e 100644 --- a/spec/requests/user_sends_malformed_strings_spec.rb +++ b/spec/requests/user_sends_malformed_strings_spec.rb @@ -2,9 +2,7 @@ require 'spec_helper' -RSpec.describe 'User sends malformed strings' do - include GitHttpHelpers - +RSpec.describe 'User sends malformed strings as params' do let(:null_byte) { "\u0000" } let(:invalid_string) { "mal\xC0formed" } @@ -19,10 +17,4 @@ RSpec.describe 'User sends malformed strings' do expect(response).to have_gitlab_http_status(:bad_request) end - - it 'raises a 400 error with null bytes in the auth headers' do - clone_get("project/path", user: "hello#{null_byte}", password: "nothing to see") - - expect(response).to have_gitlab_http_status(:bad_request) - end end