diff --git a/activerecord-session_store.gemspec b/activerecord-session_store.gemspec index 33a7e1b..8ffceed 100644 --- a/activerecord-session_store.gemspec +++ b/activerecord-session_store.gemspec @@ -19,10 +19,10 @@ Gem::Specification.new do |s| s.extra_rdoc_files = %w( README.md ) s.rdoc_options.concat ['--main', 'README.md'] - s.add_dependency('activerecord', '>= 5.2') - s.add_dependency('actionpack', '>= 5.2') - s.add_dependency('railties', '>= 5.2') - s.add_dependency('rack', '>= 2.0.0', '< 3') + s.add_dependency('activerecord', '>= 5.2.4.1') + s.add_dependency('actionpack', '>= 5.2.4.1') + s.add_dependency('railties', '>= 5.2.4.1') + s.add_dependency('rack', '>= 2.0.8', '< 3') s.add_dependency('multi_json', '~> 1.11', '>= 1.11.2') s.add_development_dependency('sqlite3') diff --git a/lib/action_dispatch/session/active_record_store.rb b/lib/action_dispatch/session/active_record_store.rb index 4179378..99bd8e2 100644 --- a/lib/action_dispatch/session/active_record_store.rb +++ b/lib/action_dispatch/session/active_record_store.rb @@ -52,7 +52,7 @@ module Session # # The example SqlBypass class is a generic SQL session store. You may # use it as a basis for high-performance database-specific stores. - class ActiveRecordStore < ActionDispatch::Session::AbstractStore + class ActiveRecordStore < ActionDispatch::Session::AbstractSecureStore # The class used for session storage. Defaults to # ActiveRecord::SessionStore::Session cattr_accessor :session_class @@ -63,11 +63,11 @@ class ActiveRecordStore < ActionDispatch::Session::AbstractStore private def get_session(request, sid) logger.silence do - unless sid and session = @@session_class.find_by_session_id(sid) + unless sid and session = get_session_with_fallback(sid) # If the sid was nil or if there is no pre-existing session under the sid, # force the generation of a new sid and associate a new session associated with the new sid sid = generate_sid - session = @@session_class.new(:session_id => sid, :data => {}) + session = @@session_class.new(:session_id => sid.private_id, :data => {}) end request.env[SESSION_RECORD_KEY] = session [sid, session.data] @@ -76,7 +76,7 @@ def get_session(request, sid) def write_session(request, sid, session_data, options) logger.silence do - record = get_session_model(request, sid) + record, sid = get_session_model(request, sid) record.data = session_data return false unless record.save @@ -94,7 +94,7 @@ def write_session(request, sid, session_data, options) def delete_session(request, session_id, options) logger.silence do if sid = current_session_id(request) - if model = @@session_class.find_by_session_id(sid) + if model = get_session_with_fallback(sid) data = model.data model.destroy end @@ -106,7 +106,7 @@ def delete_session(request, session_id, options) new_sid = generate_sid if options[:renew] - new_model = @@session_class.new(:session_id => new_sid, :data => data) + new_model = @@session_class.new(:session_id => new_sid.private_id, :data => data) new_model.save request.env[SESSION_RECORD_KEY] = new_model end @@ -117,10 +117,10 @@ def delete_session(request, session_id, options) def get_session_model(request, id) logger.silence do - model = @@session_class.find_by_session_id(id) - if !model + model = get_session_with_fallback(id) + unless model id = generate_sid - model = @@session_class.new(:session_id => id, :data => {}) + model = @@session_class.new(:session_id => id.private_id, :data => {}) model.save end if request.env[ENV_SESSION_OPTIONS_KEY][:id].nil? @@ -128,13 +128,24 @@ def get_session_model(request, id) else request.env[SESSION_RECORD_KEY] ||= model end - model + [model, id] + end + end + + def get_session_with_fallback(sid) + if sid && !self.class.private_session_id?(sid.public_id) + if (secure_session = @@session_class.find_by_session_id(sid.private_id)) + secure_session + elsif (insecure_session = @@session_class.find_by_session_id(sid.public_id)) + insecure_session.session_id = sid.private_id # this causes the session to be secured + insecure_session + end end end def find_session(request, id) - model = get_session_model(request, id) - [model.session_id, model.data] + model, id = get_session_model(request, id) + [id, model.data] end module NilLogger @@ -146,7 +157,12 @@ def self.silence def logger ActiveRecord::Base.logger || NilLogger end + + def self.private_session_id?(session_id) + # user tried to retrieve a session by a private key? + session_id =~ /\A\d+::/ + end + end end end - diff --git a/lib/active_record/session_store/session.rb b/lib/active_record/session_store/session.rb index bef1ba4..aec3253 100644 --- a/lib/active_record/session_store/session.rb +++ b/lib/active_record/session_store/session.rb @@ -38,8 +38,8 @@ def setup_sessid_compatibility! # Reset column info since it may be stale. reset_column_information if columns_hash['sessid'] - def self.find_by_session_id(*args) - find_by_sessid(*args) + def self.find_by_session_id(session_id) + find_by_sessid(session_id) end define_method(:session_id) { sessid } @@ -71,6 +71,27 @@ def loaded? @data end + # This method was introduced when addressing CVE-2019-16782 + # (see https://github.com/rack/rack/security/advisories/GHSA-hrqr-hxpp-chr3). + # Sessions created on version <= 1.1.3 were guessable via a timing attack. + # To secure sessions created on those old versions, this method can be called + # on all existing sessions in the database. Users will not lose their session + # when this is done. + def secure! + session_id_column = if self.class.columns_hash['sessid'] + :sessid + else + :session_id + end + raw_session_id = read_attribute(session_id_column) + if ActionDispatch::Session::ActiveRecordStore.private_session_id?(raw_session_id) + # is already private, nothing to do + else + session_id_object = Rack::Session::SessionId.new(raw_session_id) + update_column(session_id_column, session_id_object.private_id) + end + end + private def serialize_data! unless loaded? diff --git a/lib/active_record/session_store/sql_bypass.rb b/lib/active_record/session_store/sql_bypass.rb index 40a033b..3a1bb63 100644 --- a/lib/active_record/session_store/sql_bypass.rb +++ b/lib/active_record/session_store/sql_bypass.rb @@ -60,15 +60,16 @@ def connection_pool # Look up a session by id and deserialize its data if found. def find_by_session_id(session_id) - if record = connection.select_one("SELECT #{connection.quote_column_name(data_column)} AS data FROM #{@@table_name} WHERE #{connection.quote_column_name(@@session_id_column)}=#{connection.quote(session_id.to_s)}") - new(:session_id => session_id, :serialized_data => record['data']) + if record = connection.select_one("SELECT #{connection.quote_column_name(data_column)} AS data FROM #{@@table_name} WHERE #{connection.quote_column_name(@@session_id_column)}=#{connection.quote(session_id)}") + new(:session_id => session_id, :retrieved_by => session_id, :serialized_data => record['data']) end end end delegate :connection, :connection=, :connection_pool, :connection_pool=, :to => self - attr_reader :session_id, :new_record + attr_reader :new_record + attr_accessor :session_id alias :new_record? :new_record attr_writer :data @@ -77,7 +78,8 @@ def find_by_session_id(session_id) # telling us to postpone deserializing until the data is requested. # We need to handle a normal data attribute in case of a new record. def initialize(attributes) - @session_id = attributes[:session_id] + @session_id = attributes[:session_id] + @retrieved_by = attributes[:retrieved_by] @data = attributes[:data] @serialized_data = attributes[:serialized_data] @new_record = @serialized_data.nil? @@ -122,8 +124,10 @@ def save else connect.update <<-end_sql, 'Update session' UPDATE #{table_name} - SET #{connect.quote_column_name(data_column)}=#{connect.quote(serialized_data)} - WHERE #{connect.quote_column_name(session_id_column)}=#{connect.quote(session_id)} + SET + #{connect.quote_column_name(data_column)}=#{connect.quote(serialized_data)}, + #{connect.quote_column_name(session_id_column)}=#{connect.quote(@session_id)} + WHERE #{connect.quote_column_name(session_id_column)}=#{connect.quote(@retrieved_by)} end_sql end end @@ -134,7 +138,7 @@ def destroy connect = connection connect.delete <<-end_sql, 'Destroy session' DELETE FROM #{table_name} - WHERE #{connect.quote_column_name(session_id_column)}=#{connect.quote(session_id.to_s)} + WHERE #{connect.quote_column_name(session_id_column)}=#{connect.quote(session_id)} end_sql end end diff --git a/test/action_controller_test.rb b/test/action_controller_test.rb index b4815df..06badbd 100644 --- a/test/action_controller_test.rb +++ b/test/action_controller_test.rb @@ -18,7 +18,7 @@ def get_session_value end def get_session_id - render :plain => "#{request.session.id}" + render :plain => "#{request.session['session_id']}" end def call_reset_session @@ -257,4 +257,65 @@ def test_session_store_with_all_domains assert_response :success end end + + %w{ session sql_bypass }.each do |class_name| + define_method :"test_sessions_are_indexed_by_a_hashed_session_id_for_#{class_name}" do + with_store(class_name) do + with_test_route_set do + get '/set_session_value' + assert_response :success + public_session_id = cookies['_session_id'] + + session = ActiveRecord::SessionStore::Session.last + assert session + assert_not_equal public_session_id, session.session_id + + expected_private_id = Rack::Session::SessionId.new(public_session_id).private_id + + assert_equal expected_private_id, session.session_id + end + end + end + + define_method :"test_unsecured_sessions_are_retrieved_and_migrated_for_#{class_name}" do + with_store(class_name) do + with_test_route_set do + get '/set_session_value', params: { foo: 'baz' } + assert_response :success + public_session_id = cookies['_session_id'] + + session = ActiveRecord::SessionStore::Session.last + session.data # otherwise we cannot save + session.session_id = public_session_id + session.save! + + get '/get_session_value' + assert_response :success + assert_equal 'foo: "baz"', response.body + + session = ActiveRecord::SessionStore::Session.last + assert_not_equal public_session_id, session.read_attribute(:session_id) + end + end + end + + # to avoid a different kind of timing attack + define_method :"test_sessions_cannot_be_retrieved_by_their_private_session_id_for_#{class_name}" do + with_store(class_name) do + with_test_route_set do + get '/set_session_value', params: { foo: 'baz' } + assert_response :success + + session = ActiveRecord::SessionStore::Session.last + private_session_id = session.read_attribute(:session_id) + + cookies.merge("_session_id=#{private_session_id};path=/") + + get '/get_session_value' + assert_response :success + assert_equal 'foo: nil', response.body + end + end + end + end end diff --git a/test/session_test.rb b/test/session_test.rb index e904e34..e49c76d 100644 --- a/test/session_test.rb +++ b/test/session_test.rb @@ -123,6 +123,64 @@ def test_loaded? s = Session.new assert !s.loaded?, 'session is not loaded' end + + def test_session_can_be_secured + Session.create_table! + session_id = 'unsecure' + session = session_klass.create!(:data => 'world', :session_id => 'foo') + session.update_column(:session_id, session_id) + + assert_equal 'unsecure', session.read_attribute(:session_id) + + session.secure! + + secured = Rack::Session::SessionId.new(session_id).private_id + assert_equal secured, session.reload.read_attribute(:session_id) + end + + def test_session_can_be_secured_with_sessid_compatibility + # Force class reload, as we need to redo the meta-programming + ActiveRecord::SessionStore.send(:remove_const, :Session) + load 'active_record/session_store/session.rb' + + Session.reset_column_information + klass = Class.new(Session) do + def self.session_id_column + 'sessid' + end + end + klass.create_table! + session_id = 'unsecure' + session = klass.create!(:data => 'world', :sessid => 'foo') + session.update_column(:sessid, session_id) + + assert_equal 'unsecure', session.read_attribute(:sessid) + + session.secure! + + secured = Rack::Session::SessionId.new(session_id).private_id + assert_equal secured, session.reload.read_attribute(:sessid) + ensure + klass.drop_table! + Session.reset_column_information + end + + def test_secure_is_idempotent + Session.create_table! + session_id = 'unsecure' + session = session_klass.create!(:data => 'world', :session_id => 'foo') + session.update_column(:session_id, session_id) + + assert_equal 'unsecure', session.read_attribute(:session_id) + + session.secure! + private_id = session.read_attribute(:session_id) + session.secure! + session.reload + session.secure! + + assert_equal private_id, session.reload.read_attribute(:session_id) + end end end end