Re: rails update
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 <beuc@debian.org>
+
+From 7a3ee4fea90b7555f8d09c6c05c15fe7ab5a06ec Mon Sep 17 00:00:00 2001
+From: Jack McCracken <jack.mccracken@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 <beuc@debian.org>
+
+From f7e077f85e61fc0b7381963eda0ceb0e457546b5 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.
+
+[CVE-2020-8165]
+
+From 467e3399c9007996c03ffe3212689d48dd25ae99 Mon Sep 17 00:00:00 2001
+From: Dylan Thacker-Smith <Dylan.Smith@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
Reply to: