Update `devise-two-factor` to version 5.0.0 (#28325)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
main-rebase-security-fix
Matt Jankowski 2024-05-02 05:31:41 -04:00 committed by GitHub
parent 309f352e6a
commit 1e7d5d2957
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 162 additions and 18 deletions

View File

@ -31,7 +31,7 @@ gem 'browser'
gem 'charlock_holmes', '~> 0.7.7' gem 'charlock_holmes', '~> 0.7.7'
gem 'chewy', '~> 7.3' gem 'chewy', '~> 7.3'
gem 'devise', '~> 4.9' gem 'devise', '~> 4.9'
gem 'devise-two-factor', '~> 4.1' gem 'devise-two-factor'
group :pam_authentication, optional: true do group :pam_authentication, optional: true do
gem 'devise_pam_authenticatable2', '~> 9.2' gem 'devise_pam_authenticatable2', '~> 9.2'

View File

@ -97,8 +97,6 @@ GEM
activerecord (>= 3.2, < 8.0) activerecord (>= 3.2, < 8.0)
rake (>= 10.4, < 14.0) rake (>= 10.4, < 14.0)
ast (2.4.2) ast (2.4.2)
attr_encrypted (4.0.0)
encryptor (~> 3.0.0)
attr_required (1.0.2) attr_required (1.0.2)
awrence (1.2.1) awrence (1.2.1)
aws-eventstream (1.3.0) aws-eventstream (1.3.0)
@ -204,9 +202,8 @@ GEM
railties (>= 4.1.0) railties (>= 4.1.0)
responders responders
warden (~> 1.2.3) warden (~> 1.2.3)
devise-two-factor (4.1.1) devise-two-factor (5.0.0)
activesupport (~> 7.0) activesupport (~> 7.0)
attr_encrypted (>= 1.3, < 5, != 2)
devise (~> 4.0) devise (~> 4.0)
railties (~> 7.0) railties (~> 7.0)
rotp (~> 6.0) rotp (~> 6.0)
@ -236,7 +233,6 @@ GEM
htmlentities (~> 4.3.3) htmlentities (~> 4.3.3)
launchy (~> 2.1) launchy (~> 2.1)
mail (~> 2.7) mail (~> 2.7)
encryptor (3.0.0)
erubi (1.12.0) erubi (1.12.0)
et-orbi (1.2.11) et-orbi (1.2.11)
tzinfo tzinfo
@ -842,7 +838,7 @@ DEPENDENCIES
database_cleaner-active_record database_cleaner-active_record
debug (~> 1.8) debug (~> 1.8)
devise (~> 4.9) devise (~> 4.9)
devise-two-factor (~> 4.1) devise-two-factor
devise_pam_authenticatable2 (~> 9.2) devise_pam_authenticatable2 (~> 9.2)
discard (~> 1.2) discard (~> 1.2)
doorkeeper (~> 5.6) doorkeeper (~> 5.6)

View File

@ -0,0 +1,77 @@
# frozen_string_literal: true
# TODO: This file is here for legacy support during devise-two-factor upgrade.
# It should be removed after all records have been migrated.
module LegacyOtpSecret
extend ActiveSupport::Concern
private
# Decrypt and return the `encrypted_otp_secret` attribute which was used in
# prior versions of devise-two-factor
# @return [String] The decrypted OTP secret
def legacy_otp_secret
return nil unless self[:encrypted_otp_secret]
return nil unless self.class.otp_secret_encryption_key
hmac_iterations = 2000 # a default set by the Encryptor gem
key = self.class.otp_secret_encryption_key
salt = Base64.decode64(encrypted_otp_secret_salt)
iv = Base64.decode64(encrypted_otp_secret_iv)
raw_cipher_text = Base64.decode64(encrypted_otp_secret)
# The last 16 bytes of the ciphertext are the authentication tag - we use
# Galois Counter Mode which is an authenticated encryption mode
cipher_text = raw_cipher_text[0..-17]
auth_tag = raw_cipher_text[-16..-1] # rubocop:disable Style/SlicingWithRange
# this alrorithm lifted from
# https://github.com/attr-encrypted/encryptor/blob/master/lib/encryptor.rb#L54
# create an OpenSSL object which will decrypt the AES cipher with 256 bit
# keys in Galois Counter Mode (GCM). See
# https://ruby.github.io/openssl/OpenSSL/Cipher.html
cipher = OpenSSL::Cipher.new('aes-256-gcm')
# tell the cipher we want to decrypt. Symmetric algorithms use a very
# similar process for encryption and decryption, hence the same object can
# do both.
cipher.decrypt
# Use a Password-Based Key Derivation Function to generate the key actually
# used for encryptoin from the key we got as input.
cipher.key = OpenSSL::PKCS5.pbkdf2_hmac_sha1(key, salt, hmac_iterations, cipher.key_len)
# set the Initialization Vector (IV)
cipher.iv = iv
# The tag must be set after calling Cipher#decrypt, Cipher#key= and
# Cipher#iv=, but before calling Cipher#final. After all decryption is
# performed, the tag is verified automatically in the call to Cipher#final.
#
# If the auth_tag does not verify, then #final will raise OpenSSL::Cipher::CipherError
cipher.auth_tag = auth_tag
# auth_data must be set after auth_tag has been set when decrypting See
# http://ruby-doc.org/stdlib-2.0.0/libdoc/openssl/rdoc/OpenSSL/Cipher.html#method-i-auth_data-3D
# we are not adding any authenticated data but OpenSSL docs say this should
# still be called.
cipher.auth_data = ''
# #update is (somewhat confusingly named) the method which actually
# performs the decryption on the given chunk of data. Our OTP secret is
# short so we only need to call it once.
#
# It is very important that we call #final because:
#
# 1. The authentication tag is checked during the call to #final
# 2. Block based cipher modes (e.g. CBC) work on fixed size chunks. We need
# to call #final to get it to process the last chunk properly. The output
# of #final should be appended to the decrypted value. This isn't
# required for streaming cipher modes but including it is a best practice
# so that your code will continue to function correctly even if you later
# change to a block cipher mode.
cipher.update(cipher_text) + cipher.final
end
end

View File

@ -39,6 +39,7 @@
# role_id :bigint(8) # role_id :bigint(8)
# settings :text # settings :text
# time_zone :string # time_zone :string
# otp_secret :string
# #
class User < ApplicationRecord class User < ApplicationRecord
@ -72,6 +73,8 @@ class User < ApplicationRecord
devise :two_factor_authenticatable, devise :two_factor_authenticatable,
otp_secret_encryption_key: Rails.configuration.x.otp_secret otp_secret_encryption_key: Rails.configuration.x.otp_secret
include LegacyOtpSecret # Must be after the above `devise` line in order to override the legacy method
devise :two_factor_backupable, devise :two_factor_backupable,
otp_number_of_backup_codes: 10 otp_number_of_backup_codes: 10
@ -131,11 +134,6 @@ class User < ApplicationRecord
normalizes :time_zone, with: ->(time_zone) { ActiveSupport::TimeZone[time_zone].nil? ? nil : time_zone } normalizes :time_zone, with: ->(time_zone) { ActiveSupport::TimeZone[time_zone].nil? ? nil : time_zone }
normalizes :chosen_languages, with: ->(chosen_languages) { chosen_languages.compact_blank.presence } normalizes :chosen_languages, with: ->(chosen_languages) { chosen_languages.compact_blank.presence }
# This avoids a deprecation warning from Rails 5.1
# It seems possible that a future release of devise-two-factor will
# handle this itself, and this can be removed from our User class.
attribute :otp_secret
has_many :session_activations, dependent: :destroy has_many :session_activations, dependent: :destroy
delegate :can?, to: :role delegate :can?, to: :role

View File

@ -87,8 +87,7 @@ Rails.application.configure do
# Otherwise, use letter_opener, which launches a browser window to view sent mail. # Otherwise, use letter_opener, which launches a browser window to view sent mail.
config.action_mailer.delivery_method = ENV['HEROKU'] || ENV['VAGRANT'] || ENV['REMOTE_DEV'] ? :letter_opener_web : :letter_opener config.action_mailer.delivery_method = ENV['HEROKU'] || ENV['VAGRANT'] || ENV['REMOTE_DEV'] ? :letter_opener_web : :letter_opener
# We provide a default secret for the development environment here. # TODO: Remove once devise-two-factor data migration complete
# This value should not be used in production environments!
config.x.otp_secret = ENV.fetch('OTP_SECRET', '1fc2b87989afa6351912abeebe31ffc5c476ead9bf8b3d74cbc4a302c7b69a45b40b1bbef3506ddad73e942e15ed5ca4b402bf9a66423626051104f4b5f05109') config.x.otp_secret = ENV.fetch('OTP_SECRET', '1fc2b87989afa6351912abeebe31ffc5c476ead9bf8b3d74cbc4a302c7b69a45b40b1bbef3506ddad73e942e15ed5ca4b402bf9a66423626051104f4b5f05109')
# Raise error when a before_action's only/except options reference missing actions # Raise error when a before_action's only/except options reference missing actions

View File

@ -157,6 +157,7 @@ Rails.application.configure do
'Referrer-Policy' => 'same-origin', 'Referrer-Policy' => 'same-origin',
} }
# TODO: Remove once devise-two-factor data migration complete
config.x.otp_secret = ENV.fetch('OTP_SECRET') config.x.otp_secret = ENV.fetch('OTP_SECRET')
# Enable DNS rebinding protection and other `Host` header attacks. # Enable DNS rebinding protection and other `Host` header attacks.

