Validate each upload param key in `multipart.rb`

Keys with unbalanced or incoherent brackets will be rejected.
Too long keys will be rejected.

changelogs/unreleased/security-10io-multipart-check-brackets-balance-in-param-names.yml

+---
+title: Validate each upload param key in multipart.rb
+merge_request:
+author:
+type: security

lib/gitlab/middleware/multipart.rb

@@ -31,6 +31,7 @@ module Gitlab
       RACK_ENV_KEY = 'HTTP_GITLAB_WORKHORSE_MULTIPART_FIELDS'
       JWT_PARAM_SUFFIX = '.gitlab-workhorse-upload'
       JWT_PARAM_FIXED_KEY = 'upload'
+      REWRITTEN_FIELD_NAME_MAX_LENGTH = 10000.freeze
 
       class Handler
         def initialize(env, message)
@@ -41,6 +42,8 @@ module Gitlab
 
         def with_open_files
           @rewritten_fields.each do |field, tmp_path|
+            raise "invalid field: #{field.inspect}" unless valid_field_name?(field)
+
             parsed_field = Rack::Utils.parse_nested_query(field)
             raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1
 
@@ -108,6 +111,17 @@ module Gitlab
 
         private
 
+        def valid_field_name?(name)
+          # length validation
+          return false if name.size >= REWRITTEN_FIELD_NAME_MAX_LENGTH
+
+          # brackets validation
+          return false if name.include?('[]') || name.start_with?('[', ']')
+          return false unless ::Gitlab::Utils.valid_brackets?(name, allow_nested: false)
+
+          true
+        end
+
         def package_allowed_paths
           packages_config = ::Gitlab.config.packages
           return [] unless allow_packages_storage_path?(packages_config)
@@ -141,6 +155,8 @@ module Gitlab
       class HandlerForJWTParams < Handler
         def with_open_files
           @rewritten_fields.keys.each do |field|
+            raise "invalid field: #{field.inspect}" unless valid_field_name?(field)
+
             parsed_field = Rack::Utils.parse_nested_query(field)
             raise "unexpected field: #{field.inspect}" unless parsed_field.count == 1
 

lib/gitlab/utils.rb

@@ -208,5 +208,33 @@ module Gitlab
     def stable_sort_by(list)
       list.sort_by.with_index { |x, idx| [yield(x), idx] }
     end
+
+    # Check for valid brackets (`[` and `]`) in a string using this aspects:
+    # * open brackets count == closed brackets count
+    # * (optionally) reject nested brackets via `allow_nested: false`
+    # * open / close brackets coherence, eg. ][[] -> invalid
+    def valid_brackets?(string = '', allow_nested: true)
+      # remove everything except brackets
+      brackets = string.remove(/[^\[\]]/)
+
+      return true if brackets.empty?
+      # balanced counts check
+      return false if brackets.size.odd?
+
+      unless allow_nested
+        # nested brackets check
+        return false if brackets.include?('[[') || brackets.include?(']]')
+      end
+
+      # open / close brackets coherence check
+      untrimmed = brackets
+      loop do
+        trimmed = untrimmed.gsub('[]', '')
+        return true if trimmed.empty?
+        return false if trimmed == untrimmed
+
+        untrimmed = trimmed
+      end
+    end
   end
 end

spec/features/file_uploads/multipart_invalid_uploads_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Invalid uploads that must be rejected', :api, :js do
+  include_context 'file upload requests helpers'
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user, :admin) }
+  let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
+
+  context 'invalid upload key', :capybara_ignore_server_errors do
+    let(:api_path) { "/projects/#{project.id}/packages/nuget/" }
+    let(:url) { capybara_url(api(api_path)) }
+    let(:file) { fixture_file_upload('spec/fixtures/dk.png') }
+
+    subject do
+      HTTParty.put(
+        url,
+        basic_auth: { user: user.username, password: personal_access_token.token },
+        body: body
+      )
+    end
+
+    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(500) }
+
+        it { expect(subject.body).to include(message.presence || "invalid field: \"#{key_name}\"") }
+      end
+    end
+
+    RSpec.shared_examples 'by rejecting uploads with an invalid key' do
+      it_behaves_like 'rejecting invalid keys', key_name: 'package[test'
+      it_behaves_like 'rejecting invalid keys', key_name: '[]'
+      it_behaves_like 'rejecting invalid keys', key_name: '[package]test'
+      it_behaves_like 'rejecting invalid keys', key_name: 'package][test]]'
+      it_behaves_like 'rejecting invalid keys', key_name: 'package[test[nested]]'
+    end
+
+    # 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 'handling file uploads', 'by rejecting uploads with an invalid key'
+  end
+end

spec/lib/gitlab/middleware/multipart_with_handler_for_jwt_params_spec.rb

@@ -123,15 +123,46 @@ RSpec.describe Gitlab::Middleware::Multipart do
         end
       end
 
-      context 'with invalid key in parameters' do
+      context 'with an invalid upload key' do
         include_context 'with one temporary file for multipart'
 
