Merge pull request from GHSA-3fjr-858r-92rw

* Fix insufficient origin validation

app/controllers/concerns/signature_verification.rb

@@ -219,7 +219,7 @@ def account_from_key_id(key_id)
       stoplight_wrap_request { ResolveAccountService.new.call(key_id.gsub(/\Aacct:/, '')) }
     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) }
       account
     end
   rescue Mastodon::HostValidationError

app/helpers/jsonld_helper.rb

@@ -143,8 +143,8 @@ def safe_for_forwarding?(original, compacted)
     end
   end
 
-  def fetch_resource(uri, id, on_behalf_of = nil)
-    unless id
+  def fetch_resource(uri, id_is_known, on_behalf_of = nil)
+    unless id_is_known
       json = fetch_resource_without_id_validation(uri, on_behalf_of)
 
       return if !json.is_a?(Hash) || unsupported_uri_scheme?(json['id'])

app/lib/activitypub/activity.rb

@@ -204,7 +204,8 @@ def follow_from_object
   def fetch_remote_original_status
     if object_uri.start_with?('http')
       return if ActivityPub::TagManager.instance.local_uri?(object_uri)
-      ActivityPub::FetchRemoteStatusService.new.call(object_uri, id: true, on_behalf_of: @account.followers.local.first)
+
+      ActivityPub::FetchRemoteStatusService.new.call(object_uri, on_behalf_of: @account.followers.local.first)
     elsif @object['url'].present?
       ::FetchRemoteStatusService.new.call(@object['url'])
     end

app/lib/activitypub/linked_data_signature.rb

@@ -18,18 +18,18 @@ def verify_account!
 
     return unless type == 'RsaSignature2017'
 
-    creator   = ActivityPub::TagManager.instance.uri_to_resource(creator_uri, Account)
-    creator ||= ActivityPub::FetchRemoteKeyService.new.call(creator_uri, id: false)
+    creator = ActivityPub::TagManager.instance.uri_to_resource(creator_uri, Account)
+    creator = ActivityPub::FetchRemoteKeyService.new.call(creator_uri) if creator&.public_key.blank?
 
     return if creator.nil?
 
     options_hash   = hash(@json['signature'].without('type', 'id', 'signatureValue').merge('@context' => CONTEXT))
     document_hash  = hash(@json.without('signature'))
     to_be_verified = options_hash + document_hash
 
-    if creator.keypair.public_key.verify(OpenSSL::Digest.new('SHA256'), Base64.decode64(signature), to_be_verified)
-      creator
-    end
+    creator if creator.keypair.public_key.verify(OpenSSL::Digest.new('SHA256'), Base64.decode64(signature), to_be_verified)
+  rescue OpenSSL::PKey::RSAError
+    false
   end
 
   def sign!(creator, sign_with: nil)

app/services/activitypub/fetch_remote_account_service.rb

@@ -8,15 +8,15 @@ class ActivityPub::FetchRemoteAccountService < BaseService
   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, prefetched_body: nil, break_on_redirect: false, only_key: false)
     return if domain_not_allowed?(uri)
     return ActivityPub::TagManager.instance.uri_to_resource(uri, Account) if ActivityPub::TagManager.instance.local_uri?(uri)
 
     @json = begin
       if prefetched_body.nil?
-        fetch_resource(uri, id)
+        fetch_resource(uri, true)
       else
-        body_to_json(prefetched_body, compare_id: id ? uri : nil)
+        body_to_json(prefetched_body, compare_id: uri)
       end
     end
 

app/services/activitypub/fetch_remote_key_service.rb

@@ -4,23 +4,10 @@ class ActivityPub::FetchRemoteKeyService < BaseService
   include JsonLdHelper
 
   # Returns account that owns the key
-  def call(uri, id: true, prefetched_body: nil)
+  def call(uri)
     return if uri.blank?
 
