Handle invalid strings in authorization headers

When using git-over-http the GitHttpClientController would try to look
up the user or token read from the Authorization headers.

If one of those headers would contain a base64 encoded null-byte,
this would result in an ArgumentError.

This adds support for that to the middleware by decoding the
authorization headers and validating them beforehand.

It will also avoid trying to decode non-base64 encoded headers, and
instead validate the content without as-is.

This reverts commit 44cebe45.

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

+---
+title: Handle nullbytes in auth headers
+merge_request: 47206
+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,18 @@ module Gitlab
         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.
@@ -63,6 +87,8 @@ module Gitlab
       end
 
       def string_malformed?(string)
+        # We're using match rather than include, because that will raise an ArgumentError
+        # when  the string contains invalid UTF8
         string.match?(NULL_BYTE_REGEX)
       rescue ArgumentError
         # If we're here, we caught a malformed string. Return true

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,39 @@ 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
+
+    it 'does not reject correct non-basic-auth tokens' do
+      # This token is known to include a null-byte when we were to try to decode it
+      # as Base64, while it wasn't encoded at such.
+      special_token = 'GL-Geo ta8KakZWpu0AcledQ6n0:eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJkYXRhIjoie1wic2NvcGVcIjpcImdlb19hcGlcIn0iLCJqdGkiOiIwYWFmNzVlYi1lNWRkLTRkZjEtODQzYi1lM2E5ODhhNDMwMzIiLCJpYXQiOjE2MDQ3MDI4NzUsIm5iZiI6MTYwNDcwMjg3MCwiZXhwIjoxNjA0NzAyOTM1fQ.NcgDipDyxSP5uSzxc01ylzH4GkTxJRflNNjT7U6fpg4'
+      expect(Base64.decode64(special_token)).to include(null_byte)
+
+      env = env_for.merge('HTTP_AUTHORIZATION' => special_token)
+
+      expect(subject.call(env)).not_to eq error_400
+    end
+
+    it 'rejects problematic input in non-basic-auth tokens' do
+      env = env_for.merge('HTTP_AUTHORIZATION' => "GL-Geo hello#{problematic_input}world")
+
+      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,6 +121,12 @@ 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: [
@@ -98,12 +139,6 @@ RSpec.describe Gitlab::Middleware::HandleMalformedStrings do
       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 +159,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