From b1ed009c65802b70c9b780f3c7c3a866cba72478 Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 1 Feb 2024 15:56:46 +0100 Subject: [PATCH] Merge pull request from GHSA-3fjr-858r-92rw * Fix insufficient origin validation * Bump version to v3.5.17 --- CHANGELOG.md | 8 +++++++- SECURITY.md | 2 +- .../concerns/signature_verification.rb | 2 +- app/helpers/jsonld_helper.rb | 4 ++-- app/lib/activitypub/activity.rb | 3 ++- app/lib/activitypub/linked_data_signature.rb | 2 +- .../activitypub/fetch_remote_account_service.rb | 6 +++--- .../activitypub/fetch_remote_key_service.rb | 17 ++--------------- .../activitypub/fetch_remote_status_service.rb | 8 ++++---- .../activitypub/process_account_service.rb | 2 +- app/services/fetch_resource_service.rb | 10 +++++++++- docker-compose.yml | 6 +++--- lib/mastodon/version.rb | 2 +- .../activitypub/linked_data_signature_spec.rb | 4 ++-- .../fetch_remote_account_service_spec.rb | 2 +- spec/services/fetch_resource_service_spec.rb | 10 +++++----- spec/services/resolve_url_service_spec.rb | 1 + 17 files changed, 46 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90583d5e5..b5f54d312 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,9 +5,15 @@ All notable changes to this project will be documented in this file. ## End of life notice -**The 3.5.x branch will not receive any update after 2023-12-31.** +**The 3.5.x branch has reached its end of life and will not receive any further update.** This means that no security fix will be made available for this branch after this date, and you will need to update to a more recent version (such as the 4.2.x branch) to receive security fixes. +## [3.5.17] - 2024-02-01 + +### Security + +- Fix insufficient origin validation (CVE-2024-23832, [GHSA-3fjr-858r-92rw](https://github.com/mastodon/mastodon/security/advisories/GHSA-3fjr-858r-92rw)) + ## [3.5.16] - 2023-12-04 ### Changed diff --git a/SECURITY.md b/SECURITY.md index a4c76fabf..1b46ac8c5 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -15,5 +15,5 @@ A "vulnerability in Mastodon" is a vulnerability in the code distributed through | 4.2.x | Yes | | 4.1.x | Yes | | 4.0.x | No | -| 3.5.x | Until 2023-12-31 | +| 3.5.x | No | | < 3.5 | No | diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 4dd0cac55..65f45e004 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -219,7 +219,7 @@ module SignatureVerification 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 diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb index 102e4b132..18314d99f 100644 --- a/app/helpers/jsonld_helper.rb +++ b/app/helpers/jsonld_helper.rb @@ -157,8 +157,8 @@ module JsonLdHelper 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']) diff --git a/app/lib/activitypub/activity.rb b/app/lib/activitypub/activity.rb index 75939f8aa..0a909b18b 100644 --- a/app/lib/activitypub/activity.rb +++ b/app/lib/activitypub/activity.rb @@ -153,7 +153,8 @@ class ActivityPub::Activity 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, request_id: @options[:request_id]) + + ActivityPub::FetchRemoteStatusService.new.call(object_uri, on_behalf_of: @account.followers.local.first, request_id: @options[:request_id]) elsif @object['url'].present? ::FetchRemoteStatusService.new.call(@object['url'], request_id: @options[:request_id]) end diff --git a/app/lib/activitypub/linked_data_signature.rb b/app/lib/activitypub/linked_data_signature.rb index f3ca042e8..d49c7896c 100644 --- a/app/lib/activitypub/linked_data_signature.rb +++ b/app/lib/activitypub/linked_data_signature.rb @@ -19,7 +19,7 @@ class ActivityPub::LinkedDataSignature return unless type == 'RsaSignature2017' creator = ActivityPub::TagManager.instance.uri_to_resource(creator_uri, Account) - creator = ActivityPub::FetchRemoteKeyService.new.call(creator_uri, id: false) if creator&.public_key.blank? + creator = ActivityPub::FetchRemoteKeyService.new.call(creator_uri) if creator&.public_key.blank? return if creator.nil? diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb index df46e03b8..582506a7e 100644 --- a/app/services/activitypub/fetch_remote_account_service.rb +++ b/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, request_id: nil) + def call(uri, prefetched_body: nil, break_on_redirect: false, only_key: false, request_id: nil) 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 diff --git a/app/services/activitypub/fetch_remote_key_service.rb b/app/services/activitypub/fetch_remote_key_service.rb index c48288b3b..7142cae1e 100644 --- a/app/services/activitypub/fetch_remote_key_service.rb +++ b/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? diff --git a/app/services/activitypub/fetch_remote_status_service.rb b/app/services/activitypub/fetch_remote_status_service.rb index 936737bf6..fa0884fdb 100644 --- a/app/services/activitypub/fetch_remote_status_service.rb +++ b/app/services/activitypub/fetch_remote_status_service.rb @@ -7,13 +7,13 @@ class ActivityPub::FetchRemoteStatusService < BaseService DISCOVERIES_PER_REQUEST = 1000 # Should be called when uri has already been checked for locality - def call(uri, id: true, prefetched_body: nil, on_behalf_of: nil, expected_actor_uri: nil, request_id: nil) + def call(uri, prefetched_body: nil, on_behalf_of: nil, expected_actor_uri: nil, request_id: nil) @request_id = request_id || "#{Time.now.utc.to_i}-status-#{uri}" @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 @@ -63,7 +63,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService def account_from_uri(uri) actor = ActivityPub::TagManager.instance.uri_to_resource(uri, Account) - actor = ActivityPub::FetchRemoteAccountService.new.call(uri, id: true, request_id: @request_id) if actor.nil? || actor.possibly_stale? + actor = ActivityPub::FetchRemoteAccountService.new.call(uri, request_id: @request_id) if actor.nil? || actor.possibly_stale? actor end diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index b1e10e3d6..da9d77d13 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -269,7 +269,7 @@ class ActivityPub::ProcessAccountService < BaseService 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, request_id: @options[:request_id]) + account ||= ActivityPub::FetchRemoteAccountService.new.call(@json['movedTo'], break_on_redirect: true, request_id: @options[:request_id]) account end diff --git a/app/services/fetch_resource_service.rb b/app/services/fetch_resource_service.rb index 6c0093cd4..b012880c7 100644 --- a/app/services/fetch_resource_service.rb +++ b/app/services/fetch_resource_service.rb @@ -47,7 +47,15 @@ class FetchResourceService < BaseService 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) diff --git a/docker-compose.yml b/docker-compose.yml index 08f6dcd7e..eee6a48b2 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -44,7 +44,7 @@ services: web: build: . - image: ghcr.io/mastodon/mastodon:v3.5.16 + image: ghcr.io/mastodon/mastodon:v3.5.17 restart: always env_file: .env.production command: bash -c "rm -f /mastodon/tmp/pids/server.pid; bundle exec rails s -p 3000" @@ -65,7 +65,7 @@ services: streaming: build: . - image: ghcr.io/mastodon/mastodon:v3.5.16 + image: ghcr.io/mastodon/mastodon:v3.5.17 restart: always env_file: .env.production command: node ./streaming @@ -83,7 +83,7 @@ services: sidekiq: build: . - image: ghcr.io/mastodon/mastodon:v3.5.16 + image: ghcr.io/mastodon/mastodon:v3.5.17 restart: always env_file: .env.production command: bundle exec sidekiq diff --git a/lib/mastodon/version.rb b/lib/mastodon/version.rb index 3355a5197..8ddf5728d 100644 --- a/lib/mastodon/version.rb +++ b/lib/mastodon/version.rb @@ -13,7 +13,7 @@ module Mastodon end def patch - 16 + 17 end def flags diff --git a/spec/lib/activitypub/linked_data_signature_spec.rb b/spec/lib/activitypub/linked_data_signature_spec.rb index f71bb2eff..b2c340390 100644 --- a/spec/lib/activitypub/linked_data_signature_spec.rb +++ b/spec/lib/activitypub/linked_data_signature_spec.rb @@ -58,7 +58,7 @@ RSpec.describe ActivityPub::LinkedDataSignature do allow(ActivityPub::FetchRemoteKeyService).to receive(:new).and_return(service_stub) - allow(service_stub).to receive(:call).with('http://example.com/alice', id: false) do + allow(service_stub).to receive(:call).with('http://example.com/alice') do sender.update!(public_key: old_key) sender end @@ -66,7 +66,7 @@ RSpec.describe ActivityPub::LinkedDataSignature do 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', id: false).once + expect(service_stub).to have_received(:call).with('http://example.com/alice').once 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..c05e022aa 100644 --- a/spec/services/activitypub/fetch_remote_account_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_account_service_spec.rb @@ -16,7 +16,7 @@ RSpec.describe ActivityPub::FetchRemoteAccountService, type: :service do 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 diff --git a/spec/services/fetch_resource_service_spec.rb b/spec/services/fetch_resource_service_spec.rb index ded05ffbc..1697ad768 100644 --- a/spec/services/fetch_resource_service_spec.rb +++ b/spec/services/fetch_resource_service_spec.rb @@ -54,7 +54,7 @@ RSpec.describe FetchResourceService, type: :service do let(:json) do { - id: 1, + id: 'http://example.com/foo', '@context': ActivityPub::TagManager::CONTEXT, type: 'Note', }.to_json @@ -79,14 +79,14 @@ RSpec.describe FetchResourceService, type: :service do 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 @@ RSpec.describe FetchResourceService, type: :service do context 'when link header is present' do let(:headers) { { 'Link' => '; 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) { '' } - 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 diff --git a/spec/services/resolve_url_service_spec.rb b/spec/services/resolve_url_service_spec.rb index b3e3defbf..5b6b7b9a6 100644 --- a/spec/services/resolve_url_service_spec.rb +++ b/spec/services/resolve_url_service_spec.rb @@ -139,6 +139,7 @@ describe ResolveURLService, type: :service 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