-        let(:rewritten_fields) { rewritten_fields_hash('file' => uploaded_filepath) }
-        let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'wrong_key', filename: filename, remote_id: remote_id) }
+        RSpec.shared_examples 'rejecting the invalid key' do |key_in_header:, key_in_upload_params:, error_message:|
+          let(:rewritten_fields) { rewritten_fields_hash(key_in_header => uploaded_filepath) }
+          let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: key_in_upload_params, filename: filename, remote_id: remote_id) }
 
           it 'raises an error' do
-          expect { subject }.to raise_error(RuntimeError, 'Empty JWT param: file.gitlab-workhorse-upload')
-        end
+            expect { subject }.to raise_error(RuntimeError, error_message)
+          end
+        end
+
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: 'file',
+                        key_in_upload_params: 'wrong_key',
+                        error_message: 'Empty JWT param: file.gitlab-workhorse-upload'
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: 'user[avatar',
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: 'invalid field: "user[avatar"'
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: '[user]avatar',
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: 'invalid field: "[user]avatar"'
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: 'user[]avatar',
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: 'invalid field: "user[]avatar"'
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: 'user[avatar[image[url]]]',
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: 'invalid field: "user[avatar[image[url]]]"'
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: '[]',
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: 'invalid field: "[]"'
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: 'x' * 11000,
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: "invalid field: \"#{'x' * 11000}\""
       end
 
       context 'with a modified JWT payload' do

spec/lib/gitlab/middleware/multipart_with_handler_spec.rb

@@ -139,6 +139,58 @@ RSpec.describe Gitlab::Middleware::Multipart do
           subject
         end
       end
+
+      context 'with invalid key in header' do
+        include_context 'with one temporary file for multipart'
+
+        RSpec.shared_examples 'rejecting the invalid key' do |key_in_header:, key_in_upload_params:, error_message:|
+          let(:rewritten_fields) { rewritten_fields_hash(key_in_header => uploaded_filepath) }
+          let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: key_in_upload_params, filename: filename, remote_id: remote_id) }
+
+          it 'raises an error' do
+            expect { subject }.to raise_error(RuntimeError, error_message)
+          end
+        end
+
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: 'user[avatar',
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: 'invalid field: "user[avatar"'
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: '[user]avatar',
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: 'invalid field: "[user]avatar"'
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: 'user[]avatar',
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: 'invalid field: "user[]avatar"'
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: 'user[avatar[image[url]]]',
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: 'invalid field: "user[avatar[image[url]]]"'
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: '[]',
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: 'invalid field: "[]"'
+        it_behaves_like 'rejecting the invalid key',
+                        key_in_header: 'x' * 11000,
+                        key_in_upload_params: 'user[avatar]',
+                        error_message: "invalid field: \"#{'x' * 11000}\""
+      end
+
+      context 'with key with unbalanced brackets in header' do
+        include_context 'with one temporary file for multipart'
+
+        let(:invalid_key) { 'user[avatar' }
+        let(:rewritten_fields) { rewritten_fields_hash( invalid_key => uploaded_filepath) }
+        let(:params) { upload_parameters_for(filepath: uploaded_filepath, key: 'user[avatar]', filename: filename, remote_id: remote_id) }
+
+        it 'builds no UploadedFile' do
+          expect(app).not_to receive(:call)
+
+          expect { subject }.to raise_error(RuntimeError, "invalid field: \"#{invalid_key}\"")
+        end
+      end
     end
   end
 end

spec/lib/gitlab/utils_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Utils do
+  using RSpec::Parameterized::TableSyntax
+
   delegate :to_boolean, :boolean_to_yes_no, :slugify, :random_string, :which,
            :ensure_array_from_string, :to_exclusive_sentence, :bytes_to_megabytes,
            :append_path, :check_path_traversal!, :allowlisted?, :check_allowed_absolute_path!, :decode_path, :ms_to_round_sec, to: :described_class
@@ -448,4 +450,29 @@ RSpec.describe Gitlab::Utils do
       end
     end
   end
+
+  describe '.valid_brackets?' do
+    where(:input, :allow_nested, :valid) do
+      'no brackets'              | true  | true
+      'no brackets'              | false | true
+      'user[avatar]'             | true  | true
+      'user[avatar]'             | false | true
+      'user[avatar][friends]'    | true  | true
+      'user[avatar][friends]'    | false | true
+      'user[avatar[image[url]]]' | true  | true
+      'user[avatar[image[url]]]' | false | false
+      'user[avatar[]friends]'    | true  | true
+      'user[avatar[]friends]'    | false | false
+      'user[avatar]]'            | true  | false
+      'user[avatar]]'            | false | false
+      'user][avatar]]'           | true  | false
+      'user][avatar]]'           | false | false
+      'user[avatar'              | true  | false
+      'user[avatar'              | false | false
+    end
+
+    with_them do
+      it { expect(described_class.valid_brackets?(input, allow_nested: allow_nested)).to eq(valid) }
+    end
+  end
 end