diff --git a/CHANGELOG.md b/CHANGELOG.md index 36eb51a5145d9b..244be4416dd7a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,25 @@ All notable changes to this project will be documented in this file. **The 4.0.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. +## [4.0.14] - 2024-02-14 + +### Security + +- Update the `sidekiq-unique-jobs` dependency (see [GHSA-cmh9-rx85-xj38](https://github.com/mhenrixon/sidekiq-unique-jobs/security/advisories/GHSA-cmh9-rx85-xj38)) + In addition, we have disabled the web interface for `sidekiq-unique-jobs` out of caution. + If you need it, you can re-enable it by setting `ENABLE_SIDEKIQ_UNIQUE_JOBS_UI=true`. + If you only need to clear all locks, you can now use `bundle exec rake sidekiq_unique_jobs:delete_all_locks`. +- Update the `nokogiri` dependency (see [GHSA-xc9x-jj77-9p9j](https://github.com/sparklemotion/nokogiri/security/advisories/GHSA-xc9x-jj77-9p9j)) +- Disable administrative Doorkeeper routes ([ThisIsMissEm](https://github.com/mastodon/mastodon/pull/29187)) +- Fix ongoing streaming sessions not being invalidated when applications get deleted in some cases ([GHSA-7w3c-p9j8-mq3x](https://github.com/mastodon/mastodon/security/advisories/GHSA-7w3c-p9j8-mq3x)) + In some rare cases, the streaming server was not notified of access tokens revocation on application deletion. +- Change external authentication behavior to never reattach a new identity to an existing user by default ([GHSA-vm39-j3vx-pch3](https://github.com/mastodon/mastodon/security/advisories/GHSA-vm39-j3vx-pch3)) + Up until now, Mastodon has allowed new identities from external authentication providers to attach to an existing local user based on their verified e-mail address. + This allowed upgrading users from a database-stored password to an external authentication provider, or move from one authentication provider to another. + However, this behavior may be unexpected, and means that when multiple authentication providers are configured, the overall security would be that of the least secure authentication provider. + For these reasons, this behavior is now locked under the `ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH` environment variable. + In addition, regardless of this environment variable, Mastodon will refuse to attach two identities from the same authentication provider to the same account. + ## [4.0.13] - 2024-02-01 ### Security diff --git a/Gemfile.lock b/Gemfile.lock index a5731cd3111b5b..09630c3a3de83d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -402,7 +402,7 @@ GEM mime-types-data (~> 3.2015) mime-types-data (3.2022.0105) mini_mime (1.1.5) - mini_portile2 (2.8.4) + mini_portile2 (2.8.5) minitest (5.20.0) msgpack (1.5.4) multi_json (1.15.0) @@ -412,7 +412,7 @@ GEM net-ssh (>= 2.6.5, < 8.0.0) net-ssh (7.0.1) nio4r (2.5.9) - nokogiri (1.15.4) + nokogiri (1.16.2) mini_portile2 (~> 2.8.2) racc (~> 1.4) nsa (0.2.8) @@ -482,7 +482,7 @@ GEM pundit (2.2.0) activesupport (>= 3.0.0) raabro (1.4.0) - racc (1.7.1) + racc (1.7.3) rack (2.2.8) rack-attack (6.6.1) rack (>= 1.0, < 3) @@ -609,7 +609,7 @@ GEM activerecord (>= 4.0.0) railties (>= 4.0.0) semantic_range (3.0.0) - sidekiq (6.5.11) + sidekiq (6.5.12) connection_pool (>= 2.2.5, < 3) rack (~> 2.0) redis (>= 4.5.0, < 5) @@ -620,10 +620,11 @@ GEM rufus-scheduler (~> 3.2) sidekiq (>= 4, < 7) tilt (>= 1.4.0) - sidekiq-unique-jobs (7.1.27) + sidekiq-unique-jobs (7.1.33) brpoplpush-redis_script (> 0.1.1, <= 2.0.0) concurrent-ruby (~> 1.0, >= 1.0.5) - sidekiq (>= 5.0, < 8.0) + redis (< 5.0) + sidekiq (>= 5.0, < 7.0) thor (>= 0.20, < 3.0) simple-navigation (4.4.0) activesupport (>= 2.3.2) diff --git a/app/controllers/auth/omniauth_callbacks_controller.rb b/app/controllers/auth/omniauth_callbacks_controller.rb index 3d7962de56cb50..3c4984b3f3816b 100644 --- a/app/controllers/auth/omniauth_callbacks_controller.rb +++ b/app/controllers/auth/omniauth_callbacks_controller.rb @@ -5,7 +5,7 @@ class Auth::OmniauthCallbacksController < Devise::OmniauthCallbacksController def self.provides_callback_for(provider) define_method provider do - @user = User.find_for_oauth(request.env['omniauth.auth'], current_user) + @user = User.find_for_omniauth(request.env['omniauth.auth'], current_user) if @user.persisted? LoginActivity.create( diff --git a/app/lib/application_extension.rb b/app/lib/application_extension.rb index 4de69c1eaddb24..d1222656b75e18 100644 --- a/app/lib/application_extension.rb +++ b/app/lib/application_extension.rb @@ -4,12 +4,32 @@ module ApplicationExtension extend ActiveSupport::Concern included do + include Redisable + validates :name, length: { maximum: 60 } validates :website, url: true, length: { maximum: 2_000 }, if: :website? validates :redirect_uri, length: { maximum: 2_000 } + + # The relationship used between Applications and AccessTokens is using + # dependent: delete_all, which means the ActiveRecord callback in + # AccessTokenExtension is not run, so instead we manually announce to + # streaming that these tokens are being deleted. + before_destroy :push_to_streaming_api, prepend: true end def confirmation_redirect_uri redirect_uri.lines.first.strip end + + def push_to_streaming_api + # TODO: #28793 Combine into a single topic + payload = Oj.dump(event: :kill) + access_tokens.in_batches do |tokens| + redis.pipelined do |pipeline| + tokens.ids.each do |id| + pipeline.publish("timeline:access_token:#{id}", payload) + end + end + end + end end diff --git a/app/models/concerns/omniauthable.rb b/app/models/concerns/omniauthable.rb index a90d5d888a5aa6..37d8efd4843ad2 100644 --- a/app/models/concerns/omniauthable.rb +++ b/app/models/concerns/omniauthable.rb @@ -19,17 +19,18 @@ def email_present? end class_methods do - def find_for_oauth(auth, signed_in_resource = nil) + def find_for_omniauth(auth, signed_in_resource = nil) # EOLE-SSO Patch auth.uid = (auth.uid[0][:uid] || auth.uid[0][:user]) if auth.uid.is_a? Hashie::Array - identity = Identity.find_for_oauth(auth) + identity = Identity.find_for_omniauth(auth) # If a signed_in_resource is provided it always overrides the existing user # to prevent the identity being locked with accidentally created accounts. # Note that this may leave zombie accounts (with no associated identity) which # can be cleaned up at a later date. user = signed_in_resource || identity.user - user ||= create_for_oauth(auth) + user ||= reattach_for_auth(auth) + user ||= create_for_auth(auth) if identity.user.nil? identity.user = user @@ -39,19 +40,35 @@ def find_for_oauth(auth, signed_in_resource = nil) user end - def create_for_oauth(auth) - # Check if the user exists with provided email. If no email was provided, - # we assign a temporary email and ask the user to verify it on - # the next step via Auth::SetupController.show + private - strategy = Devise.omniauth_configs[auth.provider.to_sym].strategy - assume_verified = strategy&.security&.assume_email_is_verified - email_is_verified = auth.info.verified || auth.info.verified_email || auth.info.email_verified || assume_verified - email = auth.info.verified_email || auth.info.email + def reattach_for_auth(auth) + # If allowed, check if a user exists with the provided email address, + # and return it if they does not have an associated identity with the + # current authentication provider. + + # This can be used to provide a choice of alternative auth providers + # or provide smooth gradual transition between multiple auth providers, + # but this is discouraged because any insecure provider will put *all* + # local users at risk, regardless of which provider they registered with. + + return unless ENV['ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH'] == 'true' - user = User.find_by(email: email) if email_is_verified + email, email_is_verified = email_from_auth(auth) + return unless email_is_verified - return user unless user.nil? + user = User.find_by(email: email) + return if user.nil? || Identity.exists?(provider: auth.provider, user_id: user.id) + + user + end + + def create_for_auth(auth) + # Create a user for the given auth params. If no email was provided, + # we assign a temporary email and ask the user to verify it on + # the next step via Auth::SetupController.show + + email, email_is_verified = email_from_auth(auth) user = User.new(user_params_from_auth(email, auth)) @@ -61,7 +78,14 @@ def create_for_oauth(auth) user end - private + def email_from_auth(auth) + strategy = Devise.omniauth_configs[auth.provider.to_sym].strategy + assume_verified = strategy&.security&.assume_email_is_verified + email_is_verified = auth.info.verified || auth.info.verified_email || auth.info.email_verified || assume_verified + email = auth.info.verified_email || auth.info.email + + [email, email_is_verified] + end def user_params_from_auth(email, auth) { diff --git a/app/models/identity.rb b/app/models/identity.rb index 869f06372dab8b..a396d76234369a 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -16,7 +16,7 @@ class Identity < ApplicationRecord validates :uid, presence: true, uniqueness: { scope: :provider } validates :provider, presence: true - def self.find_for_oauth(auth) + def self.find_for_omniauth(auth) find_or_create_by(uid: auth.uid, provider: auth.provider) end end diff --git a/app/models/user.rb b/app/models/user.rb index 5790be6e91d81d..641850a1072fbb 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -373,6 +373,25 @@ def reset_password(new_password, new_password_confirmation) super end + def revoke_access! + Doorkeeper::AccessGrant.by_resource_owner(self).update_all(revoked_at: Time.now.utc) + + Doorkeeper::AccessToken.by_resource_owner(self).in_batches do |batch| + batch.update_all(revoked_at: Time.now.utc) + Web::PushSubscription.where(access_token_id: batch).delete_all + + # Revoke each access token for the Streaming API, since `update_all`` + # doesn't trigger ActiveRecord Callbacks: + # TODO: #28793 Combine into a single topic + payload = Oj.dump(event: :kill) + redis.pipelined do |pipeline| + batch.ids.each do |id| + pipeline.publish("timeline:access_token:#{id}", payload) + end + end + end + end + def reset_password! # First, change password to something random and deactivate all sessions transaction do @@ -381,12 +400,7 @@ def reset_password! end # Then, remove all authorized applications and connected push subscriptions - Doorkeeper::AccessGrant.by_resource_owner(self).in_batches.update_all(revoked_at: Time.now.utc) - - Doorkeeper::AccessToken.by_resource_owner(self).in_batches do |batch| - batch.update_all(revoked_at: Time.now.utc) - Web::PushSubscription.where(access_token_id: batch).delete_all - end + revoke_access! # Finally, send a reset password prompt to the user send_reset_password_instructions diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 84b649f5c1017c..8097e7c882efd6 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -19,9 +19,14 @@ user unless user&.otp_required_for_login? end - # If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below. + # Doorkeeper provides some administrative interfaces for managing OAuth + # Applications, allowing creation, edit, and deletion of applications from the + # server. At present, these administrative routes are not integrated into + # Mastodon, and as such, we've disabled them by always return a 403 forbidden + # response for them. This does not affect the ability for users to manage + # their own OAuth Applications. admin_authenticator do - current_user&.admin? || redirect_to(new_user_session_url) + head 403 end # Authorization Code expiration time (default 10 minutes). diff --git a/config/routes.rb b/config/routes.rb index 4c394bf5d58777..532040033582ae 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'sidekiq_unique_jobs/web' +require 'sidekiq_unique_jobs/web' if ENV['ENABLE_SIDEKIQ_UNIQUE_JOBS_UI'] == true require 'sidekiq-scheduler/web' Rails.application.routes.draw do diff --git a/docker-compose.yml b/docker-compose.yml index 513c5dea73cc57..a5ca81daf7acdb 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -56,7 +56,7 @@ services: web: build: . - image: ghcr.io/mastodon/mastodon:v4.0.13 + image: ghcr.io/mastodon/mastodon:v4.0.14 restart: always env_file: .env.production command: bash -c "rm -f /mastodon/tmp/pids/server.pid; bundle exec rails s -p 3000" @@ -77,7 +77,7 @@ services: streaming: build: . - image: ghcr.io/mastodon/mastodon:v4.0.13 + image: ghcr.io/mastodon/mastodon:v4.0.14 restart: always env_file: .env.production command: node ./streaming @@ -95,7 +95,7 @@ services: sidekiq: build: . - image: ghcr.io/mastodon/mastodon:v4.0.13 + image: ghcr.io/mastodon/mastodon:v4.0.14 restart: always env_file: .env.production command: bundle exec sidekiq diff --git a/lib/mastodon/version.rb b/lib/mastodon/version.rb index ca2966badc7d49..bae4429d3db92f 100644 --- a/lib/mastodon/version.rb +++ b/lib/mastodon/version.rb @@ -13,7 +13,7 @@ def minor end def patch - 13 + 14 end def flags diff --git a/lib/tasks/sidekiq_unique_jobs.rake b/lib/tasks/sidekiq_unique_jobs.rake new file mode 100644 index 00000000000000..bedc8fe4c650c4 --- /dev/null +++ b/lib/tasks/sidekiq_unique_jobs.rake @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +namespace :sidekiq_unique_jobs do + task delete_all_locks: :environment do + digests = SidekiqUniqueJobs::Digests.new + digests.delete_by_pattern('*', count: digests.count) + + expiring_digests = SidekiqUniqueJobs::ExpiringDigests.new + expiring_digests.delete_by_pattern('*', count: expiring_digests.count) + end +end diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb index 689c9b797f4f63..081c254d8200f3 100644 --- a/spec/models/identity_spec.rb +++ b/spec/models/identity_spec.rb @@ -1,16 +1,16 @@ require 'rails_helper' RSpec.describe Identity, type: :model do - describe '.find_for_oauth' do + describe '.find_for_omniauth' do let(:auth) { Fabricate(:identity, user: Fabricate(:user)) } it 'calls .find_or_create_by' do expect(described_class).to receive(:find_or_create_by).with(uid: auth.uid, provider: auth.provider) - described_class.find_for_oauth(auth) + described_class.find_for_omniauth(auth) end it 'returns an instance of Identity' do - expect(described_class.find_for_oauth(auth)).to be_instance_of Identity + expect(described_class.find_for_omniauth(auth)).to be_instance_of Identity end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a7da31e6064720..0d6f16070790de 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -313,7 +313,10 @@ def fabricate let!(:access_token) { Fabricate(:access_token, resource_owner_id: user.id) } let!(:web_push_subscription) { Fabricate(:web_push_subscription, access_token: access_token) } + let(:redis_pipeline_stub) { instance_double(Redis::Namespace, publish: nil) } + before do + allow(redis).to receive(:pipelined).and_yield(redis_pipeline_stub) user.reset_password! end @@ -329,6 +332,10 @@ def fabricate expect(Doorkeeper::AccessToken.active_for(user).count).to eq 0 end + it 'revokes streaming access for all access tokens' do + expect(redis_pipeline_stub).to have_received(:publish).with("timeline:access_token:#{access_token.id}", Oj.dump(event: :kill)).once + end + it 'removes push subscriptions' do expect(Web::PushSubscription.where(user: user).or(Web::PushSubscription.where(access_token: access_token)).count).to eq 0 end diff --git a/spec/requests/disabled_oauth_endpoints_spec.rb b/spec/requests/disabled_oauth_endpoints_spec.rb new file mode 100644 index 00000000000000..7c2c09f3804bf3 --- /dev/null +++ b/spec/requests/disabled_oauth_endpoints_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe 'Disabled OAuth routes' do + # These routes are disabled via the doorkeeper configuration for + # `admin_authenticator`, as these routes should only be accessible by server + # administrators. For now, these routes are not properly designed and + # integrated into Mastodon, so we're disabling them completely + describe 'GET /oauth/applications' do + it 'returns 403 forbidden' do + get oauth_applications_path + + expect(response).to have_http_status(403) + end + end + + describe 'POST /oauth/applications' do + it 'returns 403 forbidden' do + post oauth_applications_path + + expect(response).to have_http_status(403) + end + end + + describe 'GET /oauth/applications/new' do + it 'returns 403 forbidden' do + get new_oauth_application_path + + expect(response).to have_http_status(403) + end + end + + describe 'GET /oauth/applications/:id' do + let(:application) { Fabricate(:application, scopes: 'read') } + + it 'returns 403 forbidden' do + get oauth_application_path(application) + + expect(response).to have_http_status(403) + end + end + + describe 'PATCH /oauth/applications/:id' do + let(:application) { Fabricate(:application, scopes: 'read') } + + it 'returns 403 forbidden' do + patch oauth_application_path(application) + + expect(response).to have_http_status(403) + end + end + + describe 'PUT /oauth/applications/:id' do + let(:application) { Fabricate(:application, scopes: 'read') } + + it 'returns 403 forbidden' do + put oauth_application_path(application) + + expect(response).to have_http_status(403) + end + end + + describe 'DELETE /oauth/applications/:id' do + let(:application) { Fabricate(:application, scopes: 'read') } + + it 'returns 403 forbidden' do + delete oauth_application_path(application) + + expect(response).to have_http_status(403) + end + end + + describe 'GET /oauth/applications/:id/edit' do + let(:application) { Fabricate(:application, scopes: 'read') } + + it 'returns 403 forbidden' do + get edit_oauth_application_path(application) + + expect(response).to have_http_status(403) + end + end +end