New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Centralize site path sanitation #2882

Merged
merged 25 commits into from Nov 5, 2014

Conversation

Projects
None yet
4 participants
@parkr
Member

parkr commented Sep 7, 2014

This pull request introduces new standardized methods for accessing a directory from within the site's source or destination. The new methods introduced (and used heavily) are:

  1. Site#in_source_dir
  2. Site#in_dest_dir

These methods utilize Jekyll.sanitized_path to ensure the given path is, indeed, inside the source or destination directories to prevent filesystem traversal. A major exploit of hosted Jekyll systems is filesystem traversal, which makes the entire filesystem accessible to a malicious user (something we wish to avoid). This pull request should make the path sanitization a bit clearer to ease the review process for security teams.

❤️ 🔒

/cc @jekyll/security

override = Configuration[override].stringify_keys
unless override.delete('skip_config_files')
config = config.read_config_files(config.config_files(override))
end

This comment has been minimized.

@parkr

parkr Sep 7, 2014

Member

This unless block is the only substantive change in this file.

@parkr

parkr Sep 7, 2014

Member

This unless block is the only substantive change in this file.

Show outdated Hide outdated lib/jekyll/draft.rb
@@ -15,7 +15,7 @@ def self.valid?(name)
# Get the full path to the directory containing the draft files
def containing_dir(source, dir)
File.join(source, dir, '_drafts')
Jekyll.sanitized_path(source, File.join(dir, '_drafts'))

This comment has been minimized.

@mastahyeti

mastahyeti Sep 25, 2014

Contributor

Should this be site.in_source_dir? Not clear why this class is passed the site and the source separately.

@mastahyeti

mastahyeti Sep 25, 2014

Contributor

Should this be site.in_source_dir? Not clear why this class is passed the site and the source separately.

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Sep 25, 2014

Contributor

❤️ Sorry I didn't see this earlier. This looks great. 👍

Contributor

mastahyeti commented Sep 25, 2014

❤️ Sorry I didn't see this earlier. This looks great. 👍

@parkr parkr self-assigned this Sep 25, 2014

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Oct 21, 2014

Contributor

I read through the new commits. I ❤️ where this is going.

Contributor

mastahyeti commented Oct 21, 2014

I read through the new commits. I ❤️ where this is going.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 21, 2014

Member

@mastahyeti I think I've converted all the Jekyll.sanitized_path calls to site.in_(source|dest)_dir where a site exists. Had to leave a couple (like in Configuration). Anything else you'd like to see in this pull request? Thanks for keeping an eye on it! 😄

Member

parkr commented Oct 21, 2014

@mastahyeti I think I've converted all the Jekyll.sanitized_path calls to site.in_(source|dest)_dir where a site exists. Had to leave a couple (like in Configuration). Anything else you'd like to see in this pull request? Thanks for keeping an eye on it! 😄

@parkr parkr added this to the 2.4.1 milestone Oct 21, 2014

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Oct 22, 2014

Contributor

Anything else you'd like to see in this pull request?

Nope. That addresses the main concern with path sanitization.

Contributor

mastahyeti commented Oct 22, 2014

Anything else you'd like to see in this pull request?

Nope. That addresses the main concern with path sanitization.

parkr added some commits Aug 27, 2014

Cache the related posts most_recent_posts
/cc @mattr- is this safe to do, you think? it only happens at render
time...
Commit the paths one by one to Jekyll.sanitized_path with the proper …
…initial base.

Prevents errors like these:

[39/78] TestPost#test_: A Post processing posts should not be writable outside of destination. /Users/parker/jekyll/jekyll/test/dest
/Users/parker/jekyll/jekyll/test/dest/Users/parker/jekyll/baddie.html
 = 0.01 s
  1) Failure:
TestPost#test_: A Post processing posts should not be writable outside of destination.  [/Users/parker/jekyll/jekyll/test/test_post.rb:152]:
Failed assertion, no message given.
paths.reduce(dest) do |base, path|
Jekyll.sanitized_path(base, path)
end
end

This comment has been minimized.

@parkr

parkr Nov 4, 2014

Member

@mastahyeti I have changed these to prevent this case:

[39/78] TestPost#test_: A Post processing posts should not be writable outside of destination. 
dest = "/Users/parker/jekyll/jekyll/test/dest"
path = "/Users/parker/jekyll/jekyll/test/dest/Users/parker/jekyll/baddie.html"
 = 0.01 s
  1) Failure:
TestPost#test_: A Post processing posts should not be writable outside of destination.  [/Users/parker/jekyll/jekyll/test/test_post.rb:152]:
Failed assertion, no message given.

