Skip to content

Commit bd39a13

Browse files
dylanahsmithtenderlove
dylanahsmith
authored and
tenderlove
committed
activesupport: Deprecate Marshal.load on raw cache read in RedisCacheStore
The same value for the `raw` option should be provided for both reading and writing to avoid Marshal.load being called on untrusted data. [CVE-2020-8165]
1 parent 0a7ce52 commit bd39a13

6 files changed

Lines changed: 36 additions & 30 deletions

File tree

‎activesupport/lib/active_support/cache/redis_cache_store.rb‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -74,14 +74,6 @@ def self.supports_cache_versioning?
7474
# Support raw values in the local cache strategy.
7575
module LocalCacheWithRaw # :nodoc:
7676
private
77-
def read_entry(key, **options)
78-
entry = super
79-
if options[:raw] && local_cache && entry
80-
entry = deserialize_entry(entry.value)
81-
end
82-
entry
83-
end
84-
8577
def write_entry(key, entry, **options)
8678
if options[:raw] && local_cache
8779
raw_entry = Entry.new(serialize_entry(entry, raw: true))
@@ -348,7 +340,8 @@ def set_redis_capabilities
348340
# Read an entry from the cache.
349341
def read_entry(key, **options)
350342
failsafe :read_entry do
351-
deserialize_entry redis.with { |c| c.get(key) }
343+
raw = options&.fetch(:raw, false)
344+
deserialize_entry(redis.with { |c| c.get(key) }, raw: raw)
352345
end
353346
end
354347

@@ -364,6 +357,7 @@ def read_multi_mget(*names)
364357
options = names.extract_options!
365358
options = merged_options(options)
366359
return {} if names == []
360+
raw = options&.fetch(:raw, false)
367361

368362
keys = names.map { |name| normalize_key(name, options) }
369363

@@ -373,7 +367,7 @@ def read_multi_mget(*names)
373367

374368
names.zip(values).each_with_object({}) do |(name, value), results|
375369
if value
376-
entry = deserialize_entry(value)
370+
entry = deserialize_entry(value, raw: raw)
377371
unless entry.nil? || entry.expired? || entry.mismatched?(normalize_version(name, options))
378372
results[name] = entry.value
379373
end
@@ -448,9 +442,20 @@ def truncate_key(key)
448442
end
449443
end
450444

451-
def deserialize_entry(serialized_entry)
445+
def deserialize_entry(serialized_entry, raw:)
452446
if serialized_entry
453447
entry = Marshal.load(serialized_entry) rescue serialized_entry
448+
449+
written_raw = serialized_entry.equal?(entry)
450+
if raw != written_raw
451+
ActiveSupport::Deprecation.warn(<<-MSG.squish)
452+
Using a different value for the raw option when reading and writing
453+
to a cache key is deprecated for :redis_cache_store and Rails 6.0
454+
will stop automatically detecting the format when reading to avoid
455+
marshal loading untrusted raw strings.
456+
MSG
457+
end
458+
454459
entry.is_a?(Entry) ? entry : Entry.new(entry)
455460
end
456461
end

‎activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,23 +3,23 @@
33
module CacheIncrementDecrementBehavior
44
def test_increment
55
@cache.write("foo", 1, raw: true)
6-
assert_equal 1, @cache.read("foo").to_i
6+
assert_equal 1, @cache.read("foo", raw: true).to_i
77
assert_equal 2, @cache.increment("foo")
8-
assert_equal 2, @cache.read("foo").to_i
8+
assert_equal 2, @cache.read("foo", raw: true).to_i
99
assert_equal 3, @cache.increment("foo")
10-
assert_equal 3, @cache.read("foo").to_i
10+
assert_equal 3, @cache.read("foo", raw: true).to_i
1111

1212
missing = @cache.increment("bar")
1313
assert(missing.nil? || missing == 1)
1414
end
1515

1616
def test_decrement
1717
@cache.write("foo", 3, raw: true)
18-
assert_equal 3, @cache.read("foo").to_i
18+
assert_equal 3, @cache.read("foo", raw: true).to_i
1919
assert_equal 2, @cache.decrement("foo")
20-
assert_equal 2, @cache.read("foo").to_i
20+
assert_equal 2, @cache.read("foo", raw: true).to_i
2121
assert_equal 1, @cache.decrement("foo")
22-
assert_equal 1, @cache.read("foo").to_i
22+
assert_equal 1, @cache.read("foo", raw: true).to_i
2323

2424
missing = @cache.decrement("bar")
2525
assert(missing.nil? || missing == -1)

