Merge pull request rails#151 from rails-lts/secure-session-store

Author: Javi Manso

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
@@ -67,11 +67,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]
@@ -80,7 +80,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
 
@@ -98,7 +98,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
@@ -110,7 +110,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
@@ -121,24 +121,35 @@ 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 unless request.session_options[:skip]
           end
           if request.env[ENV_SESSION_OPTIONS_KEY][:id].nil?
             request.env[SESSION_RECORD_KEY] = model
           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
@@ -150,6 +161,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

lib/active_record/session_store/session.rb

@@ -45,8 +45,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 }
@@ -78,6 +78,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?

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

test/action_controller_test.rb

@@ -23,9 +23,9 @@ def get_session_value
 
     def get_session_id
       if ActiveRecord::VERSION::MAJOR == 4
-        render :text => "#{request.session.id}"
+        render :text => "#{request.session['session_id']}"
       else
-        render :plain => "#{request.session.id}"
+        render :plain => "#{request.session['session_id']}"
       end
     end
 
@@ -285,4 +285,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

test/destroy_session_test.rb

@@ -22,7 +22,7 @@ def test_destroy_without_renew
           s['set_something_so_it_loads'] = true
 
           session_model = req.env[record_key]
-          session_model.update_attributes(:data => {'rails' => 'ftw'})
+          session_model.update(:data => {'rails' => 'ftw'})
 
           s.destroy
 
@@ -35,7 +35,7 @@ def test_destroy_with_renew
           s['set_something_so_it_loads'] = true
 
           session_model = req.env[record_key]
-          session_model.update_attributes(:data => {'rails' => 'ftw'})
+          session_model.update(:data => {'rails' => 'ftw'})
 
           s.destroy
 
@@ -50,4 +50,4 @@ def store
       end
     end
   end
-end
+end

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