Skip to content

Commit

Permalink
Merge pull request from GHSA-3fjr-858r-92rw
Browse files Browse the repository at this point in the history
* Fix insufficient origin validation
  • Loading branch information
ClearlyClaire authored and chasedream1129 committed Feb 13, 2024
1 parent 93ffdee commit 149dd2f
Show file tree
Hide file tree
Showing 14 changed files with 95 additions and 39 deletions.
2 changes: 1 addition & 1 deletion app/controllers/concerns/signature_verification.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions app/helpers/jsonld_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'])
Expand Down
3 changes: 2 additions & 1 deletion app/lib/activitypub/activity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions app/lib/activitypub/linked_data_signature.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions app/services/activitypub/fetch_remote_account_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
17 changes: 2 additions & 15 deletions app/services/activitypub/fetch_remote_key_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
12 changes: 9 additions & 3 deletions app/services/activitypub/fetch_remote_status_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/services/activitypub/process_account_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
10 changes: 9 additions & 1 deletion app/services/fetch_resource_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion lib/mastodon/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def minor
end

def patch
6
7
end

def flags
Expand Down
34 changes: 34 additions & 0 deletions spec/lib/activitypub/linked_data_signature_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions spec/services/fetch_resource_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@

let(:json) do
{
id: 1,
id: 'http://example.com/foo',
'@context': ActivityPub::TagManager::CONTEXT,
type: 'Note',
}.to_json
Expand All @@ -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
Expand All @@ -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
Expand Down
20 changes: 20 additions & 0 deletions spec/services/resolve_url_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 149dd2f

Please sign in to comment.