From 92ec9f270d696a17ba7604b3d02931f07c3e7c6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 17 Dec 2019 18:44:59 -0300 Subject: [PATCH] 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 | 4 +- actionpack/CHANGELOG.md | 7 ++ actionpack/actionpack.gemspec | 2 +- actionpack/lib/action_dispatch.rb | 9 +-- .../middleware/session/abstract_store.rb | 15 +++++ .../middleware/session/cache_store.rb | 17 +++-- .../middleware/session/cookie_store.rb | 24 +++++-- .../lib/action_dispatch/request/session.rb | 8 ++- .../session/abstract_secure_store_test.rb | 67 +++++++++++++++++++ .../test/dispatch/session/cache_store_test.rb | 53 +++++++++++++-- .../dispatch/session/cookie_store_test.rb | 2 +- 11 files changed, 180 insertions(+), 28 deletions(-) create mode 100644 actionpack/test/dispatch/session/abstract_secure_store_test.rb diff --git a/Gemfile.lock b/Gemfile.lock index d3038ace599..ee87d3e844a 100644 --- a/Gemfile.lock +++ b/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) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index cb7819ff81c..bdd81f41ff3 100644 --- a/actionpack/CHANGELOG.md +++ b/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 diff --git a/actionpack/actionpack.gemspec b/actionpack/actionpack.gemspec index fb724470a97..8e587a59816 100644 --- a/actionpack/actionpack.gemspec +++ b/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" diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb index c539fd6f82e..f5cee794330 100644 --- a/actionpack/lib/action_dispatch.rb +++ b/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 diff --git a/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb b/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb index 3815971acbf..536f9991e5c 100644 --- a/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb +++ b/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 diff --git a/actionpack/lib/action_dispatch/middleware/session/cache_store.rb b/actionpack/lib/action_dispatch/middleware/session/cache_store.rb index a6d965a6441..c7b61f96ef8 100644 --- a/actionpack/lib/action_dispatch/middleware/session/cache_store.rb +++ b/actionpack/lib/action_dispatch/middleware/session/cache_store.rb @@ -12,7 +12,7 @@ module ActionDispatch # * cache - The cache to use. If it is not specified, Rails.cache will be used. # * expire_after - The length of time a session will be stored before automatically expiring. # By default, the :expires_in 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 diff --git a/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb b/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb index 892d88803e3..6010c152a72 100644 --- a/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb +++ b/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 :key, :secure and # :httponly. - 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) diff --git a/actionpack/lib/action_dispatch/request/session.rb b/actionpack/lib/action_dispatch/request/session.rb index 8faedf15b9c..e714e74475c 100644 --- a/actionpack/lib/action_dispatch/request/session.rb +++ b/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 diff --git a/actionpack/test/dispatch/session/abstract_secure_store_test.rb b/actionpack/test/dispatch/session/abstract_secure_store_test.rb new file mode 100644 index 00000000000..25e2adaa13e --- /dev/null +++ b/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 diff --git a/actionpack/test/dispatch/session/cache_store_test.rb b/actionpack/test/dispatch/session/cache_store_test.rb index 06e67fac9f9..20d16640b5f 100644 --- a/actionpack/test/dispatch/session/cache_store_test.rb +++ b/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 diff --git a/actionpack/test/dispatch/session/cookie_store_test.rb b/actionpack/test/dispatch/session/cookie_store_test.rb index ddbae3f6417..05fc4a04559 100644 --- a/actionpack/test/dispatch/session/cookie_store_test.rb +++ b/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