View File

@ -44,6 +44,7 @@ Rails.application.configure do
# Print deprecation notices to the stderr. # Print deprecation notices to the stderr.
config.active_support.deprecation = :stderr config.active_support.deprecation = :stderr
# TODO: Remove once devise-two-factor data migration complete
config.x.otp_secret = '100c7faeef00caa29242f6b04156742bf76065771fd4117990c4282b8748ff3d99f8fdae97c982ab5bd2e6756a159121377cce4421f4a8ecd2d67bd7749a3fb4' config.x.otp_secret = '100c7faeef00caa29242f6b04156742bf76065771fd4117990c4282b8748ff3d99f8fdae97c982ab5bd2e6756a159121377cce4421f4a8ecd2d67bd7749a3fb4'
# Generate random VAPID keys # Generate random VAPID keys

View File

@ -0,0 +1,7 @@
# frozen_string_literal: true
class AddOtpSecretToUser < ActiveRecord::Migration[7.1]
def change
add_column :users, :otp_secret, :string
end
end

View File

@ -0,0 +1,39 @@
# frozen_string_literal: true
class MigrateDeviseTwoFactorSecrets < ActiveRecord::Migration[7.1]
disable_ddl_transaction!
class MigrationUser < ApplicationRecord
self.table_name = :users
devise :two_factor_authenticatable,
otp_secret_encryption_key: Rails.configuration.x.otp_secret
include LegacyOtpSecret # Must be after the above `devise` line in order to override the legacy method
end
def up
MigrationUser.reset_column_information
users_with_otp_enabled.find_each do |user|
# Gets the new value on already-updated users
# Falls back to legacy value on not-yet-migrated users
otp_secret = user.otp_secret
Rails.logger.debug { "Processing #{user.email}" }
# This is a no-op for migrated users and updates format for not migrated
user.update!(otp_secret: otp_secret)
end
end
def down
raise ActiveRecord::IrreversibleMigration
end
private
def users_with_otp_enabled
MigrationUser.where(otp_required_for_login: true, otp_secret: nil)
end
end

