Skip to content

Commit

Permalink
Protect forgotten password changes from MFA bypass.
Browse files Browse the repository at this point in the history
It was previously possible to submit the password update form directly
with only the confirmation token, bypassing MFA protections.
  • Loading branch information
martinemde authored and simi committed Jan 8, 2024
1 parent 6893b94 commit 0b3272a
Show file tree
Hide file tree
Showing 17 changed files with 269 additions and 89 deletions.
2 changes: 1 addition & 1 deletion app/controllers/adoptions_controller.rb
Expand Up @@ -3,7 +3,7 @@ class AdoptionsController < ApplicationController

before_action :find_rubygem
before_action :verify_ownership_requestable
before_action :redirect_to_verify, if: -> { current_user_is_owner? && !password_session_active? }
before_action :redirect_to_verify, if: -> { current_user_is_owner? && !verified_session_active? }

def index
@ownership_call = @rubygem.ownership_call
Expand Down
4 changes: 0 additions & 4 deletions app/controllers/application_controller.rb
Expand Up @@ -153,10 +153,6 @@ def set_cache_headers
response.headers["Expires"] = "Fri, 01 Jan 1990 00:00:00 GMT"
end

def password_session_active?
session[:verification] && session[:verification] > Time.current && session.fetch(:verified_user, "") == current_user.id
end

def set_error_context_user
return unless current_user

Expand Down
13 changes: 12 additions & 1 deletion app/controllers/concerns/session_verifiable.rb
Expand Up @@ -6,7 +6,7 @@ def verify_session_before(**opts)
before_action :redirect_to_signin, **opts, unless: :signed_in?
before_action :redirect_to_new_mfa, **opts, if: :mfa_required_not_yet_enabled?
before_action :redirect_to_settings_strong_mfa_required, **opts, if: :mfa_required_weak_level_enabled?
before_action :redirect_to_verify, **opts, unless: :password_session_active?
before_action :redirect_to_verify, **opts, unless: :verified_session_active?
end
end

Expand All @@ -25,5 +25,16 @@ def redirect_to_verify
session[:redirect_uri] = verify_session_redirect_path
redirect_to verify_session_path
end

def session_verified
session[:verified_user] = current_user.id
session[:verification] = Gemcutter::PASSWORD_VERIFICATION_EXPIRY.from_now
end

def verified_session_active?
session[:verification] &&
session[:verification] > Time.current &&
session.fetch(:verified_user, "") == current_user.id
end
end
end
34 changes: 28 additions & 6 deletions app/controllers/passwords_controller.rb
@@ -1,10 +1,17 @@
class PasswordsController < Clearance::PasswordsController
include MfaExpiryMethods
include WebauthnVerifiable
include SessionVerifiable

before_action :validate_confirmation_token, only: %i[edit otp_edit webauthn_edit]
after_action :delete_mfa_expiry_session, only: %i[otp_edit webauthn_edit]

# By default, clearance expects the token to be submitted with the password update.
# We already invalidated the token when the user became verified by token(+mfa).
skip_before_action :ensure_existing_user, only: %i[update]
# Instead of the token, we now require the user to have been verified recently.
verify_session_before only: %i[update]

def edit
if @user.mfa_enabled?
@otp_verification_url = otp_edit_user_password_url(@user, token: @user.confirmation_token)
Expand All @@ -14,17 +21,16 @@ def edit

render template: "multifactor_auths/prompt"
else
# When user doesn't have mfa, a valid token is a full "magic link" sign in.
verified_sign_in
render template: "passwords/edit"
end
end

def update
@user = find_user_for_update

if @user.update_password password_from_password_reset_params
@user.reset_api_key! if reset_params[:reset_api_key] == "true"
@user.api_keys.expire_all! if reset_params[:reset_api_keys] == "true"
sign_in @user
if current_user.update_password password_from_password_reset_params
current_user.reset_api_key! if reset_params[:reset_api_key] == "true"
current_user.api_keys.expire_all! if reset_params[:reset_api_keys] == "true"
redirect_to url_after_update
session[:password_reset_token] = nil
else
Expand All @@ -35,6 +41,8 @@ def update

def otp_edit
if otp_edit_conditions_met?
# When the user identified by the email token submits adequate totp, they are logged in
verified_sign_in
render template: "passwords/edit"
elsif !session_active?
login_failure(t("multifactor_auths.session_expired"))
Expand All @@ -51,11 +59,20 @@ def webauthn_edit

return login_failure(@webauthn_error) unless webauthn_credential_verified?

# When the user identified by the email token submits verified webauthn, they are logged in
verified_sign_in
render template: "passwords/edit"
end

private

def verified_sign_in
sign_in @user
session_verified
@user.update!(confirmation_token: nil)
StatsD.increment "login.success"
end

def url_after_update
dashboard_path
end
Expand All @@ -81,4 +98,9 @@ def login_failure(message)
flash.now.alert = message
render template: "multifactor_auths/prompt", status: :unauthorized
end

def redirect_to_verify
session[:redirect_uri] = verify_session_redirect_path
redirect_to verify_session_path, alert: t("verification_expired")
end
end
4 changes: 2 additions & 2 deletions app/controllers/sessions_controller.rb
@@ -1,6 +1,7 @@
class SessionsController < Clearance::SessionsController
include MfaExpiryMethods
include WebauthnVerifiable
include SessionVerifiable

before_action :redirect_to_signin, unless: :signed_in?, only: %i[verify webauthn_authenticate authenticate]
before_action :redirect_to_new_mfa, if: :mfa_required_not_yet_enabled?, only: %i[verify webauthn_authenticate authenticate]
Expand Down Expand Up @@ -93,8 +94,7 @@ def webauthn_authenticate
private

