diff options
author | Arthur Neves <arthurnn@gmail.com> | 2016-02-02 12:34:11 -0500 |
---|---|---|
committer | Rafael Mendonça França <rafaelmfranca@gmail.com> | 2016-02-29 15:39:02 -0300 |
commit | af9b9132f82d1f468836997c716a02f14e61c38c (patch) | |
tree | 206e4b89c5486826efdd5223f17a8767e047cc48 /actionpack | |
parent | 9892626579d1c62c367e5344a1d1642708340f88 (diff) | |
download | rails-af9b9132f82d1f468836997c716a02f14e61c38c.tar.gz rails-af9b9132f82d1f468836997c716a02f14e61c38c.tar.bz2 rails-af9b9132f82d1f468836997c716a02f14e61c38c.zip |
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
Diffstat (limited to 'actionpack')
-rw-r--r-- | actionpack/lib/abstract_controller/rendering.rb | 3 | ||||
-rw-r--r-- | actionpack/lib/action_view/lookup_context.rb | 4 | ||||
-rw-r--r-- | actionpack/lib/action_view/path_set.rb | 26 | ||||
-rw-r--r-- | actionpack/lib/action_view/renderer/abstract_renderer.rb | 2 | ||||
-rw-r--r-- | actionpack/lib/action_view/renderer/template_renderer.rb | 5 | ||||
-rw-r--r-- | actionpack/lib/action_view/template/resolver.rb | 32 | ||||
-rw-r--r-- | actionpack/lib/action_view/testing/resolvers.rb | 5 | ||||
-rw-r--r-- | actionpack/test/controller/new_base/render_file_test.rb | 49 | ||||
-rw-r--r-- | actionpack/test/controller/render_test.rb | 56 |
9 files changed, 79 insertions, 103 deletions
diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb index f74fd6ac4a..5b45a13269 100644 --- a/actionpack/lib/abstract_controller/rendering.rb +++ b/actionpack/lib/abstract_controller/rendering.rb @@ -147,12 +147,11 @@ module AbstractController options = action when String, Symbol action = action.to_s - key = action.include?(?/) ? :file : :action + key = action.include?(?/) ? :app_template_file : :action options[key] = action else options[:partial] = action end - options end diff --git a/actionpack/lib/action_view/lookup_context.rb b/actionpack/lib/action_view/lookup_context.rb index 9331d13577..91ca4a7c93 100644 --- a/actionpack/lib/action_view/lookup_context.rb +++ b/actionpack/lib/action_view/lookup_context.rb @@ -127,6 +127,10 @@ module ActionView @view_paths.find_all(*args_for_lookup(name, prefixes, partial, keys, options)) end + def find_file(name, prefixes = [], partial = false, keys = [], options = {}) + @view_paths.find_file(*args_for_lookup(name, prefixes, partial, keys, options)) + end + def exists?(name, prefixes = [], partial = false, keys = [], options = {}) @view_paths.exists?(*args_for_lookup(name, prefixes, partial, keys, options)) end diff --git a/actionpack/lib/action_view/path_set.rb b/actionpack/lib/action_view/path_set.rb index bbb1af8154..1c54a970cb 100644 --- a/actionpack/lib/action_view/path_set.rb +++ b/actionpack/lib/action_view/path_set.rb @@ -58,23 +58,35 @@ module ActionView #:nodoc: find_all(*args).first || raise(MissingTemplate.new(self, *args)) end + def find_file(path, prefixes = [], *args) + _find_all(path, prefixes, args, true).first || raise(MissingTemplate.new(self, path, prefixes, *args)) + end + def find_all(path, prefixes = [], *args) + _find_all path, prefixes, args, false + end + + def exists?(path, prefixes, *args) + find_all(path, prefixes, *args).any? + end + + private + + def _find_all(path, prefixes, args, outside_app) prefixes = [prefixes] if String === prefixes prefixes.each do |prefix| paths.each do |resolver| - templates = resolver.find_all(path, prefix, *args) + if outside_app + templates = resolver.find_all_anywhere(path, prefix, *args) + else + templates = resolver.find_all(path, prefix, *args) + end return templates unless templates.empty? end end [] end - def exists?(path, prefixes, *args) - find_all(path, prefixes, *args).any? - end - - private - def typecast(paths) paths.map do |path| case path diff --git a/actionpack/lib/action_view/renderer/abstract_renderer.rb b/actionpack/lib/action_view/renderer/abstract_renderer.rb index 0b5d3785d4..021fb198a9 100644 --- a/actionpack/lib/action_view/renderer/abstract_renderer.rb +++ b/actionpack/lib/action_view/renderer/abstract_renderer.rb @@ -1,6 +1,6 @@ module ActionView class AbstractRenderer #:nodoc: - delegate :find_template, :template_exists?, :with_fallbacks, :update_details, + delegate :find_template, :find_file, :template_exists?, :with_fallbacks, :update_details, :with_layout_format, :formats, :to => :@lookup_context def initialize(lookup_context) diff --git a/actionpack/lib/action_view/renderer/template_renderer.rb b/actionpack/lib/action_view/renderer/template_renderer.rb index a27d5dd1b1..578db362f6 100644 --- a/actionpack/lib/action_view/renderer/template_renderer.rb +++ b/actionpack/lib/action_view/renderer/template_renderer.rb @@ -21,11 +21,12 @@ module ActionView # Determine the template to be rendered using the given options. def determine_template(options) #:nodoc: keys = options[:locals].try(:keys) || [] - if options.key?(:text) Template::Text.new(options[:text], formats.try(:first)) + elsif options.key?(:app_template_file) + find_template(options[:app_template_file], nil, false, keys, @details) elsif options.key?(:file) - with_fallbacks { find_template(options[:file], nil, false, keys, @details) } + with_fallbacks { find_file(options[:file], nil, false, keys, @details) } elsif options.key?(:inline) handler = Template.handler_for_extension(options[:type] || "erb") Template.new(options[:inline], "inline template", handler, :locals => keys) diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb index d3a6d1a3b8..bea8c221c3 100644 --- a/actionpack/lib/action_view/template/resolver.rb +++ b/actionpack/lib/action_view/template/resolver.rb @@ -43,7 +43,13 @@ module ActionView # Normalizes the arguments and passes it on to find_template. def find_all(name, prefix=nil, partial=false, details={}, key=nil, locals=[]) cached(key, [name, prefix, partial], details, locals) do - find_templates(name, prefix, partial, details) + find_templates(name, prefix, partial, details, false) + end + end + + def find_all_anywhere(name, prefix, partial=false, details={}, key=nil, locals=[]) + cached(key, [name, prefix, partial], details, locals) do + find_templates(name, prefix, partial, details, true) end end @@ -54,8 +60,8 @@ module ActionView # This is what child classes implement. No defaults are needed # because Resolver guarantees that the arguments are present and # normalized. - def find_templates(name, prefix, partial, details) - raise NotImplementedError, "Subclasses must implement a find_templates(name, prefix, partial, details) method" + def find_templates(name, prefix, partial, details, outside_app_allowed = false) + raise NotImplementedError, "Subclasses must implement a find_templates(name, prefix, partial, details, outside_app_allowed) method" end # Helpers that builds a path. Useful for building virtual paths. @@ -110,24 +116,21 @@ module ActionView super() end - cattr_accessor :allow_external_files, :instance_reader => false, :instance_writer => false - self.allow_external_files = false + cattr_accessor :instance_reader => false, :instance_writer => false private - def find_templates(name, prefix, partial, details) + def find_templates(name, prefix, partial, details, outside_app_allowed = false) path = Path.build(name, prefix, partial) - query(path, details, details[:formats]) + query(path, details, details[:formats], outside_app_allowed) end - def query(path, details, formats) + def query(path, details, formats, outside_app_allowed) query = build_query(path, details) template_paths = find_template_paths query - unless self.class.allow_external_files - template_paths = reject_files_external_to_app(template_paths) - end + template_paths = reject_files_external_to_app(template_paths) unless outside_app_allowed template_paths.map { |template| handler, format = extract_handler_and_format(template, formats) @@ -267,7 +270,12 @@ module ActionView class OptimizedFileSystemResolver < FileSystemResolver #:nodoc: def build_query(path, details) exts = EXTENSIONS.map { |ext| details[ext] } - query = escape_entry(File.join(@path, path)) + + if path.to_s.starts_with? @path.to_s + query = escape_entry(path) + else + query = escape_entry(File.join(@path, path)) + end query + exts.map { |ext| "{#{ext.compact.uniq.map { |e| ".#{e}," }.join}}" diff --git a/actionpack/lib/action_view/testing/resolvers.rb b/actionpack/lib/action_view/testing/resolvers.rb index 7afa2fa613..d552151a1d 100644 --- a/actionpack/lib/action_view/testing/resolvers.rb +++ b/actionpack/lib/action_view/testing/resolvers.rb @@ -19,7 +19,7 @@ module ActionView #:nodoc: private - def query(path, exts, formats) + def query(path, exts, formats, outside_app_allowed) query = "" EXTENSIONS.each do |ext| query << '(' << exts[ext].map {|e| e && Regexp.escape(".#{e}") }.join('|') << '|)' @@ -40,11 +40,10 @@ module ActionView #:nodoc: end class NullResolver < PathResolver - def query(path, exts, formats) + def query(path, exts, formats, outside_app_allowed) handler, format = extract_handler_and_format(path, formats) [ActionView::Template.new("Template generated by Null Resolver", path, handler, :virtual_path => path, :format => format)] end end end - 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: |