activesupport: Avoid Marshal.load on raw cache value 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]
This commit is contained in:
Dylan Thacker-Smith 2018-09-22 21:17:07 -04:00 committed by Aaron Patterson
parent 0a4cf42311
commit 9b4aef4be3
No known key found for this signature in database
GPG Key ID: 953170BCB4FFAFC6
6 changed files with 29 additions and 31 deletions

View File

@ -74,14 +74,6 @@ module ActiveSupport
# Support raw values in the local cache strategy.
module LocalCacheWithRaw # :nodoc:
private
def read_entry(key, **options)
entry = super
if options[:raw] && local_cache && entry
entry = deserialize_entry(entry.value)
end
entry
end
def write_entry(key, entry, **options)
if options[:raw] && local_cache
raw_entry = Entry.new(serialize_entry(entry, raw: true))
@ -352,7 +344,8 @@ module ActiveSupport
# Read an entry from the cache.
def read_entry(key, **options)
failsafe :read_entry do
deserialize_entry redis.with { |c| c.get(key) }
raw = options&.fetch(:raw, false)
deserialize_entry(redis.with { |c| c.get(key) }, raw: raw)
end
end
@ -368,6 +361,7 @@ module ActiveSupport
options = names.extract_options!
options = merged_options(options)
return {} if names == []
raw = options&.fetch(:raw, false)
keys = names.map { |name| normalize_key(name, options) }
@ -377,7 +371,7 @@ module ActiveSupport
names.zip(values).each_with_object({}) do |(name, value), results|
if value
entry = deserialize_entry(value)
entry = deserialize_entry(value, raw: raw)
unless entry.nil? || entry.expired? || entry.mismatched?(normalize_version(name, options))
results[name] = entry.value
end
@ -457,10 +451,13 @@ module ActiveSupport
end
end
def deserialize_entry(serialized_entry)
def deserialize_entry(serialized_entry, raw:)
if serialized_entry
entry = Marshal.load(serialized_entry) rescue serialized_entry
entry.is_a?(Entry) ? entry : Entry.new(entry)
if raw
Entry.new(serialized_entry)
else
Marshal.load(serialized_entry)
end
end
end

View File

@ -3,11 +3,11 @@
module CacheIncrementDecrementBehavior
def test_increment
@cache.write("foo", 1, raw: true)
assert_equal 1, @cache.read("foo").to_i
assert_equal 1, @cache.read("foo", raw: true).to_i
assert_equal 2, @cache.increment("foo")
assert_equal 2, @cache.read("foo").to_i
assert_equal 2, @cache.read("foo", raw: true).to_i
assert_equal 3, @cache.increment("foo")
assert_equal 3, @cache.read("foo").to_i
assert_equal 3, @cache.read("foo", raw: true).to_i
missing = @cache.increment("bar")
assert(missing.nil? || missing == 1)
@ -15,11 +15,11 @@ module CacheIncrementDecrementBehavior
def test_decrement
@cache.write("foo", 3, raw: true)
assert_equal 3, @cache.read("foo").to_i
assert_equal 3, @cache.read("foo", raw: true).to_i
assert_equal 2, @cache.decrement("foo")
assert_equal 2, @cache.read("foo").to_i
assert_equal 2, @cache.read("foo", raw: true).to_i
assert_equal 1, @cache.decrement("foo")
assert_equal 1, @cache.read("foo").to_i
assert_equal 1, @cache.read("foo", raw: true).to_i
missing = @cache.decrement("bar")
assert(missing.nil? || missing == -1)

View File

@ -472,8 +472,8 @@ module CacheStoreBehavior
def test_crazy_key_characters
crazy_key = "#/:*(<+=> )&$%@?;'\"\'`~-"
assert @cache.write(crazy_key, "1", raw: true)
assert_equal "1", @cache.read(crazy_key)
assert_equal "1", @cache.fetch(crazy_key)
assert_equal "1", @cache.read(crazy_key, raw: true)
assert_equal "1", @cache.fetch(crazy_key, raw: true)
assert @cache.delete(crazy_key)
assert_equal "2", @cache.fetch(crazy_key, raw: true) { "2" }
assert_equal 3, @cache.increment(crazy_key)
@ -497,7 +497,7 @@ module CacheStoreBehavior
@events << ActiveSupport::Notifications::Event.new(*args)
end
assert @cache.write(key, "1", raw: true)
assert @cache.fetch(key) { }
assert @cache.fetch(key, raw: true) { }
assert_equal 1, @events.length
assert_equal "cache_read.active_support", @events[0].name
assert_equal :fetch, @events[0].payload[:super_operation]

View File

@ -8,8 +8,8 @@ module EncodedKeyCacheBehavior
define_method "test_#{encoding.name.underscore}_encoded_values" do
key = (+"foo").force_encoding(encoding)
assert @cache.write(key, "1", raw: true)
assert_equal "1", @cache.read(key)
assert_equal "1", @cache.fetch(key)
assert_equal "1", @cache.read(key, raw: true)
assert_equal "1", @cache.fetch(key, raw: true)
assert @cache.delete(key)
assert_equal "2", @cache.fetch(key, raw: true) { "2" }
assert_equal 3, @cache.increment(key)
@ -20,8 +20,8 @@ module EncodedKeyCacheBehavior
def test_common_utf8_values
key = (+"\xC3\xBCmlaut").force_encoding(Encoding::UTF_8)
assert @cache.write(key, "1", raw: true)
assert_equal "1", @cache.read(key)
assert_equal "1", @cache.fetch(key)
assert_equal "1", @cache.read(key, raw: true)
assert_equal "1", @cache.fetch(key, raw: true)
assert @cache.delete(key)
assert_equal "2", @cache.fetch(key, raw: true) { "2" }
assert_equal 3, @cache.increment(key)

View File

@ -115,7 +115,7 @@ module LocalCacheBehavior
@cache.write("foo", 1, raw: true)
@peek.write("foo", 2, raw: true)
@cache.increment("foo")
assert_equal 3, @cache.read("foo")
assert_equal 3, @cache.read("foo", raw: true)
end
end
@ -124,7 +124,7 @@ module LocalCacheBehavior
@cache.write("foo", 1, raw: true)
@peek.write("foo", 3, raw: true)
@cache.decrement("foo")
assert_equal 2, @cache.read("foo")
assert_equal 2, @cache.read("foo", raw: true)
end
end
@ -142,9 +142,9 @@ module LocalCacheBehavior
@cache.with_local_cache do
@cache.write("foo", "foo", raw: true)
@cache.write("bar", "bar", raw: true)
values = @cache.read_multi("foo", "bar")
assert_equal "foo", @cache.read("foo")
assert_equal "bar", @cache.read("bar")
values = @cache.read_multi("foo", "bar", raw: true)
assert_equal "foo", @cache.read("foo", raw: true)
assert_equal "bar", @cache.read("bar", raw: true)
assert_equal "foo", values["foo"]
assert_equal "bar", values["bar"]
end

View File

@ -17,6 +17,7 @@ class SlowRedis < Redis
def get(key)
if /latency/.match?(key)
sleep 3
super
else
super
end