From f52181db94661aa5986e4c8ccd136b646c908b69 Mon Sep 17 00:00:00 2001 From: albert Date: Mon, 4 Mar 2013 22:55:41 -0500 Subject: [PATCH] Major revamp of security. Passwords are first SHA1 hashed and then that hash is bcrypted. Bcrypted hashes are stored in a new column on users. This separate column is only to allow for rollbacks, eventually the old SHA1 hash column will be removed. Sensitive cookie details are now encrypted to prevent user tampering and more stringent checks on secret_token and session_secret_key are enforced. --- Gemfile | 2 +- app/logical/session_creator.rb | 4 +- app/logical/session_loader.rb | 4 +- app/models/user.rb | 49 +++++++++++-------- config/application.rb | 3 +- config/danbooru_default_config.rb | 5 -- config/initializers/secret_token.rb | 18 +++---- config/state_checker.rb | 36 ++++++++++++++ ...130305005138_add_bcrypt_fields_to_users.rb | 7 +++ db/structure.sql | 7 ++- script/fixes/004.rb | 4 +- .../user/password_resets_controller_test.rb | 4 +- test/unit/user_test.rb | 33 ++++++------- 13 files changed, 108 insertions(+), 68 deletions(-) create mode 100644 config/state_checker.rb create mode 100644 db/migrate/20130305005138_add_bcrypt_fields_to_users.rb diff --git a/Gemfile b/Gemfile index 26d6482a8..a879e99d4 100644 --- a/Gemfile +++ b/Gemfile @@ -32,7 +32,7 @@ gem 'net-sftp' gem 'newrelic_rpm' gem 'term-ansicolor', :require => "term/ansicolor" gem 'diff-lcs', :require => "diff/lcs/array" -gem 'bcrypt-ruby' +gem 'bcrypt-ruby', :require => "bcrypt" group :development do gem 'ruby-prof' diff --git a/app/logical/session_creator.rb b/app/logical/session_creator.rb index 0aa0a6ade..3997e8fb1 100644 --- a/app/logical/session_creator.rb +++ b/app/logical/session_creator.rb @@ -15,8 +15,8 @@ class SessionCreator user.update_column(:last_logged_in_at, Time.now) if remember.present? - cookies[:user_name] = {:expires => 1.year.from_now, :value => user.name} - cookies[:cookie_password_hash] = {:expires => 1.year.from_now, :value => user.cookie_password_hash} + cookies.permanent.signed[:user_name] = user.name + cookies.permanent.signed[:password_hash] = user.bcrypt_password_hash end session[:user_id] = user.id diff --git a/app/logical/session_loader.rb b/app/logical/session_loader.rb index 4513ed36e..46edc8177 100644 --- a/app/logical/session_loader.rb +++ b/app/logical/session_loader.rb @@ -32,7 +32,7 @@ private end def load_cookie_user - CurrentUser.user = User.find_by_name(cookies[:user_name]) + CurrentUser.user = User.find_by_name(cookies.signed[:user_name]) CurrentUser.ip_addr = request.remote_ip end @@ -41,7 +41,7 @@ private end def cookie_password_hash_valid? - cookies[:cookie_password_hash] && User.authenticate_cookie_hash(cookies[:user_name], cookies[:cookie_password_hash]) + cookies[:password_hash] && User.authenticate_cookie_hash(cookies.signed[:user_name], cookies.signed[:password_hash]) end def update_last_logged_in_at diff --git a/app/models/user.rb b/app/models/user.rb index 54cfdd8d1..fd42862c1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -121,19 +121,24 @@ class User < ActiveRecord::Base end module PasswordMethods + def bcrypt_password + BCrypt::Password.new(bcrypt_password_hash) + end + def encrypt_password_on_create - self.password_hash = User.sha1(password) + self.password_hash = "" + self.bcrypt_password_hash = User.bcrypt(password) end def encrypt_password_on_update return if password.blank? return if old_password.blank? - if User.sha1(old_password) == password_hash - self.password_hash = User.sha1(password) + if bcrypt_password == User.sha1(old_password) + self.bcrypt_password_hash = User.bcrypt(password) return true else - errors[:old_password] << "is incorrect" + errors[:old_password] = "is incorrect" return false end end @@ -149,7 +154,7 @@ class User < ActiveRecord::Base end pass << rand(100).to_s - update_column(:password_hash, User.sha1(pass)) + update_column(:bcrypt_password_hash, User.bcrypt(pass)) pass end @@ -168,29 +173,31 @@ class User < ActiveRecord::Base end def authenticate_hash(name, hash) - where(["lower(name) = ? AND password_hash = ?", name.downcase, hash]).first != nil + user = find_by_name(name) + if user && user.bcrypt_password == hash + user + else + nil + end end - + def authenticate_cookie_hash(name, hash) - user = User.find_by_name(name) - return nil if user.nil? - return hash == user.cookie_password_hash + user = find_by_name(name) + if user && user.bcrypt_password_hash == hash + user + else + nil + end + end + + def bcrypt(pass) + BCrypt::Password.create(sha1(pass)) end def sha1(pass) Digest::SHA1.hexdigest("#{Danbooru.config.password_salt}--#{pass}--") end end - - def cookie_password_hash - hash = password_hash - - (name.size + 8).times do - hash = User.sha1(hash) - end - - return hash - end end module FavoriteMethods @@ -456,7 +463,7 @@ class User < ActiveRecord::Base module ApiMethods def hidden_attributes - super + [:password_hash, :email, :email_verification_key] + super + [:password_hash, :bcrypt_password_hash, :email, :email_verification_key] end def serializable_hash(options = {}) diff --git a/config/application.rb b/config/application.rb index 9ad924f98..d3c023f34 100644 --- a/config/application.rb +++ b/config/application.rb @@ -1,5 +1,4 @@ require File.expand_path('../boot', __FILE__) - require 'rails/all' if defined?(Bundler) @@ -11,6 +10,7 @@ end module Danbooru class Application < Rails::Application + config.active_record.schema_format = :sql config.encoding = "utf-8" config.filter_parameters += [:password] @@ -25,4 +25,3 @@ module Danbooru config.log_tags = [lambda {|req| "PID:#{Process.pid}"}] end end - diff --git a/config/danbooru_default_config.rb b/config/danbooru_default_config.rb index b30fbfe02..3eb61329c 100644 --- a/config/danbooru_default_config.rb +++ b/config/danbooru_default_config.rb @@ -112,11 +112,6 @@ module Danbooru !user.is_privileged? end - # This is required for Rails 2.0. - def session_secret_key - "This should be at least 30 characters long" - end - # Users cannot search for more than X regular tags at a time. def base_tag_query_limit 6 diff --git a/config/initializers/secret_token.rb b/config/initializers/secret_token.rb index 38b12c5bd..924f5f907 100644 --- a/config/initializers/secret_token.rb +++ b/config/initializers/secret_token.rb @@ -1,13 +1,9 @@ -# Be sure to restart your server when you modify this file. +require File.expand_path('../../state_checker', __FILE__) -# Your secret key for verifying the integrity of signed cookies. -# If you change this key, all old signed cookies will become invalid! -# Make sure the secret is at least 30 characters and all random, -# no regular words or you'll be exposed to dictionary attacks. - -if File.exists?(File.expand_path("~/.danbooru/secret_token")) - Danbooru::Application.config.secret_token = File.read(File.expand_path("~/.danbooru/secret_token")) -else - Danbooru::Application.config.secret_token = SecureRandom.hex(64) -end +StateChecker.new.check! +Danbooru::Application.config.action_dispatch.session = { + :key => '_danbooru2_session', + :secret => File.read(File.expand_path("~/.danbooru/session_secret_key")) +} +Danbooru::Application.config.secret_token = File.read(File.expand_path("~/.danbooru/secret_token")) diff --git a/config/state_checker.rb b/config/state_checker.rb new file mode 100644 index 000000000..a6f4b0a61 --- /dev/null +++ b/config/state_checker.rb @@ -0,0 +1,36 @@ +class StateChecker + def check! + check_secret_token + check_session_secret_key + end + +private + + def secret_token_path + File.expand_path("~/.danbooru/secret_token") + end + + def check_secret_token + unless File.exists?(secret_token_path) + raise "You must create a file in #{secret_token_path} containing a secret key. It should be a string of at least 32 random characters." + end + + if File.stat(secret_token_path).world_readable? || File.stat(secret_token_path).world_writable? + raise "#{secret_token_path} must not be world readable or writable" + end + end + + def session_secret_key_path + File.expand_path("~/.danbooru/session_secret_key") + end + + def check_session_secret_key + unless File.exists?(session_secret_key_path) + raise "You must create a file in #{session_secret_key_path} containing a secret key. It should be a string of at least 32 random characters." + end + + if File.stat(session_secret_key_path).world_readable? || File.stat(session_secret_key_path).world_writable? + raise "#{session_secret_key_path} must not be world readable or writable" + end + end +end diff --git a/db/migrate/20130305005138_add_bcrypt_fields_to_users.rb b/db/migrate/20130305005138_add_bcrypt_fields_to_users.rb new file mode 100644 index 000000000..5e2b54bbf --- /dev/null +++ b/db/migrate/20130305005138_add_bcrypt_fields_to_users.rb @@ -0,0 +1,7 @@ +class AddBcryptFieldsToUsers < ActiveRecord::Migration + def change + execute "set statement_timeout = 0" + + add_column :users, :bcrypt_password_hash, :text + end +end diff --git a/db/structure.sql b/db/structure.sql index 32f8a2cb7..f486d06bb 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2596,7 +2596,8 @@ CREATE TABLE users ( default_image_size character varying(255) DEFAULT 'large'::character varying NOT NULL, favorite_tags text, blacklisted_tags text, - time_zone character varying(255) DEFAULT 'Eastern Time (US & Canada)'::character varying NOT NULL + time_zone character varying(255) DEFAULT 'Eastern Time (US & Canada)'::character varying NOT NULL, + bcrypt_password_hash text ); @@ -6207,4 +6208,6 @@ INSERT INTO schema_migrations (version) VALUES ('20130221032344'); INSERT INTO schema_migrations (version) VALUES ('20130221035518'); -INSERT INTO schema_migrations (version) VALUES ('20130221214811'); \ No newline at end of file +INSERT INTO schema_migrations (version) VALUES ('20130221214811'); + +INSERT INTO schema_migrations (version) VALUES ('20130305005138'); \ No newline at end of file diff --git a/script/fixes/004.rb b/script/fixes/004.rb index e346061ea..f95e4fe91 100644 --- a/script/fixes/004.rb +++ b/script/fixes/004.rb @@ -2,4 +2,6 @@ require File.expand_path(File.join(File.dirname(__FILE__), '..', '..', 'config', 'environment')) -puts Post.count \ No newline at end of file +User.find_each do |user| + user.update_column(:bcrypt_password_hash, BCrypt::Password.create(user.password_hash)) +end diff --git a/test/functional/maintenance/user/password_resets_controller_test.rb b/test/functional/maintenance/user/password_resets_controller_test.rb index 8c03073a5..1ba087270 100644 --- a/test/functional/maintenance/user/password_resets_controller_test.rb +++ b/test/functional/maintenance/user/password_resets_controller_test.rb @@ -101,7 +101,7 @@ module Maintenance @user = FactoryGirl.create(:user) @nonce = FactoryGirl.create(:user_password_reset_nonce, :email => @user.email) ActionMailer::Base.deliveries.clear - @old_password = @user.password_hash + @old_password = @user.bcrypt_password_hash post :update, :email => @nonce.email, :key => @nonce.key end @@ -115,7 +115,7 @@ module Maintenance should "change the password" do @user.reload - assert_not_equal(@old_password, @user.password_hash) + assert_not_equal(@old_password, @user.bcrypt_password_hash) end should "delete the nonce" do diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 0f470a5b1..3b73e5b90 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -35,7 +35,7 @@ class UserTest < ActiveSupport::TestCase assert_difference("ModAction.count") do @user.invite!(User::Levels::CONTRIBUTOR) end - assert_equal("#{@user.id} level changed Member -> Contributor by #{CurrentUser.name}", ModAction.first.description) + assert_equal("#{@user.name} level changed Member -> Contributor by #{CurrentUser.name}", ModAction.last.description) end end @@ -199,21 +199,16 @@ class UserTest < ActiveSupport::TestCase end end - context "cookie password hash" do - setup do - @user = FactoryGirl.create(:user, :name => "albert", :password_hash => "1234") - end - - should "be correct" do - assert_equal("8ac3b1d04bdb95ba92f9e355897c880e0d88ac5a", @user.cookie_password_hash) - end - - should "validate" do - assert(User.authenticate_cookie_hash(@user.name, "8ac3b1d04bdb95ba92f9e355897c880e0d88ac5a")) - end - end - context "password" do + should "match the cookie hash" do + @user = FactoryGirl.create(:user) + @user.password = "zugzug5" + @user.password_confirmation = "zugzug5" + @user.save + @user.reload + assert(User.authenticate_cookie_hash(@user.name, @user.bcrypt_password_hash)) + end + should "match the confirmation" do @user = FactoryGirl.create(:user) @user.password = "zugzug5" @@ -248,25 +243,25 @@ class UserTest < ActiveSupport::TestCase should "not change the password if the password and old password are blank" do @user = FactoryGirl.create(:user, :password => "67890") @user.update_attributes(:password => "", :old_password => "") - assert_equal(User.sha1("67890"), @user.password_hash) + assert(@user.bcrypt_password == User.sha1("67890")) end should "not change the password if the old password is incorrect" do @user = FactoryGirl.create(:user, :password => "67890") @user.update_attributes(:password => "12345", :old_password => "abcdefg") - assert_equal(User.sha1("67890"), @user.password_hash) + assert(@user.bcrypt_password == User.sha1("67890")) end should "not change the password if the old password is blank" do @user = FactoryGirl.create(:user, :password => "67890") @user.update_attributes(:password => "12345", :old_password => "") - assert_equal(User.sha1("67890"), @user.password_hash) + assert(@user.bcrypt_password == User.sha1("67890")) end should "change the password if the old password is correct" do @user = FactoryGirl.create(:user, :password => "67890") @user.update_attributes(:password => "12345", :old_password => "67890") - assert_equal(User.sha1("12345"), @user.password_hash) + assert(@user.bcrypt_password == User.sha1("12345")) end context "in the json representation" do