Skip to content

Commit 2a52a38

Browse files
rafaelfrancarafaelfranca
committed
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
1 parent 8bec77c commit 2a52a38

File tree

11 files changed

+180
-28
lines changed

11 files changed

+180
-28
lines changed

‎Gemfile.lock

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ PATH
3939
actionpack (5.2.4)
4040
actionview (= 5.2.4)
4141
activesupport (= 5.2.4)
42-
rack (~> 2.0)
42+
rack (~> 2.0, >= 2.0.8)
4343
rack-test (>= 0.6.3)
4444
rails-dom-testing (~> 2.0)
4545
rails-html-sanitizer (~> 1.0, >= 1.0.2)
@@ -352,7 +352,7 @@ GEM
352352
thor
353353
raabro (1.1.6)
354354
racc (1.4.15)
355-
rack (2.0.7)
355+
rack (2.0.8)
356356
rack-cache (1.10.0)
357357
rack (>= 0.4)
358358
rack-protection (2.0.7)

‎actionpack/CHANGELOG.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,11 @@
1+
* Fix possible information leak / session hijacking vulnerability.
2+
3+
The `ActionDispatch::Session::MemcacheStore` is still vulnerable given it requires the
4+
gem dalli to be updated as well.
5+
6+
CVE-2019-16782.
7+
8+
19
## Rails 5.2.4 (November 27, 2019) ##
210

311
* No changes.

‎actionpack/actionpack.gemspec

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ Gem::Specification.new do |s|
2828

2929
s.add_dependency "activesupport", version
3030

31-
s.add_dependency "rack", "~> 2.0"
31+
s.add_dependency "rack", "~> 2.0", ">= 2.0.8"
3232
s.add_dependency "rack-test", ">= 0.6.3"
3333
s.add_dependency "rails-html-sanitizer", "~> 1.0", ">= 1.0.2"
3434
s.add_dependency "rails-dom-testing", "~> 2.0"

‎actionpack/lib/action_dispatch.rb

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,11 @@ module Http
8383
end
8484

8585
module Session
86-
autoload :AbstractStore, "action_dispatch/middleware/session/abstract_store"
87-
autoload :CookieStore, "action_dispatch/middleware/session/cookie_store"
88-
autoload :MemCacheStore, "action_dispatch/middleware/session/mem_cache_store"
89-
autoload :CacheStore, "action_dispatch/middleware/session/cache_store"
86+
autoload :AbstractStore, "action_dispatch/middleware/session/abstract_store"
87+
autoload :AbstractSecureStore, "action_dispatch/middleware/session/abstract_store"
88+
autoload :CookieStore, "action_dispatch/middleware/session/cookie_store"
89+
autoload :MemCacheStore, "action_dispatch/middleware/session/mem_cache_store"
90+
autoload :CacheStore, "action_dispatch/middleware/session/cache_store"
9091
end
9192

9293
mattr_accessor :test_app

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,21 @@ class AbstractStore < Rack::Session::Abstract::Persisted
8383
include SessionObject
8484

8585
private
86+
def set_cookie(request, session_id, cookie)
87+
request.cookie_jar[key] = cookie
88+
end
89+
end
90+
91+
class AbstractSecureStore < Rack::Session::Abstract::PersistedSecure
92+
include Compatibility
93+
include StaleSessionCheck
94+
include SessionObject
95+
96+
def generate_sid
97+
Rack::Session::SessionId.new(super)
98+
end
8699

100+
private
87101
def set_cookie(request, session_id, cookie)
88102
request.cookie_jar[key] = cookie
89103
end

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

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ module Session
1212
# * <tt>cache</tt> - The cache to use. If it is not specified, <tt>Rails.cache</tt> will be used.
1313
# * <tt>expire_after</tt> - The length of time a session will be stored before automatically expiring.
1414
# By default, the <tt>:expires_in</tt> option of the cache is used.
15-
class CacheStore < AbstractStore
15+
class CacheStore < AbstractSecureStore
1616
def initialize(app, options = {})
1717
@cache = options[:cache] || Rails.cache
1818
options[:expire_after] ||= @cache.options[:expires_in]
@@ -21,15 +21,15 @@ def initialize(app, options = {})
2121

2222
# Get a session from the cache.
2323
def find_session(env, sid)
24-
unless sid && (session = @cache.read(cache_key(sid)))
24+
unless sid && (session = get_session_with_fallback(sid))
2525
sid, session = generate_sid, {}
2626
end
2727
[sid, session]
2828
end
2929