View File

@ -1199,6 +1199,7 @@ ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do
t.bigint "role_id" t.bigint "role_id"
t.text "settings" t.text "settings"
t.string "time_zone" t.string "time_zone"
t.string "otp_secret"
t.index ["account_id"], name: "index_users_on_account_id" t.index ["account_id"], name: "index_users_on_account_id"
t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true
t.index ["created_by_application_id"], name: "index_users_on_created_by_application_id", where: "(created_by_application_id IS NOT NULL)" t.index ["created_by_application_id"], name: "index_users_on_created_by_application_id", where: "(created_by_application_id IS NOT NULL)"

View File

@ -127,6 +127,14 @@ namespace :tests do
exit(1) exit(1)
end end
# This is checking the attribute rather than the method, to avoid the legacy fallback
# and ensure the data has been migrated
unless Account.find_local('qcuser').user[:otp_secret] == 'anotpsecretthatshouldbeencrypted'
puts "DEBUG: #{Account.find_local('qcuser').user.inspect}"
puts 'OTP secret for user not preserved as expected'
exit(1)
end
puts 'No errors found. Database state is consistent with a successful migration process.' puts 'No errors found. Database state is consistent with a successful migration process.'
end end
@ -213,9 +221,15 @@ namespace :tests do
(4, 10, 'kmruser@localhost', now(), now(), false, 'ku', '{en,kmr,ku,ckb}'); (4, 10, 'kmruser@localhost', now(), now(), false, 'ku', '{en,kmr,ku,ckb}');
INSERT INTO "users" INSERT INTO "users"
(id, account_id, email, created_at, updated_at, locale) (id, account_id, email, created_at, updated_at, locale,
encrypted_otp_secret, encrypted_otp_secret_iv, encrypted_otp_secret_salt,
otp_required_for_login)
VALUES VALUES
(5, 11, 'qcuser@localhost', now(), now(), 'fr-QC'); (5, 11, 'qcuser@localhost', now(), now(), 'fr-QC',
E'Fttsy7QAa0edaDfdfSz094rRLAxc8cJweDQ4BsWH/zozcdVA8o9GLqcKhn2b\nGi/V\n',
'rys3THICkr60BoWC',
'_LMkAGvdg7a+sDIKjI3mR2Q==',
true);
INSERT INTO "settings" INSERT INTO "settings"
(id, thing_type, thing_id, var, value, created_at, updated_at) (id, thing_type, thing_id, var, value, created_at, updated_at)

