CVE-2023-42450: Improper Neutralization of CRLF Sequences in HTTP Headers ('HTTP Request/Response Splitting') (CWE-113)
value: Mastodon is a free, open-source social network server based on ActivityPub. Starting in version 4.2.0-beta1 and prior to version 4.2.0-rc2, by crafting specific input, attackers can inject arbitrary data into HTTP requests issued by Mastodon. This can be used to perform confused deputy attacks if the server configuration includes `ALLOWED_PRIVATE_ADDRESSES` to allow access to local exploitable services. Version 4.2.0-rc2 has a patch for the issue.
CWE description: The product receives data from an HTTP agent/component (e.g., web server, proxy, browser, etc.), but it does not neutralize or incorrectly neutralizes CR and LF characters before the data is included in outgoing HTTP headers.
CVSS v3 base score: 7.5
Published: 2023-09-19T16:15Z
Repository: https://github.com/mastodon/mastodon
Language: Ruby
Files changed: app/lib/request.rb, spec/controllers/concerns/signature_verification_spec.rb, spec/lib/request_spec.rb
Commit date: 2023-09-19 16:52:52 +0200
Commit message: Merge pull request from GHSA-hcqf-fw2r-52g4

* Revert "Fix request URL normalisation for bare domain and 8-bit characters (#26285)"

This reverts commit 8891d8945d837f0da16a3a5aa2dc9783e39b0acd.

* Revert "Do not normalize URL before fetching it (#26219)"

This reverts commit fd284311e79854d6bc2901a9d9363ba9e7e00513.
Diff: diff --git a/app/lib/request.rb b/app/lib/request.rb
index fa0e3472f67..5f128af734c 100644
--- a/app/lib/request.rb
+++ b/app/lib/request.rb
@@ -66,30 +66,17 @@ class Request
   # We enforce a 5s timeout on DNS resolving, 5s timeout on socket opening
   # and 5s timeout on the TLS handshake, meaning the worst case should take
   # about 15s in total
   TIMEOUT = { connect_timeout: 5, read_timeout: 10, write_timeout: 10, read_deadline: 30 }.freeze
 
-  # Workaround for overly-eager decoding of percent-encoded characters in Addressable::URI#normalized_path
-  # https://github.com/sporkmonger/addressable/issues/366
-  URI_NORMALIZER = lambda do |uri|
-    uri = HTTP::URI.parse(uri)
-
-    HTTP::URI.new(
-      scheme: uri.normalized_scheme,
-      authority: uri.normalized_authority,
-      path: Addressable::URI.normalize_path(encode_non_ascii(uri.path)).presence || '/',
-      query: encode_non_ascii(uri.query)
-    )
-  end
-
   include RoutingHelper
 
   def initialize(verb, url, **options)
     raise ArgumentError if url.blank?
 
     @verb        = verb
-    @url         = URI_NORMALIZER.call(url)
+    @url         = Addressable::URI.parse(url).normalize
     @http_client = options.delete(:http_client)
     @allow_local = options.delete(:allow_local)
     @options     = options.merge(socket_class: use_proxy? || @allow_local ? ProxySocket : Socket)
     @options     = @options.merge(timeout_class: PerOperationWithDeadline, timeout_options: TIMEOUT)
     @options     = @options.merge(proxy_url) if use_proxy?
@@ -149,18 +136,12 @@ class Request
       end
 
       %w(http https).include?(parsed_url.scheme) && parsed_url.host.present?
     end
 
-    NON_ASCII_PATTERN = /[^\x00-\x7F]+/
-
-    def encode_non_ascii(str)
-      str&.gsub(NON_ASCII_PATTERN) { |substr| CGI.escape(substr.encode(Encoding::UTF_8)) }
-    end
-
     def http_client
-      HTTP.use(:auto_inflate).use(normalize_uri: { normalizer: URI_NORMALIZER }).follow(max_hops: 3)
+      HTTP.use(:auto_inflate).follow(max_hops: 3)
     end
   end
 
   private
 
diff --git a/spec/controllers/concerns/signature_verification_spec.rb b/spec/controllers/concerns/signature_verification_spec.rb
index f8e44845d7b..650cd21eaf5 100644
--- a/spec/controllers/concerns/signature_verification_spec.rb
+++ b/spec/controllers/concerns/signature_verification_spec.rb
@@ -127,41 +127,10 @@ describe SignatureVerification do
           expect(controller.signed_request_actor).to eq actor
         end
       end
     end
 