3030
# Set a session in the cache.
3131
def write_session(env, sid, session, options)
32-
key = cache_key(sid)
32+
key = cache_key(sid.private_id)
3333
if session
3434
@cache.write(key, session, expires_in: options[:expire_after])
3535
else
@@ -40,14 +40,19 @@ def write_session(env, sid, session, options)
4040

4141
# Remove a session from the cache.
4242
def delete_session(env, sid, options)
43-
@cache.delete(cache_key(sid))
43+
@cache.delete(cache_key(sid.private_id))
44+
@cache.delete(cache_key(sid.public_id))
4445
generate_sid
4546
end
4647

4748
private
4849
# Turn the session id into a cache key.
49-
def cache_key(sid)
50-
"_session_id:#{sid}"
50+
def cache_key(id)
51+
"_session_id:#{id}"
52+
end
53+
54+
def get_session_with_fallback(sid)
55+
@cache.read(cache_key(sid.private_id)) || @cache.read(cache_key(sid.public_id))
5156
end
5257
end
5358
end

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

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,31 +51,41 @@ module Session
5151
# would set the session cookie to expire automatically 14 days after creation.
5252
# Other useful options include <tt>:key</tt>, <tt>:secure</tt> and
5353
# <tt>:httponly</tt>.
54-
class CookieStore < AbstractStore
54+
class CookieStore < AbstractSecureStore
55+
class SessionId < DelegateClass(Rack::Session::SessionId)
56+
attr_reader :cookie_value
57+
58+
def initialize(session_id, cookie_value = {})
59+
super(session_id)
60+
@cookie_value = cookie_value
61+
end
62+
end
63+
5564
def initialize(app, options = {})
5665
super(app, options.merge!(cookie_only: true))
5766
end
5867

5968
def delete_session(req, session_id, options)
6069
new_sid = generate_sid unless options[:drop]
6170
# Reset hash and Assign the new session id
62-
req.set_header("action_dispatch.request.unsigned_session_cookie", new_sid ? { "session_id" => new_sid } : {})
71+
req.set_header("action_dispatch.request.unsigned_session_cookie", new_sid ? { "session_id" => new_sid.public_id } : {})
6372
new_sid
6473
end
6574

6675
def load_session(req)
6776
stale_session_check! do
6877
data = unpacked_cookie_data(req)
6978
data = persistent_session_id!(data)
70-
[data["session_id"], data]
79+
[Rack::Session::SessionId.new(data["session_id"]), data]
7180
end
7281
end
7382

7483
private
7584

7685
def extract_session_id(req)
7786
stale_session_check! do
78-
unpacked_cookie_data(req)["session_id"]
87+
sid = unpacked_cookie_data(req)["session_id"]
88+
sid && Rack::Session::SessionId.new(sid)
7989
end
8090
end
8191

@@ -93,13 +103,13 @@ def unpacked_cookie_data(req)
93103

94104
def persistent_session_id!(data, sid = nil)
95105
data ||= {}
96-
data["session_id"] ||= sid || generate_sid
106+
data["session_id"] ||= sid || generate_sid.public_id
97107
data
98108
end
99109

100110
def write_session(req, sid, session_data, options)
101-
session_data["session_id"] = sid
102-
session_data
111+
session_data["session_id"] = sid.public_id
112+
SessionId.new(sid, session_data)
103113
end
104114

105115
def set_cookie(request, session_id, cookie)

‎actionpack/lib/action_dispatch/request/session.rb

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,13 @@ def destroy
9090
# +nil+ if the given key is not found in the session.
9191
def [](key)
9292
load_for_read!
93-
@delegate[key.to_s]
93+
key = key.to_s
94+
95+
if key == "session_id"
96+
id&.public_id
97+
else
98+
@delegate[key]
99+
end
94100
end
95101

96102
# Returns true if the session has the given key or false.

