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]

activesupport/lib/active_support/cache/redis_cache_store.rb

@@ -74,14 +74,6 @@ def self.supports_cache_versioning?
       # 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 @@ def set_redis_capabilities
         # 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 @@ def read_multi_mget(*names)
           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 @@ def read_multi_mget(*names)
 
           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 @@ def truncate_key(key)
           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
 

activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb

@@ -3,23 +3,23 @@
 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)
   end
 
   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)

activesupport/test/cache/behaviors/cache_store_behavior.rb

@@ -472,8 +472,8 @@ def test_race_condition_protection
   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 @@ def test_cache_hit_instrumentation
       @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]

activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb

@@ -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)

activesupport/test/cache/behaviors/local_cache_behavior.rb

@@ -115,7 +115,7 @@ def test_local_cache_of_increment
       @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 @@ def test_local_cache_of_decrement
       @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 @@ def test_local_cache_of_read_multi
     @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

activesupport/test/cache/stores/redis_cache_store_test.rb

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