diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 4dd0cac55..89dc828f4 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -93,11 +93,15 @@ module SignatureVerification 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 + fail_with! "Verification failed for #{account.username}@#{account.domain} #{account.uri} using rsa-sha256 (RSASSA-PKCS1-v1_5 with SHA-256)" rescue SignatureVerificationError => e - @signature_verification_failure_reason = e.message - @signed_request_account = nil + fail_with! e.message + rescue HTTP::Error, OpenSSL::SSL::SSLError => e + fail_with! "Failed to fetch remote data: #{e.message}" + rescue Mastodon::UnexptectedResponseError + fail_with! 'Failed to fetch remote data (got unexpected reply from server)' + rescue Stoplight::Error::RedLight + fail_with! 'Fetching attempt skipped because of recent connection failure' end def request_body @@ -106,6 +110,11 @@ module SignatureVerification private + def fail_with!(message) + @signature_verification_failure_reason = message + @signed_request_account = nil + end + def signature_params @signature_params ||= begin raw_signature = request.headers['Signature'] @@ -138,7 +147,17 @@ module SignatureVerification digests = request.headers['Digest'].split(',').map { |digest| digest.split('=', 2) }.map { |key, value| [key.downcase, value] } sha256 = digests.assoc('sha-256') raise SignatureVerificationError, "Mastodon only supports SHA-256 in Digest header. Offered algorithms: #{digests.map(&:first).join(', ')}" if sha256.nil? - raise SignatureVerificationError, "Invalid Digest value. Computed SHA-256 digest: #{body_digest}; given: #{sha256[1]}" if body_digest != sha256[1] + + return if body_digest == sha256[1] + + digest_size = begin + Base64.strict_decode64(sha256[1].strip).length + rescue ArgumentError + raise SignatureVerificationError, "Invalid Digest value. The provided Digest value is not a valid base64 string. Given digest: #{sha256[1]}" + end + + raise SignatureVerificationError, "Invalid Digest value. The provided Digest value is not a SHA-256 digest. Given digest: #{sha256[1]}" if digest_size != 32 + raise SignatureVerificationError, "Invalid Digest value. Computed SHA-256 digest: #{body_digest}; given: #{sha256[1]}" end def verify_signature(account, signature, compare_signed_string) @@ -216,19 +235,20 @@ module SignatureVerification end if key_id.start_with?('acct:') - stoplight_wrap_request { ResolveAccountService.new.call(key_id.gsub(/\Aacct:/, '')) } + stoplight_wrap_request { ResolveAccountService.new.call(key_id.gsub(/\Aacct:/, ''), suppress_errors: false) } elsif !ActivityPub::TagManager.instance.local_uri?(key_id) account = ActivityPub::TagManager.instance.uri_to_resource(key_id, Account) - account ||= stoplight_wrap_request { ActivityPub::FetchRemoteKeyService.new.call(key_id, id: false) } + account ||= stoplight_wrap_request { ActivityPub::FetchRemoteKeyService.new.call(key_id, id: false, suppress_errors: false) } account end - rescue Mastodon::HostValidationError - nil + rescue Mastodon::PrivateNetworkAddressError => e + raise SignatureVerificationError, "Requests to private network addresses are disallowed (tried to query #{e.host})" + rescue Mastodon::HostValidationError, ActivityPub::FetchRemoteAccountService::Error, ActivityPub::FetchRemoteKeyService::Error, Webfinger::Error => e + raise SignatureVerificationError, e.message end def stoplight_wrap_request(&block) Stoplight("source:#{request.remote_ip}", &block) - .with_fallback { nil } .with_threshold(1) .with_cool_off_time(5.minutes.seconds) .with_error_handler { |error, handle| error.is_a?(HTTP::Error) || error.is_a?(OpenSSL::SSL::SSLError) ? handle.call(error) : raise(error) } @@ -237,6 +257,10 @@ module SignatureVerification def account_refresh_key(account) return if account.local? || !account.activitypub? - ActivityPub::FetchRemoteAccountService.new.call(account.uri, only_key: true) + ActivityPub::FetchRemoteAccountService.new.call(account.uri, only_key: true, suppress_errors: false) + rescue Mastodon::PrivateNetworkAddressError => e + raise SignatureVerificationError, "Requests to private network addresses are disallowed (tried to query #{e.host})" + rescue Mastodon::HostValidationError, ActivityPub::FetchRemoteAccountService::Error, Webfinger::Error => e + raise SignatureVerificationError, e.message end end diff --git a/app/lib/request.rb b/app/lib/request.rb index f5123d776..eac04c798 100644 --- a/app/lib/request.rb +++ b/app/lib/request.rb @@ -208,7 +208,7 @@ class Request addresses.each do |address| begin - check_private_address(address) + check_private_address(address, host) sock = ::Socket.new(address.is_a?(Resolv::IPv6) ? ::Socket::AF_INET6 : ::Socket::AF_INET, ::Socket::SOCK_STREAM, 0) sockaddr = ::Socket.pack_sockaddr_in(port, address.to_s) @@ -264,10 +264,10 @@ class Request alias new open - def check_private_address(address) + def check_private_address(address, host) addr = IPAddr.new(address.to_s) return if private_address_exceptions.any? { |range| range.include?(addr) } - raise Mastodon::HostValidationError if PrivateAddressCheck.private_address?(addr) + raise Mastodon::PrivateNetworkAddressError, host if PrivateAddressCheck.private_address?(addr) end def private_address_exceptions diff --git a/app/lib/webfinger.rb b/app/lib/webfinger.rb index a681e0815..7c0c10c33 100644 --- a/app/lib/webfinger.rb +++ b/app/lib/webfinger.rb @@ -3,7 +3,7 @@ class Webfinger class Error < StandardError; end class GoneError < Error; end - class RedirectError < StandardError; end + class RedirectError < Error; end class Response attr_reader :uri diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb index 9d01f5386..d7d739c59 100644 --- a/app/services/activitypub/fetch_remote_account_service.rb +++ b/app/services/activitypub/fetch_remote_account_service.rb @@ -5,10 +5,12 @@ class ActivityPub::FetchRemoteAccountService < BaseService include DomainControlHelper include WebfingerHelper + class Error < StandardError; end + SUPPORTED_TYPES = %w(Application Group Organization Person Service).freeze # Does a WebFinger roundtrip on each call, unless `only_key` is true - def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false) + def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true) return if domain_not_allowed?(uri) return ActivityPub::TagManager.instance.uri_to_resource(uri, Account) if ActivityPub::TagManager.instance.local_uri?(uri) @@ -18,38 +20,50 @@ class ActivityPub::FetchRemoteAccountService < BaseService else body_to_json(prefetched_body, compare_id: id ? uri : nil) end + rescue Oj::ParseError + raise Error, "Error parsing JSON-LD document #{uri}" end - return if !supported_context? || !expected_type? || (break_on_redirect && @json['movedTo'].present?) + raise Error, "Error fetching actor JSON at #{uri}" if @json.nil? + raise Error, "Unsupported JSON-LD context for document #{uri}" unless supported_context? + raise Error, "Unexpected object type for actor #{uri} (expected any of: #{SUPPORTED_TYPES})" unless expected_type? + raise Error, "Actor #{uri} has moved to #{@json['movedTo']}" if break_on_redirect && @json['movedTo'].present? @uri = @json['id'] @username = @json['preferredUsername'] @domain = Addressable::URI.parse(@uri).normalized_host - return unless only_key || verified_webfinger? + check_webfinger! unless only_key ActivityPub::ProcessAccountService.new.call(@username, @domain, @json, only_key: only_key, verified_webfinger: !only_key) - rescue Oj::ParseError - nil + rescue Error => e + Rails.logger.debug "Fetching account #{uri} failed: #{e.message}" + raise unless suppress_errors end private - def verified_webfinger? + def check_webfinger! webfinger = webfinger!("acct:#{@username}@#{@domain}") confirmed_username, confirmed_domain = split_acct(webfinger.subject) - return webfinger.link('self', 'href') == @uri if @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero? + if @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero? + raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.link('self', 'href') != @uri + return + end webfinger = webfinger!("acct:#{confirmed_username}@#{confirmed_domain}") @username, @domain = split_acct(webfinger.subject) - return false unless @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero? - return false if webfinger.link('self', 'href') != @uri + unless confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero? + raise Webfinger::RedirectError, "Too many webfinger redirects for URI #{uri} (stopped at #{@username}@#{@domain})" + end - true - rescue Webfinger::Error - false + raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.link('self', 'href') != @uri + rescue Webfinger::RedirectError => e + raise Error, e.message + rescue Webfinger::Error => e + raise Error, "Webfinger error when resolving #{@username}@#{@domain}: #{e.message}" end def split_acct(acct) diff --git a/app/services/activitypub/fetch_remote_key_service.rb b/app/services/activitypub/fetch_remote_key_service.rb index c48288b3b..01008d883 100644 --- a/app/services/activitypub/fetch_remote_key_service.rb +++ b/app/services/activitypub/fetch_remote_key_service.rb @@ -3,9 +3,11 @@ class ActivityPub::FetchRemoteKeyService < BaseService include JsonLdHelper + class Error < StandardError; end + # Returns account that owns the key - def call(uri, id: true, prefetched_body: nil) - return if uri.blank? + def call(uri, id: true, prefetched_body: nil, suppress_errors: true) + raise Error, 'No key URI given' if uri.blank? if prefetched_body.nil? if id @@ -13,7 +15,7 @@ class ActivityPub::FetchRemoteKeyService < BaseService if person? @json = fetch_resource(@json['id'], true) elsif uri != @json['id'] - return + raise Error, "Fetched URI #{uri} has wrong id #{@json['id']}" end else @json = fetch_resource(uri, id) @@ -22,21 +24,29 @@ class ActivityPub::FetchRemoteKeyService < BaseService @json = body_to_json(prefetched_body, compare_id: id ? uri : nil) end - return unless supported_context?(@json) && expected_type? - return find_account(@json['id'], @json) if person? + raise Error, "Unable to fetch key JSON at #{uri}" if @json.nil? + raise Error, "Unsupported JSON-LD context for document #{uri}" unless supported_context?(@json) + raise Error, "Unexpected object type for key #{uri}" unless expected_type? + return find_account(@json['id'], @json, suppress_errors) if person? @owner = fetch_resource(owner_uri, true) - return unless supported_context?(@owner) && confirmed_owner? + raise Error, "Unable to fetch actor JSON #{owner_uri}" if @owner.nil? + raise Error, "Unsupported JSON-LD context for document #{owner_uri}" unless supported_context?(@owner) + raise Error, "Unexpected object type for actor #{owner_uri} (expected any of: #{SUPPORTED_TYPES})" unless expected_owner_type? + raise Error, "publicKey id for #{owner_uri} does not correspond to #{@json['id']}" unless confirmed_owner? - find_account(owner_uri, @owner) + find_account(owner_uri, @owner, suppress_errors) + rescue Error => e + Rails.logger.debug "Fetching key #{uri} failed: #{e.message}" + raise unless suppress_errors end private - def find_account(uri, prefetched_body) + def find_account(uri, prefetched_body, suppress_errors) account = ActivityPub::TagManager.instance.uri_to_resource(uri, Account) - account ||= ActivityPub::FetchRemoteAccountService.new.call(uri, prefetched_body: prefetched_body) + account ||= ActivityPub::FetchRemoteAccountService.new.call(uri, prefetched_body: prefetched_body, suppress_errors: suppress_errors) account end @@ -56,7 +66,11 @@ class ActivityPub::FetchRemoteKeyService < BaseService @owner_uri ||= value_or_id(@json['owner']) end + def expected_owner_type? + equals_or_includes_any?(@owner['type'], ActivityPub::FetchRemoteAccountService::SUPPORTED_TYPES) + end + def confirmed_owner? - equals_or_includes_any?(@owner['type'], ActivityPub::FetchRemoteAccountService::SUPPORTED_TYPES) && value_or_id(@owner['publicKey']) == @json['id'] + value_or_id(@owner['publicKey']) == @json['id'] end end diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 34750dba6..456b3524b 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -32,8 +32,6 @@ class ActivityPub::ProcessAccountService < BaseService process_duplicate_accounts! if @options[:verified_webfinger] end - return if @account.nil? - after_protocol_change! if protocol_changed? after_key_change! if key_changed? && !@options[:signed_with_known_key] clear_tombstones! if key_changed? diff --git a/app/services/resolve_account_service.rb b/app/services/resolve_account_service.rb index b55e45409..e3b370968 100644 --- a/app/services/resolve_account_service.rb +++ b/app/services/resolve_account_service.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true class ResolveAccountService < BaseService - include JsonLdHelper include DomainControlHelper include WebfingerHelper include Redisable @@ -13,6 +12,7 @@ class ResolveAccountService < BaseService # @param [Hash] options # @option options [Boolean] :redirected Do not follow further Webfinger redirects # @option options [Boolean] :skip_webfinger Do not attempt any webfinger query or refreshing account data + # @option options [Boolean] :suppress_errors When failing, return nil instead of raising an error # @return [Account] def call(uri, options = {}) return if uri.blank? @@ -52,15 +52,15 @@ class ResolveAccountService < BaseService # either needs to be created, or updated from fresh data fetch_account! - rescue Webfinger::Error, Oj::ParseError => e + rescue Webfinger::Error => e Rails.logger.debug "Webfinger query for #{@uri} failed: #{e}" - nil + raise unless @options[:suppress_errors] end private def process_options!(uri, options) - @options = options + @options = { suppress_errors: true }.merge(options) if uri.is_a?(Account) @account = uri @@ -96,7 +96,7 @@ class ResolveAccountService < BaseService @username, @domain = split_acct(@webfinger.subject) unless confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero? - raise Webfinger::RedirectError, "The URI #{uri} tries to hijack #{@username}@#{@domain}" + raise Webfinger::RedirectError, "Too many webfinger redirects for URI #{uri} (stopped at #{@username}@#{@domain})" end rescue Webfinger::GoneError @gone = true @@ -110,7 +110,7 @@ class ResolveAccountService < BaseService return unless activitypub_ready? with_lock("resolve:#{@username}@#{@domain}") do - @account = ActivityPub::FetchRemoteAccountService.new.call(actor_url) + @account = ActivityPub::FetchRemoteAccountService.new.call(actor_url, suppress_errors: @options[:suppress_errors]) end @account diff --git a/lib/exceptions.rb b/lib/exceptions.rb index 0c677b660..3c5ba226b 100644 --- a/lib/exceptions.rb +++ b/lib/exceptions.rb @@ -25,4 +25,13 @@ module Mastodon end end end + + class PrivateNetworkAddressError < HostValidationError + attr_reader :host + + def initialize(host) + @host = host + super() + end + end end diff --git a/spec/services/activitypub/fetch_remote_account_service_spec.rb b/spec/services/activitypub/fetch_remote_account_service_spec.rb index aa13f0a9b..ec6f1f41d 100644 --- a/spec/services/activitypub/fetch_remote_account_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_account_service_spec.rb @@ -119,6 +119,58 @@ RSpec.describe ActivityPub::FetchRemoteAccountService, type: :service do include_examples 'sets profile data' end + context 'when WebFinger returns a different URI' do + let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/bob' }] } } + + before do + stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor)) + stub_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com').to_return(body: Oj.dump(webfinger), headers: { 'Content-Type': 'application/jrd+json' }) + end + + it 'fetches resource' do + account + expect(a_request(:get, 'https://example.com/alice')).to have_been_made.once + end + + it 'looks up webfinger' do + account + expect(a_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com')).to have_been_made.once + end + + it 'does not create account' do + expect(account).to be_nil + end + end + + context 'when WebFinger returns a different URI after a redirection' do + let!(:webfinger) { { subject: 'acct:alice@iscool.af', links: [{ rel: 'self', href: 'https://example.com/bob' }] } } + + before do + stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor)) + stub_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com').to_return(body: Oj.dump(webfinger), headers: { 'Content-Type': 'application/jrd+json' }) + stub_request(:get, 'https://iscool.af/.well-known/webfinger?resource=acct:alice@iscool.af').to_return(body: Oj.dump(webfinger), headers: { 'Content-Type': 'application/jrd+json' }) + end + + it 'fetches resource' do + account + expect(a_request(:get, 'https://example.com/alice')).to have_been_made.once + end + + it 'looks up webfinger' do + account + expect(a_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com')).to have_been_made.once + end + + it 'looks up "redirected" webfinger' do + account + expect(a_request(:get, 'https://iscool.af/.well-known/webfinger?resource=acct:alice@iscool.af')).to have_been_made.once + end + + it 'does not create account' do + expect(account).to be_nil + end + end + context 'with wrong id' do it 'does not create account' do expect(subject.call('https://fake.address/@foo', prefetched_body: Oj.dump(actor))).to be_nil diff --git a/spec/services/resolve_account_service_spec.rb b/spec/services/resolve_account_service_spec.rb index 8c302e1d8..654606bea 100644 --- a/spec/services/resolve_account_service_spec.rb +++ b/spec/services/resolve_account_service_spec.rb @@ -137,8 +137,8 @@ RSpec.describe ResolveAccountService, type: :service do stub_request(:get, 'https://evil.example.com/.well-known/webfinger?resource=acct:foo@evil.example.com').to_return(body: Oj.dump(webfinger2), headers: { 'Content-Type': 'application/jrd+json' }) end - it 'returns new remote account' do - expect { subject.call('Foo@redirected.example.com') }.to raise_error Webfinger::RedirectError + it 'does not return a new remote account' do + expect(subject.call('Foo@redirected.example.com')).to be_nil end end