Fix possible information leak / session hijacking vulnerability.

The `ActionDispatch::Session::MemcacheStore` is still vulnerable
given it requires the gem dalli to be updated as well.

CVE-2019-16782

Gemfile.lock

@@ -39,7 +39,7 @@ PATH
     actionpack (6.1.0.alpha)
       actionview (= 6.1.0.alpha)
       activesupport (= 6.1.0.alpha)
-      rack (~> 2.0)
+      rack (~> 2.0, >= 2.0.8)
       rack-test (>= 0.6.3)
       rails-dom-testing (~> 2.0)
       rails-html-sanitizer (~> 1.0, >= 1.2.0)
@@ -367,7 +367,7 @@ GEM
       thor
     raabro (1.1.6)
     racc (1.4.15)
-    rack (2.0.7)
+    rack (2.0.8)
     rack-cache (1.9.0)
       rack (>= 0.4)
     rack-protection (2.0.7)

actionpack/CHANGELOG.md

@@ -1,3 +1,10 @@
+*   Fix possible information leak / session hijacking vulnerability.
+
+    The `ActionDispatch::Session::MemcacheStore` is still vulnerable given it requires the
+    gem dalli to be updated as well.
+
+    CVE-2019-16782.
+
 *   Include child session assertion count in ActionDispatch::IntegrationTest
 
     `IntegrationTest#open_session` uses `dup` to create the new session, which

actionpack/actionpack.gemspec

@@ -34,7 +34,7 @@ Gem::Specification.new do |s|
 
   s.add_dependency "activesupport", version
 
-  s.add_dependency "rack",      "~> 2.0"
+  s.add_dependency "rack",      "~> 2.0", ">= 2.0.8"
   s.add_dependency "rack-test", ">= 0.6.3"
   s.add_dependency "rails-html-sanitizer", "~> 1.0", ">= 1.2.0"
   s.add_dependency "rails-dom-testing", "~> 2.0"

actionpack/lib/action_dispatch.rb

@@ -89,10 +89,11 @@ module ActionDispatch
   end
 
   module Session
-    autoload :AbstractStore,     "action_dispatch/middleware/session/abstract_store"
-    autoload :CookieStore,       "action_dispatch/middleware/session/cookie_store"
-    autoload :MemCacheStore,     "action_dispatch/middleware/session/mem_cache_store"
-    autoload :CacheStore,        "action_dispatch/middleware/session/cache_store"
+    autoload :AbstractStore,       "action_dispatch/middleware/session/abstract_store"
+    autoload :AbstractSecureStore, "action_dispatch/middleware/session/abstract_store"
+    autoload :CookieStore,         "action_dispatch/middleware/session/cookie_store"
+    autoload :MemCacheStore,       "action_dispatch/middleware/session/mem_cache_store"
+    autoload :CacheStore,          "action_dispatch/middleware/session/cache_store"
   end
 
   mattr_accessor :test_app

actionpack/lib/action_dispatch/middleware/session/abstract_store.rb

@@ -86,5 +86,20 @@ module ActionDispatch
           request.cookie_jar[key] = cookie
         end
     end
+
+    class AbstractSecureStore < Rack::Session::Abstract::PersistedSecure
+      include Compatibility
+      include StaleSessionCheck
+      include SessionObject
+
+      def generate_sid
+        Rack::Session::SessionId.new(super)
+      end
+
+      private
+        def set_cookie(request, session_id, cookie)
+          request.cookie_jar[key] = cookie
+        end
+    end
   end
 end

actionpack/lib/action_dispatch/middleware/session/cache_store.rb

@@ -12,7 +12,7 @@ module ActionDispatch
     # * <tt>cache</tt>         - The cache to use. If it is not specified, <tt>Rails.cache</tt> will be used.
     # * <tt>expire_after</tt>  - The length of time a session will be stored before automatically expiring.
     #   By default, the <tt>:expires_in</tt> option of the cache is used.
-    class CacheStore < AbstractStore
+    class CacheStore < AbstractSecureStore
       def initialize(app, options = {})
         @cache = options[:cache] || Rails.cache
         options[:expire_after] ||= @cache.options[:expires_in]
@@ -21,7 +21,7 @@ module ActionDispatch
 
       # Get a session from the cache.
       def find_session(env, sid)
