Subject: wu-ftpd-2.5.0 Potential buffer overflow (proposed patch)

From: Sylvain Robitaille <[email-masked]>
Date: Fri, 27 Aug 1999 11:33:47 -0400 (EDT)

On Sun, 4 Jul 1999 Michal Zalewski <[email-masked]> posted a message to
Bugtraq which included the following:

> - ----------------------------
> wu-ftpd 2.5, VR and BeroFTPD
> - ----------------------------
> 
> Compromise: remote root
> 
> Solution: add strlen() check somewhere
> 
> There's an overflow in wu-ftpd 2.5 and prior releases (including VR and
> BeroFTPD) in mapped_path when mapping current working directory to
> command-line. While I discovered this vunerability by myself, I don't want
> to provide exploit code, as all other, hard work has been done
> independently by someone else. Instead of that, there's a .diff file with
> patch, attached somewhere as ftpd.diff.

I checked the current version (2.5.0) and sure enough the code is
potentially vulnerable (I say "potentially" because it depends on having
MAPPING_CHDIR defined at compile time, which I'm not sure is or isn't
the case in all installations).

The patch supplied by Michal misses one potential overflow, though, and
makes an improper assumption about MAXPATHLEN (he hard-codes his length
check to 4095 bytes, while MAXPATHLEN on at least some Linux systems is
1024 bytes!)

I have prepared a patch myself (appended below my signature), against the
wu-ftpd-2.5.0 source found at ftp.vr.net/wu-ftpd/wu-ftpd-2.5.0.tar.gz.
Please look this over and consider incorporating it into a future release
of the distribution. In the meantime, I suggest that admins should make
a point to apply this patch.

-- 
----------------------------------------------------------------------
Sylvain Robitaille                              [email-masked]
 
Systems Manager                                   Concordia University
Instructional & Information Technology        Montreal, Quebec, Canada
----------------------------------------------------------------------
# 1999/08/27 Sylvain Robitaille
# ----------
#
# Michal Zalewski <[email-masked]> posted an announcement and proposed
# fix to Bugtraq on Sun, 4 Jul 1999 (message ID not available):
#
# Compromise: remote root
#
# Solution: add strlen() check somewhere
#
# There's an overflow in wu-ftpd 2.5 and prior releases (including VR
# and BeroFTPD) in mapped_path when mapping current working directory
# to command-line. While I discovered this vunerability by myself,
# I don't want to provide exploit code, as all other, hard work has
# been done independently by someone else. Instead of that, there's
# a .diff file with patch, attached somewhere as ftpd.diff.
#
# His patch overlooked one possible overflow (and makes an inappropriate
# assumption about MAXPATHLEN). I have created the following patch
# instead:

--- ftpd.c.original	Fri May 21 10:44:01 1999
+++ ftpd.c	Fri Aug 27 11:05:57 1999
@@ -1194,7 +1194,7 @@
 
 #ifdef MAPPING_CHDIR
 /* Keep track of the path the user has chdir'd into and respond with
- * that to pwd commands.  This is to avoid having the absolue disk
+ * that to pwd commands.  This is to avoid having the absolute disk
  * path returned, which I want to avoid.
  */
 char mapped_path[ MAXPATHLEN ] = "/";
@@ -1207,6 +1207,10 @@
       char *path;
 #endif
 {
+      /*
+       * 1999/08/26 Sylvain Robitaille: path is defined in the calling
+       *            routine as MAXPATHLEN + 1 long, so this is safe.
+       */
       strcpy( path, mapped_path );
       return path;
 }
@@ -1243,9 +1247,20 @@
       }
 
       /* append the dir part with a leading / unless at root */
-      if( !(mapped_path[0] == '/' && mapped_path[1] == '\0') )
-              strcat( mapped_path, "/" );
-      strcat( mapped_path, dir );
+      /*
+       * 1999/08/26 Sylvain Robitaille: Check first that we won't
+       *            overrun the mapped_path buffer. Complain if we
+       *            would. Note that in our length check, the extra
+       *            2 is accounted for by the "/" we're going to
+       *            append and the terminating null.
+       */
+      if( (strlen(mapped_path) + strlen(dir) + 2) < sizeof(mapped_path) ) {
+            if( !(mapped_path[0] == '/' && mapped_path[1] == '\0') )
+                    strcat( mapped_path, "/" );
+            strcat( mapped_path, dir );
+      } else {
+            syslog(LOG_ERR, "FTPD mapped_path attack ");
+      }
 }
 
 int
@@ -1259,6 +1274,11 @@
       int ret;
       char *sl, *path;
 
+      /*
+       * 1999/08/26 Sylvain Robitaille: Both old_mapped_path and
+       *            mapped_path are MAXPATHLEN long so this is
+       *            safe.
+       */
       strcpy( old_mapped_path, mapped_path );
       path = &pathspace[0];
       strcpy( path, orig_path );
@@ -1284,6 +1304,11 @@
               do_elem( path );
 
       if( (ret = chdir( mapped_path )) < 0 ){
+      /* 
+       * 1999/08/26 Sylvain Robitaille: Both old_mapped_path and
+       *            mapped_path are MAXPATHLEN long so this is
+       *            safe.
+       */
               strcpy( mapped_path, old_mapped_path );
       }

---

From: Gregory A Lundberg <[email-masked]>
Date: Fri, 27 Aug 1999 12:22:55 -0400

On Fri, Aug 27, 1999 at 11:33:47AM -0400, Sylvain Robitaille wrote:

> MAPPING_CHDIR defined at compile time, which I'm not sure is or isn't the
> case in all installations).
> 
> of the distribution. In the meantime, I suggest that admins should make a
> point to apply this patch.

The WU-FTPD Development Group released a patch for this problem yesterday.
That patch includes yet another case missed by your patch.  The
announcement of that patch is available through our web site:

  http://www.wu-ftpd.org/

and is on our ftp site:

  ftp://ftp.wu-ftpd.org/pub/wu-ftpd/2.5.0.Security.Update.asc

please refer to that announcement for a full discussion of these issues.

-- 

Gregory A Lundberg              WU-FTPD Development Group
1441 Elmdale Drive              [email-masked]
Kettering, OH 45409-1615 USA    1-800-809-2195

---

From: Gregory A Lundberg <[email-masked]>
Date: Fri, 27 Aug 1999 13:03:41 -0400

On Fri, Aug 27, 1999 at 09:57:49AM -0700, Eugene Choi wrote:

> is there any tests i can perform to check if the patches were typed
> correctly?

You should download the patches from ftp.wu-ftpd.org and then you'd not
need to worry about typing or MIME/mailer munging.

Please see the security notice at http://www.wu-ftpd.org/ for a full
discussion of the patches and this recent security problem.

--

---

From: Sylvain Robitaille <[email-masked]>
Date: Fri, 27 Aug 1999 20:01:11 -0400 (EDT)

> The WU-FTPD Development Group released a patch for this problem

Thanks Gregory. I did try to make sure to look for such before I sent in
my patch, but I very plainly missed the official patch. :-(

> That patch includes yet another case missed by your patch.

Hrmmm... I don't see it, though I realize I should take your word for
it. :-)

If you're talking about the strcpy in mapping_getcwd, isn't the path
passed to that declared as an array of MAXPATHLEN (or was that the one
that's MAXPATHLEN + 1?) before the call?

> The announcement of that patch is available through our web site:

Again thanks. I'm sorry I missed that earlier.