Merge branch 'bvl-handle-invalid-headers' into 'master'

Handle nullbytes in auth headers

See merge request gitlab-org/gitlab!46985

changelogs/unreleased/bvl-handle-invalid-headers.yml

+---
+title: Handle nullbytes in auth headers
+merge_request: 46985
+author:
+type: fixed

lib/gitlab/middleware/handle_malformed_strings.rb

@@ -5,6 +5,8 @@ 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
@@ -21,16 +23,26 @@ module Gitlab
 
       private
 
-      def request_contains_malformed_string?(request)
+      def request_contains_malformed_string?(env)
         return false if ENV['DISABLE_REQUEST_VALIDATION'] == '1'
 
-        request = Rack::Request.new(request)
+        # 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)
@@ -40,6 +52,13 @@ 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.

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|
+    RSpec.shared_examples 'rejecting invalid keys' do |key_name:, message: nil, status: 500|
       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(500) }
+        it { expect(subject.code).to eq(status) }
 
         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', message: 'Puma caught this error: expected Hash (got Array)'
+    it_behaves_like 'rejecting invalid keys', key_name: 'package[]test', status: 400, message: 'Bad Request'
 
     it_behaves_like 'handling file uploads', 'by rejecting uploads with an invalid key'
   end

spec/lib/gitlab/middleware/handle_malformed_strings_spec.rb

@@ -4,6 +4,8 @@ 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" }
@@ -57,6 +59,22 @@ 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
@@ -86,24 +104,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 }
@@ -124,4 +142,10 @@ 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

spec/requests/user_sends_malformed_strings_spec.rb

@@ -2,7 +2,9 @@
 
 require 'spec_helper'
 
-RSpec.describe 'User sends malformed strings as params' do
+RSpec.describe 'User sends malformed strings' do
+  include GitHttpHelpers
+
   let(:null_byte) { "\u0000" }
   let(:invalid_string) { "mal\xC0formed" }
 
@@ -17,4 +19,10 @@ RSpec.describe 'User sends malformed strings as params' 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