Subject: ftpaccess/upload bug

From: Andrew Ford <[email-masked]>
Date: Tue, 21 Jan 1997 18:28:12 +0100

I sent the enclosed two messages to [email-masked].  I
haven't yet had any response (other than an automatic
acknowledgement), but they may also be of interest on this list.  Note
however that I cannot guarantee that the included patch has been fully
tested.

Andrew
-- 
Andrew Ford                             Email:  [email-masked]
Independent Software Consultant         WWW:    http://www.nhbs.co.uk/aford/
"Brittany", Wells Road,                 Tel:    +44 1452 770836
Eastcombe, Stroud, GL6 7EE, GB          Fax:    +44 1452 770835

--------------------------------------------------------------------------------
Message 1:

Version: wu-ftpd-2.4.2-beta-11
O/S:	 Linux 1.2.3		(but I think its a general bug)
CC:	 gcc 2.7.2

ftpaccess includes:

	upload	/ftp	/for-andrew	yes andrew c+a 0660 dirs
	upload	/ftp	/pub/incoming	yes root root 0600 nodirs

Ftping as a guest user and making a directory in /for-andrew results
in 530 (permission denied).  Removing the second upload or setting it
to "dirs" allows directories to be created in /for-andrew.  I.e. the
directives are interfering with each other.

dir_check() in extensions.c includes the following code:

  i = match_value;  /* i is -1 here */
  while (getaclentry("upload", &entry) && ARG0 && ARG1 && ARG2 != NULL) {
 /* CERT suggested fix (change && to || ) */
    if ( (!strcmp(ARG0, pw->pw_dir))  ||
		   ((i = path_compare(ARG1, cwdir)) >= match_value) ) {
          match_value = i;
          ap2 = ARG2;
          if (ARG3)  ap3 = ARG3;
          else       ap3 = NULL;
          if (ARG4)  ap4 = ARG4;
          else       ap4 = NULL;
          if (ARG6)  ap6 = ARG6;
          else       ap6 = NULL;
      }
    if ( ((ap2 && !strcasecmp(ap2, "no")) && 
	  (ap3 && strcasecmp(ap3, "dirs"))) || 
	 (ap3 && !strcasecmp(ap3, "nodirs")) ||
		(ap6 && !strcasecmp(ap6, "nodirs")) ) {
      reply(530, "%s: Permission denied.  (Mkdir)", name);
      return(0);
    }
  }

The original code (as the comment) suggests had && instead of ||, but
also finished the while loop after the first if-statement.  The effect
of the change is that any "upload" which disallows creating
directories will deny permission for all directory creation for all
directories for which the root-dir matches.

Andrew
-- 
Andrew Ford                             Email:  [email-masked]
Independent Software Consultant         WWW:    http://www.nhbs.co.uk/aford/
"Brittany", Wells Road,                 Tel:    +44 1452 770836
Eastcombe, Stroud, GL6 7EE, GB          Fax:    +44 1452 770835

