From 5c656a271a890cca4b3d438cc1fc76ff98011cbe Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 20 Jan 2016 10:39:19 -0800 Subject: [PATCH] allow :file to be outside rails root, but anything else must be inside the rails view directory Conflicts: actionpack/test/controller/render_test.rb actionview/lib/action_view/template/resolver.rb CVE-2016-0752 --- actionpack/lib/abstract_controller/rendering.rb | 8 +++++- actionpack/test/controller/render_test.rb | 31 ++++++++++++++++++++++ actionview/lib/action_view/lookup_context.rb | 4 +++ actionview/lib/action_view/path_set.rb | 26 +++++++++++++----- .../lib/action_view/renderer/abstract_renderer.rb | 2 +- .../lib/action_view/renderer/template_renderer.rb | 2 +- actionview/lib/action_view/template/resolver.rb | 25 ++++++++++++++--- actionview/lib/action_view/testing/resolvers.rb | 4 +-- actionview/test/template/render_test.rb | 7 +++++ 9 files changed, 93 insertions(+), 16 deletions(-) diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb index 9d10140..e80d97f 100644 --- a/actionpack/lib/abstract_controller/rendering.rb +++ b/actionpack/lib/abstract_controller/rendering.rb @@ -77,7 +77,13 @@ module AbstractController # render "foo/bar" to render :file => "foo/bar". # :api: plugin def _normalize_args(action=nil, options={}) - if action.is_a? Hash + case action + when ActionController::Parameters + unless action.permitted? + raise ArgumentError, "render parameters are not permitted" + end + action + when Hash action else options diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index 26806fb..17a019e 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -52,6 +52,16 @@ class TestController < ActionController::Base end end + def dynamic_render + render params[:id] # => String, AC:Params + end + + def dynamic_render_with_file + # This is extremely bad, but should be possible to do. + file = params[:id] # => String, AC:Params + render file: file + end + def conditional_hello_with_public_header if stale?(:last_modified => Time.now.utc.beginning_of_day, :etag => [:foo, 123], :public => true) render :action => 'hello_world' @@ -251,6 +261,27 @@ end class ExpiresInRenderTest < ActionController::TestCase tests TestController + 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 + 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' } } + end + end + def test_expires_in_header get :conditional_hello_with_expires_in assert_equal "max-age=60, private", @response.headers["Cache-Control"] diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index 855fed0..93ef701 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -125,6 +125,10 @@ module ActionView end alias :find_template :find + def find_file(name, prefixes = [], partial = false, keys = [], options = {}) + @view_paths.find_file(*args_for_lookup(name, prefixes, partial, keys, options)) + end + def find_all(name, prefixes = [], partial = false, keys = [], options = {}) @view_paths.find_all(*args_for_lookup(name, prefixes, partial, keys, options)) end diff --git a/actionview/lib/action_view/path_set.rb b/actionview/lib/action_view/path_set.rb index 91ee2ea..8d21913 100644 --- a/actionview/lib/action_view/path_set.rb +++ b/actionview/lib/action_view/path_set.rb @@ -46,23 +46,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/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb index 73c19a0..8457008 100644 --- a/actionview/lib/action_view/renderer/abstract_renderer.rb +++ b/actionview/lib/action_view/renderer/abstract_renderer.rb @@ -15,7 +15,7 @@ module ActionView # that new object is called in turn. This abstracts the setup and rendering # into a separate classes for partials and templates. class AbstractRenderer #:nodoc: - delegate :find_template, :template_exists?, :with_fallbacks, :with_layout_format, :formats, :to => :@lookup_context + delegate :find_template, :find_file, :template_exists?, :with_fallbacks, :with_layout_format, :formats, :to => :@lookup_context def initialize(lookup_context) @lookup_context = lookup_context diff --git a/actionview/lib/action_view/renderer/template_renderer.rb b/actionview/lib/action_view/renderer/template_renderer.rb index be17097..66b611d 100644 --- a/actionview/lib/action_view/renderer/template_renderer.rb +++ b/actionview/lib/action_view/renderer/template_renderer.rb @@ -30,7 +30,7 @@ module ActionView elsif options.key?(:html) Template::HTML.new(options[:html], formats.first) 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/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index f1bb47a..8d8a37e 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -112,7 +112,13 @@ module ActionView # Normalizes the arguments and passes it on to find_templates. 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 @@ -173,15 +179,16 @@ module ActionView 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 + template_paths = reject_files_external_to_app(template_paths) unless outside_app_allowed template_paths.map { |template| handler, format, variant = extract_handler_and_format_and_variant(template, formats) @@ -196,6 +203,10 @@ module ActionView } end + def reject_files_external_to_app(files) + files.reject { |filename| !inside_path?(@path, filename) } + end + if RUBY_VERSION >= '2.2.0' def find_template_paths(query) Dir[query].reject { |filename| @@ -216,6 +227,12 @@ module ActionView end end + def inside_path?(path, filename) + filename = File.expand_path(filename) + path = File.join(path, '') + filename.start_with?(path) + end + # Helper for building query glob string based on resolver's pattern. def build_query(path, details) query = @pattern.dup diff --git a/actionview/lib/action_view/testing/resolvers.rb b/actionview/lib/action_view/testing/resolvers.rb index dfb7d46..e88f425 100644 --- a/actionview/lib/action_view/testing/resolvers.rb +++ b/actionview/lib/action_view/testing/resolvers.rb @@ -19,7 +19,7 @@ module ActionView #:nodoc: private - def query(path, exts, formats) + def query(path, exts, formats, _) query = "" EXTENSIONS.each_key do |ext| query << '(' << exts[ext].map {|e| e && Regexp.escape(".#{e}") }.join('|') << '|)' @@ -44,7 +44,7 @@ module ActionView #:nodoc: end class NullResolver < PathResolver - def query(path, exts, formats) + def query(path, exts, formats, _) handler, format, variant = extract_handler_and_format_and_variant(path, formats) [ActionView::Template.new("Template generated by Null Resolver", path, handler, :virtual_path => path, :format => format, :variant => variant)] end diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index 1316f85..caf6d13 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -142,6 +142,13 @@ module RenderTestCases assert_equal "only partial", @view.render("test/partial_only") end + def test_render_outside_path + assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')) + assert_raises ActionView::MissingTemplate do + @view.render(:template => "../\\../test/abstract_unit.rb") + end + end + def test_render_partial assert_equal "only partial", @view.render(:partial => "test/partial_only") end -- 2.2.1