CVE-2024-25619: Insufficient Session Expiration (CWE-613)
value: Mastodon is a free, open-source social network server based on ActivityPub. When an OAuth Application is destroyed, the streaming server wasn't being informed that the Access Tokens had also been destroyed, this could have posed security risks to users by allowing an application to continue listening to streaming after the application had been destroyed. Essentially this comes down to the fact that when Doorkeeper sets up the relationship between Applications and Access Tokens, it uses a `dependent: delete_all` configuration, which means the `after_commit` callback setup on `AccessTokenExtension` didn't actually fire, since `delete_all` doesn't trigger ActiveRecord callbacks. To mitigate, we need to add a `before_destroy` callback to `ApplicationExtension` which announces to streaming that all the Application's Access Tokens are being "killed". Impact should be negligible given the affected application had to be owned by the user. None the less 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 workaround for this vulnerability.
CWE description: According to WASC, "Insufficient Session Expiration is when a web site permits an attacker to reuse old session credentials or session IDs for authorization."
Published: 2024-02-14T21:15Z
Repository: https://github.com/mastodon/mastodon
Language: Ruby
Files changed: app/lib/application_extension.rb, app/models/user.rb, spec/models/user_spec.rb
Commit date: 2024-02-14 15:15:34 +0100
Commit message: Merge pull request from GHSA-7w3c-p9j8-mq3x

* Ensure destruction of OAuth Applications notifies streaming

Due to doorkeeper using a dependent: delete_all relationship, the destroy of an OAuth Application bypassed the existing AccessTokenExtension callbacks for announcing destructing of access tokens.

* Ensure password resets revoke access to Streaming API

* Improve performance of deleting OAuth tokens

---------

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
Diff: diff --git a/app/lib/application_extension.rb b/app/lib/application_extension.rb
index fb442e2c2d2..400c51a023d 100644
--- a/app/lib/application_extension.rb
+++ b/app/lib/application_extension.rb
@@ -2,16 +2,36 @@
 
 module ApplicationExtension
   extend ActiveSupport::Concern
 
   included do
+    include Redisable
+
     has_many :created_users, class_name: 'User', foreign_key: 'created_by_application_id', inverse_of: :created_by_application
 
     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/user.rb b/app/models/user.rb
index 95fdc431ac4..f706c91eff5 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -342,10 +342,20 @@ class User < ApplicationRecord
     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
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 5ac41c0ff1d..845335e8734 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -418,11 +418,14 @@ RSpec.describe User do
 
     let!(:session_activation) { Fabricate(:session_activation, user: user) }
     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
 
     it 'changes the password immediately' do
       expect(user.external_or_valid_password?('foobar12345')).to be false
@@ -435,10 +438,14 @@ RSpec.describe User do
 
     it 'revokes all access tokens' do
       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
       expect { web_push_subscription.reload }.to raise_error(ActiveRecord::RecordNotFound)
     end
   end

Fixed code:     include Redisable


    # 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

  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

      # 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
    let(:redis_pipeline_stub) { instance_double(Redis::Namespace, publish: nil) }

      allow(redis).to receive(:pipelined).and_yield(redis_pipeline_stub)
    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