Possible Information Leak / Session Hijack Vulnerability in Rack

@@ -8,11 +8,38 @@
 require 'rack/request'
 require 'rack/response'
 require 'securerandom'
+require 'digest/sha2'
 
 module Rack
 
   module Session
 
+    class SessionId
+      ID_VERSION = 2
+
+      attr_reader :public_id
+
+      def initialize(public_id)
+        @public_id = public_id
+      end
+
+      def private_id
+        "#{ID_VERSION}::#{hash_sid(public_id)}"
+      end
+
+      alias :cookie_value :public_id
+
+      def empty?; false; end
+      def to_s; raise; end
+      def inspect; public_id.inspect; end
+
+      private
+
+      def hash_sid(sid)
+        Digest::SHA256.hexdigest(sid)
+      end
+    end
+
     module Abstract
       # SessionHash is responsible to lazily load the session from store.
 
@@ -375,14 +402,18 @@ def commit_session(req, res)
             req.get_header(RACK_ERRORS).puts("Deferring cookie for #{session_id}") if $VERBOSE
           else
             cookie = Hash.new
-            cookie[:value] = data
+            cookie[:value] = cookie_value(data)
             cookie[:expires] = Time.now + options[:expire_after] if options[:expire_after]
             cookie[:expires] = Time.now + options[:max_age] if options[:max_age]
             set_cookie(req, res, cookie.merge!(options))
           end
         end
         public :commit_session
 
+        def cookie_value(data)
+          data
+        end
+
         # Sets the cookie back to the client with session id. We skip the cookie
         # setting if the value didn't change (sid is the same) or expires was given.
 
@@ -424,6 +455,40 @@ def delete_session(req, sid, options)
         end
       end
 
+      class PersistedSecure < Persisted
+        class SecureSessionHash < SessionHash
+          def [](key)
+            if key == "session_id"
+              load_for_read!
+              id.public_id
+            else
+              super
+            end
+          end
+        end
+
+        def generate_sid(*)
+          public_id = super
+
+          SessionId.new(public_id)
+        end
+
+        def extract_session_id(*)
+          public_id = super
+          public_id && SessionId.new(public_id)
+        end
+
+        private
+
+        def session_class
+          SecureSessionHash
+        end
+
+        def cookie_value(data)
+          data.cookie_value
+        end
+      end
+
       class ID < Persisted
         def self.inherited(klass)
           k = klass.ancestors.find { |kl| kl.respond_to?(:superclass) && kl.superclass == ID }

@@ -48,7 +48,7 @@ module Session
     #   })
     #
 
-    class Cookie < Abstract::Persisted
+    class Cookie < Abstract::PersistedSecure
       # Encode session cookies as Base64
       class Base64
         def encode(str)
@@ -154,6 +154,15 @@ def persistent_session_id!(data, sid = nil)
         data
       end
 
+      class SessionId < DelegateClass(Session::SessionId)
+        attr_reader :cookie_value
+
+        def initialize(session_id, cookie_value)
+          super(session_id)
+          @cookie_value = cookie_value
+        end
+      end
+
       def write_session(req, session_id, session, options)
         session = session.merge("session_id" => session_id)
         session_data = coder.encode(session)
@@ -166,7 +175,7 @@ def write_session(req, session_id, session, options)
           req.get_header(RACK_ERRORS).puts("Warning! Rack::Session::Cookie data size exceeds 4K.")
           nil
         else
-          session_data
+          SessionId.new(session_id, session_data)
         end
       end
 

@@ -26,7 +26,7 @@ module Session
     #   )
     #   Rack::Handler::WEBrick.run sessioned
 
-    class Pool < Abstract::Persisted
+    class Pool < Abstract::PersistedSecure
       attr_reader :mutex, :pool
       DEFAULT_OPTIONS = Abstract::ID::DEFAULT_OPTIONS.merge drop: false
 
@@ -39,30 +39,31 @@ def initialize(app, options = {})
       def generate_sid
         loop do
           sid = super
-          break sid unless @pool.key? sid
+          break sid unless @pool.key? sid.private_id
         end
       end
 
       def find_session(req, sid)
         with_lock(req) do
-          unless sid and session = @pool[sid]
+          unless sid and session = get_session_with_fallback(sid)
             sid, session = generate_sid, {}
-            @pool.store sid, session
+            @pool.store sid.private_id, session
           end
           [sid, session]
         end
       end
 
       def write_session(req, session_id, new_session, options)
         with_lock(req) do
