Fixed session ID fixation for ActiveRecord::SessionStore

I have found that Rails will take an invalid session ID specified by the
client and materialize a session based on that session ID. This means
that it is possible, among other things, for a client to use an
arbitrarily weak session ID or for a client to resurrect a previous used
session ID. In other words, we cannot guarantee that all session IDs are
generated by the server and that they are (statistically) unique through
time.

The fix is to always generate a new session ID in #get_session if an
existing session cannot be found under the incoming session ID.

Also added new tests that make sure that an invalid session ID is never
materialized into a new session, regardless of whether it comes in via a
cookie or a URL parameter (when :cookie_only => false).

1 files changed, 31 insertions, 0 deletions

diff --git a/actionpack/test/activerecord/active_record_store_test.rb b/actionpack/test/activerecord/active_record_store_test.rb
index f0fb113860..768ac713ca 100644
--- a/actionpack/test/activerecord/active_record_store_test.rb
+++ b/actionpack/test/activerecord/active_record_store_test.rb
@@ -225,6 +225,36 @@ class ActiveRecordStoreTest < ActionDispatch::IntegrationTest
       assert_equal session_id, cookies['_session_id']
     end
   end
+  
+  def test_incoming_invalid_session_id_via_cookie_should_be_ignored
+    with_test_route_set do
+      open_session do |sess|
+        sess.cookies['_session_id'] = 'INVALID'
+        
+        sess.get '/set_session_value'
+        new_session_id = sess.cookies['_session_id']
+        assert_not_equal 'INVALID', new_session_id
+        
+        sess.get '/get_session_value'
+        new_session_id_2 = sess.cookies['_session_id']
+        assert_equal new_session_id, new_session_id_2
+      end
+    end
+  end
+
+  def test_incoming_invalid_session_id_via_parameter_should_be_ignored
+    with_test_route_set(:cookie_only => false) do
+      open_session do |sess|
+        sess.get '/set_session_value', :_session_id => 'INVALID'
+        new_session_id = sess.cookies['_session_id']
+        assert_not_equal 'INVALID', new_session_id
+        
+        sess.get '/get_session_value'
+        new_session_id_2 = sess.cookies['_session_id']
+        assert_equal new_session_id, new_session_id_2
+      end
+    end
+  end
 
   private
 
@@ -247,6 +277,7 @@ class ActiveRecordStoreTest < ActionDispatch::IntegrationTest
       session_class, ActiveRecord::SessionStore.session_class =
         ActiveRecord::SessionStore.session_class, "ActiveRecord::SessionStore::#{class_name.camelize}".constantize
       yield
+    ensure
       ActiveRecord::SessionStore.session_class = session_class
     end
 end