From e836024dd9dac4083aa6a735e5dbecca5a907d3a Mon Sep 17 00:00:00 2001 From: Evan Huus Date: Fri, 12 Sep 2014 17:34:33 +0000 Subject: [PATCH 1/4] Check and handle when a tainted variable is used --- lib/liquid/errors.rb | 1 + lib/liquid/template.rb | 10 ++++++++++ lib/liquid/variable.rb | 10 ++++++++++ 3 files changed, 21 insertions(+) diff --git a/lib/liquid/errors.rb b/lib/liquid/errors.rb index 8407411..bb9132d 100644 --- a/lib/liquid/errors.rb +++ b/lib/liquid/errors.rb @@ -54,5 +54,6 @@ module Liquid class StandardError < Error; end class SyntaxError < Error; end class StackLevelError < Error; end + class TaintedError < Error; end class MemoryError < Error; end end diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index 392a9f9..20d2a93 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -60,6 +60,12 @@ module Liquid # :strict will enforce correct syntax. attr_writer :error_mode + # Sets how strict the taint checker should be. + # :lax ignores the taint flag completely (like previous liquid versions) + # :warn adds a warning, but does not interrupt the rendering + # :error raises an error when tainted output is used + attr_writer :taint_mode + def file_system @@file_system end @@ -80,6 +86,10 @@ module Liquid @error_mode || :lax end + def taint_mode + @taint_mode || :lax + end + # Pass a module with filter methods which should be available # to all liquid views. Good for registering the standard library def register_filter(mod) diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index 5b5d446..63ca1cd 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -94,6 +94,16 @@ module Liquid end filterargs << keyword_args unless keyword_args.empty? output = context.invoke(filter[0], output, *filterargs) + end.tap do |obj| + if obj.tainted? + case Template.taint_mode + when :warn + @warnings ||= [] + @warnings << "variable '#{@name}' is tainted and was not escaped" + when :error + raise TaintedError, "Error - variable '#{@name}' is tainted and was not escaped" + end + end end end end From 1d151885beb2de36ae69efec9e27a4881ee5d814 Mon Sep 17 00:00:00 2001 From: Evan Huus Date: Fri, 12 Sep 2014 17:34:53 +0000 Subject: [PATCH 2/4] Auto-untaint variables passed through 'escape' --- lib/liquid/standardfilters.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/liquid/standardfilters.rb b/lib/liquid/standardfilters.rb index 8cf51ea..bbbda38 100644 --- a/lib/liquid/standardfilters.rb +++ b/lib/liquid/standardfilters.rb @@ -34,7 +34,7 @@ module Liquid end def escape(input) - CGI.escapeHTML(input) rescue input + CGI.escapeHTML(input).untaint rescue input end alias_method :h, :escape From 67b2c320a18665ddcc44a299aa50bdd76674fe03 Mon Sep 17 00:00:00 2001 From: Evan Huus Date: Fri, 12 Sep 2014 17:35:10 +0000 Subject: [PATCH 3/4] Add tainting tests --- test/integration/drop_test.rb | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/integration/drop_test.rb b/test/integration/drop_test.rb index 7290298..afeba5e 100644 --- a/test/integration/drop_test.rb +++ b/test/integration/drop_test.rb @@ -48,6 +48,10 @@ class ProductDrop < Liquid::Drop ContextDrop.new end + def user_input + "foo".taint + end + protected def callmenot "protected" @@ -108,6 +112,30 @@ class DropsTest < Minitest::Test assert_equal ' ', tpl.render!('product' => ProductDrop.new) end + def test_rendering_raises_on_tainted_attr + Liquid::Template.taint_mode = :error + tpl = Liquid::Template.parse('{{ product.user_input }}') + assert_raises TaintedError do + tpl.render!('product' => ProductDrop.new) + end + Liquid::Template.taint_mode = :lax + end + + def test_rendering_warns_on_tainted_attr + Liquid::Template.taint_mode = :warn + tpl = Liquid::Template.parse('{{ product.user_input }}') + tpl.render!('product' => ProductDrop.new) + assert_match /tainted/, tpl.warnings.first + Liquid::Template.taint_mode = :lax + end + + def test_rendering_doesnt_raise_on_escaped_tainted_attr + Liquid::Template.taint_mode = :error + tpl = Liquid::Template.parse('{{ product.user_input | escape }}') + tpl.render!('product' => ProductDrop.new) + Liquid::Template.taint_mode = :lax + end + def test_drop_does_only_respond_to_whitelisted_methods assert_equal "", Liquid::Template.parse("{{ product.inspect }}").render!('product' => ProductDrop.new) assert_equal "", Liquid::Template.parse("{{ product.pretty_inspect }}").render!('product' => ProductDrop.new) From eeb061ef44416f3bee71475b918e58b745f6174a Mon Sep 17 00:00:00 2001 From: Evan Huus Date: Mon, 15 Sep 2014 13:32:21 +0000 Subject: [PATCH 4/4] Address code review comments - clean up comment wording - fix potentially leaky tests --- lib/liquid/template.rb | 2 +- test/integration/drop_test.rb | 28 ++++++++++++++-------------- test/test_helper.rb | 8 ++++++++ 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index 20d2a93..8862564 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -61,7 +61,7 @@ module Liquid attr_writer :error_mode # Sets how strict the taint checker should be. - # :lax ignores the taint flag completely (like previous liquid versions) + # :lax is the default, and ignores the taint flag completely # :warn adds a warning, but does not interrupt the rendering # :error raises an error when tainted output is used attr_writer :taint_mode diff --git a/test/integration/drop_test.rb b/test/integration/drop_test.rb index afeba5e..96e7fa5 100644 --- a/test/integration/drop_test.rb +++ b/test/integration/drop_test.rb @@ -113,27 +113,27 @@ class DropsTest < Minitest::Test end def test_rendering_raises_on_tainted_attr - Liquid::Template.taint_mode = :error - tpl = Liquid::Template.parse('{{ product.user_input }}') - assert_raises TaintedError do - tpl.render!('product' => ProductDrop.new) + with_taint_mode(:error) do + tpl = Liquid::Template.parse('{{ product.user_input }}') + assert_raises TaintedError do + tpl.render!('product' => ProductDrop.new) + end end - Liquid::Template.taint_mode = :lax end def test_rendering_warns_on_tainted_attr - Liquid::Template.taint_mode = :warn - tpl = Liquid::Template.parse('{{ product.user_input }}') - tpl.render!('product' => ProductDrop.new) - assert_match /tainted/, tpl.warnings.first - Liquid::Template.taint_mode = :lax + with_taint_mode(:warn) do + tpl = Liquid::Template.parse('{{ product.user_input }}') + tpl.render!('product' => ProductDrop.new) + assert_match /tainted/, tpl.warnings.first + end end def test_rendering_doesnt_raise_on_escaped_tainted_attr - Liquid::Template.taint_mode = :error - tpl = Liquid::Template.parse('{{ product.user_input | escape }}') - tpl.render!('product' => ProductDrop.new) - Liquid::Template.taint_mode = :lax + with_taint_mode(:error) do + tpl = Liquid::Template.parse('{{ product.user_input | escape }}') + tpl.render!('product' => ProductDrop.new) + end end def test_drop_does_only_respond_to_whitelisted_methods diff --git a/test/test_helper.rb b/test/test_helper.rb index 9c0daa9..75d5491 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -57,6 +57,14 @@ module Minitest Liquid::Strainer.class_variable_set(:@@filters, original_filters) end + def with_taint_mode(mode) + old_mode = Liquid::Template.taint_mode + Liquid::Template.taint_mode = mode + yield + ensure + Liquid::Template.taint_mode = old_mode + end + def with_error_mode(mode) old_mode = Liquid::Template.error_mode Liquid::Template.error_mode = mode