[Mongrel] [SECURITY] Must Fix This Now! (Re: Arbitrary system files readable in 1.0.4 - 1.1.2)

Zed A. Shaw zedshaw at zedshaw.com
Fri Dec 28 21:22:23 EST 2007

On Fri, 28 Dec 2007 19:28:25 -0500
"Evan Weaver" <evan at cloudbur.st> wrote:

> I guess expand_path doesn't interact well with HTTP escaping.
> This is pretty critical, can you file a ticket against it?

No, you're miss-reading the change set.  The 1.0.4 change *removed* the
expand path on one of the conditions, so now it's using a relative
path on one of the if branches, AND removed the check that ensured the
expanded_path began with the expanded web root. Notice mine has
expand_path on the if and the else so it's always done, and then makes
sure that the expanded path begins with the web root.

No matter what you do, you *must* expand path all paths before you do
any comparisons or reads and never use an indirect path.  It might be
better to setup any paths you're doing, and then the very last thing is
always expand path.

Here's the change again:

-      req_path = HttpRequest.unescape(path_info)
-      if @path
-        req_path = File.expand_path(File.join(@path, path_info), @path)
-      else
-        req_path = File.expand_path(req_path)
-      end
-      if req_path.index(@path) == 0 and File.exist? req_path
-        # it exists and it's in the right location
+      # Add the drive letter or root path
+      req_path = File.join(@path, req_path) if @path
+      req_path = File.expand_path req_path
+      if File.exist? req_path
+        # It exists and it's in the right location
         if File.directory? req_path

Notice the - lines have "if req_path.index(@path) == 0..." that's the
part that ensures that the given path (after expansion) begins with the
path being used by the web server as the root.  If you don't have the
required expand path before this, AND make sure that the beginning of
the expanded path is always the root path, then you have this bug.

I haven't looked at the real code yet, but don't wait for a patch, fix
this and I'd say remove the gem so it doesn't go out more since it's a
*huge* vulnerability.

In fact, pushing out a 1.0.5 that reverts this change to fix it and
doing it now is probably the best.

Zed A. Shaw
- Hate: http://savingtheinternetwithhate.com/
- Good: http://www.zedshaw.com/
- Evil: http://yearofevil.com/

More information about the Mongrel-users mailing list