-    context 'with non-normalized URL' do
-      before do
-        get :success
-
-        fake_request = Request.new(:get, 'http://test.host/subdir/../success')
-        fake_request.on_behalf_of(author)
-
-        request.headers.merge!(fake_request.headers)
-
-        allow(controller).to receive(:actor_refresh_key!).and_return(author)
-      end
-
-      describe '#build_signed_string' do
-        it 'includes the normalized request path' do
-          expect(controller.send(:build_signed_string)).to start_with "(request-target): get /success\n"
-        end
-      end
-
-      describe '#signed_request?' do
-        it 'returns true' do
-          expect(controller.signed_request?).to be true
-        end
-      end
-
-      describe '#signed_request_actor' do
-        it 'returns an account' do
-          expect(controller.signed_request_account).to eq author
-        end
-      end
-    end
-
     context 'with request with unparsable Date header' do
       before do
         get :success
 
         fake_request = Request.new(:get, request.url)
@@ -231,11 +200,11 @@ describe SignatureVerification do
         fake_request.on_behalf_of(author)
         author.destroy
 
         request.headers.merge!(fake_request.headers)
 
-        stub_request(:get, 'http://localhost:5000/actor').to_raise(Mastodon::HostValidationError)
+        stub_request(:get, 'http://localhost:5000/actor#main-key').to_raise(Mastodon::HostValidationError)
       end
 
       describe '#signed_request?' do
         it 'returns true' do
           expect(controller.signed_request?).to be true
diff --git a/spec/lib/request_spec.rb b/spec/lib/request_spec.rb
index 8ccfcacef24..f0861376b99 100644
--- a/spec/lib/request_spec.rb
+++ b/spec/lib/request_spec.rb
@@ -2,13 +2,11 @@
 
 require 'rails_helper'
 require 'securerandom'
 
 describe Request do
-  subject { described_class.new(:get, url) }
-
-  let(:url) { 'http://example.com' }
+  subject { described_class.new(:get, 'http://example.com') }
 
   describe '#headers' do
     it 'returns user agent' do
       expect(subject.headers['User-Agent']).to be_present
     end
@@ -92,156 +90,10 @@ describe Request do
         allow(Resolv::DNS).to receive(:open).and_yield(resolver)
 
         expect { subject.perform }.to raise_error Mastodon::ValidationError
       end
     end
