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

4 files changed, 69 insertions, 4 deletions

diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb
index 47ea8a3c9b..c6db6685e4 100644
--- a/actionpack/lib/action_view/template/resolver.rb
+++ b/actionpack/lib/action_view/template/resolver.rb
@@ -110,6 +110,9 @@ module ActionView
       super()
     end
 
+    cattr_accessor :allow_external_files, instance_reader: false, instance_writer: false
+    self.allow_external_files = false
+
     private
 
     def find_templates(name, prefix, partial, details)
@@ -122,6 +125,10 @@ module ActionView
 
       template_paths = find_template_paths query
 
+      unless self.class.allow_external_files
+        template_paths = reject_files_external_to_app(template_paths)
+      end
+
       template_paths.map { |template|
         handler, format = extract_handler_and_format(template, formats)
         contents = File.binread template
@@ -133,6 +140,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|
@@ -153,6 +164,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/actionpack/test/controller/new_base/render_file_test.rb b/actionpack/test/controller/new_base/render_file_test.rb
index a961cbf849..c0e23db457 100644
--- a/actionpack/test/controller/new_base/render_file_test.rb
+++ b/actionpack/test/controller/new_base/render_file_test.rb
@@ -72,13 +72,23 @@ module RenderFile
     end
 
     test "rendering a relative path" do
-      get :relative_path
-      assert_response "The secret is in the sauce\n"
+      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
-      get :relative_path_with_dot
-      assert_response "The secret is in the sauce\n"
+      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
diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
index 3964540def..6210524cef 100644
--- a/actionpack/test/controller/render_test.rb
+++ b/actionpack/test/controller/render_test.rb
@@ -60,6 +60,16 @@ class TestController < ActionController::Base
     end
   end
 
+  def dynamic_render
+    render params[:id] # => String, Hash
+  end
+
+  def dynamic_render_with_file
+    # This is extremely bad, but should be possible to do.
+    file = params[:id] # => String, Hash
+    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'
@@ -235,6 +245,27 @@ 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
+    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 accessing_controller_name_in_template
     render :inline =>  "<%= controller_name %>"
   end
diff --git a/actionpack/test/template/render_test.rb b/actionpack/test/template/render_test.rb
index 03f3a34681..f207ad731f 100644
--- a/actionpack/test/template/render_test.rb
+++ b/actionpack/test/template/render_test.rb
@@ -126,6 +126,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