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]

Author: dylanahsmith

activesupport/lib/active_support/cache/redis_cache_store.rb

@@ -70,14 +70,6 @@ class RedisCacheStore < Store
       # 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))
@@ -328,7 +320,8 @@ def set_redis_capabilities
         # Read an entry from the cache.
         def read_entry(key, options = nil)
           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
 
@@ -343,6 +336,7 @@ def read_multi_entries(names, _options)
         def read_multi_mget(*names)
           options = names.extract_options!
           options = merged_options(options)
+          raw = options&.fetch(:raw, false)
 
           keys = names.map { |name| normalize_key(name, options) }
 
@@ -352,7 +346,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
@@ -421,9 +415,20 @@ 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
+
+            written_raw = serialized_entry.equal?(entry)
+            if raw != written_raw
+              ActiveSupport::Deprecation.warn(<<-MSG.squish)
+                Using a different value for the raw option when reading and writing
+                to a cache key is deprecated for :redis_cache_store and Rails 6.0
+                will stop automatically detecting the format when reading to avoid
+                marshal loading untrusted raw strings.
+              MSG
+            end
+
             entry.is_a?(Entry) ? entry : Entry.new(entry)
           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

@@ -392,8 +392,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)
@@ -417,7 +417,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".dup.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".dup.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

@@ -106,7 +106,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
 
@@ -115,7 +115,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
 
@@ -133,9 +133,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

@@ -14,9 +14,10 @@
 # Emulates a latency on Redis's back-end for the key latency to facilitate
 # connection pool testing.
 class SlowRedis < Redis
-  def get(key, options = {})
+  def get(key)
     if key =~ /latency/
       sleep 3
+      super
     else
       super
     end