Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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).
  • Loading branch information...
commit 63fdd2c90abd6eca30081d86f9317e42ca10ed26 1 parent 5c591e5
@jhtwong jhtwong authored
View
31 actionpack/test/activerecord/active_record_store_test.rb
@@ -225,6 +225,36 @@ def test_allows_session_fixation
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 @@ def with_store(class_name)
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
View
8 activerecord/lib/active_record/session_store.rb
@@ -297,8 +297,12 @@ def destroy
private
def get_session(env, sid)
Base.silence do
- sid ||= generate_sid
- session = find_session(sid)
+ unless sid and session = @@session_class.find_by_session_id(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 => {})
+ end
env[SESSION_RECORD_KEY] = session
[sid, session.data]
end
Please sign in to comment.
Something went wrong with that request. Please try again.