From 956edc393225ee53d6027bce11ea1441cb186925 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 18 Dec 2019 09:22:19 -0800 Subject: [PATCH] Squashed commit of the following: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit commit f1a79b208c4ea877420beee62646e0b146402bd0 Author: Rafael Mendonça França Date: Mon Oct 21 16:39:00 2019 -0400 Introduce a new base class to avoid breaking when upgrading Third-party session store would still need to be chaged to be more secure but only upgrading rack will not break any application. commit 5b1cab667270d7ad1a4d2088adf5ff4eb9845496 Author: Rafael Mendonça França Date: Wed Oct 16 14:07:36 2019 -0400 Add a version prefix to the private id to make easier to migrate old values commit 1e96e0f197777458216bb3dfdbcce57a0bbba0c5 Author: Rafael Mendonça França Date: Wed Oct 9 19:14:08 2019 -0400 Fallback to the public id when reading the session in the pool adapter commit 3ba123d278f1085ba78fc000df954e507af2d622 Author: Rafael Mendonça França Date: Wed Oct 9 18:06:23 2019 -0400 Also drop the session with the public id when destroying sessions commit 6a04bbf6b742c305d3a56f9bd6242e6c943cc2ad Author: Rafael Mendonça França Date: Wed Oct 9 17:50:45 2019 -0400 Fallback to the legacy id when the new id is not found This will avoid all session to be invalidated. commit dc45a06b339c707c1f658c123ec7216151878f7a Author: Aaron Patterson Date: Tue Aug 13 17:16:23 2019 -0400 Add the private id commit 73a5f79f6854eed81ecc3e5fb9f8154e967ccc49 Author: Aaron Patterson Date: Tue Aug 13 16:48:41 2019 -0400 revert conditionals to master commit 4e322629e0c6698c75a3fb541a42571f8543c34c Author: Aaron Patterson Date: Tue Aug 13 16:45:04 2019 -0400 remove NullSession commit 1c7e3b259f0741c869dcfbabeb3e0670c4d3f848 Author: Aaron Patterson Date: Tue Aug 13 16:38:01 2019 -0400 remove || raise and get closer to master commit 2b205ed5a047d9e50a13bb7a411bc48745b515ec Author: Aaron Patterson Date: Tue Aug 13 16:31:06 2019 -0400 store hashed id, send public id commit bb3d486644755b2e0c7824b3910db1a83c98fcd2 Author: Aaron Patterson Date: Tue Aug 13 16:20:32 2019 -0400 use session id objects commit 77f3aab73089abe518f62c46268b104bacd7114b Author: Aaron Patterson Date: Tue Aug 13 15:43:58 2019 -0400 remove more nils commit 83d4bd12c7e88455d21230bc24ec3a543654e2aa Author: Aaron Patterson Date: Tue Aug 13 15:32:20 2019 -0400 try to ensure we always have some kind of object --- lib/rack/session/abstract/id.rb | 67 ++++++++++++++++++++++++++++++++- lib/rack/session/cookie.rb | 13 ++++++- lib/rack/session/memcache.rb | 34 ++++++++++------- lib/rack/session/pool.rb | 19 +++++++--- test/spec_session_memcache.rb | 43 +++++++++++++++++++-- test/spec_session_pool.rb | 43 +++++++++++++++++++-- 6 files changed, 190 insertions(+), 29 deletions(-) diff --git a/lib/rack/session/abstract/id.rb b/lib/rack/session/abstract/id.rb index 1bb8d5d..9d63ca2 100644 --- a/lib/rack/session/abstract/id.rb +++ b/lib/rack/session/abstract/id.rb @@ -6,11 +6,38 @@ require 'time' 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. @@ -357,7 +384,7 @@ module Rack 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)) @@ -365,6 +392,10 @@ module Rack 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. @@ -406,6 +437,40 @@ module Rack 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 } diff --git a/lib/rack/session/cookie.rb b/lib/rack/session/cookie.rb index 71bb96f..90ed5cf 100644 --- a/lib/rack/session/cookie.rb +++ b/lib/rack/session/cookie.rb @@ -45,7 +45,7 @@ module Rack # }) # - class Cookie < Abstract::Persisted + class Cookie < Abstract::PersistedSecure # Encode session cookies as Base64 class Base64 def encode(str) @@ -153,6 +153,15 @@ module Rack 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) @@ -165,7 +174,7 @@ module Rack 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 diff --git a/lib/rack/session/memcache.rb b/lib/rack/session/memcache.rb index 4cf5ea0..1f9d3ec 100644 --- a/lib/rack/session/memcache.rb +++ b/lib/rack/session/memcache.rb @@ -19,7 +19,7 @@ module Rack # 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 @@ module Rack 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) + def find_session(req, sid) + with_lock(req) do + 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 @@ -58,25 +58,26 @@ module Rack end end - def set_session(env, session_id, new_session, options) + def write_session(req, session_id, new_session, options) expiry = options[:expire_after] expiry = expiry.nil? ? 0 : expiry + 1 - with_lock(env) do - @pool.set session_id, new_session, expiry + with_lock(req) do + @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) + def delete_session(req, session_id, options) + with_lock(req) do + @pool.delete(session_id.public_id) + @pool.delete(session_id.private_id) generate_sid unless options[:drop] end end - def with_lock(env) - @mutex.lock if env[RACK_MULTITHREAD] + def with_lock(req) + @mutex.lock if req.multithread? yield rescue MemCache::MemCacheError, Errno::ECONNREFUSED if $VERBOSE @@ -88,6 +89,11 @@ module Rack @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 diff --git a/lib/rack/session/pool.rb b/lib/rack/session/pool.rb index 4c9c25c..6c0f668 100644 --- a/lib/rack/session/pool.rb +++ b/lib/rack/session/pool.rb @@ -24,7 +24,7 @@ module Rack # ) # 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 @@ -37,15 +37,15 @@ module Rack 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 @@ -53,14 +53,15 @@ module Rack 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 @@ -71,6 +72,12 @@ module Rack 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 diff --git a/test/spec_session_memcache.rb b/test/spec_session_memcache.rb index 93a03d1..824db68 100644 --- a/test/spec_session_memcache.rb +++ b/test/spec_session_memcache.rb @@ -226,15 +226,52 @@ begin 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.wont_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"].must_be_nil + res1.body.must_equal '{"counter"=>2}' + pool.pool.get(session_id.private_id, true).wont_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"].must_be_nil + res2.body.must_equal '{"counter"=>2}' + pool.pool.get(session_id.private_id, true).must_be_nil + pool.pool.get(session_id.public_id, true).must_be_nil + end + # anyone know how to do this better? it "cleanly merges sessions when multithreaded" do skip unless $DEBUG diff --git a/test/spec_session_pool.rb b/test/spec_session_pool.rb index 2d06169..62a0505 100644 --- a/test/spec_session_pool.rb +++ b/test/spec_session_pool.rb @@ -6,7 +6,7 @@ require 'rack/session/pool' 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 @@ -14,7 +14,7 @@ describe Rack::Session::Pool do 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) @@ -143,6 +143,43 @@ describe Rack::Session::Pool do 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 @@ -191,7 +228,7 @@ describe Rack::Session::Pool do 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 -- 2.21.0