Hi Sylvain, rails maintainers, On Mon, Jun 29, 2020 at 01:06:49PM +0200, Sylvain Beucler wrote: > Hi, > > On 25/06/2020 18:20, Sylvain Beucler wrote: > > On 22/06/2020 13:23, Sylvain Beucler wrote: > >> On 22/06/2020 11:56, Utkarsh Gupta wrote: > >>> On Mon, Jun 22, 2020 at 3:11 PM Sylvain Beucler <b...@beuc.net> wrote: > >>>> Hmm, are you the only active maintainer for rails? > >>> > >>> There are 3 maintainers. CC'ed rails@p.d.o. > >>> However, since you have already worked on preparing the fix for > >>> Jessie, it's much easier on your part to do it for Stretch and Buster. > >>> But that's volunteer work :) > >>> > >>> If you don't want to work, don't :) > >> > >> For rails@d.p.o's info, I explained at: > >> https://lists.debian.org/debian-lts/2020/06/msg00063.html > >> that I prepared the jessie (4.1.8) and stretch (4.2.7.1) updates at: > >> https://www.beuc.net/tmp/debian-lts/rails/ > >> > >> However the buster version (5.2.2.1) is affected by a different set of > >> vulnerabilities, is much closer to bullseye (5.2.4.3), and apparently > >> the update causes new issues. > >> > >> That's why I think it'd make more sense for the rails maintainers to > >> backport the latest bullseye update. > >> > >> Let me know what you plan to do. > >> > >>>> Which security update broke what, exactly? > >>> > >>> The latest security update from 5.2.4.2 to 5.2.4.3, which contained > >>> fixes for CVE-2020-816{2,4,5,6,7}. > >>> JavaScript bundle generation for Activestorage didn't work w/o that > >>> patch. We had to switch to node-babel7 for that. > >> > >> I updated > >> https://wiki.debian.org/LTS/TestSuites/rails > >> accordingly. > >> > >> The stretch updates passes this new test. > >> > >> (Though in this particular case it may have just been due to node-babel > >> changes in unstable since March, e.g. babel7 is pulled through > >> node-regenerator-transform.) > > > > Status update: jessie and stretch are affected by new important > > CVE-2020-8163. > > buster and above not affected. > > Currently waiting for upstream's feedback on a second regression, then > > I'll prepare an update for jessie & stretch. > > https://www.beuc.net/tmp/debian-lts/rails/ is updated. > > Upstream showed little care for 4.x and I don't expect further feedback, > so I went ahead and backported: > https://github.com/rails/rails/commit/d9ff835b99ff3c7567ccde9b1379b4deeabee32f > to fix the regression, including tests. > > Rationale at: > https://github.com/rails/rails/issues/39301#issuecomment-648885623 > > Note: redmine/stretch (< 3.4) was not affected by the regression.
Attaching the debdiff for reference. The changes looks good to me, but I defintively would like to see a second pair of eyes here from the rails maintainers, in particular for CVE-2020-8163, Utkarsh? There is no lost work, but if we want to release a rails update for stretch (before it moves to LTS), we should try to get as well a rails update beeing prepared for buster, Utkarsh you indicated lack of time currently, any one other up from the rails maintainers? Regards, Salvatore
diff -Nru rails-4.2.7.1/debian/changelog rails-4.2.7.1/debian/changelog --- rails-4.2.7.1/debian/changelog 2020-03-22 13:35:32.000000000 +0100 +++ rails-4.2.7.1/debian/changelog 2020-06-29 09:55:00.000000000 +0200 @@ -1,3 +1,14 @@ +rails (2:4.2.7.1-1+deb9u3) stretch-security; urgency=high + + * Non-maintainer upload by the LTS Security Team. + * CVE-2020-8164: possible Strong Parameters Bypass in ActionPack + * CVE-2020-8165: potentially unintended unmarshalling of user-provided + objects in MemCacheStore + * CVE-2020-8163: potential remote code execution of user-provided + local names + + -- Sylvain Beucler <b...@debian.org> Mon, 29 Jun 2020 09:55:00 +0200 + rails (2:4.2.7.1-1+deb9u2) stretch; urgency=high * Team upload. diff -Nru rails-4.2.7.1/debian/patches/CVE-2020-8163.patch rails-4.2.7.1/debian/patches/CVE-2020-8163.patch --- rails-4.2.7.1/debian/patches/CVE-2020-8163.patch 1970-01-01 01:00:00.000000000 +0100 +++ rails-4.2.7.1/debian/patches/CVE-2020-8163.patch 2020-06-29 09:55:00.000000000 +0200 @@ -0,0 +1,96 @@ +Origin: https://github.com/rails/rails/commit/4c46a15e0a7815ca9e4cd7c7fda042eb8c1b7724 +Origin: https://github.com/rails/rails/commit/d9ff835b99ff3c7567ccde9b1379b4deeabee32f +Last-Update: 2029-06-29 +Reviewed-by: Sylvain Beucler <b...@debian.org> + +Index: rails-4.2.7.1/actionview/lib/action_view/template.rb +=================================================================== +--- rails-4.2.7.1.orig/actionview/lib/action_view/template.rb ++++ rails-4.2.7.1/actionview/lib/action_view/template.rb +@@ -312,8 +312,12 @@ module ActionView + end + + def locals_code #:nodoc: ++ # Only locals with valid variable names get set directly. Others will ++ # still be available in local_assigns. ++ locals = @locals.to_set - Module::RUBY_RESERVED_WORDS ++ locals = locals.grep(/\A(?![A-Z0-9])(?:[[:alnum:]_]|[^\0-\177])+\z/) + # Double assign to suppress the dreaded 'assigned but unused variable' warning +- @locals.each_with_object('') { |key, code| code << "#{key} = #{key} = local_assigns[:#{key}];" } ++ locals.each_with_object('') { |key, code| code << "#{key} = #{key} = local_assigns[:#{key}];" } + end + + def method_name #:nodoc: +Index: rails-4.2.7.1/actionview/test/fixtures/test/render_file_inspect_local_assigns.erb +=================================================================== +--- /dev/null ++++ rails-4.2.7.1/actionview/test/fixtures/test/render_file_inspect_local_assigns.erb +@@ -0,0 +1 @@ ++<%= local_assigns.inspect.html_safe %> +\ No newline at end of file +Index: rails-4.2.7.1/actionview/test/fixtures/test/render_file_unicode_local.erb +=================================================================== +--- /dev/null ++++ rails-4.2.7.1/actionview/test/fixtures/test/render_file_unicode_local.erb +@@ -0,0 +1 @@ ++<%= 🎃 %> +\ No newline at end of file +Index: rails-4.2.7.1/actionview/test/fixtures/test/render_file_with_ruby_keyword_locals.erb +=================================================================== +--- /dev/null ++++ rails-4.2.7.1/actionview/test/fixtures/test/render_file_with_ruby_keyword_locals.erb +@@ -0,0 +1 @@ ++The class is <%= local_assigns[:class] %> +\ No newline at end of file +Index: rails-4.2.7.1/actionview/test/fixtures/test/test_template_with_delegation_reserved_keywords.erb +=================================================================== +--- /dev/null ++++ rails-4.2.7.1/actionview/test/fixtures/test/test_template_with_delegation_reserved_keywords.erb +@@ -0,0 +1 @@ ++<%= _ %> <%= arg %> <%= args %> <%= block %> +\ No newline at end of file +Index: rails-4.2.7.1/actionview/test/template/compiled_templates_test.rb +=================================================================== +--- rails-4.2.7.1.orig/actionview/test/template/compiled_templates_test.rb ++++ rails-4.2.7.1/actionview/test/template/compiled_templates_test.rb +@@ -1,3 +1,4 @@ ++# coding: utf-8 + require 'abstract_unit' + + class CompiledTemplatesTest < ActiveSupport::TestCase +@@ -9,6 +10,35 @@ class CompiledTemplatesTest < ActiveSupp + assert_equal "This is nil: \n", render(:template => "test/nil_return") + end + ++ def test_template_with_ruby_keyword_locals ++ assert_equal "The class is foo", ++ render(file: "test/render_file_with_ruby_keyword_locals", locals: { class: "foo" }) ++ end ++ ++ def test_template_with_invalid_identifier_locals ++ locals = { ++ foo: "bar", ++ Foo: "bar", ++ "d-a-s-h-e-s": "", ++ "white space": "", ++ } ++ assert_equal locals.inspect, render(file: "test/render_file_inspect_local_assigns", locals: locals) ++ end ++ ++ def test_template_with_delegation_reserved_keywords ++ locals = { ++ _: "one", ++ arg: "two", ++ args: "three", ++ block: "four", ++ } ++ assert_equal "one two three four", render(file: "test/test_template_with_delegation_reserved_keywords", locals: locals) ++ end ++ ++ def test_template_with_unicode_identifier ++ assert_equal "🎂", render(file: "test/render_file_unicode_local", locals: { 🎃: "🎂" }) ++ end ++ + def test_template_gets_recompiled_when_using_different_keys_in_local_assigns + assert_equal "one", render(:file => "test/render_file_with_locals_and_default") + assert_equal "two", render(:file => "test/render_file_with_locals_and_default", :locals => { :secret => "two" }) diff -Nru rails-4.2.7.1/debian/patches/CVE-2020-8164.patch rails-4.2.7.1/debian/patches/CVE-2020-8164.patch --- rails-4.2.7.1/debian/patches/CVE-2020-8164.patch 1970-01-01 01:00:00.000000000 +0100 +++ rails-4.2.7.1/debian/patches/CVE-2020-8164.patch 2020-06-18 18:07:48.000000000 +0200 @@ -0,0 +1,48 @@ +Origin: https://github.com/rails/rails/commit/7a3ee4fea90b7555f8d09c6c05c15fe7ab5a06ec +Last-Update: 2029-06-18 +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(+) + +Index: rails-4.2.7.1/actionpack/lib/action_controller/metal/strong_parameters.rb +=================================================================== +--- rails-4.2.7.1.orig/actionpack/lib/action_controller/metal/strong_parameters.rb ++++ rails-4.2.7.1/actionpack/lib/action_controller/metal/strong_parameters.rb +@@ -400,6 +400,8 @@ module ActionController + else + super + end ++ ++ self + end + + # This method is here only to make sure that the returned object has the +Index: rails-4.2.7.1/actionpack/test/controller/parameters/accessors_test.rb +=================================================================== +--- rails-4.2.7.1.orig/actionpack/test/controller/parameters/accessors_test.rb ++++ rails-4.2.7.1/actionpack/test/controller/parameters/accessors_test.rb +@@ -16,6 +16,14 @@ class ParametersAccessorsTest < ActiveSu + ) + 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 @params[:person].permitted? diff -Nru rails-4.2.7.1/debian/patches/CVE-2020-8165.patch rails-4.2.7.1/debian/patches/CVE-2020-8165.patch --- rails-4.2.7.1/debian/patches/CVE-2020-8165.patch 1970-01-01 01:00:00.000000000 +0100 +++ rails-4.2.7.1/debian/patches/CVE-2020-8165.patch 2020-06-18 18:13:17.000000000 +0200 @@ -0,0 +1,82 @@ +Origin: https://github.com/rails/rails/commit/f7e077f85e61fc0b7381963eda0ceb0e457546b5 +Last-Update: 2029-06-18 +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] +--- + .../lib/active_support/cache/mem_cache_store.rb | 14 ++------------ + .../test/cache/stores/mem_cache_store_test.rb | 4 ++-- + 2 files changed, 4 insertions(+), 14 deletions(-) + +Index: rails-4.2.7.1/activesupport/lib/active_support/cache/mem_cache_store.rb +=================================================================== +--- rails-4.2.7.1.orig/activesupport/lib/active_support/cache/mem_cache_store.rb ++++ rails-4.2.7.1/activesupport/lib/active_support/cache/mem_cache_store.rb +@@ -6,7 +6,6 @@ rescue LoadError => e + end + + require 'digest/md5' +-require 'active_support/core_ext/marshal' + require 'active_support/core_ext/array/extract_options' + + module ActiveSupport +@@ -163,9 +162,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) + else + nil +@@ -175,14 +173,6 @@ module ActiveSupport + # Provide support for raw values in the local cache strategy. + module LocalCacheWithRaw # :nodoc: + protected +- 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) # :nodoc: + retval = super + if options[:raw] && local_cache && retval +Index: rails-4.2.7.1/activesupport/test/caching_test.rb +=================================================================== +--- rails-4.2.7.1.orig/activesupport/test/caching_test.rb ++++ rails-4.2.7.1/activesupport/test/caching_test.rb +@@ -928,7 +928,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 +@@ -945,7 +945,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 + diff -Nru rails-4.2.7.1/debian/patches/series rails-4.2.7.1/debian/patches/series --- rails-4.2.7.1/debian/patches/series 2020-03-22 13:34:25.000000000 +0100 +++ rails-4.2.7.1/debian/patches/series 2020-06-29 09:50:22.000000000 +0200 @@ -5,3 +5,6 @@ 006-CVE-2018-16476.patch 007-CVE-2019-5418_CVE-2019-5419.patch CVE-2020-5267.patch +CVE-2020-8164.patch +CVE-2020-8165.patch +CVE-2020-8163.patch
