Hi, On 10/07/2020 10:28, Moritz Mühlenhoff wrote: > On Wed, Jul 08, 2020 at 12:45:08PM +0200, Sylvain Beucler wrote: >> Hi, >> >> - buster update >> >> I now "up-ported" my stretch work at: >> https://www.beuc.net/tmp/debian-lts/rails-buster/ >> + added the redis side of CVE-2020-8165 > > What do you mean with up-ported? Applying a patch made for an older release > to a more recent release will miss all code which wasn't present in > the older suite.
To phrase it more precisely, I went back to the upstream patches for 5.2, applied them and unit-tested them. (debdiff.txt from the above URL attached for reference.) Cheers! Sylvain
diff -Nru rails-5.2.2.1+dfsg/debian/changelog rails-5.2.2.1+dfsg/debian/changelog --- rails-5.2.2.1+dfsg/debian/changelog 2020-03-22 14:17:31.000000000 +0100 +++ rails-5.2.2.1+dfsg/debian/changelog 2020-07-08 11:38:00.000000000 +0200 @@ -1,3 +1,12 @@ +rails (2:5.2.2.1+dfsg-1+deb10u2) UNRELEASED; urgency=high + + [ Sylvain Beucler ] + * CVE-2020-8164: possible Strong Parameters Bypass in ActionPack + * CVE-2020-8165: potentially unintended unmarshalling of user-provided + objects in MemCacheStore and RedisCacheStore + + -- debian <debian@buster> Wed, 08 Jul 2020 11:38:00 +0200 + rails (2:5.2.2.1+dfsg-1+deb10u1) buster; urgency=high * Team upload. diff -Nru rails-5.2.2.1+dfsg/debian/patches/CVE-2020-8164.patch rails-5.2.2.1+dfsg/debian/patches/CVE-2020-8164.patch --- rails-5.2.2.1+dfsg/debian/patches/CVE-2020-8164.patch 1970-01-01 01:00:00.000000000 +0100 +++ rails-5.2.2.1+dfsg/debian/patches/CVE-2020-8164.patch 2020-07-08 11:38:00.000000000 +0200 @@ -0,0 +1,48 @@ +Origin: https://github.com/rails/rails/commit/7a3ee4fea90b7555f8d09c6c05c15fe7ab5a06ec +Last-Update: 2029-07-08 +Reviewed-by: Sylvain Beucler <b...@debian.org> + +From 7a3ee4fea90b7555f8d09c6c05c15fe7ab5a06ec Mon Sep 17 00:00:00 2001 +From: Jack McCracken <jack.mccrac...@shopify.com> +Date: Wed, 13 May 2020 15:25:12 -0400 +Subject: [PATCH] Return self when calling #each, #each_pair, and #each_value + instead of the raw @parameters hash + +[CVE-2020-8164] +--- + .../lib/action_controller/metal/strong_parameters.rb | 2 ++ + actionpack/test/controller/parameters/accessors_test.rb | 8 ++++++++ + 2 files changed, 10 insertions(+) + +diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb +index 510cb353b493..8532b21d94b0 100644 +--- a/actionpack/lib/action_controller/metal/strong_parameters.rb ++++ b/actionpack/lib/action_controller/metal/strong_parameters.rb +@@ -337,6 +337,8 @@ def each_pair(&block) + @parameters.each_pair do |key, value| + yield [key, convert_hashes_to_parameters(key, value)] + end ++ ++ self + end + alias_method :each, :each_pair + +diff --git a/actionpack/test/controller/parameters/accessors_test.rb b/actionpack/test/controller/parameters/accessors_test.rb +index db9359876c9a..25a9cee0109e 100644 +--- a/actionpack/test/controller/parameters/accessors_test.rb ++++ b/actionpack/test/controller/parameters/accessors_test.rb +@@ -20,6 +20,14 @@ class ParametersAccessorsTest < ActiveSupport::TestCase + ) + end + ++ test "each returns self" do ++ assert_same @params, @params.each { |_| _ } ++ end ++ ++ test "each_pair returns self" do ++ assert_same @params, @params.each_pair { |_| _ } ++ end ++ + test "[] retains permitted status" do + @params.permit! + assert_predicate @params[:person], :permitted? diff -Nru rails-5.2.2.1+dfsg/debian/patches/CVE-2020-8165.patch rails-5.2.2.1+dfsg/debian/patches/CVE-2020-8165.patch --- rails-5.2.2.1+dfsg/debian/patches/CVE-2020-8165.patch 1970-01-01 01:00:00.000000000 +0100 +++ rails-5.2.2.1+dfsg/debian/patches/CVE-2020-8165.patch 2020-07-08 11:38:00.000000000 +0200 @@ -0,0 +1,293 @@ +Origin: https://github.com/rails/rails/commit/f7e077f85e61fc0b7381963eda0ceb0e457546b5 +Origin: https://github.com/rails/rails/commit/467e3399c9007996c03ffe3212689d48dd25ae99 +Last-Update: 2029-07-08 +Reviewed-by: Sylvain Beucler <b...@debian.org> + +From f7e077f85e61fc0b7381963eda0ceb0e457546b5 Mon Sep 17 00:00:00 2001 +From: Dylan Thacker-Smith <dylan.sm...@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. + +[CVE-2020-8165] + +From 467e3399c9007996c03ffe3212689d48dd25ae99 Mon Sep 17 00:00:00 2001 +From: Dylan Thacker-Smith <dylan.sm...@shopify.com> +Date: Sat, 22 Sep 2018 21:17:07 -0400 +Subject: [PATCH] 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] + +Index: rails-5.2.2.1+dfsg/activesupport/lib/active_support/cache/mem_cache_store.rb +=================================================================== +--- rails-5.2.2.1+dfsg.orig/activesupport/lib/active_support/cache/mem_cache_store.rb ++++ rails-5.2.2.1+dfsg/activesupport/lib/active_support/cache/mem_cache_store.rb +@@ -7,7 +7,6 @@ rescue LoadError => e + raise e + end + +-require "active_support/core_ext/marshal" + require "active_support/core_ext/array/extract_options" + + module ActiveSupport +@@ -28,14 +27,6 @@ module ActiveSupport + # 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) +@@ -189,9 +180,8 @@ module ActiveSupport + 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 +Index: rails-5.2.2.1+dfsg/activesupport/test/cache/stores/mem_cache_store_test.rb +=================================================================== +--- rails-5.2.2.1+dfsg.orig/activesupport/test/cache/stores/mem_cache_store_test.rb ++++ rails-5.2.2.1+dfsg/activesupport/test/cache/stores/mem_cache_store_test.rb +@@ -66,7 +66,7 @@ class MemCacheStoreTest < ActiveSupport: + cache = ActiveSupport::Cache.lookup_store(:mem_cache_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 +@@ -99,7 +99,7 @@ class MemCacheStoreTest < ActiveSupport: + 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 + +Index: rails-5.2.2.1+dfsg/activesupport/lib/active_support/cache/redis_cache_store.rb +=================================================================== +--- rails-5.2.2.1+dfsg.orig/activesupport/lib/active_support/cache/redis_cache_store.rb ++++ rails-5.2.2.1+dfsg/activesupport/lib/active_support/cache/redis_cache_store.rb +@@ -70,14 +70,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)) +@@ -328,7 +320,8 @@ module ActiveSupport + # 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 @@ module ActiveSupport + 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 @@ 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 +@@ -421,9 +415,20 @@ 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 ++ ++ 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 +Index: rails-5.2.2.1+dfsg/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb +=================================================================== +--- rails-5.2.2.1+dfsg.orig/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb ++++ rails-5.2.2.1+dfsg/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 @@ 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) +Index: rails-5.2.2.1+dfsg/activesupport/test/cache/behaviors/cache_store_behavior.rb +=================================================================== +--- rails-5.2.2.1+dfsg.orig/activesupport/test/cache/behaviors/cache_store_behavior.rb ++++ rails-5.2.2.1+dfsg/activesupport/test/cache/behaviors/cache_store_behavior.rb +@@ -392,8 +392,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) +@@ -417,7 +417,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] +Index: rails-5.2.2.1+dfsg/activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb +=================================================================== +--- rails-5.2.2.1+dfsg.orig/activesupport/test/cache/behaviors/encoded_key_cache_behavior.rb ++++ rails-5.2.2.1+dfsg/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) +Index: rails-5.2.2.1+dfsg/activesupport/test/cache/behaviors/local_cache_behavior.rb +=================================================================== +--- rails-5.2.2.1+dfsg.orig/activesupport/test/cache/behaviors/local_cache_behavior.rb ++++ rails-5.2.2.1+dfsg/activesupport/test/cache/behaviors/local_cache_behavior.rb +@@ -106,7 +106,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 + +@@ -115,7 +115,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 + +@@ -133,9 +133,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 +Index: rails-5.2.2.1+dfsg/activesupport/test/cache/stores/redis_cache_store_test.rb +=================================================================== +--- rails-5.2.2.1+dfsg.orig/activesupport/test/cache/stores/redis_cache_store_test.rb ++++ rails-5.2.2.1+dfsg/activesupport/test/cache/stores/redis_cache_store_test.rb +@@ -14,9 +14,10 @@ Redis::Connection.drivers.append(driver) + # 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 diff -Nru rails-5.2.2.1+dfsg/debian/patches/series rails-5.2.2.1+dfsg/debian/patches/series --- rails-5.2.2.1+dfsg/debian/patches/series 2020-03-22 14:16:39.000000000 +0100 +++ rails-5.2.2.1+dfsg/debian/patches/series 2020-07-08 11:38:00.000000000 +0200 @@ -1,3 +1,5 @@ 0001-Be-careful-with-that-bundler.patch 0002-disable-uglify-in-activestorage-rollup-config-js.patch CVE-2020-5267.patch +CVE-2020-8164.patch +CVE-2020-8165.patch