-
-    context 'with bare domain URL' do
-      let(:url) { 'http://example.com' }
-
-      before do
-        stub_request(:get, 'http://example.com')
-      end
-
-      it 'normalizes path' do
-        subject.perform do |response|
-          expect(response.request.uri.path).to eq '/'
-        end
-      end
-
-      it 'normalizes path used for request signing' do
-        subject.perform
-
-        headers = subject.instance_variable_get(:@headers)
-        expect(headers[Request::REQUEST_TARGET]).to eq 'get /'
-      end
-
-      it 'normalizes path used in request line' do
-        subject.perform do |response|
-          expect(response.request.headline).to eq 'GET / HTTP/1.1'
-        end
-      end
-    end
-
-    context 'with unnormalized URL' do
-      let(:url) { 'HTTP://EXAMPLE.com:80/foo%41%3A?bar=%41%3A#baz' }
-
-      before do
-        stub_request(:get, 'http://example.com/foo%41%3A?bar=%41%3A')
-      end
-
-      it 'normalizes scheme' do
-        subject.perform do |response|
-          expect(response.request.uri.scheme).to eq 'http'
-        end
-      end
-
-      it 'normalizes host' do
-        subject.perform do |response|
-          expect(response.request.uri.authority).to eq 'example.com'
-        end
-      end
-
-      it 'does not modify path' do
-        subject.perform do |response|
-          expect(response.request.uri.path).to eq '/foo%41%3A'
-        end
-      end
-
-      it 'does not modify query string' do
-        subject.perform do |response|
-          expect(response.request.uri.query).to eq 'bar=%41%3A'
-        end
-      end
-
-      it 'does not modify path used for request signing' do
-        subject.perform
-
-        headers = subject.instance_variable_get(:@headers)
-        expect(headers[Request::REQUEST_TARGET]).to eq 'get /foo%41%3A'
-      end
-
-      it 'does not modify path used in request line' do
-        subject.perform do |response|
-          expect(response.request.headline).to eq 'GET /foo%41%3A?bar=%41%3A HTTP/1.1'
-        end
-      end
-
-      it 'strips fragment' do
-        subject.perform do |response|
-          expect(response.request.uri.fragment).to be_nil
-        end
-      end
-    end
-
-    context 'with non-ASCII URL' do
-      let(:url) { 'http://éxample.com:81/föo?bär=1' }
-
-      before do
-        stub_request(:get, 'http://xn--xample-9ua.com:81/f%C3%B6o?b%C3%A4r=1')
-      end
-
-      it 'IDN-encodes host' do
-        subject.perform do |response|
-          expect(response.request.uri.authority).to eq 'xn--xample-9ua.com:81'
-        end
-      end
-
-      it 'IDN-encodes host in Host header' do
-        subject.perform do |response|
-          expect(response.request.headers['Host']).to eq 'xn--xample-9ua.com'
-        end
-      end
-
-      it 'percent-escapes path used for request signing' do
-        subject.perform
-
-        headers = subject.instance_variable_get(:@headers)
-        expect(headers[Request::REQUEST_TARGET]).to eq 'get /f%C3%B6o'
-      end
-
-      it 'normalizes path used in request line' do
-        subject.perform do |response|
-          expect(response.request.headline).to eq 'GET /f%C3%B6o?b%C3%A4r=1 HTTP/1.1'
-        end
-      end
-    end
-
-    context 'with redirecting URL' do
-      let(:url) { 'http://example.com/foo' }
-
-      before do
-        stub_request(:get, 'http://example.com/foo').to_return(status: 302, headers: { 'Location' => 'HTTPS://EXAMPLE.net/Bar' })
-        stub_request(:get, 'https://example.net/Bar').to_return(body: 'Lorem ipsum')
-      end
-
-      it 'resolves redirect' do
-        subject.perform do |response|
-          expect(response.body.to_s).to eq 'Lorem ipsum'
-        end
-
-        expect(a_request(:get, 'https://example.net/Bar')).to have_been_made
-      end
-
-      it 'normalizes destination scheme' do
-        subject.perform do |response|
-          expect(response.request.uri.scheme).to eq 'https'
-        end
-      end
-
-      it 'normalizes destination host' do
-        subject.perform do |response|
-          expect(response.request.uri.authority).to eq 'example.net'
-        end
-      end
-
-      it 'does modify path' do
-        subject.perform do |response|
-          expect(response.request.uri.path).to eq '/Bar'
-        end
-      end
-    end
   end
 
   describe "response's body_with_limit method" do
     it 'rejects body more than 1 megabyte by default' do
       stub_request(:any, 'http://example.com').to_return(body: SecureRandom.random_bytes(2.megabytes))