‎activesupport/test/cache/behaviors/cache_store_behavior.rb‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -456,8 +456,8 @@ def test_race_condition_protection
456456
def test_crazy_key_characters
457457
crazy_key = "#/:*(<+=> )&$%@?;'\"\'`~-"
458458
assert @cache.write(crazy_key, "1", raw: true)
459-
assert_equal "1", @cache.read(crazy_key)
460-
assert_equal "1", @cache.fetch(crazy_key)
459+
assert_equal "1", @cache.read(crazy_key, raw: true)
460+
assert_equal "1", @cache.fetch(crazy_key, raw: true)
461461
assert @cache.delete(crazy_key)
462462
assert_equal "2", @cache.fetch(crazy_key, raw: true) { "2" }
463463
assert_equal 3, @cache.increment(crazy_key)
@@ -481,7 +481,7 @@ def test_cache_hit_instrumentation
481481
@events << ActiveSupport::Notifications::Event.new(*args)
482482
end
483483
assert @cache.write(key, "1", raw: true)
484-
assert @cache.fetch(key) { }
484+
assert @cache.fetch(key, raw: true) { }
485485
assert_equal 1, @events.length
486486
assert_equal "cache_read.active_support", @events[0].name
487487
assert_equal :fetch, @events[0].payload[:super_operation]

‎activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ module EncodedKeyCacheBehavior
88
define_method "test_#{encoding.name.underscore}_encoded_values" do
99
key = (+"foo").force_encoding(encoding)
1010
assert @cache.write(key, "1", raw: true)
11-
assert_equal "1", @cache.read(key)
12-
assert_equal "1", @cache.fetch(key)
11+
assert_equal "1", @cache.read(key, raw: true)
12+
assert_equal "1", @cache.fetch(key, raw: true)
1313
assert @cache.delete(key)
1414
assert_equal "2", @cache.fetch(key, raw: true) { "2" }
1515
assert_equal 3, @cache.increment(key)
@@ -20,8 +20,8 @@ module EncodedKeyCacheBehavior
2020
def test_common_utf8_values
2121
key = (+"\xC3\xBCmlaut").force_encoding(Encoding::UTF_8)
2222
assert @cache.write(key, "1", raw: true)
23-
assert_equal "1", @cache.read(key)
24-
assert_equal "1", @cache.fetch(key)
23+
assert_equal "1", @cache.read(key, raw: true)
24+
assert_equal "1", @cache.fetch(key, raw: true)
2525
assert @cache.delete(key)
2626
assert_equal "2", @cache.fetch(key, raw: true) { "2" }
2727
assert_equal 3, @cache.increment(key)

‎activesupport/test/cache/behaviors/local_cache_behavior.rb‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ def test_local_cache_of_increment
106106
@cache.write("foo", 1, raw: true)
107107
@peek.write("foo", 2, raw: true)
108108
@cache.increment("foo")
109-
assert_equal 3, @cache.read("foo")
109+
assert_equal 3, @cache.read("foo", raw: true)
110110
end
111111
end
112112

@@ -115,7 +115,7 @@ def test_local_cache_of_decrement
115115
@cache.write("foo", 1, raw: true)
116116
@peek.write("foo", 3, raw: true)
117117
@cache.decrement("foo")
118-
assert_equal 2, @cache.read("foo")
118+
assert_equal 2, @cache.read("foo", raw: true)
119119
end
120120
end
121121

@@ -133,9 +133,9 @@ def test_local_cache_of_read_multi
133133
@cache.with_local_cache do
134134
@cache.write("foo", "foo", raw: true)
135135
@cache.write("bar", "bar", raw: true)
136-
values = @cache.read_multi("foo", "bar")
137-
assert_equal "foo", @cache.read("foo")
138-
assert_equal "bar", @cache.read("bar")
136+
values = @cache.read_multi("foo", "bar", raw: true)
137+
assert_equal "foo", @cache.read("foo", raw: true)
138+
assert_equal "bar", @cache.read("bar", raw: true)
139139
assert_equal "foo", values["foo"]
140140
assert_equal "bar", values["bar"]
141141
end

‎activesupport/test/cache/stores/redis_cache_store_test.rb‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,10 @@
1414
# Emulates a latency on Redis's back-end for the key latency to facilitate
1515
# connection pool testing.
1616
class SlowRedis < Redis
17-
def get(key, options = {})
17+
def get(key)
1818
if key =~ /latency/
1919
sleep 3
20+
super
2021
else
2122
super
2223
end

0 commit comments

Comments
 (0)