From fa3f78e4bf1b5e2b6e8b11f161dd3c02348bf3d4 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 23 Jun 2020 02:57:00 +0200 Subject: [PATCH 1/4] Fix other sessions not being logged out on password change While OAuth tokens were immediately revoked, accessing the home controller immediately generated new OAuth tokens and "revived" the session due to a combination of using remember_me tokens and overwriting the `authenticate_user!` method --- app/controllers/auth/passwords_controller.rb | 5 ++++- app/controllers/auth/registrations_controller.rb | 8 +++++++- app/controllers/home_controller.rb | 4 +++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/controllers/auth/passwords_controller.rb b/app/controllers/auth/passwords_controller.rb index b98bcecd0..5db2668f7 100644 --- a/app/controllers/auth/passwords_controller.rb +++ b/app/controllers/auth/passwords_controller.rb @@ -8,7 +8,10 @@ class Auth::PasswordsController < Devise::PasswordsController def update super do |resource| - resource.session_activations.destroy_all if resource.errors.empty? + if resource.errors.empty? + resource.session_activations.destroy_all + resource.forget_me! + end end end diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index 78feb1631..d31966248 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Auth::RegistrationsController < Devise::RegistrationsController + include Devise::Controllers::Rememberable + layout :determine_layout before_action :set_invite, only: [:new, :create] @@ -24,7 +26,11 @@ class Auth::RegistrationsController < Devise::RegistrationsController def update super do |resource| - resource.clear_other_sessions(current_session.session_id) if resource.saved_change_to_encrypted_password? + if resource.saved_change_to_encrypted_password? + resource.clear_other_sessions(current_session.session_id) + resource.forget_me! + remember_me(resource) + end end end diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb index 7c8a18d17..702889cd0 100644 --- a/app/controllers/home_controller.rb +++ b/app/controllers/home_controller.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class HomeController < ApplicationController + before_action :redirect_unauthenticated_to_permalinks! before_action :authenticate_user! before_action :set_referrer_policy_header @@ -10,7 +11,7 @@ class HomeController < ApplicationController private - def authenticate_user! + def redirect_unauthenticated_to_permalinks! return if user_signed_in? matches = request.path.match(/\A\/web\/(statuses|accounts)\/([\d]+)\z/) @@ -35,6 +36,7 @@ class HomeController < ApplicationController end matches = request.path.match(%r{\A/web/timelines/tag/(?.+)\z}) + redirect_to(matches ? tag_path(CGI.unescape(matches[:tag])) : default_redirect_path) end From 951e997b26cb5bf93539a22221efda97ad70079e Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 24 Jun 2020 00:21:03 +0200 Subject: [PATCH 2/4] Change rate limits for various paths - Rate limit login attempts by target account - Rate limit password resets and e-mail re-confirmations by target account - Rate limit sign-up/login attempts, password resets, and e-mail re-confirmations by IP like before --- config/initializers/rack_attack.rb | 37 +++++++++++++++------- config/initializers/rack_attack_logging.rb | 1 + 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 09458c540..cd29afac5 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -38,15 +38,6 @@ class Rack::Attack end end - PROTECTED_PATHS = %w( - /auth/sign_in - /auth - /auth/password - /auth/confirmation - ).freeze - - PROTECTED_PATHS_REGEX = Regexp.union(PROTECTED_PATHS.map { |path| /\A#{Regexp.escape(path)}/ }) - Rack::Attack.safelist('allow from localhost') do |req| req.remote_ip == '127.0.0.1' || req.remote_ip == '::1' end @@ -86,8 +77,32 @@ class Rack::Attack req.authenticated_user_id if (req.post? && req.path =~ API_DELETE_REBLOG_REGEX) || (req.delete? && req.path =~ API_DELETE_STATUS_REGEX) end - throttle('protected_paths', limit: 25, period: 5.minutes) do |req| - req.remote_ip if req.post? && req.path =~ PROTECTED_PATHS_REGEX + throttle('throttle_sign_up_attempts/ip', limit: 25, period: 5.minutes) do |req| + req.remote_ip if req.post? && req.path == '/auth' + end + + throttle('throttle_password_resets/ip', limit: 25, period: 5.minutes) do |req| + req.remote_ip if req.post? && req.path == '/auth/password' + end + + throttle('throttle_password_resets/email', limit: 5, period: 30.minutes) do |req| + req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/password' + end + + throttle('throttle_email_confirmations/ip', limit: 25, period: 5.minutes) do |req| + req.remote_ip if req.post? && req.path == '/auth/confirmation' + end + + throttle('throttle_email_confirmations/email', limit: 5, period: 30.minutes) do |req| + req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/password' + end + + throttle('throttle_login_attempts/ip', limit: 25, period: 5.minutes) do |req| + req.remote_ip if req.post? && req.path == '/auth/sign_in' + end + + throttle('throttle_login_attempts/email', limit: 25, period: 1.hour) do |req| + req.session[:attempt_user_id] || req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/sign_in' end self.throttled_response = lambda do |env| diff --git a/config/initializers/rack_attack_logging.rb b/config/initializers/rack_attack_logging.rb index c30bd8a64..ab4822e96 100644 --- a/config/initializers/rack_attack_logging.rb +++ b/config/initializers/rack_attack_logging.rb @@ -2,5 +2,6 @@ ActiveSupport::Notifications.subscribe(/rack_attack/) do |_name, _start, _finish req = payload[:request] next unless [:throttle, :blacklist].include? req.env['rack.attack.match_type'] + Rails.logger.info("Rate limit hit (#{req.env['rack.attack.match_type']}): #{req.ip} #{req.request_method} #{req.fullpath}") end From 2d2e3651eee12364b53f658077dae9343aca5e09 Mon Sep 17 00:00:00 2001 From: Thibaut Girka Date: Mon, 22 Jun 2020 21:09:18 +0200 Subject: [PATCH 3/4] Fix media attachment enumeration Signed-off-by: Eugen Rochko --- app/controllers/media_proxy_controller.rb | 5 ++- spec/controllers/media_controller_spec.rb | 3 +- .../media_proxy_controller_spec.rb | 42 +++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 spec/controllers/media_proxy_controller_spec.rb diff --git a/app/controllers/media_proxy_controller.rb b/app/controllers/media_proxy_controller.rb index 014b89de1..e36673fc4 100644 --- a/app/controllers/media_proxy_controller.rb +++ b/app/controllers/media_proxy_controller.rb @@ -2,6 +2,7 @@ class MediaProxyController < ApplicationController include RoutingHelper + include Authorization skip_before_action :store_current_location skip_before_action :require_functional! @@ -10,12 +11,14 @@ class MediaProxyController < ApplicationController rescue_from ActiveRecord::RecordInvalid, with: :not_found rescue_from Mastodon::UnexpectedResponseError, with: :not_found + rescue_from Mastodon::NotPermittedError, with: :not_found rescue_from HTTP::TimeoutError, HTTP::ConnectionError, OpenSSL::SSL::SSLError, with: :internal_server_error def show RedisLock.acquire(lock_options) do |lock| if lock.acquired? - @media_attachment = MediaAttachment.remote.find(params[:id]) + @media_attachment = MediaAttachment.remote.attached.find(params[:id]) + authorize @media_attachment.status, :show? redownload! if @media_attachment.needs_redownload? && !reject_media? else raise Mastodon::RaceConditionError diff --git a/spec/controllers/media_controller_spec.rb b/spec/controllers/media_controller_spec.rb index ac44a76f2..2925aed59 100644 --- a/spec/controllers/media_controller_spec.rb +++ b/spec/controllers/media_controller_spec.rb @@ -28,9 +28,8 @@ describe MediaController do end it 'raises when not permitted to view' do - status = Fabricate(:status) + status = Fabricate(:status, visibility: :direct) media_attachment = Fabricate(:media_attachment, status: status) - allow_any_instance_of(MediaController).to receive(:authorize).and_raise(ActiveRecord::RecordNotFound) get :show, params: { id: media_attachment.to_param } expect(response).to have_http_status(404) diff --git a/spec/controllers/media_proxy_controller_spec.rb b/spec/controllers/media_proxy_controller_spec.rb new file mode 100644 index 000000000..32510cf43 --- /dev/null +++ b/spec/controllers/media_proxy_controller_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe MediaProxyController do + render_views + + before do + stub_request(:get, 'http://example.com/attachment.png').to_return(request_fixture('avatar.txt')) + end + + describe '#show' do + it 'redirects when attached to a status' do + status = Fabricate(:status) + media_attachment = Fabricate(:media_attachment, status: status, remote_url: 'http://example.com/attachment.png') + get :show, params: { id: media_attachment.id } + + expect(response).to have_http_status(302) + end + + it 'responds with missing when there is not an attached status' do + media_attachment = Fabricate(:media_attachment, status: nil, remote_url: 'http://example.com/attachment.png') + get :show, params: { id: media_attachment.id } + + expect(response).to have_http_status(404) + end + + it 'raises when id cant be found' do + get :show, params: { id: 'missing' } + + expect(response).to have_http_status(404) + end + + it 'raises when not permitted to view' do + status = Fabricate(:status, visibility: :direct) + media_attachment = Fabricate(:media_attachment, status: status, remote_url: 'http://example.com/attachment.png') + get :show, params: { id: media_attachment.id } + + expect(response).to have_http_status(404) + end + end +end From 661f3f26b041dd6f1f0ea646e55616f7139bb957 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 7 Jul 2020 15:22:47 +0200 Subject: [PATCH 4/4] Bump version to 3.1.5 --- CHANGELOG.md | 7 +++++++ lib/mastodon/version.rb | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d0110936..6296f0016 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,13 @@ Changelog All notable changes to this project will be documented in this file. +## [v3.1.5] - 2020-07-07 +### Security + +- Fix media attachment enumeration ([ThibG](https://github.com/tootsuite/mastodon/pull/14254)) +- Change rate limits for various paths ([Gargron](https://github.com/tootsuite/mastodon/pull/14253)) +- Fix other sessions not being logged out on password change ([Gargron](https://github.com/tootsuite/mastodon/pull/14252)) + ## [v3.1.4] - 2020-05-14 ### Added diff --git a/lib/mastodon/version.rb b/lib/mastodon/version.rb index f73b8805a..49523520d 100644 --- a/lib/mastodon/version.rb +++ b/lib/mastodon/version.rb @@ -13,7 +13,7 @@ module Mastodon end def patch - 4 + 5 end def flags