Skip to content

Commit

Permalink
Revert "Fix Mastodon not correctly processing HTTP Signatures with qu…
Browse files Browse the repository at this point in the history 
…ery strings (mastodon#28476)"

This reverts commit 59ad2cb.
  • Loading branch information
@noellabo
noellabo committed Jan 27, 2024
1 parent 94b00c3 commit 57ad912
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 127 deletions.
33 changes: 8 additions & 25 deletions app/controllers/concerns/signature_verification.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,24 +83,15 @@ def signed_request_account
raise SignatureVerificationError, "Public key not found for key #{signature_params['keyId']}" if account.nil?

signature = Base64.decode64(signature_params['signature'])
compare_signed_string = build_signed_string(include_query_string: true)
compare_signed_string = build_signed_string

return account unless verify_signature(account, signature, compare_signed_string).nil?

# Compatibility quirk with older Mastodon versions
compare_signed_string = build_signed_string(include_query_string: false)
return actor unless verify_signature(actor, signature, compare_signed_string).nil?

actor = stoplight_wrap_request { actor_refresh_key!(actor) }
account = stoplight_wrap_request { account.possibly_stale? ? account.refresh! : account_refresh_key(account) }

raise SignatureVerificationError, "Public key not found for key #{signature_params['keyId']}" if account.nil?

compare_signed_string = build_signed_string(include_query_string: true)
return actor unless verify_signature(actor, signature, compare_signed_string).nil?

# Compatibility quirk with older Mastodon versions
compare_signed_string = build_signed_string(include_query_string: false)
return actor unless verify_signature(actor, signature, compare_signed_string).nil?
return account unless verify_signature(account, signature, compare_signed_string).nil?

@signature_verification_failure_reason = "Verification failed for #{account.username}@#{account.domain} #{account.uri} using rsa-sha256 (RSASSA-PKCS1-v1_5 with SHA-256)"
@signed_request_account = nil
Expand Down Expand Up @@ -159,24 +150,16 @@ def verify_signature(account, signature, compare_signed_string)
nil
end

def build_signed_string(include_query_string: true)
def build_signed_string
signed_headers.map do |signed_header|
case signed_header
when Request::REQUEST_TARGET
if include_query_string
"#{Request::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}"
end
when '(created)'
if signed_header == Request::REQUEST_TARGET
"#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.path}"
elsif signed_header == '(created)'
raise SignatureVerificationError, 'Invalid pseudo-header (created) for rsa-sha256' unless signature_algorithm == 'hs2019'
raise SignatureVerificationError, 'Pseudo-header (created) used but corresponding argument missing' if signature_params['created'].blank?

"(created): #{signature_params['created']}"
when '(expires)'
elsif signed_header == '(expires)'
raise SignatureVerificationError, 'Invalid pseudo-header (expires) for rsa-sha256' unless signature_algorithm == 'hs2019'
raise SignatureVerificationError, 'Pseudo-header (expires) used but corresponding argument missing' if signature_params['expires'].blank?

Expand Down
44 changes: 8 additions & 36 deletions app/lib/request.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,14 @@
require 'socket'
require 'resolv'

# Use our own timeout class to avoid using HTTP.rb's timeout block
# Monkey-patch the HTTP.rb timeout class to avoid using a timeout block
# around the Socket#open method, since we use our own timeout blocks inside
# that method
#
# Also changes how the read timeout behaves so that it is cumulative (closer
# to HTTP::Timeout::Global, but still having distinct timeouts for other
# operation types)
class PerOperationWithDeadline < HTTP::Timeout::PerOperation
READ_DEADLINE = 30

def initialize(*args)
super

@read_deadline = options.fetch(:read_deadline, READ_DEADLINE)
end

class HTTP::Timeout::PerOperation
def connect(socket_class, host, port, nodelay = false)
@socket = socket_class.open(host, port)
@socket.setsockopt(Socket::IPPROTO_TCP, Socket::TCP_NODELAY, 1) if nodelay
Expand All @@ -32,7 +24,7 @@ def reset_counter

# Read data from the socket
def readpartial(size, buffer = nil)
@deadline ||= Process.clock_gettime(Process::CLOCK_MONOTONIC) + @read_deadline
@deadline ||= Process.clock_gettime(Process::CLOCK_MONOTONIC) + @read_timeout

timeout = false
loop do
Expand All @@ -41,8 +33,7 @@ def readpartial(size, buffer = nil)
return :eof if result.nil?

remaining_time = @deadline - Process.clock_gettime(Process::CLOCK_MONOTONIC)
raise HTTP::TimeoutError, "Read timed out after #{@read_timeout} seconds" if timeout
raise HTTP::TimeoutError, "Read timed out after a total of #{@read_deadline} seconds" if remaining_time <= 0
raise HTTP::TimeoutError, "Read timed out after #{@read_timeout} seconds" if timeout || remaining_time <= 0
return result if result != :wait_readable

# marking the socket for timeout. Why is this not being raised immediately?
Expand All @@ -55,7 +46,7 @@ def readpartial(size, buffer = nil)
# timeout. Else, the first timeout was a proper timeout.
# This hack has to be done because io/wait#wait_readable doesn't provide a value for when
# the socket is closed by the server, and HTTP::Parser doesn't provide the limit for the chunks.
timeout = true unless @socket.to_io.wait_readable([remaining_time, @read_timeout].min)
timeout = true unless @socket.to_io.wait_readable(remaining_time)
end
end
end
Expand All @@ -76,11 +67,8 @@ def initialize(verb, url, **options)
@verb = verb
@url = Addressable::URI.parse(url).normalize
@http_client = options.delete(:http_client)
@allow_local = options.delete(:allow_local)
@full_path = options.delete(:with_query_string)
@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?
@options = options.merge(socket_class: use_proxy? ? ProxySocket : Socket)
@options = @options.merge(Rails.configuration.x.http_client_proxy) if use_proxy?
@headers = {}

raise Mastodon::HostValidationError, 'Instance does not support hidden service connections' if block_hidden_service?
Expand Down Expand Up @@ -150,7 +138,7 @@ def http_client
private

def set_common_headers!
@headers[REQUEST_TARGET] = request_target
@headers[REQUEST_TARGET] = "#{@verb} #{@url.path}"
@headers['User-Agent'] = Mastodon::Version.user_agent
@headers['Host'] = @url.host
@headers['Date'] = Time.now.utc.httpdate
Expand All @@ -161,14 +149,6 @@ 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
end

def signature
algorithm = 'rsa-sha256'
signature = Base64.strict_encode64(@keypair.sign(OpenSSL::Digest.new('SHA256'), signed_string))
Expand Down Expand Up @@ -201,14 +181,6 @@ def use_proxy?
Rails.configuration.x.http_client_proxy.present?
end

def proxy_url
if hidden_service? && Rails.configuration.x.http_client_hidden_proxy.present?
Rails.configuration.x.http_client_hidden_proxy
else
Rails.configuration.x.http_client_proxy
end
end

def block_hidden_service?
!Rails.configuration.x.access_to_hidden_service && /\.(onion|i2p)$/.match?(@url.host)
end
Expand Down
66 changes: 0 additions & 66 deletions spec/requests/signature_verification_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,72 +94,6 @@
end
end

context 'with a valid signature on a GET request that has a query string' do
let(:signature_header) do
'keyId="https://remote.domain/users/bob#main-key",algorithm="rsa-sha256",headers="date host (request-target)",signature="SDMa4r/DQYMXYxVgYO2yEqGWWUXugKjVuz0I8dniQAk+aunzBaF2aPu+4grBfawAshlx1Xytl8lhb0H2MllEz16/tKY7rUrb70MK0w8ohXgpb0qs3YvQgdj4X24L1x2MnkFfKHR/J+7TBlnivq0HZqXm8EIkPWLv+eQxu8fbowLwHIVvRd/3t6FzvcfsE0UZKkoMEX02542MhwSif6cu7Ec/clsY9qgKahb9JVGOGS1op9Lvg/9y1mc8KCgD83U5IxVygYeYXaVQ6gixA9NgZiTCwEWzHM5ELm7w5hpdLFYxYOHg/3G3fiqJzpzNQAcCD4S4JxfE7hMI0IzVlNLT6A=="' # rubocop:disable Layout/LineLength
end

it 'successfuly verifies signature', :aggregate_failures do
expect(signature_header).to eq build_signature_string(actor_keypair, 'https://remote.domain/users/bob#main-key', 'get /activitypub/success?foo=42', { 'Date' => 'Wed, 20 Dec 2023 10:00:00 GMT', 'Host' => 'www.example.com' })

get '/activitypub/success?foo=42', headers: {
'Host' => 'www.example.com',
'Date' => 'Wed, 20 Dec 2023 10:00:00 GMT',
'Signature' => signature_header,
}

expect(response).to have_http_status(200)
expect(body_as_json).to match(
signed_request: true,
signature_actor_id: actor.id.to_s
)
end
end

context 'when the query string is missing from the signature verification (compatibility quirk)' do
let(:signature_header) do
'keyId="https://remote.domain/users/bob#main-key",algorithm="rsa-sha256",headers="date host (request-target)",signature="Z8ilar3J7bOwqZkMp7sL8sRs4B1FT+UorbmvWoE+A5UeoOJ3KBcUmbsh+k3wQwbP5gMNUrra9rEWabpasZGphLsbDxfbsWL3Cf0PllAc7c1c7AFEwnewtExI83/qqgEkfWc2z7UDutXc2NfgAx89Ox8DXU/fA2GG0jILjB6UpFyNugkY9rg6oI31UnvfVi3R7sr3/x8Ea3I9thPvqI2byF6cojknSpDAwYzeKdngX3TAQEGzFHz3SDWwyp3jeMWfwvVVbM38FxhvAnSumw7YwWW4L7M7h4M68isLimoT3yfCn2ucBVL5Dz8koBpYf/40w7QidClAwCafZQFC29yDOg=="' # rubocop:disable Layout/LineLength
end

it 'successfuly verifies signature', :aggregate_failures do
expect(signature_header).to eq build_signature_string(actor_keypair, 'https://remote.domain/users/bob#main-key', 'get /activitypub/success', { 'Date' => 'Wed, 20 Dec 2023 10:00:00 GMT', 'Host' => 'www.example.com' })

get '/activitypub/success?foo=42', headers: {
'Host' => 'www.example.com',
'Date' => 'Wed, 20 Dec 2023 10:00:00 GMT',
'Signature' => signature_header,
}

expect(response).to have_http_status(200)
expect(body_as_json).to match(
signed_request: true,
signature_actor_id: actor.id.to_s
)
end
end

context 'with mismatching query string' do
let(:signature_header) do
'keyId="https://remote.domain/users/bob#main-key",algorithm="rsa-sha256",headers="date host (request-target)",signature="SDMa4r/DQYMXYxVgYO2yEqGWWUXugKjVuz0I8dniQAk+aunzBaF2aPu+4grBfawAshlx1Xytl8lhb0H2MllEz16/tKY7rUrb70MK0w8ohXgpb0qs3YvQgdj4X24L1x2MnkFfKHR/J+7TBlnivq0HZqXm8EIkPWLv+eQxu8fbowLwHIVvRd/3t6FzvcfsE0UZKkoMEX02542MhwSif6cu7Ec/clsY9qgKahb9JVGOGS1op9Lvg/9y1mc8KCgD83U5IxVygYeYXaVQ6gixA9NgZiTCwEWzHM5ELm7w5hpdLFYxYOHg/3G3fiqJzpzNQAcCD4S4JxfE7hMI0IzVlNLT6A=="' # rubocop:disable Layout/LineLength
end

it 'fails to verify signature', :aggregate_failures do
expect(signature_header).to eq build_signature_string(actor_keypair, 'https://remote.domain/users/bob#main-key', 'get /activitypub/success?foo=42', { 'Date' => 'Wed, 20 Dec 2023 10:00:00 GMT', 'Host' => 'www.example.com' })

get '/activitypub/success?foo=43', headers: {
'Host' => 'www.example.com',
'Date' => 'Wed, 20 Dec 2023 10:00:00 GMT',
'Signature' => signature_header,
}

expect(body_as_json).to match(
signed_request: true,
signature_actor_id: nil,
error: anything
)
end
end

context 'with a mismatching path' do
it 'fails to verify signature', :aggregate_failures do
get '/activitypub/alternative-path', headers: {
Expand Down

0 comments on commit 57ad912

Please sign in to comment.