Skip to content

Loading…

Update lib/rack/file.rb #522

Closed
wants to merge 6 commits into from

2 participants

@snyff

Fix XSS in path_info

snyff added some commits
@snyff snyff Update lib/rack/file.rb
Fix XSS in path_info
dd2d7d5
@snyff snyff Update lib/rack/directory.rb
fix XSS
66b41c8
@snyff snyff Update lib/rack/file.rb
prevent symlink usage to access files outside of the "mounted" directory
a9abcc7
@snyff snyff Encoding issue
Can lead to XSS
7367a70
@snyff snyff Update lib/rack/auth/digest/md5.rb
secure_compare for digest authentication
df06eb2
@snyff snyff Update example/protectedlobster.rb
More secure example
b8168b5
@rkh rkh commented on the diff
lib/rack/auth/digest/md5.rb
@@ -96,7 +96,7 @@ def valid_nonce?(auth)
def valid_digest?(auth)
pw = @authenticator.call(auth.username)
- pw && digest(auth, pw) == auth.response
+ pw && Rack::Utils.secure_compare(digest(auth, pw), auth.response)
@rkh Official Rack repositories member
rkh added a note

Could you do this in a separate PR? I'd like to merge that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rkh rkh commented on the diff
lib/rack/directory.rb
@@ -98,7 +98,7 @@ def list_directory
url << '/' if stat.directory?
basename << '/' if stat.directory?
- @files << [ url, basename, size, type, mtime ]
+ @files << [ url, Utils.escape_html(basename), size, type, mtime ]
@rkh Official Rack repositories member
rkh added a note

This one is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rkh rkh commented on the diff
lib/rack/directory.rb
@@ -127,7 +127,7 @@ def list_path
end
def entity_not_found
- body = "Entity not found: #{@path_info}\n"
+ body = "Entity not found: #{Utils.escape_html(@path_info)}\n"
@rkh Official Rack repositories member
rkh added a note

Again, HTML escaping in a text document?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rkh rkh commented on the diff
lib/rack/file.rb
@@ -51,7 +51,7 @@ def _call(env)
@path = F.join(@root, *clean)
available = begin
- F.file?(@path) && F.readable?(@path)
+ F.file?(@path) && F.readable?(@path) && !F.symlink?(@path)
@rkh Official Rack repositories member
rkh added a note

This is actually changed behavior. I'm not sure we don't want to follow symlinks. Maybe make this an option?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@raggi raggi added a commit that closed this pull request
@raggi raggi Sure up HTML escaping in Rack::Directory
 * Supersedes & closes #522
ba98d5f
@raggi raggi closed this in ba98d5f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 20, 2013
  1. @snyff

    Update lib/rack/file.rb

    snyff committed
    Fix XSS in path_info
  2. @snyff

    Update lib/rack/directory.rb

    snyff committed
    fix XSS
  3. @snyff

    Update lib/rack/file.rb

    snyff committed
    prevent symlink usage to access files outside of the "mounted" directory
Commits on Feb 28, 2013
  1. @snyff

    Encoding issue

    snyff committed
    Can lead to XSS
  2. @snyff

    Update lib/rack/auth/digest/md5.rb

    snyff committed
    secure_compare for digest authentication
  3. @snyff

    Update example/protectedlobster.rb

    snyff committed
    More secure example
Showing with 7 additions and 7 deletions.
  1. +1 −1 example/protectedlobster.rb
  2. +1 −1 lib/rack/auth/digest/md5.rb
  3. +3 −3 lib/rack/directory.rb
  4. +2 −2 lib/rack/file.rb
View
2 example/protectedlobster.rb
@@ -4,7 +4,7 @@
lobster = Rack::Lobster.new
protected_lobster = Rack::Auth::Basic.new(lobster) do |username, password|
- 'secret' == password
+ Rack::Utils.secure_compare('secret', password)
end
protected_lobster.realm = 'Lobster 2.0'
View
2 lib/rack/auth/digest/md5.rb
@@ -96,7 +96,7 @@ def valid_nonce?(auth)
def valid_digest?(auth)
pw = @authenticator.call(auth.username)
- pw && digest(auth, pw) == auth.response
+ pw && Rack::Utils.secure_compare(digest(auth, pw), auth.response)
@rkh Official Rack repositories member
rkh added a note

Could you do this in a separate PR? I'd like to merge that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
def md5(data)
View
6 lib/rack/directory.rb
@@ -98,7 +98,7 @@ def list_directory
url << '/' if stat.directory?
basename << '/' if stat.directory?
- @files << [ url, basename, size, type, mtime ]
+ @files << [ url, Utils.escape_html(basename), size, type, mtime ]
@rkh Official Rack repositories member
rkh added a note

This one is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
return [ 200, {'Content-Type'=>'text/html; charset=utf-8'}, self ]
@@ -127,7 +127,7 @@ def list_path
end
def entity_not_found
- body = "Entity not found: #{@path_info}\n"
+ body = "Entity not found: #{Utils.escape_html(@path_info)}\n"
@rkh Official Rack repositories member
rkh added a note

Again, HTML escaping in a text document?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
size = Rack::Utils.bytesize(body)
return [404, {"Content-Type" => "text/plain",
"Content-Length" => size.to_s,
@@ -135,7 +135,7 @@ def entity_not_found
end
def each
- show_path = @path.sub(/^#{@root}/,'')
+ show_path = Utils.escape_html(@path.sub(/^#{@root}/,''))
files = @files.map{|f| DIR_FILE % f }*"\n"
page = DIR_PAGE % [ show_path, show_path , files ]
page.each_line{|l| yield l }
View
4 lib/rack/file.rb
@@ -51,7 +51,7 @@ def _call(env)
@path = F.join(@root, *clean)
available = begin
- F.file?(@path) && F.readable?(@path)
+ F.file?(@path) && F.readable?(@path) && !F.symlink?(@path)
@rkh Official Rack repositories member
rkh added a note

This is actually changed behavior. I'm not sure we don't want to follow symlinks. Maybe make this an option?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
rescue SystemCallError
false
end
@@ -59,7 +59,7 @@ def _call(env)
if available
serving(env)
else
- fail(404, "File not found: #{path_info}")
+ fail(404, "File not found: #{Utils.escape_html(path_info)}")
end
end
Something went wrong with that request. Please try again.