‎actionpack/test/dispatch/session/abstract_secure_store_test.rb

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
# frozen_string_literal: true
2+
3+
require "abstract_unit"
4+
require "action_dispatch/middleware/session/abstract_store"
5+
6+
module ActionDispatch
7+
module Session
8+
class AbstractSecureStoreTest < ActiveSupport::TestCase
9+
class MemoryStore < AbstractSecureStore
10+
class SessionId < Rack::Session::SessionId
11+
attr_reader :cookie_value
12+
13+
def initialize(session_id, cookie_value)
14+
super(session_id)
15+
@cookie_value = cookie_value
16+
end
17+
end
18+
19+
def initialize(app)
20+
@sessions = {}
21+
super
22+
end
23+
24+
def find_session(env, sid)
25+
sid ||= 1
26+
session = @sessions[sid] ||= {}
27+
[sid, session]
28+
end
29+
30+
def write_session(env, sid, session, options)
31+
@sessions[sid] = SessionId.new(sid, session)
32+
end
33+
end
34+
35+
def test_session_is_set
36+
env = {}
37+
as = MemoryStore.new app
38+
as.call(env)
39+
40+
assert @env
41+
assert Request::Session.find ActionDispatch::Request.new @env
42+
end
43+
44+
def test_new_session_object_is_merged_with_old
45+
env = {}
46+
as = MemoryStore.new app
47+
as.call(env)
48+
49+
assert @env
50+
session = Request::Session.find ActionDispatch::Request.new @env
51+
session["foo"] = "bar"
52+
53+
as.call(@env)
54+
session1 = Request::Session.find ActionDispatch::Request.new @env
55+
56+
assert_not_equal session, session1
57+
assert_equal session.to_hash, session1.to_hash
58+
end
59+
60+
private
61+
def app(&block)
62+
@env = nil
63+
lambda { |env| @env = env }
64+
end
65+
end
66+
end
67+
end

‎actionpack/test/dispatch/session/cache_store_test.rb

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ def get_session_value
2424
end
2525

2626
def get_session_id
27-
render plain: "#{request.session.id}"
27+
render plain: "#{request.session.id.public_id}"
2828
end
2929

3030
def call_reset_session
@@ -150,15 +150,56 @@ def test_doesnt_write_session_cookie_if_session_id_is_already_exists
150150

151151
def test_prevents_session_fixation
152152
with_test_route_set do
153-
assert_nil @cache.read("_session_id:0xhax")
153+
sid = Rack::Session::SessionId.new("0xhax")
154+
assert_nil @cache.read("_session_id:#{sid.private_id}")
154155

155-
cookies["_session_id"] = "0xhax"
156+
cookies["_session_id"] = sid.public_id
156157
get "/set_session_value"
157158

158159
assert_response :success
159-
assert_not_equal "0xhax", cookies["_session_id"]
160-
assert_nil @cache.read("_session_id:0xhax")
161-
assert_equal({ "foo" => "bar" }, @cache.read("_session_id:#{cookies['_session_id']}"))
160+
assert_not_equal sid.public_id, cookies["_session_id"]
161+
assert_nil @cache.read("_session_id:#{sid.private_id}")
162+
assert_equal(
163+
{ "foo" => "bar" },
164+
@cache.read("_session_id:#{Rack::Session::SessionId.new(cookies['_session_id']).private_id}")
165+
)
166+
end
167+
end
168+
169+
def test_can_read_session_with_legacy_id
170+
with_test_route_set do
171+
get "/set_session_value"
172+
assert_response :success
173+
assert cookies["_session_id"]
174+
175+
sid = Rack::Session::SessionId.new(cookies['_session_id'])
176+
session = @cache.read("_session_id:#{sid.private_id}")
177+
@cache.delete("_session_id:#{sid.private_id}")
178+
@cache.write("_session_id:#{sid.public_id}", session)
179+
180+
get "/get_session_value"
181+
assert_response :success
182+
assert_equal 'foo: "bar"', response.body
183+
end
184+
end
185+
186+
def test_drop_session_in_the_legacy_id_as_well
187+
with_test_route_set do
188+
get "/set_session_value"
189+
assert_response :success
190+
assert cookies["_session_id"]
191+
192+
sid = Rack::Session::SessionId.new(cookies['_session_id'])
193+
session = @cache.read("_session_id:#{sid.private_id}")
194+
@cache.delete("_session_id:#{sid.private_id}")
195+
@cache.write("_session_id:#{sid.public_id}", session)
196+
197+
get "/call_reset_session"
198+
assert_response :success
199+
assert_not_equal [], headers["Set-Cookie"]
200+
201+
assert_nil @cache.read("_session_id:#{sid.private_id}")
202+
assert_nil @cache.read("_session_id:#{sid.public_id}")
162203
end
163204
end
164205

‎actionpack/test/dispatch/session/cookie_store_test.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ def get_session_value
3636
end
3737

3838
def get_session_id
39-
render plain: "id: #{request.session.id}"
39+
render plain: "id: #{request.session.id&.public_id}"
4040
end
4141

4242
def get_class_after_reset_session

0 commit comments

Comments
 (0)