Merge branch '241831-null-bytes' into 'master'

Add new middleware to disallow sending Null Bytes

See merge request gitlab-org/gitlab!45223

changelogs/unreleased/241831-null-bytes.yml

+---
+title: Disallow NULL Bytes (U+0000) in requests
+merge_request: 45223
+author:
+type: added

config/application.rb

@@ -28,6 +28,7 @@ module Gitlab
     require_dependency Rails.root.join('lib/gitlab/middleware/basic_health_check')
     require_dependency Rails.root.join('lib/gitlab/middleware/same_site_cookies')
     require_dependency Rails.root.join('lib/gitlab/middleware/handle_ip_spoof_attack_error')
+    require_dependency Rails.root.join('lib/gitlab/middleware/handle_null_bytes')
     require_dependency Rails.root.join('lib/gitlab/runtime')
 
     # Settings in config/environments/* take precedence over those specified here.
@@ -251,6 +252,8 @@ module Gitlab
 
     config.middleware.insert_before ActionDispatch::RemoteIp, ::Gitlab::Middleware::HandleIpSpoofAttackError
 
+    config.middleware.use ::Gitlab::Middleware::HandleNullBytes
+
     # Allow access to GitLab API from other domains
     config.middleware.insert_before Warden::Manager, Rack::Cors do
       headers_to_expose = %w[Link X-Total X-Total-Pages X-Per-Page X-Page X-Next-Page X-Prev-Page X-Gitlab-Blob-Id X-Gitlab-Commit-Id X-Gitlab-Content-Sha256 X-Gitlab-Encoding X-Gitlab-File-Name X-Gitlab-File-Path X-Gitlab-Last-Commit-Id X-Gitlab-Ref X-Gitlab-Size]

lib/gitlab/middleware/handle_null_bytes.rb

+# 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

spec/lib/gitlab/middleware/handle_null_bytes_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require "rack/test"
+
+RSpec.describe Gitlab::Middleware::HandleNullBytes do
+  let(:null_byte) { "\u0000" }
+  let(:error_400) { [400, {}, ["Bad Request"]] }
+  let(:app) { double(:app) }
+
+  subject { described_class.new(app) }
+
+  before do
+    allow(app).to receive(:call) do |args|
+      args
+    end
+  end
+
+  def env_for(params = {})
+    Rack::MockRequest.env_for('/', { params: params })
+  end
+
+  context 'with null bytes in params' do
+    it 'rejects null bytes in a top level param' do
+      env = env_for(name: "null#{null_byte}byte")
+
+      expect(subject.call(env)).to eq error_400
+    end
+
+    it "responds with 400 BadRequest for hashes with strings" do
+      env = env_for(name: { inner_key: "I am #{null_byte} bad" })
+
+      expect(subject.call(env)).to eq error_400
+    end
+
+    it "responds with 400 BadRequest for arrays with strings" do
+      env = env_for(name: ["I am #{null_byte} bad"])
+
+      expect(subject.call(env)).to eq error_400
+    end
+
+    it "responds with 400 BadRequest for arrays containing hashes with string values" do
+      env = env_for(name: [
+        {
+          inner_key: "I am #{null_byte} bad"
+        }
+      ])
+
+      expect(subject.call(env)).to eq error_400
+    end
+
+    it "gives up and does not 400 with too deeply nested params" do
+      env = env_for(name: [
+        {
+          inner_key: { deeper_key: [{ hash_inside_array_key: "I am #{null_byte} bad" }] }
+        }
+      ])
+
+      expect(subject.call(env)).not_to eq error_400
+    end
+  end
+
+  context 'without null bytes in params' do
+    it "does not respond with a 400 for strings" do
+      env = env_for(name: "safe name")
+
+      expect(subject.call(env)).not_to eq error_400
+    end
+
+    it "does not respond with a 400 with no params" do
+      env = env_for
+
+      expect(subject.call(env)).not_to eq error_400
+    end
+  end
+
+  context 'when disabled via env flag' do
+    before do
+      stub_env('REJECT_NULL_BYTES', '1')
+    end
+
+    it 'does not respond with a 400 no matter what' do
+      env = env_for(name: "null#{null_byte}byte")
+
+      expect(subject.call(env)).not_to eq error_400
+    end
+  end
+end

spec/requests/user_sends_null_bytes_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'User sends null bytes as params' do
+  let(:null_byte) { "\u0000" }
+
+  it 'raises a 400 error' do
+    post '/nonexistent', params: { a: "A #{null_byte} nasty string" }
+
+    expect(response).to have_gitlab_http_status(:bad_request)
+    expect(response.body).to eq('Bad Request')
+  end
+end