-    if prefetched_body.nil?
-      if id
-        @json = fetch_resource_without_id_validation(uri)
-        if person?
-          @json = fetch_resource(@json['id'], true)
-        elsif uri != @json['id']
-          return
-        end
-      else
-        @json = fetch_resource(uri, id)
-      end
-    else
-      @json = body_to_json(prefetched_body, compare_id: id ? uri : nil)
-    end
+    @json = fetch_resource(uri, false)
 
     return unless supported_context?(@json) && expected_type?
     return find_account(@json['id'], @json) if person?

app/services/activitypub/fetch_remote_status_service.rb

@@ -4,12 +4,12 @@ class ActivityPub::FetchRemoteStatusService < BaseService
   include JsonLdHelper
 
   # Should be called when uri has already been checked for locality
-  def call(uri, id: true, prefetched_body: nil, on_behalf_of: nil)
+  def call(uri, prefetched_body: nil, on_behalf_of: nil)
     @json = begin
       if prefetched_body.nil?
-        fetch_resource(uri, id, on_behalf_of)
+        fetch_resource(uri, true, on_behalf_of)
       else
-        body_to_json(prefetched_body, compare_id: id ? uri : nil)
+        body_to_json(prefetched_body, compare_id: uri)
       end
     end
 
@@ -43,6 +43,12 @@ def trustworthy_attribution?(uri, attributed_to)
     Addressable::URI.parse(uri).normalized_host.casecmp(Addressable::URI.parse(attributed_to).normalized_host).zero?
   end
 
+  def account_from_uri(uri)
+    actor = ActivityPub::TagManager.instance.uri_to_resource(uri, Account)
+    actor = ActivityPub::FetchRemoteAccountService.new.call(uri) if actor.nil? || actor.possibly_stale?
+    actor
+  end
+
   def supported_context?
     super(@json)
   end

app/services/activitypub/process_account_service.rb

@@ -252,7 +252,7 @@ def collection_info(type)
 
   def moved_account
     account   = ActivityPub::TagManager.instance.uri_to_resource(@json['movedTo'], Account)
-    account ||= ActivityPub::FetchRemoteAccountService.new.call(@json['movedTo'], id: true, break_on_redirect: true)
+    account ||= ActivityPub::FetchRemoteAccountService.new.call(@json['movedTo'], break_on_redirect: true)
     account
   end
 

app/services/fetch_resource_service.rb

@@ -47,7 +47,15 @@ def process_response(response, terminal = false)
       body = response.body_with_limit
       json = body_to_json(body)
 
-      [json['id'], { prefetched_body: body, id: true }] if supported_context?(json) && (equals_or_includes_any?(json['type'], ActivityPub::FetchRemoteAccountService::SUPPORTED_TYPES) || expected_type?(json))
+      return unless supported_context?(json) && (equals_or_includes_any?(json['type'], ActivityPub::FetchRemoteAccountService::SUPPORTED_TYPES) || expected_type?(json))
+
+      if json['id'] != @url
+        return if terminal
+
+        return process(json['id'], terminal: true)
+      end
+
+      [@url, { prefetched_body: body }]
     elsif !terminal
       link_header = response['Link'] && parse_link_header(response)
 

lib/mastodon/version.rb

@@ -13,7 +13,7 @@ def minor
     end
 
     def patch
-      6
+      7
     end
 
     def flags

spec/lib/activitypub/linked_data_signature_spec.rb

@@ -36,6 +36,40 @@
       end
     end
 