def mark_verified
session[:verified_user] = current_user.id
session[:verification] = Gemcutter::PASSWORD_VERIFICATION_EXPIRY.from_now
session_verified
redirect_to session.delete(:redirect_uri) || root_path
end

Expand Down
4 changes: 2 additions & 2 deletions app/views/passwords/edit.html.erb
@@ -1,9 +1,9 @@
<% @title = t('.title') %>
<%= form_for(:password_reset,
:url => user_password_path(@user, :token => @user.confirmation_token),
:url => user_password_path(current_user),
:html => { :method => :put }) do |form| %>
<%= error_messages_for @user %>
<%= error_messages_for current_user %>
<div class="password_field">
<%= form.label :password, "Password", :class => 'form__label' %>
<%= form.password_field :password, autocomplete: 'new-password', class: 'form__input' %>
Expand Down
1 change: 1 addition & 0 deletions config/locales/de.yml
Expand Up @@ -3,6 +3,7 @@ de:
credentials_required:
edit: Bearbeiten
failure_when_forbidden:
verification_expired:
feed_latest: RubyGems.org | Neueste Gems
feed_subscribed: RubyGems.org | Abonnierte Gems
footer_about_html:
Expand Down
1 change: 1 addition & 0 deletions config/locales/en.yml
Expand Up @@ -3,6 +3,7 @@ en:
credentials_required: Credentials required
edit: Edit
failure_when_forbidden: Please double check the URL or try submitting it again.
verification_expired: The verification has expired. Please verify again.
feed_latest: RubyGems.org | Latest Gems
feed_subscribed: RubyGems.org | Subscribed Gems
footer_about_html:
Expand Down
1 change: 1 addition & 0 deletions config/locales/es.yml
Expand Up @@ -3,6 +3,7 @@ es:
credentials_required: Credenciales requeridas
edit: Editar
failure_when_forbidden: Por favor verifica la URL o inténtalo nuevamente.
verification_expired:
feed_latest: RubyGems.org | Gemas más recientes
feed_subscribed: RubyGems.org | Suscripciones a gemas
footer_about_html: RubyGems.org es el servicio de alojamiento de Gemas de la comunidad
Expand Down
1 change: 1 addition & 0 deletions config/locales/fr.yml
Expand Up @@ -3,6 +3,7 @@ fr:
credentials_required:
edit: Modification
failure_when_forbidden: Veuillez vérifier l'URL ou réessayer.
verification_expired:
feed_latest: RubyGems.org | Derniers Gems
feed_subscribed: RubyGems.org | Gems abonnés
footer_about_html: RubyGems.org est le service d&rsquo;hébergement de la communauté
Expand Down
1 change: 1 addition & 0 deletions config/locales/ja.yml
Expand Up @@ -3,6 +3,7 @@ ja:
credentials_required: 認証情報が必要です
edit: 編集
failure_when_forbidden: URLを見返すか、再度送信してみてください。
verification_expired:
feed_latest: RubyGems.org | 最新のgemの一覧
feed_subscribed: RubyGems.org | 購読したgemの一覧
footer_about_html: RubyGems.orgはRubyコミュニティのgemのホスティングサービスです。すぐに<a href="%{publish_docs}">gemを公開</a>して<a
Expand Down
1 change: 1 addition & 0 deletions config/locales/nl.yml
Expand Up @@ -3,6 +3,7 @@ nl:
credentials_required:
edit: Wijzig
failure_when_forbidden: Controleer het webadres, en probeer het opnieuw.
verification_expired:
feed_latest: RubyGems.org | Nieuwste Gems
feed_subscribed: RubyGems.org | Geabonneerde Gems
footer_about_html: RubyGems.org is de gem hosting service van de Ruby community.
Expand Down
1 change: 1 addition & 0 deletions config/locales/pt-BR.yml
Expand Up @@ -3,6 +3,7 @@ pt-BR:
credentials_required:
edit: Editar
failure_when_forbidden: Por favor, confira a URL ou tente submetê-la novamente.
verification_expired:
feed_latest: RubyGems.org | Últimas Gems
feed_subscribed: RubyGems.org | Gems do seu Feed
footer_about_html: RubyGems.org é o serviço de hospedagem de gems da comunidade
Expand Down
1 change: 1 addition & 0 deletions config/locales/zh-CN.yml
Expand Up @@ -3,6 +3,7 @@ zh-CN:
credentials_required: 需要凭证
edit: 编辑
failure_when_forbidden: 请再次确认 URL 或尝试重新提交
verification_expired:
feed_latest: RubyGems.org | 最新的 Gem
feed_subscribed: RubyGems.org | 订阅的 Gem
footer_about_html: RubyGems.org 是 Ruby 社区的 Gem 托管服务。 立即 <a href="%{publish_docs}">发布您的
Expand Down
1 change: 1 addition & 0 deletions config/locales/zh-TW.yml
Expand Up @@ -3,6 +3,7 @@ zh-TW:
credentials_required:
edit: 編輯
failure_when_forbidden: 請確認 URL 或再次提交
verification_expired:
feed_latest: RubyGems.org | 最新 Gems
feed_subscribed: RubyGems.org | 訂閱 Gems
footer_about_html: RubyGems.org 是 Ruby 社群的 Gem 套件管理服務,讓你能立即地發佈及安裝你的 Gem 套件,並且利用
Expand Down

0 comments on commit 0b3272a

Please sign in to comment.