Vulnerable code:   # Workaround for overly-eager decoding of percent-encoded characters in Addressable::URI#normalized_path
  # https://github.com/sporkmonger/addressable/issues/366
  URI_NORMALIZER = lambda do |uri|
    uri = HTTP::URI.parse(uri)

    HTTP::URI.new(
      scheme: uri.normalized_scheme,
      authority: uri.normalized_authority,
      path: Addressable::URI.normalize_path(encode_non_ascii(uri.path)).presence || '/',
      query: encode_non_ascii(uri.query)
    )
  end

    @url         = URI_NORMALIZER.call(url)
    NON_ASCII_PATTERN = /[^\x00-\x7F]+/

    def encode_non_ascii(str)
      str&.gsub(NON_ASCII_PATTERN) { |substr| CGI.escape(substr.encode(Encoding::UTF_8)) }
    end

      HTTP.use(:auto_inflate).use(normalize_uri: { normalizer: URI_NORMALIZER }).follow(max_hops: 3)
    context 'with non-normalized URL' do
      before do
        get :success

        fake_request = Request.new(:get, 'http://test.host/subdir/../success')
        fake_request.on_behalf_of(author)

        request.headers.merge!(fake_request.headers)

        allow(controller).to receive(:actor_refresh_key!).and_return(author)
      end

      describe '#build_signed_string' do
        it 'includes the normalized request path' do
          expect(controller.send(:build_signed_string)).to start_with "(request-target): get /success\n"
        end
      end

      describe '#signed_request?' do
        it 'returns true' do
          expect(controller.signed_request?).to be true
        end
      end

      describe '#signed_request_actor' do
        it 'returns an account' do
          expect(controller.signed_request_account).to eq author
        end
      end
    end

        stub_request(:get, 'http://localhost:5000/actor').to_raise(Mastodon::HostValidationError)
  subject { described_class.new(:get, url) }

  let(:url) { 'http://example.com' }

    context 'with bare domain URL' do
      let(:url) { 'http://example.com' }

      before do
        stub_request(:get, 'http://example.com')
      end

      it 'normalizes path' do
        subject.perform do |response|
          expect(response.request.uri.path).to eq '/'
        end
      end

      it 'normalizes path used for request signing' do
        subject.perform

        headers = subject.instance_variable_get(:@headers)
        expect(headers[Request::REQUEST_TARGET]).to eq 'get /'
      end

      it 'normalizes path used in request line' do
        subject.perform do |response|
          expect(response.request.headline).to eq 'GET / HTTP/1.1'
        end
      end
    end

    context 'with unnormalized URL' do
      let(:url) { 'HTTP://EXAMPLE.com:80/foo%41%3A?bar=%41%3A#baz' }

      before do
        stub_request(:get, 'http://example.com/foo%41%3A?bar=%41%3A')
      end

      it 'normalizes scheme' do
        subject.perform do |response|
          expect(response.request.uri.scheme).to eq 'http'
        end
      end

      it 'normalizes host' do
        subject.perform do |response|
          expect(response.request.uri.authority).to eq 'example.com'
        end
      end

      it 'does not modify path' do
        subject.perform do |response|
          expect(response.request.uri.path).to eq '/foo%41%3A'
        end
      end

      it 'does not modify query string' do
        subject.perform do |response|
          expect(response.request.uri.query).to eq 'bar=%41%3A'
        end
      end

      it 'does not modify path used for request signing' do
        subject.perform

        headers = subject.instance_variable_get(:@headers)
        expect(headers[Request::REQUEST_TARGET]).to eq 'get /foo%41%3A'
      end

      it 'does not modify path used in request line' do
        subject.perform do |response|
          expect(response.request.headline).to eq 'GET /foo%41%3A?bar=%41%3A HTTP/1.1'
        end
      end

      it 'strips fragment' do
        subject.perform do |response|
          expect(response.request.uri.fragment).to be_nil
        end
      end
    end

    context 'with non-ASCII URL' do
      let(:url) { 'http://éxample.com:81/föo?bär=1' }

      before do
        stub_request(:get, 'http://xn--xample-9ua.com:81/f%C3%B6o?b%C3%A4r=1')
      end

      it 'IDN-encodes host' do
        subject.perform do |response|
          expect(response.request.uri.authority).to eq 'xn--xample-9ua.com:81'
        end
      end

      it 'IDN-encodes host in Host header' do
        subject.perform do |response|
          expect(response.request.headers['Host']).to eq 'xn--xample-9ua.com'
        end
      end

      it 'percent-escapes path used for request signing' do
        subject.perform

        headers = subject.instance_variable_get(:@headers)
        expect(headers[Request::REQUEST_TARGET]).to eq 'get /f%C3%B6o'
      end

      it 'normalizes path used in request line' do
        subject.perform do |response|
          expect(response.request.headline).to eq 'GET /f%C3%B6o?b%C3%A4r=1 HTTP/1.1'
        end
      end
    end

    context 'with redirecting URL' do
      let(:url) { 'http://example.com/foo' }

      before do
        stub_request(:get, 'http://example.com/foo').to_return(status: 302, headers: { 'Location' => 'HTTPS://EXAMPLE.net/Bar' })
        stub_request(:get, 'https://example.net/Bar').to_return(body: 'Lorem ipsum')
      end

      it 'resolves redirect' do
        subject.perform do |response|
          expect(response.body.to_s).to eq 'Lorem ipsum'
        end

        expect(a_request(:get, 'https://example.net/Bar')).to have_been_made
      end

      it 'normalizes destination scheme' do
        subject.perform do |response|
          expect(response.request.uri.scheme).to eq 'https'
        end
      end

      it 'normalizes destination host' do
        subject.perform do |response|
          expect(response.request.uri.authority).to eq 'example.net'
        end
      end

      it 'does modify path' do
        subject.perform do |response|
          expect(response.request.uri.path).to eq '/Bar'
        end
      end
    end
Fixed code:     @url         = Addressable::URI.parse(url).normalize
      HTTP.use(:auto_inflate).follow(max_hops: 3)
        stub_request(:get, 'http://localhost:5000/actor#main-key').to_raise(Mastodon::HostValidationError)
  subject { described_class.new(:get, 'http://example.com') }