CVE-2024-25618: Improper Authentication (CWE-287)
value: Mastodon is a free, open-source social network server based on ActivityPub. Mastodon allows new identities from configured authentication providers (CAS, SAML, OIDC) to attach to existing local users with the same e-mail address. This results in a possible account takeover if the authentication provider allows changing the e-mail address or multiple authentication providers are configured. When a user logs in through an external authentication provider for the first time, Mastodon checks the e-mail address passed by the provider to find an existing account. However, using the e-mail address alone means that if the authentication provider allows changing the e-mail address of an account, the Mastodon account can immediately be hijacked. All users logging in through external authentication providers are affected. The severity is medium, as it also requires the external authentication provider to misbehave. However, some well-known OIDC providers (like Microsoft Azure) make it very easy to accidentally allow unverified e-mail changes. Moreover, OpenID Connect also allows dynamic client registration. This issue has been addressed in versions 4.2.6, 4.1.14, 4.0.14, and 3.5.18. Users are advised to upgrade. There are no known workarounds for this vulnerability.
CWE description: When an actor claims to have a given identity, the product does not prove or insufficiently proves that the claim is correct.
Published: 2024-02-14T21:15Z
Repository: https://github.com/mastodon/mastodon
Language: Ruby
Files changed: app/controllers/auth/omniauth_callbacks_controller.rb, app/models/concerns/user/omniauthable.rb, app/models/identity.rb, spec/models/identity_spec.rb, spec/requests/omniauth_callbacks_spec.rb
Commit date: 2024-02-14 15:16:07 +0100
Commit message: Merge pull request from GHSA-vm39-j3vx-pch3

* Prevent different identities from a same SSO provider from accessing a same account

* Lock auth provider changes behind `ALLOW_UNSAFE_AUTH_PROVIDER_REATTACH=true`

* Rename methods to avoid confusion between OAuth and OmniAuth
Diff: diff --git a/app/controllers/auth/omniauth_callbacks_controller.rb b/app/controllers/auth/omniauth_callbacks_controller.rb
index 707b50ef9e1..9b83de945be 100644
--- a/app/controllers/auth/omniauth_callbacks_controller.rb
+++ b/app/controllers/auth/omniauth_callbacks_controller.rb
@@ -5,11 +5,11 @@ class Auth::OmniauthCallbacksController < Devise::OmniauthCallbacksController
   skip_before_action :verify_authenticity_token
 
   def self.provides_callback_for(provider)
     define_method provider do
       @provider = provider
-      @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?
         record_login_activity
         sign_in_and_redirect @user, event: :authentication
         set_flash_message(:notice, :success, kind: label_for_provider) if is_navigational_format?
diff --git a/app/models/concerns/user/omniauthable.rb b/app/models/concerns/user/omniauthable.rb
index 113bfda2304..396a0598f87 100644
--- a/app/models/concerns/user/omniauthable.rb
+++ b/app/models/concerns/user/omniauthable.rb
@@ -17,43 +17,60 @@ module User::Omniauthable
       email && email !~ TEMP_EMAIL_REGEX
     end
   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
         identity.save!
       end
 
       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))
 
       begin
         user.account.avatar_remote_url = auth.info.image if /\A#{URI::DEFAULT_PARSER.make_regexp(%w(http https))}\z/.match?(auth.info.image)
@@ -64,11 +81,18 @@ module User::Omniauthable
       user.mark_email_as_confirmed! if email_is_verified
       user.save!
       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)
       {
         email: email || "#{TEMP_EMAIL_PREFIX}-#{auth.uid}-#{auth.provider}.com",
         agreement: true,
diff --git a/app/models/identity.rb b/app/models/identity.rb
index c95a68a6f63..77821b78fa2 100644
--- a/app/models/identity.rb
+++ b/app/models/identity.rb
@@ -15,9 +15,9 @@
 class Identity < ApplicationRecord
   belongs_to :user
   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/spec/models/identity_spec.rb b/spec/models/identity_spec.rb
index 70224544433..d5a2ffbc869 100644
--- a/spec/models/identity_spec.rb
+++ b/spec/models/identity_spec.rb
@@ -1,21 +1,21 @@
 # frozen_string_literal: true
 
 require 'rails_helper'
 
 RSpec.describe Identity 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
       allow(described_class).to receive(:find_or_create_by)
 
-      described_class.find_for_oauth(auth)
+      described_class.find_for_omniauth(auth)
 
       expect(described_class).to have_received(:find_or_create_by).with(uid: auth.uid, provider: auth.provider)
     end
 
     it 'returns an instance of Identity' do
-      expect(described_class.find_for_oauth(auth)).to be_instance_of described_class
+      expect(described_class.find_for_omniauth(auth)).to be_instance_of described_class
     end
   end
 end
diff --git a/spec/requests/omniauth_callbacks_spec.rb b/spec/requests/omniauth_callbacks_spec.rb
index 0d37c411403..b478ca1ce61 100644
--- a/spec/requests/omniauth_callbacks_spec.rb
+++ b/spec/requests/omniauth_callbacks_spec.rb
@@ -94,11 +94,11 @@ describe 'OmniAuth callbacks' do
       end
     end
 
     context 'when a user cannot be built' do
       before do
-        allow(User).to receive(:find_for_oauth).and_return(User.new)
+        allow(User).to receive(:find_for_omniauth).and_return(User.new)
       end
 
       it 'redirects to the new user signup page' do
         expect { subject }
           .to not_change(User, :count)

Vulnerable code:       @user = User.find_for_oauth(request.env['omniauth.auth'], current_user)
    def find_for_oauth(auth, signed_in_resource = nil)
      identity = Identity.find_for_oauth(auth)
      user ||= create_for_oauth(auth)
    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
      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
      user = User.find_by(email: email) if email_is_verified
      return user unless user.nil?
    private
  def self.find_for_oauth(auth)
  describe '.find_for_oauth' do
      described_class.find_for_oauth(auth)
      expect(described_class.find_for_oauth(auth)).to be_instance_of described_class
        allow(User).to receive(:find_for_oauth).and_return(User.new)
Fixed code:       @user = User.find_for_omniauth(request.env['omniauth.auth'], current_user)
    def find_for_omniauth(auth, signed_in_resource = nil)
      identity = Identity.find_for_omniauth(auth)
      user ||= reattach_for_auth(auth)
      user ||= create_for_auth(auth)
    private
    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'
      email, email_is_verified = email_from_auth(auth)
      return unless email_is_verified
      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)
    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 self.find_for_omniauth(auth)
  describe '.find_for_omniauth' do
      described_class.find_for_omniauth(auth)
      expect(described_class.find_for_omniauth(auth)).to be_instance_of described_class
        allow(User).to receive(:find_for_omniauth).and_return(User.new)