-        unless sid && (session = @cache.read(cache_key(sid)))
+        unless sid && (session = get_session_with_fallback(sid))
           sid, session = generate_sid, {}
         end
         [sid, session]
@@ -29,7 +29,7 @@ module ActionDispatch
 
       # Set a session in the cache.
       def write_session(env, sid, session, options)
-        key = cache_key(sid)
+        key = cache_key(sid.private_id)
         if session
           @cache.write(key, session, expires_in: options[:expire_after])
         else
@@ -40,14 +40,19 @@ module ActionDispatch
 
       # Remove a session from the cache.
       def delete_session(env, sid, options)
-        @cache.delete(cache_key(sid))
+        @cache.delete(cache_key(sid.private_id))
+        @cache.delete(cache_key(sid.public_id))
         generate_sid
       end
 
       private
         # Turn the session id into a cache key.
-        def cache_key(sid)
-          "_session_id:#{sid}"
+        def cache_key(id)
+          "_session_id:#{id}"
+        end
+
+        def get_session_with_fallback(sid)
+          @cache.read(cache_key(sid.private_id)) || @cache.read(cache_key(sid.public_id))
         end
     end
   end

actionpack/lib/action_dispatch/middleware/session/cookie_store.rb

@@ -46,7 +46,16 @@ module ActionDispatch
     # would set the session cookie to expire automatically 14 days after creation.
     # Other useful options include <tt>:key</tt>, <tt>:secure</tt> and
     # <tt>:httponly</tt>.
-    class CookieStore < AbstractStore
+    class CookieStore < AbstractSecureStore
+      class SessionId < DelegateClass(Rack::Session::SessionId)
+        attr_reader :cookie_value
+
+        def initialize(session_id, cookie_value = {})
+          super(session_id)
+          @cookie_value = cookie_value
+        end
+      end
+
       def initialize(app, options = {})
         super(app, options.merge!(cookie_only: true))
       end
@@ -54,7 +63,7 @@ module ActionDispatch
       def delete_session(req, session_id, options)
         new_sid = generate_sid unless options[:drop]
         # Reset hash and Assign the new session id
-        req.set_header("action_dispatch.request.unsigned_session_cookie", new_sid ? { "session_id" => new_sid } : {})
+        req.set_header("action_dispatch.request.unsigned_session_cookie", new_sid ? { "session_id" => new_sid.public_id } : {})
         new_sid
       end
 
@@ -62,14 +71,15 @@ module ActionDispatch
         stale_session_check! do
           data = unpacked_cookie_data(req)
           data = persistent_session_id!(data)
-          [data["session_id"], data]
+          [Rack::Session::SessionId.new(data["session_id"]), data]
         end
       end
 
       private
         def extract_session_id(req)
           stale_session_check! do
-            unpacked_cookie_data(req)["session_id"]
+            sid = unpacked_cookie_data(req)["session_id"]
+            sid && Rack::Session::SessionId.new(sid)
           end
         end
 
@@ -87,13 +97,13 @@ module ActionDispatch
 
         def persistent_session_id!(data, sid = nil)
           data ||= {}
-          data["session_id"] ||= sid || generate_sid
+          data["session_id"] ||= sid || generate_sid.public_id
           data
         end
 
         def write_session(req, sid, session_data, options)
-          session_data["session_id"] = sid
-          session_data
+          session_data["session_id"] = sid.public_id
+          SessionId.new(sid, session_data)
         end
 
         def set_cookie(request, session_id, cookie)

actionpack/lib/action_dispatch/request/session.rb

@@ -90,7 +90,13 @@ module ActionDispatch
       # +nil+ if the given key is not found in the session.
       def [](key)
         load_for_read!
-        @delegate[key.to_s]
+        key = key.to_s
+
+        if key == "session_id"
+          id&.public_id
+        else
+          @delegate[key]
+        end
       end
 
       # Returns the nested value specified by the sequence of keys, returning

actionpack/test/dispatch/session/abstract_secure_store_test.rb

