From 21158ebd6fe23f777cdd0c0edb3b1b20db1819ff Mon Sep 17 00:00:00 2001
From: Dylan Thacker-Smith <Dylan.Smith@shopify.com>
Date: Sat, 22 Sep 2018 17:57:58 -0400
Subject: [PATCH] activesupport: Avoid Marshal.load on raw cache value in
 MemCacheStore

Dalli is already being used for marshalling, so we should also rely
on it for unmarshalling. Since Dalli tags the cache value as marshalled
it can avoid unmarshalling a raw string which might have come from
an untrusted source.

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]
---
 .../active_support/cache/mem_cache_store.rb   | 14 ++--------
 .../active_support/cache/redis_cache_store.rb | 27 +++++++++++--------
 .../cache_increment_decrement_behavior.rb     | 12 ++++-----
 .../cache/behaviors/cache_store_behavior.rb   |  6 ++---
 .../behaviors/encoded_key_cache_behavior.rb   |  8 +++---
 .../cache/behaviors/local_cache_behavior.rb   | 10 +++----
 .../test/cache/stores/mem_cache_store_test.rb |  4 +--
 .../cache/stores/redis_cache_store_test.rb    |  3 ++-
 8 files changed, 40 insertions(+), 44 deletions(-)

diff --git a/activesupport/lib/active_support/cache/mem_cache_store.rb b/activesupport/lib/active_support/cache/mem_cache_store.rb
index 18a62f52d3..fc1c1c8aa8 100644
--- a/activesupport/lib/active_support/cache/mem_cache_store.rb
+++ b/activesupport/lib/active_support/cache/mem_cache_store.rb
@@ -7,7 +7,6 @@
   raise e
 end
 
-require "active_support/core_ext/marshal"
 require "active_support/core_ext/array/extract_options"
 
 module ActiveSupport
@@ -28,14 +27,6 @@ class MemCacheStore < Store
       # Provide support for 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(entry.value.to_s)
@@ -194,9 +185,8 @@ def normalize_key(key, options)
           key
         end
 
-        def deserialize_entry(raw_value)
-          if raw_value
-            entry = Marshal.load(raw_value) rescue raw_value
+        def deserialize_entry(entry)
+          if entry
             entry.is_a?(Entry) ? entry : Entry.new(entry)
           end
         end
diff --git a/activesupport/lib/active_support/cache/redis_cache_store.rb b/activesupport/lib/active_support/cache/redis_cache_store.rb
index f43989ec44..cf41ef3803 100644
--- a/activesupport/lib/active_support/cache/redis_cache_store.rb
+++ b/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))
@@ -348,7 +340,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
 
@@ -364,6 +357,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) }
 
@@ -373,7 +367,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
@@ -448,9 +442,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
diff --git a/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb b/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb
index 16b7abc679..5ffc08ea3a 100644
--- a/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb
+++ b/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb
@@ -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 @@ def test_increment
 
   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)
diff --git a/activesupport/test/cache/behaviors/cache_store_behavior.rb b/activesupport/test/cache/behaviors/cache_store_behavior.rb
index e4498f3e0e..aab600a90a 100644
--- a/activesupport/test/cache/behaviors/cache_store_behavior.rb
+++ b/activesupport/test/cache/behaviors/cache_store_behavior.rb
@@ -456,8 +456,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)
@@ -481,7 +481,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]
diff --git a/activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb b/activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb
index 842400f4a3..2304ba94c0 100644
--- a/activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb
+++ b/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)
diff --git a/activesupport/test/cache/behaviors/local_cache_behavior.rb b/activesupport/test/cache/behaviors/local_cache_behavior.rb
index baa38ba6ac..97c8e3f247 100644
--- a/activesupport/test/cache/behaviors/local_cache_behavior.rb
+++ b/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
diff --git a/activesupport/test/cache/stores/mem_cache_store_test.rb b/activesupport/test/cache/stores/mem_cache_store_test.rb
index 3917d14d1c..1d32524047 100644
--- a/activesupport/test/cache/stores/mem_cache_store_test.rb
+++ b/activesupport/test/cache/stores/mem_cache_store_test.rb
@@ -67,7 +67,7 @@ def test_raw_values_with_marshal
     cache = ActiveSupport::Cache.lookup_store(*store, raw: true)
     cache.clear
     cache.write("foo", Marshal.dump([]))
-    assert_equal [], cache.read("foo")
+    assert_equal Marshal.dump([]), cache.read("foo")
   end
 
   def test_local_cache_raw_values
@@ -100,7 +100,7 @@ def test_local_cache_raw_values_with_marshal
     cache.clear
     cache.with_local_cache do
       cache.write("foo", Marshal.dump([]))
-      assert_equal [], cache.read("foo")
+      assert_equal Marshal.dump([]), cache.read("foo")
     end
   end
 
diff --git a/activesupport/test/cache/stores/redis_cache_store_test.rb b/activesupport/test/cache/stores/redis_cache_store_test.rb
index bdde24fe3b..2fa997079b 100644
--- a/activesupport/test/cache/stores/redis_cache_store_test.rb
+++ b/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
-- 
2.26.2