By putting each path through Jekyll.sanitized_path, we prevent the attempted traversal from running amok.

@parkr

parkr Nov 4, 2014

Member

@mastahyeti I have changed these to prevent this case:

[39/78] TestPost#test_: A Post processing posts should not be writable outside of destination. 
dest = "/Users/parker/jekyll/jekyll/test/dest"
path = "/Users/parker/jekyll/jekyll/test/dest/Users/parker/jekyll/baddie.html"
 = 0.01 s
  1) Failure:
TestPost#test_: A Post processing posts should not be writable outside of destination.  [/Users/parker/jekyll/jekyll/test/test_post.rb:152]:
Failed assertion, no message given.

By putting each path through Jekyll.sanitized_path, we prevent the attempted traversal from running amok.

This comment has been minimized.

@mastahyeti

mastahyeti Nov 4, 2014

Contributor

I'm not clear on what the problem was.

@mastahyeti

mastahyeti Nov 4, 2014

Contributor

I'm not clear on what the problem was.

This comment has been minimized.

@parkr

parkr Nov 4, 2014

Member

Before, this method looked like this:

def in_dest_dir(*paths)
  Jekyll.sanitized_path(dest, File.join(*paths))
end

In the case that paths = %w{/Users/parker/jekyll/jekyll/test/dest ../../baddie.html}, the File.expand_path(File.join("/Users/parker/jekyll/jekyll/test/dest", "../../baddie.html")) call will not have the proper dest base, so we tack it on inside Jekyll.sanitized_path. The new solution applies Jekyll.sanitized_path for each input path such that

dest = "/Users/parker/jekyll/jekyll/test/dest"
Jekyll.sanitized_path(dest, "/Users/parker/jekyll/jekyll/test/dest") # => "/Users/parker/jekyll/jekyll/test/dest"
Jekyll.sanitized_path(_, "../../baddie.html") # => "/Users/parker/jekyll/jekyll/test/dest/baddie.html"

We need to apply Jekyll.sanitized_path to each path successively and pass the result as the base each time.

@parkr

parkr Nov 4, 2014

Member

Before, this method looked like this:

def in_dest_dir(*paths)
  Jekyll.sanitized_path(dest, File.join(*paths))
end

In the case that paths = %w{/Users/parker/jekyll/jekyll/test/dest ../../baddie.html}, the File.expand_path(File.join("/Users/parker/jekyll/jekyll/test/dest", "../../baddie.html")) call will not have the proper dest base, so we tack it on inside Jekyll.sanitized_path. The new solution applies Jekyll.sanitized_path for each input path such that

dest = "/Users/parker/jekyll/jekyll/test/dest"
Jekyll.sanitized_path(dest, "/Users/parker/jekyll/jekyll/test/dest") # => "/Users/parker/jekyll/jekyll/test/dest"
Jekyll.sanitized_path(_, "../../baddie.html") # => "/Users/parker/jekyll/jekyll/test/dest/baddie.html"

We need to apply Jekyll.sanitized_path to each path successively and pass the result as the base each time.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 4, 2014

Member

@mastahyeti If this gets a 👍 from you, I will merge and release a new version of Jekyll.

Member

parkr commented Nov 4, 2014

@mastahyeti If this gets a 👍 from you, I will merge and release a new version of Jekyll.

@mastahyeti

This comment has been minimized.

Show comment
Hide comment
@mastahyeti

mastahyeti Nov 5, 2014

Contributor

👍

Contributor

mastahyeti commented Nov 5, 2014

👍

@parkr parkr merged commit 2ec1dc1 into master Nov 5, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@parkr parkr deleted the security/centralize-path-sanitation branch Nov 5, 2014

parkr added a commit that referenced this pull request Nov 5, 2014

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 5, 2014

Member

Excellent, thank you @mastahyeti! We'll have this out this week.

Member

parkr commented Nov 5, 2014

Excellent, thank you @mastahyeti! We'll have this out this week.

@carljparker

This comment has been minimized.

Show comment
Hide comment
@carljparker

carljparker Mar 28, 2015

I think this PR might have introduced a bug that I describe in the following issue:

#3488 (comment)

Test case is at:

https://github.com/carljparker/test-case-jekyll-directory

(And strangely, my name is also Parker.)

carljparker commented Mar 28, 2015

I think this PR might have introduced a bug that I describe in the following issue:

#3488 (comment)

Test case is at:

https://github.com/carljparker/test-case-jekyll-directory

(And strangely, my name is also Parker.)

@parkr parkr referenced this pull request Oct 15, 2015

Closed

Jekyll fails finding files #3248

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.