Merge pull request #1065 from jkowens/fix-null-byte

Return 400 if Rack::File or Rack::Directory path contains null byte

5 files changed, 34 insertions, 2 deletions

diff --git a/lib/rack/directory.rb b/lib/rack/directory.rb
index c026c42a..89cfe807 100644
--- a/lib/rack/directory.rb
+++ b/lib/rack/directory.rb
@@ -71,7 +71,9 @@ table { width:100%%; }
       script_name = env[SCRIPT_NAME]
       path_info = Utils.unescape_path(env[PATH_INFO])
 
-      if forbidden = check_forbidden(path_info)
+      if bad_request = check_bad_request(path_info)
+        bad_request
+      elsif forbidden = check_forbidden(path_info)
         forbidden
       else
         path = ::File.join(@root, path_info)
@@ -79,6 +81,16 @@ table { width:100%%; }
       end
     end
 
+    def check_bad_request(path_info)
+      return if Utils.valid_path?(path_info)
+
+      body = "Bad Request\n"
+      size = body.bytesize
+      return [400, {CONTENT_TYPE => "text/plain",
+        CONTENT_LENGTH => size.to_s,
+        "X-Cascade" => "pass"}, [body]]
+    end
+
     def check_forbidden(path_info)
       return unless path_info.include? ".."
 
diff --git a/lib/rack/file.rb b/lib/rack/file.rb
index 227a7284..0a257b3d 100644
--- a/lib/rack/file.rb
+++ b/lib/rack/file.rb
@@ -38,8 +38,9 @@ module Rack
       end
 
       path_info = Utils.unescape_path request.path_info
-      clean_path_info = Utils.clean_path_info(path_info)
+      return fail(400, "Bad Request") unless Utils.valid_path?(path_info)
 
+      clean_path_info = Utils.clean_path_info(path_info)
       path = ::File.join(@root, clean_path_info)
 
       available = begin
diff --git a/lib/rack/utils.rb b/lib/rack/utils.rb
index 74dc1de5..7b842125 100644
--- a/lib/rack/utils.rb
+++ b/lib/rack/utils.rb
@@ -609,5 +609,12 @@ module Rack
     end
     module_function :clean_path_info
 
+    NULL_BYTE = "\0".freeze
+
+    def valid_path?(path)
+      path.valid_encoding? && !path.include?(NULL_BYTE)
+    end
+    module_function :valid_path?
+
   end
 end
diff --git a/test/spec_directory.rb b/test/spec_directory.rb
index 6ba0d406..42bdea9f 100644
--- a/test/spec_directory.rb
+++ b/test/spec_directory.rb
@@ -63,6 +63,13 @@ describe Rack::Directory do
     assert_match(res, /passed!/)
   end
 
+  it "serve uri with URL encoded null byte (%00) in filenames" do
+    res = Rack::MockRequest.new(Rack::Lint.new(app))
+      .get("/cgi/test%00")
+
+    res.must_be :bad_request?
+  end
+
   it "not allow directory traversal" do
     res = Rack::MockRequest.new(Rack::Lint.new(app)).
       get("/cgi/../test")
diff --git a/test/spec_file.rb b/test/spec_file.rb
index 353dcdfe..3106e629 100644
--- a/test/spec_file.rb
+++ b/test/spec_file.rb
@@ -68,6 +68,11 @@ describe Rack::File do
     assert_match(res, /ruby/)
   end
 
+  it "serve uri with URL encoded null byte (%00) in filenames" do
+    res = Rack::MockRequest.new(file(DOCROOT)).get("/cgi/test%00")
+    res.must_be :bad_request?
+  end
+
   it "allow safe directory traversal" do
     req = Rack::MockRequest.new(file(DOCROOT))