From 9e796d0627e3148356205c64cb4ae964d8ce718f Mon Sep 17 00:00:00 2001 From: Ben Balter Date: Mon, 6 Jan 2014 19:30:13 -0500 Subject: [PATCH 01/15] failing test Signed-off-by: Parker Moore --- test/source/_posts/2014-01-06-permalink-traversal.md | 5 +++++ test/test_post.rb | 9 +++++++++ 2 files changed, 14 insertions(+) create mode 100644 test/source/_posts/2014-01-06-permalink-traversal.md diff --git a/test/source/_posts/2014-01-06-permalink-traversal.md b/test/source/_posts/2014-01-06-permalink-traversal.md new file mode 100644 index 00000000..1b3fdf88 --- /dev/null +++ b/test/source/_posts/2014-01-06-permalink-traversal.md @@ -0,0 +1,5 @@ +--- +permalink: /%2e%2e/baddie.html +--- + +# Test diff --git a/test/test_post.rb b/test/test_post.rb index 418e60d7..580f0065 100644 --- a/test/test_post.rb +++ b/test/test_post.rb @@ -103,6 +103,15 @@ class TestPost < Test::Unit::TestCase assert_equal "/my_category/permalinked-post", @post.url end + should "not be writable outside of destination" do + post = setup_post("2014-01-06-permalink-traversal.md") + do_render(post) + post.write(dest_dir) + + assert !File.exist?(File.expand_path("../baddie.html", dest_dir)) + assert File.exist(File.expand_path("/baddie.html", dest_dir)) + end + context "with CRLF linebreaks" do setup do @real_file = "2009-05-24-yaml-linebreak.markdown" From 9b3068c15d57c2ba8a125941f09dc904f0b6d181 Mon Sep 17 00:00:00 2001 From: Ben Balter Date: Mon, 6 Jan 2014 19:50:59 -0500 Subject: [PATCH 02/15] url escape before sanitizing Signed-off-by: Parker Moore --- lib/jekyll/url.rb | 9 +++++++-- test/test_post.rb | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/jekyll/url.rb b/lib/jekyll/url.rb index 10e1cb5d..f2318ae0 100644 --- a/lib/jekyll/url.rb +++ b/lib/jekyll/url.rb @@ -50,8 +50,12 @@ module Jekyll # Returns a sanitized String URL def sanitize_url(in_url) + + # prevent escaped periods from bypassing sanitization + url = URI.unescape(in_url) + # Remove all double slashes - url = in_url.gsub(/\/\//, "/") + url = url.gsub(/\/\//, "/") # Remove every URL segment that consists solely of dots url = url.split('/').reject{ |part| part =~ /^\.+$/ }.join('/') @@ -61,7 +65,8 @@ module Jekyll # Always add a leading slash url.gsub!(/\A([^\/])/, '/\1') - url + + URI.escape url end end end diff --git a/test/test_post.rb b/test/test_post.rb index 580f0065..9cac2984 100644 --- a/test/test_post.rb +++ b/test/test_post.rb @@ -109,7 +109,7 @@ class TestPost < Test::Unit::TestCase post.write(dest_dir) assert !File.exist?(File.expand_path("../baddie.html", dest_dir)) - assert File.exist(File.expand_path("/baddie.html", dest_dir)) + assert File.exist?(File.expand_path("baddie.html", dest_dir)) end context "with CRLF linebreaks" do From f49ee211363f9c274956469318eef3e1063d12ed Mon Sep 17 00:00:00 2001 From: Ben Balter Date: Mon, 6 Jan 2014 19:55:53 -0500 Subject: [PATCH 03/15] fix failing post count test Signed-off-by: Parker Moore --- test/test_generated_site.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_generated_site.rb b/test/test_generated_site.rb index 2afbb1a7..c7398cd5 100644 --- a/test/test_generated_site.rb +++ b/test/test_generated_site.rb @@ -14,7 +14,7 @@ class TestGeneratedSite < Test::Unit::TestCase end should "ensure post count is as expected" do - assert_equal 38, @site.posts.size + assert_equal 39, @site.posts.size end should "insert site.posts into the index" do From 0acbe9579796c6ecc51aec343e54a486d079e980 Mon Sep 17 00:00:00 2001 From: Ben Balter Date: Mon, 6 Jan 2014 19:59:41 -0500 Subject: [PATCH 04/15] test multiple traversals Signed-off-by: Parker Moore --- test/source/_posts/2014-01-06-permalink-traversal.md | 2 +- test/test_post.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/source/_posts/2014-01-06-permalink-traversal.md b/test/source/_posts/2014-01-06-permalink-traversal.md index 1b3fdf88..c3f77d1b 100644 --- a/test/source/_posts/2014-01-06-permalink-traversal.md +++ b/test/source/_posts/2014-01-06-permalink-traversal.md @@ -1,5 +1,5 @@ --- -permalink: /%2e%2e/baddie.html +permalink: /%2e%2e/%2e%2e/%2e%2e/baddie.html --- # Test diff --git a/test/test_post.rb b/test/test_post.rb index 9cac2984..9f189d2b 100644 --- a/test/test_post.rb +++ b/test/test_post.rb @@ -108,7 +108,7 @@ class TestPost < Test::Unit::TestCase do_render(post) post.write(dest_dir) - assert !File.exist?(File.expand_path("../baddie.html", dest_dir)) + assert !File.exist?(File.expand_path("../../../baddie.html", dest_dir)) assert File.exist?(File.expand_path("baddie.html", dest_dir)) end From dfec551cc4d727b9c23cd8ce2950c9932d985893 Mon Sep 17 00:00:00 2001 From: Ben Balter Date: Mon, 6 Jan 2014 22:11:13 -0500 Subject: [PATCH 05/15] add symlink failing test Signed-off-by: Parker Moore --- test/source/_includes/about.html | 1 + test/source/about.html | 7 +------ test/test_tags.rb | 21 +++++++++++++++++++++ 3 files changed, 23 insertions(+), 6 deletions(-) create mode 120000 test/source/_includes/about.html mode change 100644 => 120000 test/source/about.html diff --git a/test/source/_includes/about.html b/test/source/_includes/about.html new file mode 120000 index 00000000..8bf60f5d --- /dev/null +++ b/test/source/_includes/about.html @@ -0,0 +1 @@ +../about.html \ No newline at end of file diff --git a/test/source/about.html b/test/source/about.html deleted file mode 100644 index a6a79f93..00000000 --- a/test/source/about.html +++ /dev/null @@ -1,6 +0,0 @@ ---- -title: About -permalink: /about/ ---- - -About the site diff --git a/test/source/about.html b/test/source/about.html new file mode 120000 index 00000000..ef437915 --- /dev/null +++ b/test/source/about.html @@ -0,0 +1 @@ +/tmp/pages-test \ No newline at end of file diff --git a/test/test_tags.rb b/test/test_tags.rb index 8ecaf19b..4c738b64 100644 --- a/test/test_tags.rb +++ b/test/test_tags.rb @@ -363,6 +363,27 @@ CONTENT end context "include tag with parameters" do + + context "with symlink'd include" do + + setup do + content = < 'pretty', 'source' => source_dir, 'destination' => dest_dir, 'read_posts' => true}) + end + + should "not allow symlink includes" do + File.open("/tmp/pages-test", 'w') { |file| file.write("SYMLINK TEST") } + assert_no_match /SYMLINK TEST/, @result + end + end + context "with one parameter" do setup do content = < Date: Mon, 6 Jan 2014 22:18:53 -0500 Subject: [PATCH 06/15] unbreak tests Signed-off-by: Parker Moore --- test/source/about.html | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) mode change 120000 => 100644 test/source/about.html diff --git a/test/source/about.html b/test/source/about.html deleted file mode 120000 index ef437915..00000000 --- a/test/source/about.html +++ /dev/null @@ -1 +0,0 @@ -/tmp/pages-test \ No newline at end of file diff --git a/test/source/about.html b/test/source/about.html new file mode 100644 index 00000000..a6a79f93 --- /dev/null +++ b/test/source/about.html @@ -0,0 +1,6 @@ +--- +title: About +permalink: /about/ +--- + +About the site From ce425eec8b3d34c6a37d5285879d115c7dd85b3b Mon Sep 17 00:00:00 2001 From: Ben Balter Date: Mon, 6 Jan 2014 22:22:51 -0500 Subject: [PATCH 07/15] fix symlink so tests fail Signed-off-by: Parker Moore --- test/source/_includes/about.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/source/_includes/about.html b/test/source/_includes/about.html index 8bf60f5d..ef437915 120000 --- a/test/source/_includes/about.html +++ b/test/source/_includes/about.html @@ -1 +1 @@ -../about.html \ No newline at end of file +/tmp/pages-test \ No newline at end of file From 323d14845fa7128fad83536ddd891ee2c6768673 Mon Sep 17 00:00:00 2001 From: Ben Balter Date: Mon, 6 Jan 2014 22:23:51 -0500 Subject: [PATCH 08/15] rebreak tests, move sanitization closer to write Signed-off-by: Parker Moore --- lib/jekyll/url.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/jekyll/url.rb b/lib/jekyll/url.rb index f2318ae0..813b9c87 100644 --- a/lib/jekyll/url.rb +++ b/lib/jekyll/url.rb @@ -51,11 +51,8 @@ module Jekyll # Returns a sanitized String URL def sanitize_url(in_url) - # prevent escaped periods from bypassing sanitization - url = URI.unescape(in_url) - # Remove all double slashes - url = url.gsub(/\/\//, "/") + url = in_url.gsub(/\/\//, "/") # Remove every URL segment that consists solely of dots url = url.split('/').reject{ |part| part =~ /^\.+$/ }.join('/') @@ -66,7 +63,7 @@ module Jekyll # Always add a leading slash url.gsub!(/\A([^\/])/, '/\1') - URI.escape url + url end end end From 4e318cd1926498a6796a426ca79e0ed9fd2f2ec5 Mon Sep 17 00:00:00 2001 From: Ben Balter Date: Mon, 6 Jan 2014 22:49:04 -0500 Subject: [PATCH 09/15] test symlinkd dir, not file Signed-off-by: Parker Moore --- test/source/_includes/about.html | 1 - test/source/_includes/tmp | 1 + test/test_tags.rb | 4 ++-- 3 files changed, 3 insertions(+), 3 deletions(-) delete mode 120000 test/source/_includes/about.html create mode 120000 test/source/_includes/tmp diff --git a/test/source/_includes/about.html b/test/source/_includes/about.html deleted file mode 120000 index ef437915..00000000 --- a/test/source/_includes/about.html +++ /dev/null @@ -1 +0,0 @@ -/tmp/pages-test \ No newline at end of file diff --git a/test/source/_includes/tmp b/test/source/_includes/tmp new file mode 120000 index 00000000..cad23091 --- /dev/null +++ b/test/source/_includes/tmp @@ -0,0 +1 @@ +/tmp \ No newline at end of file diff --git a/test/test_tags.rb b/test/test_tags.rb index 4c738b64..68507c76 100644 --- a/test/test_tags.rb +++ b/test/test_tags.rb @@ -372,10 +372,10 @@ CONTENT title: Include symlink --- -{% include about.html %} +{% include tmp/pages-test %} CONTENT - create_post(content, {'permalink' => 'pretty', 'source' => source_dir, 'destination' => dest_dir, 'read_posts' => true}) + create_post(content, {'permalink' => 'pretty', 'source' => source_dir, 'destination' => dest_dir, 'read_posts' => true, 'safe' => true }) end should "not allow symlink includes" do From a799e41b7061ea33276f9e56237c21e9faa16f6c Mon Sep 17 00:00:00 2001 From: Ben Balter Date: Mon, 6 Jan 2014 23:02:01 -0500 Subject: [PATCH 10/15] patch symlink vuln and properly test Signed-off-by: Parker Moore --- lib/jekyll/tags/include.rb | 19 ++++++------------- test/test_tags.rb | 13 ++++++------- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/lib/jekyll/tags/include.rb b/lib/jekyll/tags/include.rb index 5c679dc1..394d4e67 100644 --- a/lib/jekyll/tags/include.rb +++ b/lib/jekyll/tags/include.rb @@ -96,13 +96,12 @@ eos def render(context) dir = File.join(context.registers[:site].source, INCLUDES_DIR) - validate_dir(dir, context.registers[:site].safe) file = render_variable(context) || @file validate_file_name(file) path = File.join(dir, file) - validate_file(path, context.registers[:site].safe) + validate_path(path, context.registers[:site].safe) begin partial = Liquid::Template.parse(source(path, context)) @@ -116,17 +115,11 @@ eos end end - def validate_dir(dir, safe) - if File.symlink?(dir) && safe - raise IOError.new "Includes directory '#{dir}' cannot be a symlink" - end - end - - def validate_file(file, safe) - if !File.exists?(file) - raise IOError.new "Included file '#{file}' not found" - elsif File.symlink?(file) && safe - raise IOError.new "The included file '#{file}' should not be a symlink" + def validate_path(path, safe) + if !File.exist?(path) + raise IOError.new "Included file '#{path}' not found" + elsif path != File.realpath(path) && safe + raise IOError.new "The included file '#{path}' should not be a symlink" end end diff --git a/test/test_tags.rb b/test/test_tags.rb index 68507c76..194846f1 100644 --- a/test/test_tags.rb +++ b/test/test_tags.rb @@ -366,8 +366,10 @@ CONTENT context "with symlink'd include" do - setup do - content = < 'pretty', 'source' => source_dir, 'destination' => dest_dir, 'read_posts' => true, 'safe' => true }) - end - - should "not allow symlink includes" do - File.open("/tmp/pages-test", 'w') { |file| file.write("SYMLINK TEST") } + create_post(content, {'permalink' => 'pretty', 'source' => source_dir, 'destination' => dest_dir, 'read_posts' => true, 'safe' => true }) + end assert_no_match /SYMLINK TEST/, @result end end From c84cb5c007c540273965931ceb16953145a26bff Mon Sep 17 00:00:00 2001 From: Ben Balter Date: Mon, 6 Jan 2014 23:34:13 -0500 Subject: [PATCH 11/15] escape relative post permalinks, cleanup Signed-off-by: Parker Moore --- lib/jekyll/post.rb | 2 +- test/test_post.rb | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/jekyll/post.rb b/lib/jekyll/post.rb index 23e131ee..f9e678c6 100644 --- a/lib/jekyll/post.rb +++ b/lib/jekyll/post.rb @@ -270,7 +270,7 @@ module Jekyll # Returns destination file path String. def destination(dest) # The url needs to be unescaped in order to preserve the correct filename - path = File.join(dest, CGI.unescape(self.url)) + path = File.join(dest, File.expand_path(CGI.unescape(self.url), "/")) path = File.join(path, "index.html") if path[/\.html$/].nil? path end diff --git a/test/test_post.rb b/test/test_post.rb index 9f189d2b..8145d755 100644 --- a/test/test_post.rb +++ b/test/test_post.rb @@ -104,11 +104,13 @@ class TestPost < Test::Unit::TestCase end should "not be writable outside of destination" do + unexpected = File.expand_path("../../../baddie.html", dest_dir) + File.delete unexpected if File.exist?(unexpected) post = setup_post("2014-01-06-permalink-traversal.md") do_render(post) post.write(dest_dir) - assert !File.exist?(File.expand_path("../../../baddie.html", dest_dir)) + assert !File.exist?(unexpected) assert File.exist?(File.expand_path("baddie.html", dest_dir)) end From a8dd34420b2c59c883ed070d1f2ebee346b4688b Mon Sep 17 00:00:00 2001 From: Andy Lindeman Date: Mon, 6 Jan 2014 23:39:39 -0500 Subject: [PATCH 12/15] Prevents disclosure of file existence Signed-off-by: Parker Moore --- lib/jekyll/tags/include.rb | 16 ++++++++++------ test/test_tags.rb | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/lib/jekyll/tags/include.rb b/lib/jekyll/tags/include.rb index 394d4e67..db19ed8a 100644 --- a/lib/jekyll/tags/include.rb +++ b/lib/jekyll/tags/include.rb @@ -95,13 +95,13 @@ eos end def render(context) - dir = File.join(context.registers[:site].source, INCLUDES_DIR) + dir = File.join(File.realpath(context.registers[:site].source), INCLUDES_DIR) file = render_variable(context) || @file validate_file_name(file) path = File.join(dir, file) - validate_path(path, context.registers[:site].safe) + validate_path(path, dir, context.registers[:site].safe) begin partial = Liquid::Template.parse(source(path, context)) @@ -115,14 +115,18 @@ eos end end - def validate_path(path, safe) - if !File.exist?(path) + def validate_path(path, dir, safe) + if safe && !realpath_prefixed_with?(path, dir) + raise IOError.new "The included file '#{path}' should exist and should not be a symlink" + elsif !File.exist?(path) raise IOError.new "Included file '#{path}' not found" - elsif path != File.realpath(path) && safe - raise IOError.new "The included file '#{path}' should not be a symlink" end end + def realpath_prefixed_with?(path, dir) + File.exist?(path) && File.realpath(path).start_with?(dir) + end + def blank? false end diff --git a/test/test_tags.rb b/test/test_tags.rb index 194846f1..37e1d583 100644 --- a/test/test_tags.rb +++ b/test/test_tags.rb @@ -381,6 +381,21 @@ CONTENT end assert_no_match /SYMLINK TEST/, @result end + + should "not expose the existence of symlinked files" do + ex = assert_raise IOError do + content = < 'pretty', 'source' => source_dir, 'destination' => dest_dir, 'read_posts' => true, 'safe' => true }) + end + assert_match /should exist and should not be a symlink/, ex.message + end end context "with one parameter" do From e3be74e3767405b1f9b19030cb849ecb590bfd27 Mon Sep 17 00:00:00 2001 From: Ben Balter Date: Mon, 6 Jan 2014 23:51:13 -0500 Subject: [PATCH 13/15] sanity check for pages permalink traversal Signed-off-by: Parker Moore --- lib/jekyll/page.rb | 2 +- test/source/exploit.md | 5 +++++ test/test_filters.rb | 2 +- test/test_page.rb | 10 ++++++++++ test/test_site.rb | 2 +- 5 files changed, 18 insertions(+), 3 deletions(-) create mode 100644 test/source/exploit.md diff --git a/lib/jekyll/page.rb b/lib/jekyll/page.rb index 469dbb53..b2dfe361 100644 --- a/lib/jekyll/page.rb +++ b/lib/jekyll/page.rb @@ -135,7 +135,7 @@ module Jekyll # # Returns the destination file path String. def destination(dest) - path = File.join(dest, self.url) + path = File.join(dest, File.expand_path(self.url, "/")) path = File.join(path, "index.html") if self.url =~ /\/$/ path end diff --git a/test/source/exploit.md b/test/source/exploit.md new file mode 100644 index 00000000..c3f77d1b --- /dev/null +++ b/test/source/exploit.md @@ -0,0 +1,5 @@ +--- +permalink: /%2e%2e/%2e%2e/%2e%2e/baddie.html +--- + +# Test diff --git a/test/test_filters.rb b/test/test_filters.rb index 0d1da02f..4b5c8212 100644 --- a/test/test_filters.rb +++ b/test/test_filters.rb @@ -131,7 +131,7 @@ class TestFilters < Test::Unit::TestCase assert_equal 2, g["items"].size when "" assert g["items"].is_a?(Array), "The list of grouped items for '' is not an Array." - assert_equal 5, g["items"].size + assert_equal 6, g["items"].size end end end diff --git a/test/test_page.rb b/test/test_page.rb index 1147a63d..8924aeec 100644 --- a/test/test_page.rb +++ b/test/test_page.rb @@ -124,6 +124,16 @@ class TestPage < Test::Unit::TestCase assert_equal @page.permalink, @page.url assert_equal "/about/", @page.dir end + + should "not be writable outside of destination" do + unexpected = File.expand_path("../../../baddie.html", dest_dir) + File.delete unexpected if File.exist?(unexpected) + page = setup_page("exploit.md") + do_render(page) + page.write(dest_dir) + + assert !File.exist?(unexpected) + end end context "with specified layout of nil" do diff --git a/test/test_site.rb b/test/test_site.rb index 2d45ea7c..aeaeba6c 100644 --- a/test/test_site.rb +++ b/test/test_site.rb @@ -157,7 +157,7 @@ class TestSite < Test::Unit::TestCase should "sort pages alphabetically" do stub.proxy(Dir).entries { |entries| entries.reverse } @site.process - sorted_pages = %w(.htaccess about.html bar.html contacts.html deal.with.dots.html foo.md index.html index.html properties.html sitemap.xml symlinked-file) + sorted_pages = %w(.htaccess about.html bar.html contacts.html deal.with.dots.html exploit.md foo.md index.html index.html properties.html sitemap.xml symlinked-file) assert_equal sorted_pages, @site.pages.map(&:name) end From ac8d8a7cb822f435f499baf9972dc5e756ae62aa Mon Sep 17 00:00:00 2001 From: Parker Moore Date: Sat, 8 Feb 2014 00:31:26 -0500 Subject: [PATCH 14/15] Fix some tests --- test/test_filters.rb | 2 +- test/test_site.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_filters.rb b/test/test_filters.rb index 4b5c8212..a1cec1c7 100644 --- a/test/test_filters.rb +++ b/test/test_filters.rb @@ -131,7 +131,7 @@ class TestFilters < Test::Unit::TestCase assert_equal 2, g["items"].size when "" assert g["items"].is_a?(Array), "The list of grouped items for '' is not an Array." - assert_equal 6, g["items"].size + assert_equal 7, g["items"].size end end end diff --git a/test/test_site.rb b/test/test_site.rb index 5e639f50..7dc78dbc 100644 --- a/test/test_site.rb +++ b/test/test_site.rb @@ -157,7 +157,7 @@ class TestSite < Test::Unit::TestCase should "sort pages alphabetically" do stub.proxy(Dir).entries { |entries| entries.reverse } @site.process - sorted_pages = %w(.htaccess about.html bar.html coffeescript.coffee contacts.html deal.with.dots.html foo.md index.html index.html properties.html sitemap.xml symlinked-file) + sorted_pages = %w(.htaccess about.html bar.html coffeescript.coffee contacts.html deal.with.dots.html exploit.md foo.md index.html index.html properties.html sitemap.xml symlinked-file) assert_equal sorted_pages, @site.pages.map(&:name) end From 264dfc164d5a5f0e567eace68398843f057c53bb Mon Sep 17 00:00:00 2001 From: Parker Moore Date: Sun, 16 Feb 2014 22:00:33 -0500 Subject: [PATCH 15/15] When an include cannot be found, only print file path relative to source. --- lib/jekyll/tags/include.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/jekyll/tags/include.rb b/lib/jekyll/tags/include.rb index db19ed8a..aa608bc0 100644 --- a/lib/jekyll/tags/include.rb +++ b/lib/jekyll/tags/include.rb @@ -119,10 +119,14 @@ eos if safe && !realpath_prefixed_with?(path, dir) raise IOError.new "The included file '#{path}' should exist and should not be a symlink" elsif !File.exist?(path) - raise IOError.new "Included file '#{path}' not found" + raise IOError.new "Included file '#{path_relative_to_source(dir, path)}' not found" end end + def path_relative_to_source(dir, path) + File.join(INCLUDES_DIR, path.sub(Regexp.new("^#{dir}"), "")) + end + def realpath_prefixed_with?(path, dir) File.exist?(path) && File.realpath(path).start_with?(dir) end