Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix for session ID fixation issue in ActiveRecord::SessionStore #2016

Closed
wants to merge 3 commits into from

3 participants

@jhtwong

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.

@NZKoz here's the pull request as per our earlier emails

@jhtwong jhtwong 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.
fa3c0e4
@josevalim
Owner

Hey mate, thanks for the pull request. Could you also provide a test case?

@jhtwong

I've got an integration test in my app that tests this. Let me see if I can put the test cases into actionpack/test/activerecord/active_record_store_test.rb.

@jhtwong

Just got the active_record_store_test.rb to run using jdbcsqlite3 as is. Seems that the SqlBypass store has been broken for a little while. Should I add the fix to this pull request or open a new one?

jhtwong added some commits
@jhtwong jhtwong New integrations tests for #2016 - Fix for session ID fixation issue in
ActiveRecord::SessionStore

These new tests 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).
d0e2340
@jhtwong jhtwong Fix for SqlBypass session store
Two issues fixed:
1) connection_pool is not defined - needed by SessionStore#drop_table!
and create_table! since c94651f

2) initialization of connection to the default of AR::Base.connection
only occurred at the singleton level - the instance level method defined
by cattr_accessor did not have this logic
f14bca2
@jhtwong

I decided to put the SqlBypass fix on the same branch, since the tests would break horribly otherwise.

@spastorino
Owner

Can you squash your commits? at least the ones that makes sense to be together.
Thanks.

@jhtwong

I can squash fa3c0e4 (the original session fixation fix) and d0e2340 (the integration test) together. Probably makes sense for the SqlBypass fix f14bca2 to stay separate?

@spastorino
Owner

Yes

@spastorino
Owner

Can you also provide the same patch for master?

@jhtwong

Alright, there are now 4 new pull requests replacing this one:

for 3-1-stable
#2039 - session id patch with test
#2040 - SqlBypass patch

for master
#2041 - session id patch with test
#2042 - SqlBypass patch

@spastorino spastorino closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 8, 2011
  1. @jhtwong

    Fixed session ID fixation for ActiveRecord::SessionStore

    jhtwong authored
    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.
  2. @jhtwong

    New integrations tests for #2016 - Fix for session ID fixation issue in

    jhtwong authored
    ActiveRecord::SessionStore
    
    These new tests 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).
  3. @jhtwong

    Fix for SqlBypass session store

    jhtwong authored
    Two issues fixed:
    1) connection_pool is not defined - needed by SessionStore#drop_table!
    and create_table! since c94651f
    
    2) initialization of connection to the default of AR::Base.connection
    only occurred at the singleton level - the instance level method defined
    by cattr_accessor did not have this logic
This page is out of date. Refresh to see the latest.
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
28 activerecord/lib/active_record/session_store.rb
@@ -183,11 +183,6 @@ class SqlBypass
##
# :singleton-method:
- # Use the ActiveRecord::Base.connection by default.
- cattr_accessor :connection
-
- ##
- # :singleton-method:
# The table name defaults to 'sessions'.
cattr_accessor :table_name
@@table_name = 'sessions'
@@ -206,10 +201,19 @@ class SqlBypass
class << self
alias :data_column_name :data_column
+
+ # Use the ActiveRecord::Base.connection by default.
+ attr_writer :connection
+
+ # Use the ActiveRecord::Base.connection_pool by default.
+ attr_writer :connection_pool
- remove_method :connection
def connection
- @@connection ||= ActiveRecord::Base.connection
+ @connection ||= ActiveRecord::Base.connection
+ end
+
+ def connection_pool
+ @connection_pool ||= ActiveRecord::Base.connection_pool
end
# Look up a session by id and unmarshal its data if found.
@@ -219,6 +223,8 @@ def find_by_session_id(session_id)
end
end
end
+
+ delegate :connection, :connection=, :connection_pool, :connection_pool=, :to => self
attr_reader :session_id, :new_record
alias :new_record? :new_record
@@ -297,8 +303,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
Something went wrong with that request. Please try again.