From af9b9132f82d1f468836997c716a02f14e61c38c Mon Sep 17 00:00:00 2001 From: Arthur Neves Date: Tue, 2 Feb 2016 12:34:11 -0500 Subject: Complete work on 3.2 for render_data_leak patch. Render could leak access to external files before this patch. A previous patch(CVE-2016-0752), attempted to fix this. However the tests were miss-placed outside the TestCase subclass, so they were not running. We should allow :file to be outside rails root, but anything else must be inside the rails view directory. The implementation has changed a bit though. Now the patch is more similar with the 4.x series patches. Now `render 'foo/bar'`, will add a special key in the options hash, and not use the :file one, so when we look up that file, we don't set the fallbacks, and only lookup a template, to constraint the folders that can be accessed. CVE-2016-2097 --- .../test/controller/new_base/render_file_test.rb | 49 ------------------- actionpack/test/controller/render_test.rb | 56 +++++++++++----------- 2 files changed, 29 insertions(+), 76 deletions(-) (limited to 'actionpack/test') diff --git a/actionpack/test/controller/new_base/render_file_test.rb b/actionpack/test/controller/new_base/render_file_test.rb index c0e23db457..3fdf7a67c0 100644 --- a/actionpack/test/controller/new_base/render_file_test.rb +++ b/actionpack/test/controller/new_base/render_file_test.rb @@ -13,15 +13,6 @@ module RenderFile render :file => File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_ivar') end - def without_file_key - render File.join(File.dirname(__FILE__), *%w[.. .. fixtures test hello_world]) - end - - def without_file_key_with_instance_variable - @secret = 'in the sauce' - render File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_ivar') - end - def relative_path @secret = 'in the sauce' render :file => '../../fixtures/test/render_file_with_ivar' @@ -41,11 +32,6 @@ module RenderFile path = File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_locals') render :file => path, :locals => {:secret => 'in the sauce'} end - - def without_file_key_with_locals - path = FIXTURES.join('test/render_file_with_locals').to_s - render path, :locals => {:secret => 'in the sauce'} - end end class TestBasic < Rack::TestCase @@ -61,36 +47,6 @@ module RenderFile assert_response "The secret is in the sauce\n" end - test "rendering path without specifying the :file key" do - get :without_file_key - assert_response "Hello world!" - end - - test "rendering path without specifying the :file key with ivar" do - get :without_file_key_with_instance_variable - assert_response "The secret is in the sauce\n" - end - - test "rendering a relative path" do - begin - ActionView::PathResolver.allow_external_files = true - get :relative_path - assert_response "The secret is in the sauce\n" - ensure - ActionView::PathResolver.allow_external_files = false - end - end - - test "rendering a relative path with dot" do - begin - ActionView::PathResolver.allow_external_files = true - get :relative_path_with_dot - assert_response "The secret is in the sauce\n" - ensure - ActionView::PathResolver.allow_external_files = false - end - end - test "rendering a Pathname" do get :pathname assert_response "The secret is in the sauce\n" @@ -100,10 +56,5 @@ module RenderFile get :with_locals assert_response "The secret is in the sauce\n" end - - test "rendering path without specifying the :file key with locals" do - get :without_file_key_with_locals - assert_response "The secret is in the sauce\n" - end end end diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index 355523293f..bc42c6614a 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -245,33 +245,6 @@ class TestController < ActionController::Base render :inline => "<%= action_name %>" end - def test_dynamic_render_with_file - # This is extremely bad, but should be possible to do. - assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')) - response = get :dynamic_render_with_file, { :id => '../\\../test/abstract_unit.rb' } - assert_equal File.read(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')), - response.body - end - - def test_dynamic_render_with_absolute_path - file = Tempfile.new('name') - file.write "secrets!" - file.flush - assert_raises ActionView::MissingTemplate do - response = get :dynamic_render, { :id => file.path } - end - ensure - file.close - file.unlink - end - - def test_dynamic_render - assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')) - assert_raises ActionView::MissingTemplate do - get :dynamic_render, { :id => '../\\../test/abstract_unit.rb' } - end - end - def test_dynamic_render_file_hash assert_raises ArgumentError do get :dynamic_render, { :id => { :file => '../\\../test/abstract_unit.rb' } } @@ -779,6 +752,35 @@ class RenderTest < ActionController::TestCase @controller.logger = Logger.new(nil) @request.host = "www.nextangle.com" + ActionController::Base.view_paths.paths.each(&:clear_cache) + end + + def test_dynamic_render_with_file + # This is extremely bad, but should be possible to do. + assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')) + response = get :dynamic_render_with_file, { :id => '../\\../test/abstract_unit.rb' } + assert_equal File.read(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')), + response.body + end + + # :ported: + def test_dynamic_render_with_absolute_path + file = Tempfile.new('name') + file.write "secrets!" + file.flush + assert_raises ActionView::MissingTemplate do + get :dynamic_render, { :id => file.path } + end + ensure + file.close + file.unlink + end + + def test_dynamic_render + assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')) + assert_raises ActionView::MissingTemplate do + get :dynamic_render, { :id => '../\\../test/abstract_unit.rb' } + end end # :ported: -- cgit v1.2.3