From 81f551ac040ee64ad6048bd4eea6a1da5f1d5ca3 Mon Sep 17 00:00:00 2001
From: Tobias Kraze <tobias.kraze@makandra.de>
Date: Fri, 20 Dec 2019 13:12:44 +0100
Subject: [PATCH 1/3] secure session store [CVE-2019-16782]

---
 .../session/active_record_store.rb            | 40 ++++++++----
 lib/active_record/session_store/session.rb    | 19 +++++-
 lib/active_record/session_store/sql_bypass.rb | 18 +++---
 6 files changed, 179 insertions(+), 27 deletions(-)

diff --git a/lib/action_dispatch/session/active_record_store.rb b/lib/action_dispatch/session/active_record_store.rb
index 9895400..e17680b 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(env, sid)
           logger.silence_logger 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
             env[SESSION_RECORD_KEY] = session
             [sid, session.data]
@@ -94,7 +94,7 @@ def set_session(env, sid, session_data, options)
         def destroy_session(env, session_id, options)
           logger.silence_logger do
             if sid = current_session_id(env)
-              get_session_model(env, sid).destroy
+              get_session_with_fallback(sid).destroy
               env[SESSION_RECORD_KEY] = nil
             end
 
@@ -104,9 +104,18 @@ def destroy_session(env, session_id, options)
 
         def get_session_model(env, sid)
           if env[ENV_SESSION_OPTIONS_KEY][:id].nil?
-            env[SESSION_RECORD_KEY] = find_session(sid)
+            env[SESSION_RECORD_KEY] = get_session_with_fallback(sid)
           else
-            env[SESSION_RECORD_KEY] ||= find_session(sid)
+            env[SESSION_RECORD_KEY] ||= get_session_with_fallback(sid)
+          end
+        end
+
+        def get_session_with_fallback(sid)
+          if sid && !self.class.private_session_id?(sid.public_id)
+            if (session = @@session_class.find_by_session_id(sid.private_id) || @@session_class.find_by_session_id(sid.public_id))
+              session.session_id = sid.private_id
+              session
+            end
           end
         end
 
@@ -118,6 +127,11 @@ module ActionDispatch
         def logger
           ActiveRecord::Base.logger || ActiveRecord::SessionStore::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..a14f0d9 100644
--- a/lib/active_record/session_store/session.rb
+++ b/lib/active_record/session_store/session.rb
@@ -43,8 +43,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 }
@@ -76,6 +76,21 @@ def loaded?
         @data
       end
 
+      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 marshal_data!
           return false 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 unmarshal 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, :marshaled_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, :marshaled_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 unmarshaling 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]
         @marshaled_data = attributes[:marshaled_data]
         @new_record     = @marshaled_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(marshaled_data)}
-            WHERE #{connect.quote_column_name(session_id_column)}=#{connect.quote(session_id)}
+            SET
+              #{connect.quote_column_name(data_column)}=#{connect.quote(marshaled_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

From 7564029ff5a549ae485d00914d46972d732e8f83 Mon Sep 17 00:00:00 2001
From: Tobias Kraze <tobias.kraze@makandra.de>
Date: Sun, 22 Dec 2019 12:21:03 +0100
Subject: [PATCH 2/3] prevent unnecessary database writes

---
 lib/action_dispatch/session/active_record_store.rb | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/action_dispatch/session/active_record_store.rb b/lib/action_dispatch/session/active_record_store.rb
index e17680b..3cf10b4 100644
--- a/lib/action_dispatch/session/active_record_store.rb
+++ b/lib/action_dispatch/session/active_record_store.rb
@@ -112,9 +112,11 @@ def get_session_model(env, id)
 
         def get_session_with_fallback(sid)
           if sid && !self.class.private_session_id?(sid.public_id)
-            if (session = @@session_class.find_by_session_id(sid.private_id) || @@session_class.find_by_session_id(sid.public_id))
-              session.session_id = sid.private_id
-              session
+            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

From d091c7ea819350bbb9433d4dd065379832d593a3 Mon Sep 17 00:00:00 2001
From: Tobias Kraze <tobias.kraze@makandra.de>
Date: Wed, 8 Jan 2020 11:42:38 +0100
Subject: [PATCH 3/3] add documentation for a new method

---
 lib/active_record/session_store/session.rb | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/active_record/session_store/session.rb b/lib/active_record/session_store/session.rb
index a14f0d9..aec3253 100644
--- a/lib/active_record/session_store/session.rb
+++ b/lib/active_record/session_store/session.rb
@@ -76,6 +76,12 @@ 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