-          @pool.store session_id, new_session
+          @pool.store session_id.private_id, new_session
           session_id
         end
       end
 
       def delete_session(req, session_id, options)
         with_lock(req) do
-          @pool.delete(session_id)
+          @pool.delete(session_id.public_id)
+          @pool.delete(session_id.private_id)
           generate_sid unless options[:drop]
         end
       end
@@ -73,6 +74,12 @@ def with_lock(req)
       ensure
         @mutex.unlock if @mutex.locked?
       end
+
+      private
+
+      def get_session_with_fallback(sid)
+        @pool[sid.private_id] || @pool[sid.public_id]
+      end
     end
   end
 end

@@ -8,15 +8,15 @@
 
 describe Rack::Session::Pool do
   session_key = Rack::Session::Pool::DEFAULT_OPTIONS[:key]
-  session_match = /#{session_key}=[0-9a-fA-F]+;/
+  session_match = /#{session_key}=([0-9a-fA-F]+);/
 
   incrementor = lambda do |env|
     env["rack.session"]["counter"] ||= 0
     env["rack.session"]["counter"] += 1
     Rack::Response.new(env["rack.session"].inspect).to_a
   end
 
-  session_id = Rack::Lint.new(lambda do |env|
+  get_session_id = Rack::Lint.new(lambda do |env|
     Rack::Response.new(env["rack.session"].inspect).to_a
   end)
 
@@ -145,6 +145,43 @@
     pool.pool.size.must_equal 1
   end
 
+  it "can read the session with the legacy id" do
+    pool = Rack::Session::Pool.new(incrementor)
+    req = Rack::MockRequest.new(pool)
+
+    res0 = req.get("/")
+    cookie = res0["Set-Cookie"]
+    session_id = Rack::Session::SessionId.new cookie[session_match, 1]
+    ses0 = pool.pool[session_id.private_id]
+    pool.pool[session_id.public_id] = ses0
+    pool.pool.delete(session_id.private_id)
+
+    res1 = req.get("/", "HTTP_COOKIE" => cookie)
+    res1["Set-Cookie"].must_be_nil
+    res1.body.must_equal '{"counter"=>2}'
+    pool.pool[session_id.private_id].wont_be_nil
+  end
+
+  it "drops the session in the legacy id as well" do
+    pool = Rack::Session::Pool.new(incrementor)
+    req = Rack::MockRequest.new(pool)
+    drop = Rack::Utils::Context.new(pool, drop_session)
+    dreq = Rack::MockRequest.new(drop)
+
+    res0 = req.get("/")
+    cookie = res0["Set-Cookie"]
+    session_id = Rack::Session::SessionId.new cookie[session_match, 1]
+    ses0 = pool.pool[session_id.private_id]
+    pool.pool[session_id.public_id] = ses0
+    pool.pool.delete(session_id.private_id)
+
+    res2 = dreq.get("/", "HTTP_COOKIE" => cookie)
+    res2["Set-Cookie"].must_be_nil
+    res2.body.must_equal '{"counter"=>2}'
+    pool.pool[session_id.private_id].must_be_nil
+    pool.pool[session_id.public_id].must_be_nil
+  end
+
   # anyone know how to do this better?
   it "should merge sessions when multithreaded" do
     unless $DEBUG
@@ -193,7 +230,7 @@
   end
 
   it "does not return a cookie if cookie was not written (only read)" do
-    app = Rack::Session::Pool.new(session_id)
+    app = Rack::Session::Pool.new(get_session_id)
     res = Rack::MockRequest.new(app).get("/")
     res["Set-Cookie"].must_be_nil
   end

@@ -18,7 +18,7 @@ def self.version
     VERSION.join(".")
   end
 
-  RELEASE = "2.0.7"
+  RELEASE = "2.0.8"
 
   # Return the Rack release as a dotted string.
   def self.release

@@ -9,11 +9,38 @@
 rescue LoadError
   # We just won't get securerandom
 end
+require "digest/sha2"
 
 module Rack
 
   module Session
 
+    class SessionId
+      ID_VERSION = 2
+
+      attr_reader :public_id
+
+      def initialize(public_id)
+        @public_id = public_id
+      end
+
+      def private_id
+        "#{ID_VERSION}::#{hash_sid(public_id)}"
+      end
+
+      alias :cookie_value :public_id
+
+      def empty?; false; end
+      def to_s; raise; end
+      def inspect; public_id.inspect; end
+
+      private
+
+      def hash_sid(sid)
+        Digest::SHA256.hexdigest(sid)
+      end
+    end
+
     module Abstract
       ENV_SESSION_KEY = 'rack.session'.freeze
       ENV_SESSION_OPTIONS_KEY = 'rack.session.options'.freeze
@@ -191,7 +218,7 @@ def stringify_keys(other)
       # Not included by default; you must require 'rack/session/abstract/id'
       # to use.
 
-      class ID
+      class Persisted
         DEFAULT_OPTIONS = {
           :key =>           'rack.session',
           :path =>          '/',
@@ -342,10 +369,10 @@ def commit_session(env, status, headers, body)
           if not data = set_session(env, session_id, session_data, options)
             env["rack.errors"].puts("Warning! #{self.class.name} failed to save session. Content dropped.")
           elsif options[:defer] and not options[:renew]
-            env["rack.errors"].puts("Deferring cookie for #{session_id}") if $VERBOSE
+            env["rack.errors"].puts("Deferring cookie for #{session_id.public_id}") if $VERBOSE
           else
             cookie = Hash.new
-            cookie[:value] = data
+            cookie[:value] = cookie_value(data)
             cookie[:expires] = Time.now + options[:expire_after] if options[:expire_after]
             cookie[:expires] = Time.now + options[:max_age] if options[:max_age]
             set_cookie(env, headers, cookie.merge!(options))
@@ -354,6 +381,10 @@ def commit_session(env, status, headers, body)
           [status, headers, body]
         end
 
+        def cookie_value(data)
+          data
+        end
+
         # Sets the cookie back to the client with session id. We skip the cookie
         # setting if the value didn't change (sid is the same) or expires was given.
 
@@ -394,6 +425,51 @@ def destroy_session(env, sid, options)
           raise '#destroy_session not implemented'
         end
       end
+
+      class PersistedSecure < Persisted
+        class SecureSessionHash < SessionHash
+          def [](key)
+            if key == "session_id"
+              load_for_read!
+              id.public_id
+            else
+              super
+            end
+          end
+        end
+
+        def generate_sid(*)
+          public_id = super
+
+          SessionId.new(public_id)
+        end
+
+        def extract_session_id(*)
+          public_id = super
+          public_id && SessionId.new(public_id)
+        end
+
+        private
+
+        def session_class
+          SecureSessionHash
+        end
+
+        def cookie_value(data)
+          data.cookie_value
+        end
+      end
+
+      class ID < Persisted
+        def self.inherited(klass)
+          k = klass.ancestors.find { |kl| kl.respond_to?(:superclass) && kl.superclass == ID }
+          unless k.instance_variable_defined?(:"@_rack_warned")
+            warn "#{klass} is inheriting from #{ID}.  Inheriting from #{ID} is deprecated, please inherit from #{Persisted} instead" if $VERBOSE
+            k.instance_variable_set(:"@_rack_warned", true)
+          end
+          super
+        end
+      end
     end
   end
 end

@@ -44,7 +44,7 @@ module Session
     #   })
     #
 
-    class Cookie < Abstract::ID
+    class Cookie < Abstract::PersistedSecure
       # Encode session cookies as Base64
       class Base64
         def encode(str)
@@ -151,6 +151,15 @@ def persistent_session_id!(data, sid=nil)
         data
       end
 
+      class SessionId < DelegateClass(Session::SessionId)
+        attr_reader :cookie_value
+
+        def initialize(session_id, cookie_value)
+          super(session_id)
+          @cookie_value = cookie_value
+        end
+      end
+
       def set_session(env, session_id, session, options)
         session = session.merge("session_id" => session_id)
         session_data = coder.encode(session)
@@ -163,7 +172,7 @@ def set_session(env, session_id, session, options)
           env["rack.errors"].puts("Warning! Rack::Session::Cookie data size exceeds 4K.")
           nil
         else
-          session_data
+          SessionId.new(session_id, session_data)
         end
       end
 

@@ -19,7 +19,7 @@ module Session
     # Note that memcache does drop data before it may be listed to expire. For
     # a full description of behaviour, please see memcache's documentation.
 
-    class Memcache < Abstract::ID
+    class Memcache < Abstract::PersistedSecure
       attr_reader :mutex, :pool
 
       DEFAULT_OPTIONS = Abstract::ID::DEFAULT_OPTIONS.merge \
@@ -42,15 +42,15 @@ def initialize(app, options={})
       def generate_sid
         loop do
           sid = super
-          break sid unless @pool.get(sid, true)
+          break sid unless @pool.get(sid.private_id, true)
         end
       end
 
       def get_session(env, sid)
         with_lock(env) do
-          unless sid and session = @pool.get(sid)
+          unless sid and session = get_session_with_fallback(sid)
             sid, session = generate_sid, {}
-            unless /^STORED/ =~ @pool.add(sid, session)
+            unless /^STORED/ =~ @pool.add(sid.private_id, session)
               raise "Session collision on '#{sid.inspect}'"
             end
           end
@@ -63,14 +63,15 @@ def set_session(env, session_id, new_session, options)
         expiry = expiry.nil? ? 0 : expiry + 1
 
         with_lock(env) do
-          @pool.set session_id, new_session, expiry
+          @pool.set session_id.private_id, new_session, expiry
           session_id
         end
       end
 
       def destroy_session(env, session_id, options)
         with_lock(env) do
-          @pool.delete(session_id)
+          @pool.delete(session_id.public_id)
+          @pool.delete(session_id.private_id)
           generate_sid unless options[:drop]
         end
       end
@@ -88,6 +89,11 @@ def with_lock(env)
         @mutex.unlock if @mutex.locked?
       end
 
+      private
+
+      def get_session_with_fallback(sid)
+        @pool.get(sid.private_id) || @pool.get(sid.public_id)
+      end
     end
   end
 end

@@ -24,7 +24,7 @@ module Session
     #   )
     #   Rack::Handler::WEBrick.run sessioned
 
-    class Pool < Abstract::ID
+    class Pool < Abstract::PersistedSecure
       attr_reader :mutex, :pool
       DEFAULT_OPTIONS = Abstract::ID::DEFAULT_OPTIONS.merge :drop => false
 
@@ -37,30 +37,31 @@ def initialize(app, options={})
       def generate_sid
         loop do
           sid = super
-          break sid unless @pool.key? sid
+          break sid unless @pool.key? sid.private_id
         end
       end
 
       def get_session(env, sid)
         with_lock(env) do
-          unless sid and session = @pool[sid]
+          unless sid and session = get_session_with_fallback(sid)
             sid, session = generate_sid, {}
-            @pool.store sid, session
+            @pool.store sid.private_id, session
           end
           [sid, session]
         end
       end
 
       def set_session(env, session_id, new_session, options)
         with_lock(env) do
-          @pool.store session_id, new_session
+          @pool.store session_id.private_id, new_session
           session_id
         end
       end
 
       def destroy_session(env, session_id, options)
         with_lock(env) do
-          @pool.delete(session_id)
+          @pool.delete(session_id.public_id)
+          @pool.delete(session_id.private_id)
           generate_sid unless options[:drop]
         end
       end
@@ -71,6 +72,12 @@ def with_lock(env)
       ensure
         @mutex.unlock if @mutex.locked?
       end
+
+      private
+
+      def get_session_with_fallback(sid)
+        @pool[sid.private_id] || @pool[sid.public_id]
+      end
     end
   end
 end

@@ -47,7 +47,7 @@ def hex(*args)
       end
     end
     id = Rack::Session::Abstract::ID.new nil, :secure_random => secure_random.new
-    id.send(:generate_sid).should.eql 'fake_hex'
+    id.send(:generate_sid).should.equal 'fake_hex'
   end
 
 end

@@ -225,15 +225,52 @@
       req = Rack::MockRequest.new(pool)
 
       res0 = req.get("/")
-      session_id = (cookie = res0["Set-Cookie"])[session_match, 1]
-      ses0 = pool.pool.get(session_id, true)
+      session_id = Rack::Session::SessionId.new (cookie = res0["Set-Cookie"])[session_match, 1]
+      ses0 = pool.pool.get(session_id.private_id, true)
 
       req.get("/", "HTTP_COOKIE" => cookie)
-      ses1 = pool.pool.get(session_id, true)
+      ses1 = pool.pool.get(session_id.private_id, true)
 
       ses1.should.not.equal ses0
     end
 
+    it "can read the session with the legacy id" do
+      pool = Rack::Session::Memcache.new(incrementor)
+      req = Rack::MockRequest.new(pool)
+
+      res0 = req.get("/")
+      cookie = res0["Set-Cookie"]
+      session_id = Rack::Session::SessionId.new cookie[session_match, 1]
+      ses0 = pool.pool.get(session_id.private_id, true)
+      pool.pool.set(session_id.public_id, ses0, 0, true)
+      pool.pool.delete(session_id.private_id)
+
+      res1 = req.get("/", "HTTP_COOKIE" => cookie)
+      res1["Set-Cookie"].should.be.nil
+      res1.body.should.equal '{"counter"=>2}'
+      pool.pool.get(session_id.private_id, true).should.not.be.nil
+    end
+
+    it "drops the session in the legacy id as well" do
+      pool = Rack::Session::Memcache.new(incrementor)
+      req = Rack::MockRequest.new(pool)
+      drop = Rack::Utils::Context.new(pool, drop_session)
+      dreq = Rack::MockRequest.new(drop)
+
+      res0 = req.get("/")
+      cookie = res0["Set-Cookie"]
+      session_id = Rack::Session::SessionId.new cookie[session_match, 1]
+      ses0 = pool.pool.get(session_id.private_id, true)
+      pool.pool.set(session_id.public_id, ses0, 0, true)
+      pool.pool.delete(session_id.private_id)
+
+      res2 = dreq.get("/", "HTTP_COOKIE" => cookie)
+      res2["Set-Cookie"].should.be.nil
+      res2.body.should.equal '{"counter"=>2}'
+      pool.pool.get(session_id.private_id, true).should.be.nil
+      pool.pool.get(session_id.public_id, true).should.be.nil
+    end
+
     # anyone know how to do this better?
     it "cleanly merges sessions when multithreaded" do
       unless $DEBUG

@@ -5,15 +5,15 @@
 
 describe Rack::Session::Pool do
   session_key = Rack::Session::Pool::DEFAULT_OPTIONS[:key]
-  session_match = /#{session_key}=[0-9a-fA-F]+;/
+  session_match = /#{session_key}=([0-9a-fA-F]+);/
 
   incrementor = lambda do |env|
     env["rack.session"]["counter"] ||= 0
     env["rack.session"]["counter"] += 1
     Rack::Response.new(env["rack.session"].inspect).to_a
   end
 
-  session_id = Rack::Lint.new(lambda do |env|
+  get_session_id = Rack::Lint.new(lambda do |env|
     Rack::Response.new(env["rack.session"].inspect).to_a
   end)
 
@@ -142,6 +142,43 @@
     pool.pool.size.should.equal 1
   end
 
+  it "can read the session with the legacy id" do
+    pool = Rack::Session::Pool.new(incrementor)
+    req = Rack::MockRequest.new(pool)
+
+    res0 = req.get("/")
+    cookie = res0["Set-Cookie"]
+    session_id = Rack::Session::SessionId.new cookie[session_match, 1]
+    ses0 = pool.pool[session_id.private_id]
+    pool.pool[session_id.public_id] = ses0
+    pool.pool.delete(session_id.private_id)
+
+    res1 = req.get("/", "HTTP_COOKIE" => cookie)
+    res1["Set-Cookie"].should.be.nil
+    res1.body.should.equal '{"counter"=>2}'
+    pool.pool[session_id.private_id].should.not.be.nil
+  end
+
+  it "drops the session in the legacy id as well" do
+    pool = Rack::Session::Pool.new(incrementor)
+    req = Rack::MockRequest.new(pool)
+    drop = Rack::Utils::Context.new(pool, drop_session)
+    dreq = Rack::MockRequest.new(drop)
+
+    res0 = req.get("/")
+    cookie = res0["Set-Cookie"]
+    session_id = Rack::Session::SessionId.new cookie[session_match, 1]
+    ses0 = pool.pool[session_id.private_id]
+    pool.pool[session_id.public_id] = ses0
+    pool.pool.delete(session_id.private_id)
+
+    res2 = dreq.get("/", "HTTP_COOKIE" => cookie)
+    res2["Set-Cookie"].should.be.nil
+    res2.body.should.equal '{"counter"=>2}'
+    pool.pool[session_id.private_id].should.be.nil
+    pool.pool[session_id.public_id].should.be.nil
+  end
+
   # anyone know how to do this better?
   it "should merge sessions when multithreaded" do
     unless $DEBUG
@@ -190,7 +227,7 @@
   end
 
   it "does not return a cookie if cookie was not written (only read)" do
-    app = Rack::Session::Pool.new(session_id)
+    app = Rack::Session::Pool.new(get_session_id)
     res = Rack::MockRequest.new(app).get("/")
     res["Set-Cookie"].should.be.nil
   end