--------------------------------------------------------------------------------
Message 2:
Subject: Re: [ACADEM-SW-SUPPORT #144] ftpaccess/upload bug

Further to my bug report I have enhanced wu-ftpd-2.4.2-beta-11 to add
a mode for directory creation to the "upload" directive.  The
<dirmode> parameter is tacked onto the end of the line so it shouldn't
break existing ftpaccess files.  The patch is appended (ftpaccess.5,
ftpd.c and extensions.c), this reverses a previous change in dir_check
(which I think was wrong)

Andrew
-- 
Andrew Ford                             Email:  [email-masked]
Independent Software Consultant         WWW:    http://www.nhbs.co.uk/aford/
"Brittany", Wells Road,                 Tel:    +44 1452 770836
Eastcombe, Stroud, GL6 7EE, GB          Fax:    +44 1452 770835

*** doc/ftpaccess.5.orig	Mon Apr 15 05:50:11 1996
--- doc/ftpaccess.5	Thu Jan  9 13:40:20 1997
***************
*** 358,364 ****
  begin with a "." or a "-".  If the filename is invalid, /etc/pathmsg
  will be displayed to the user.
  .TP 0.5i
! .B upload <root-dir> <dirglob> <yes|no> <owner> <group> <mode> ["dirs"|"nodirs"]
  Define a directory with <dirglob> that permits or
  denies uploads.
  
--- 358,364 ----
  begin with a "." or a "-".  If the filename is invalid, /etc/pathmsg
  will be displayed to the user.
  .TP 0.5i
! .B upload <root-dir> <dirglob> <yes|no> <owner> <group> <mode> ["dirs" [<dirmode>]|"nodirs"]
  Define a directory with <dirglob> that permits or
  denies uploads.
  
***************
*** 384,390 ****
  
  The optional "dirs" and "nodirs" keywords can be
  specified to allow or disallow the creation of
! new subdirectories using the mkdir command.
  
  The upload keyword only applies to users who
  have a home directory (the argument to the chroot() )
--- 384,392 ----
  
  The optional "dirs" and "nodirs" keywords can be
  specified to allow or disallow the creation of
! new subdirectories using the mkdir command.  The optional <dirmode>
! specifies permissions for directories that are
! created.  If it is omitted 0777 is used.
  
  The upload keyword only applies to users who
  have a home directory (the argument to the chroot() )
*** src/extensions.c.orig	Mon Apr 15 05:51:05 1996
--- src/extensions.c	Thu Jan  9 13:31:22 1997
***************
*** 926,937 ****
  
  int
  #ifdef __STDC__
! dir_check(char *name, uid_t *uid, gid_t *gid, int *valid)
  #else
! dir_check(name,uid,gid,valid)
  char *name;
  uid_t *uid;
  gid_t *gid;
  int *valid;
  #endif
  {
--- 926,938 ----
  
  int
  #ifdef __STDC__
! dir_check(char *name, uid_t *uid, gid_t *gid, mode_t *mode, int *valid)
  #else
! dir_check(name,uid,gid,mode,valid)
  char *name;
  uid_t *uid;
  gid_t *gid;
+ mode_t *mode;
  int *valid;
  #endif
  {
***************
*** 942,948 ****
    char *ap2 = NULL,
         *ap3 = NULL,
         *ap4 = NULL,
!        *ap6 = NULL;
    char cwdir[BUFSIZ];
    char path[BUFSIZ];
    char *sp;
--- 943,950 ----
    char *ap2 = NULL,
         *ap3 = NULL,
         *ap4 = NULL,
!        *ap6 = NULL,
!        *ap7 = NULL;
    char cwdir[BUFSIZ];
    char path[BUFSIZ];
    char *sp;
***************
*** 962,968 ****
    i = match_value;  /* i is -1 here */
    while (getaclentry("upload", &entry) && ARG0 && ARG1 && ARG2 != NULL) {
   /* CERT suggested fix (change && to || ) */
!     if ( (!strcmp(ARG0, pw->pw_dir))  ||
  		   ((i = path_compare(ARG1, cwdir)) >= match_value) ) {
            match_value = i;
            ap2 = ARG2;
--- 964,971 ----
    i = match_value;  /* i is -1 here */
    while (getaclentry("upload", &entry) && ARG0 && ARG1 && ARG2 != NULL) {
   /* CERT suggested fix (change && to || ) */
!  /* The fix seems to be wrong -- changing back to && */
!     if ( (!strcmp(ARG0, pw->pw_dir))  &&
  		   ((i = path_compare(ARG1, cwdir)) >= match_value) ) {
            match_value = i;
            ap2 = ARG2;
***************
*** 972,978 ****
--- 975,985 ----
            else       ap4 = NULL;
            if (ARG6)  ap6 = ARG6;
            else       ap6 = NULL;
+ 	  if (ARG7)  ap7 = ARG7;
+ 	  else       ap7 = NULL;
      }
+   }
+   
    if ( ((ap2 && !strcasecmp(ap2, "no")) && 
  	(ap3 && strcasecmp(ap3, "dirs"))) || 
         (ap3 && !strcasecmp(ap3, "nodirs")) ||
***************
*** 980,992 ****
        reply(530, "%s: Permission denied.  (Mkdir)", name);
        return(0);
      }
-   }
    if (ap3)
       *uid = atoi(ap3);    /* the uid  */
    if (ap4) {
       *gid = atoi(ap4);    /* the gid */
       *valid = 1;
     }
    return(1);
  }
  
--- 987,1003 ----
      reply(530, "%s: Permission denied.  (Mkdir)", name);
      return(0);
    }
    if (ap3)
       *uid = atoi(ap3);    /* the uid  */
    if (ap4) {
       *gid = atoi(ap4);    /* the gid */
       *valid = 1;
    }
+   if (ap7) {		  /* the mode for the directory */
+       i = (int)strtol(ap7, &sp, 8);
+       if (sp > ap7)
+ 	  *mode = i;
+   }
    return(1);
  }
  
*** src/ftpd.c.orig	Mon Apr 15 05:51:11 1996
--- src/ftpd.c	Thu Jan  9 12:47:35 1997
***************
*** 2804,2817 ****
  char *name;
  #endif
  {
  	uid_t uid;
  	gid_t gid;
  	int   valid = 0;
  
      /*
       * check the directory, can we mkdir here?
       */
!     if ( (dir_check(name, &uid, &gid, &valid)) <= 0 )
          return;
  
      /*
--- 2804,2819 ----
  char *name;
  #endif
  {
+         mode_t mode = 0777;
  	uid_t  uid;
  	gid_t  gid;
+ 	uid_t  oldid;
  	int    valid = 0;
  
      /*
       * check the directory, can we mkdir here?
       */
!     if ( (dir_check(name, &uid, &gid, &mode, &valid)) <= 0 )
          return;
  
      /*
***************
*** 2820,2830 ****
      if ( (fn_check(name)) <= 0 )
          return;
  
!     if (mkdir(name, 0777) < 0) {
          perror_reply(550, name);
  	return;
      }
  
      reply(257, "MKD command successful.");
  }
  
--- 2822,2844 ----
      if ( (fn_check(name)) <= 0 )
          return;
  
!     if (mkdir(name, mode) < 0) {
          perror_reply(550, name);
  	return;
      }
  
+     /* if we have a uid and gid, then use them. */
+ 
+     if (valid > 0) {
+         oldid = geteuid();
+         (void) seteuid((uid_t) 0);
+         if ((chown(name, uid, gid)) < 0) {
+             (void) seteuid(oldid);
+             perror_reply(550, "chown");
+             return;
+         }
+         (void) seteuid(oldid);
+     }
      reply(257, "MKD command successful.");
  }
  
***************
*** 2836,2848 ****
  char *name;
  #endif
  {
! 	int c, d;  /* dummy variables */
          int valid = 0;
  
      /*
       * check the directory, can we rmdir here?
       */
!     if ( (dir_check(name, &c, &d, &valid)) <= 0 )
          return;
  
      /*
--- 2850,2862 ----
  char *name;
  #endif
  {
! 	int c, d, m;  /* dummy variables */
          int valid = 0;
  
      /*
       * check the directory, can we rmdir here?
       */
!     if ( (dir_check(name, &c, &d, &m, &valid)) <= 0 )
          return;
  
      /*
==END==