@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+require "abstract_unit"
+require "action_dispatch/middleware/session/abstract_store"
+
+module ActionDispatch
+  module Session
+    class AbstractSecureStoreTest < ActiveSupport::TestCase
+      class MemoryStore < AbstractSecureStore
+        class SessionId < Rack::Session::SessionId
+          attr_reader :cookie_value
+
+          def initialize(session_id, cookie_value)
+            super(session_id)
+            @cookie_value = cookie_value
+          end
+        end
+
+        def initialize(app)
+          @sessions = {}
+          super
+        end
+
+        def find_session(env, sid)
+          sid ||= 1
+          session = @sessions[sid] ||= {}
+          [sid, session]
+        end
+
+        def write_session(env, sid, session, options)
+          @sessions[sid] = SessionId.new(sid, session)
+        end
+      end
+
+      def test_session_is_set
+        env = {}
+        as = MemoryStore.new app
+        as.call(env)
+
+        assert @env
+        assert Request::Session.find ActionDispatch::Request.new @env
+      end
+
+      def test_new_session_object_is_merged_with_old
+        env = {}
+        as = MemoryStore.new app
+        as.call(env)
+
+        assert @env
+        session = Request::Session.find ActionDispatch::Request.new @env
+        session["foo"] = "bar"
+
+        as.call(@env)
+        session1 = Request::Session.find ActionDispatch::Request.new @env
+
+        assert_not_equal session, session1
+        assert_equal session.to_hash, session1.to_hash
+      end
+
+      private
+        def app(&block)
+          @env = nil
+          lambda { |env| @env = env }
+        end
+    end
+  end
+end

actionpack/test/dispatch/session/cache_store_test.rb

@@ -24,7 +24,7 @@ class CacheStoreTest < ActionDispatch::IntegrationTest
     end
 
     def get_session_id
-      render plain: "#{request.session.id}"
+      render plain: "#{request.session.id.public_id}"
     end
 
     def call_reset_session
@@ -150,15 +150,56 @@ class CacheStoreTest < ActionDispatch::IntegrationTest
 
   def test_prevents_session_fixation
     with_test_route_set do
-      assert_nil @cache.read("_session_id:0xhax")
+      sid = Rack::Session::SessionId.new("0xhax")
+      assert_nil @cache.read("_session_id:#{sid.private_id}")
 
-      cookies["_session_id"] = "0xhax"
+      cookies["_session_id"] = sid.public_id
       get "/set_session_value"
 
       assert_response :success
-      assert_not_equal "0xhax", cookies["_session_id"]
-      assert_nil @cache.read("_session_id:0xhax")
-      assert_equal({ "foo" => "bar" }, @cache.read("_session_id:#{cookies['_session_id']}"))
+      assert_not_equal sid.public_id, cookies["_session_id"]
+      assert_nil @cache.read("_session_id:#{sid.private_id}")
+      assert_equal(
+        { "foo" => "bar" },
+        @cache.read("_session_id:#{Rack::Session::SessionId.new(cookies['_session_id']).private_id}")
+      )
+    end
+  end
+
+  def test_can_read_session_with_legacy_id
+    with_test_route_set do
+      get "/set_session_value"
+      assert_response :success
+      assert cookies["_session_id"]
+
+      sid = Rack::Session::SessionId.new(cookies['_session_id'])
+      session = @cache.read("_session_id:#{sid.private_id}")
+      @cache.delete("_session_id:#{sid.private_id}")
+      @cache.write("_session_id:#{sid.public_id}", session)
+
+      get "/get_session_value"
+      assert_response :success
+      assert_equal 'foo: "bar"', response.body
+    end
+  end
+
+  def test_drop_session_in_the_legacy_id_as_well
+    with_test_route_set do
+      get "/set_session_value"
+      assert_response :success
+      assert cookies["_session_id"]
+
+      sid = Rack::Session::SessionId.new(cookies['_session_id'])
+      session = @cache.read("_session_id:#{sid.private_id}")
+      @cache.delete("_session_id:#{sid.private_id}")
+      @cache.write("_session_id:#{sid.public_id}", session)
+
+      get "/call_reset_session"
+      assert_response :success
+      assert_not_equal [], headers["Set-Cookie"]
+
+      assert_nil @cache.read("_session_id:#{sid.private_id}")
+      assert_nil @cache.read("_session_id:#{sid.public_id}")
     end
   end
 

actionpack/test/dispatch/session/cookie_store_test.rb

@@ -36,7 +36,7 @@ class CookieStoreTest < ActionDispatch::IntegrationTest
     end
 
     def get_session_id
-      render plain: "id: #{request.session.id}"
+      render plain: "id: #{request.session.id&.public_id}"
     end
 
     def get_class_after_reset_session