Skip to content

Commit

Permalink
Check for directory traversal after unescaping
Browse files Browse the repository at this point in the history
The `forbidden_request?` check could be trivially bypassed
by percent encoding .. as %2e%2e.

After auditing Sprockets and Hike and fuzzing a simple
server, I don't believe this is exploitable. However,
better safe than sorry/defense in depth/etc.
  • Loading branch information
jfirebaugh authored and josh committed Apr 26, 2012
1 parent 2997be9 commit 08ef21a
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 7 deletions.
14 changes: 7 additions & 7 deletions lib/sprockets/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,6 @@ def call(env)

msg = "Served asset #{env['PATH_INFO']} -"

# URLs containing a `".."` are rejected for security reasons.
if forbidden_request?(env)
return forbidden_response
end

# Mark session as "skipped" so no `Set-Cookie` header is set
env['rack.session.options'] ||= {}
env['rack.session.options'][:defer] = true
Expand All @@ -38,6 +33,11 @@ def call(env)
# Extract the path from everything after the leading slash
path = unescape(env['PATH_INFO'].to_s.sub(/^\//, ''))

# URLs containing a `".."` are rejected for security reasons.
if forbidden_request?(path)
return forbidden_response
end

# Strip fingerprint
if fingerprint = path_fingerprint(path)
path = path.sub("-#{fingerprint}", '')
Expand Down Expand Up @@ -85,12 +85,12 @@ def call(env)
end

private
def forbidden_request?(env)
def forbidden_request?(path)
# Prevent access to files elsewhere on the file system
#
# http://example.org/assets/../../../etc/passwd
#
env["PATH_INFO"].include?("..")
path.include?("..")
end

# Returns a 403 Forbidden response tuple
Expand Down
3 changes: 3 additions & 0 deletions test/test_server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,9 @@ def app
test "illegal require outside load path" do
get "/assets/../config/passwd"
assert_equal 403, last_response.status

get "/assets/%2e%2e/config/passwd"
assert_equal 403, last_response.status
end

test "add new source to tree" do
Expand Down

0 comments on commit 08ef21a

Please sign in to comment.