View File

@ -9,14 +9,25 @@ RSpec.describe User do
it_behaves_like 'two_factor_backupable' it_behaves_like 'two_factor_backupable'
describe 'otp_secret' do describe 'legacy_otp_secret' do
it 'is encrypted with OTP_SECRET environment variable' do it 'is encrypted with OTP_SECRET environment variable' do
user = Fabricate(:user, user = Fabricate(:user,
encrypted_otp_secret: "Fttsy7QAa0edaDfdfSz094rRLAxc8cJweDQ4BsWH/zozcdVA8o9GLqcKhn2b\nGi/V\n", encrypted_otp_secret: "Fttsy7QAa0edaDfdfSz094rRLAxc8cJweDQ4BsWH/zozcdVA8o9GLqcKhn2b\nGi/V\n",
encrypted_otp_secret_iv: 'rys3THICkr60BoWC', encrypted_otp_secret_iv: 'rys3THICkr60BoWC',
encrypted_otp_secret_salt: '_LMkAGvdg7a+sDIKjI3mR2Q==') encrypted_otp_secret_salt: '_LMkAGvdg7a+sDIKjI3mR2Q==')
expect(user.otp_secret).to eq 'anotpsecretthatshouldbeencrypted' expect(user.send(:legacy_otp_secret)).to eq 'anotpsecretthatshouldbeencrypted'
end
end
describe 'otp_secret' do
it 'encrypts the saved value' do
user = Fabricate(:user, otp_secret: '123123123')
user.reload
expect(user.otp_secret).to eq '123123123'
expect(user.attributes_before_type_cast[:otp_secret]).to_not eq '123123123'
end end
end end