From 5b291fcbe41564264954618fb1f4086a3be1c600 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 28 Jan 2025 15:44:27 +0100 Subject: [PATCH] Fix incorrect signature after HTTP redirect (#33757) --- .../concerns/signature_verification.rb | 8 +-- app/lib/http_signature_draft.rb | 31 ++++++++++ app/lib/request.rb | 58 ++++++++++--------- spec/lib/request_spec.rb | 31 ++++++++-- 4 files changed, 90 insertions(+), 38 deletions(-) create mode 100644 app/lib/http_signature_draft.rb diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 4ae63632c0cf0d..5f7ef8dd638932 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -117,7 +117,7 @@ def signed_headers def verify_signature_strength! raise SignatureVerificationError, 'Mastodon requires the Date header or (created) pseudo-header to be signed' unless signed_headers.include?('date') || signed_headers.include?('(created)') - raise SignatureVerificationError, 'Mastodon requires the Digest header or (request-target) pseudo-header to be signed' unless signed_headers.include?(Request::REQUEST_TARGET) || signed_headers.include?('digest') + raise SignatureVerificationError, 'Mastodon requires the Digest header or (request-target) pseudo-header to be signed' unless signed_headers.include?(HttpSignatureDraft::REQUEST_TARGET) || signed_headers.include?('digest') raise SignatureVerificationError, 'Mastodon requires the Host header to be signed when doing a GET request' if request.get? && !signed_headers.include?('host') raise SignatureVerificationError, 'Mastodon requires the Digest header to be signed when doing a POST request' if request.post? && !signed_headers.include?('digest') end @@ -155,14 +155,14 @@ def verify_signature(actor, signature, compare_signed_string) def build_signed_string(include_query_string: true) signed_headers.map do |signed_header| case signed_header - when Request::REQUEST_TARGET + when HttpSignatureDraft::REQUEST_TARGET if include_query_string - "#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.original_fullpath}" + "#{HttpSignatureDraft::REQUEST_TARGET}: #{request.method.downcase} #{request.original_fullpath}" else # Current versions of Mastodon incorrectly omit the query string from the (request-target) pseudo-header. # Therefore, temporarily support such incorrect signatures for compatibility. # TODO: remove eventually some time after release of the fixed version - "#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.path}" + "#{HttpSignatureDraft::REQUEST_TARGET}: #{request.method.downcase} #{request.path}" end when '(created)' raise SignatureVerificationError, 'Invalid pseudo-header (created) for rsa-sha256' unless signature_algorithm == 'hs2019' diff --git a/app/lib/http_signature_draft.rb b/app/lib/http_signature_draft.rb new file mode 100644 index 00000000000000..fc0d498b29150a --- /dev/null +++ b/app/lib/http_signature_draft.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +# This implements an older draft of HTTP Signatures: +# https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures + +class HttpSignatureDraft + REQUEST_TARGET = '(request-target)' + + def initialize(keypair, key_id, full_path: true) + @keypair = keypair + @key_id = key_id + @full_path = full_path + end + + def request_target(verb, url) + if url.query.nil? || !@full_path + "#{verb} #{url.path}" + else + "#{verb} #{url.path}?#{url.query}" + end + end + + def sign(signed_headers, verb, url) + signed_headers = signed_headers.merge(REQUEST_TARGET => request_target(verb, url)) + signed_string = signed_headers.map { |key, value| "#{key.downcase}: #{value}" }.join("\n") + algorithm = 'rsa-sha256' + signature = Base64.strict_encode64(@keypair.sign(OpenSSL::Digest.new('SHA256'), signed_string)) + + "keyId=\"#{@key_id}\",algorithm=\"#{algorithm}\",headers=\"#{signed_headers.keys.join(' ').downcase}\",signature=\"#{signature}\"" + end +end diff --git a/app/lib/request.rb b/app/lib/request.rb index f984f0e63e4d0a..4e86cc2fdfe893 100644 --- a/app/lib/request.rb +++ b/app/lib/request.rb @@ -61,8 +61,6 @@ def readpartial(size, buffer = nil) end class Request - REQUEST_TARGET = '(request-target)' - # 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 @@ -78,11 +76,18 @@ def initialize(verb, url, **options) @http_client = options.delete(:http_client) @allow_local = options.delete(:allow_local) @full_path = !options.delete(:omit_query_string) - @options = options.merge(socket_class: use_proxy? || @allow_local ? ProxySocket : Socket) - @options = @options.merge(timeout_class: PerOperationWithDeadline, timeout_options: TIMEOUT) + @options = { + follow: { + max_hops: 3, + on_redirect: ->(response, request) { re_sign_on_redirect(response, request) }, + }, + socket_class: use_proxy? || @allow_local ? ProxySocket : Socket, + }.merge(options) @options = @options.merge(proxy_url) if use_proxy? @headers = {} + @signing = nil + raise Mastodon::HostValidationError, 'Instance does not support hidden service connections' if block_hidden_service? set_common_headers! @@ -92,8 +97,9 @@ def initialize(verb, url, **options) def on_behalf_of(actor, sign_with: nil) raise ArgumentError, 'actor must not be nil' if actor.nil? - @actor = actor - @keypair = sign_with.present? ? OpenSSL::PKey::RSA.new(sign_with) : @actor.keypair + key_id = ActivityPub::TagManager.instance.key_uri_for(actor) + keypair = sign_with.present? ? OpenSSL::PKey::RSA.new(sign_with) : actor.keypair + @signing = HttpSignatureDraft.new(keypair, key_id, full_path: @full_path) self end @@ -119,7 +125,7 @@ def perform end def headers - (@actor ? @headers.merge('Signature' => signature) : @headers).without(REQUEST_TARGET) + (@signing ? @headers.merge('Signature' => signature) : @headers) end class << self @@ -134,14 +140,13 @@ def valid_url?(url) end def http_client - HTTP.use(:auto_inflate).follow(max_hops: 3) + HTTP.use(:auto_inflate) end end private def set_common_headers! - @headers[REQUEST_TARGET] = request_target @headers['User-Agent'] = Mastodon::Version.user_agent @headers['Host'] = @url.host @headers['Date'] = Time.now.utc.httpdate @@ -152,31 +157,28 @@ def set_digest! @headers['Digest'] = "SHA-256=#{Digest::SHA256.base64digest(@options[:body])}" end - def request_target - if @url.query.nil? || !@full_path - "#{@verb} #{@url.path}" - else - "#{@verb} #{@url.path}?#{@url.query}" - end + def signature + @signing.sign(@headers.without('User-Agent', 'Accept-Encoding'), @verb, @url) end - def signature - algorithm = 'rsa-sha256' - signature = Base64.strict_encode64(@keypair.sign(OpenSSL::Digest.new('SHA256'), signed_string)) + def re_sign_on_redirect(_response, request) + # Delete existing signature if there is one, since it will be invalid + request.headers.delete('Signature') - "keyId=\"#{key_id}\",algorithm=\"#{algorithm}\",headers=\"#{signed_headers.keys.join(' ').downcase}\",signature=\"#{signature}\"" - end + return unless @signing.present? && @verb == :get - def signed_string - signed_headers.map { |key, value| "#{key.downcase}: #{value}" }.join("\n") - end + signed_headers = request.headers.to_h.slice(*@headers.keys) + unless @headers.keys.all? { |key| signed_headers.key?(key) } + # We have lost some headers in the process, so don't sign the new + # request, in order to avoid issuing a valid signature with fewer + # conditions than expected. - def signed_headers - @headers.without('User-Agent', 'Accept-Encoding') - end + Rails.logger.warn { "Some headers (#{@headers.keys - signed_headers.keys}) have been lost on redirect from {@uri} to #{request.uri}, this should not happen. Skipping signatures" } + return + end - def key_id - ActivityPub::TagManager.instance.key_uri_for(@actor) + signature_value = @signing.sign(signed_headers.without('User-Agent', 'Accept-Encoding'), @verb, Addressable::URI.parse(request.uri)) + request.headers['Signature'] = signature_value end def http_client diff --git a/spec/lib/request_spec.rb b/spec/lib/request_spec.rb index f17cf637b99a31..79c400e098e489 100644 --- a/spec/lib/request_spec.rb +++ b/spec/lib/request_spec.rb @@ -60,16 +60,12 @@ expect(a_request(:get, 'http://example.com')).to have_been_made.once end - it 'sets headers' do - expect { |block| subject.perform(&block) }.to yield_control - expect(a_request(:get, 'http://example.com').with(headers: subject.headers)).to have_been_made - end - - it 'closes underlying connection' do + it 'makes a request with expected headers, yields, and closes the underlying connection' do allow(subject.send(:http_client)).to receive(:close) expect { |block| subject.perform(&block) }.to yield_control + expect(a_request(:get, 'http://example.com').with(headers: subject.headers)).to have_been_made expect(subject.send(:http_client)).to have_received(:close) end @@ -80,6 +76,29 @@ end end + context 'with a redirect and HTTP signatures' do + let(:account) { Fabricate(:account) } + + before do + stub_request(:get, 'http://example.com').to_return(status: 301, headers: { Location: 'http://redirected.example.com/foo' }) + stub_request(:get, 'http://redirected.example.com/foo').to_return(body: 'lorem ipsum') + end + + it 'makes a request with expected headers and follows redirects' do + expect { |block| subject.on_behalf_of(account).perform(&block) }.to yield_control + + # request.headers includes the `Signature` sent for the first request + expect(a_request(:get, 'http://example.com').with(headers: subject.headers)).to have_been_made.once + + # request.headers includes the `Signature`, but it has changed + expect(a_request(:get, 'http://redirected.example.com/foo').with(headers: subject.headers.merge({ 'Host' => 'redirected.example.com' }))).to_not have_been_made + + # `with(headers: )` matching tests for inclusion, so strip `Signature` + # This doesn't actually test that there is a signature, but it tests that the original signature is not passed + expect(a_request(:get, 'http://redirected.example.com/foo').with(headers: subject.headers.without('Signature').merge({ 'Host' => 'redirected.example.com' }))).to have_been_made.once + end + end + context 'with private host' do around do |example| WebMock.disable!