+    context 'when local account record is missing a public key' do
+      let(:raw_signature) do
+        {
+          'creator' => 'http://example.com/alice',
+          'created' => '2017-09-23T20:21:34Z',
+        }
+      end
+
+      let(:signature) { raw_signature.merge('type' => 'RsaSignature2017', 'signatureValue' => sign(sender, raw_signature, raw_json)) }
+
+      let(:service_stub) { instance_double(ActivityPub::FetchRemoteKeyService) }
+
+      before do
+        # Ensure signature is computed with the old key
+        signature
+
+        # Unset key
+        old_key = sender.public_key
+        sender.update!(private_key: '', public_key: '')
+
+        allow(ActivityPub::FetchRemoteKeyService).to receive(:new).and_return(service_stub)
+
+        allow(service_stub).to receive(:call).with('http://example.com/alice') do
+          sender.update!(public_key: old_key)
+          sender
+        end
+      end
+
+      it 'fetches key and returns creator' do
+        expect(subject.verify_account!).to eq sender
+        expect(service_stub).to have_received(:call).with('http://example.com/alice').once
+      end
+    end
+
     context 'when signature is missing' do
       let(:signature) { nil }
 

spec/services/activitypub/fetch_remote_account_service_spec.rb

@@ -16,7 +16,7 @@
   end
 
   describe '#call' do
-    let(:account) { subject.call('https://example.com/alice', id: true) }
+    let(:account) { subject.call('https://example.com/alice') }
 
     shared_examples 'sets profile data' do
       it 'returns an account' do

spec/services/fetch_resource_service_spec.rb

@@ -54,7 +54,7 @@
 
       let(:json) do
         {
-          id: 1,
+          id: 'http://example.com/foo',
           '@context': ActivityPub::TagManager::CONTEXT,
           type: 'Note',
         }.to_json
@@ -79,14 +79,14 @@
         let(:content_type) { 'application/activity+json; charset=utf-8' }
         let(:body) { json }
 
-        it { is_expected.to eq [1, { prefetched_body: body, id: true }] }
+        it { is_expected.to eq ['http://example.com/foo', { prefetched_body: body }] }
       end
 
       context 'when content type is ld+json with profile' do
         let(:content_type) { 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"' }
         let(:body) { json }
 
-        it { is_expected.to eq [1, { prefetched_body: body, id: true }] }
+        it { is_expected.to eq ['http://example.com/foo', { prefetched_body: body }] }
       end
 
       before do
@@ -97,14 +97,14 @@
       context 'when link header is present' do
         let(:headers) { { 'Link' => '<http://example.com/foo>; rel="alternate"; type="application/activity+json"', } }
 
-        it { is_expected.to eq [1, { prefetched_body: json, id: true }] }
+        it { is_expected.to eq ['http://example.com/foo', { prefetched_body: json }] }
       end
 
       context 'when content type is text/html' do
         let(:content_type) { 'text/html' }
         let(:body) { '<html><head><link rel="alternate" href="http://example.com/foo" type="application/activity+json"/></head></html>' }
 
-        it { is_expected.to eq [1, { prefetched_body: json, id: true }] }
+        it { is_expected.to eq ['http://example.com/foo', { prefetched_body: json }] }
       end
     end
   end

spec/services/resolve_url_service_spec.rb

@@ -112,5 +112,25 @@
         end
       end
     end
+
+    context 'searching for a link that redirects to a local public status' do
+      let(:account) { Fabricate(:account) }
+      let(:poster)  { Fabricate(:account) }
+      let!(:status) { Fabricate(:status, account: poster, visibility: :public) }
+      let(:url)     { 'https://link.to/foobar' }
+      let(:status_url) { ActivityPub::TagManager.instance.url_for(status) }
+      let(:uri)     { ActivityPub::TagManager.instance.uri_for(status) }
+
+      before do
+        stub_request(:get, url).to_return(status: 302, headers: { 'Location' => status_url })
+        body = ActiveModelSerializers::SerializableResource.new(status, serializer: ActivityPub::NoteSerializer, adapter: ActivityPub::Adapter).to_json
+        stub_request(:get, status_url).to_return(body: body, headers: { 'Content-Type' => 'application/activity+json' })
+        stub_request(:get, uri).to_return(body: body, headers: { 'Content-Type' => 'application/activity+json' })
+      end
+
+      it 'returns status by url' do
+        expect(subject.call(url, on_behalf_of: account)).to eq(status)
+      end
+    end
   end
 end