Re: diff: simplify globbing in ftpd(8)

[Jan Klemkow]

On Mon, Jan 13, 2020 at 10:30:11AM -0700, Todd C. Miller wrote:
> On Mon, 13 Jan 2020 13:45:55 +0100, Jan Klemkow wrote:
> 
> > This diff simplifies globbing for the ftpd(8) ls and stat command.  And
> > it avoids to glob for the parameters "-lgA".
> >
> > Due, we just pass a single string to the ftpd_ls() function, we don't
> > need to provide infrastructure for a dynamic list of arguments.
> 
> This conflicts with the diff to support NLIST arguments from SASANO
> Takayoshi.  I think we can support this by adding a flag to ftpd_ls()
> that indicates whether it is a long list or not.

Here is an improved version of the diff, but as I explaind here [1]
without argument injection.  I stopped argument injection by adding
"--" before client paramenters for ls(1).

[1]: https://marc.info/?l=openbsd-tech&m=157900764005335&w=2

OK?

Thanks,
Jan

Index: extern.h
===================================================================
RCS file: /cvs/src/libexec/ftpd/extern.h,v
retrieving revision 1.20
diff -u -p -r1.20 extern.h
--- extern.h    8 May 2019 23:56:48 -0000       1.20
+++ extern.h    13 Jan 2020 11:03:39 -0000
@@ -68,7 +68,7 @@ void  delete(char *);
 void   dologout(int);
 void   fatal(char *);
 int    ftpd_pclose(FILE *, pid_t);
-FILE   *ftpd_ls(char *, char *, pid_t *);
+FILE   *ftpd_ls(const char *, pid_t *);
 int     get_line(char *, int, FILE *);
 void   ftpdlogwtmp(char *, char *, char *);
 void   lreply(int, const char *, ...);
Index: ftpd.c
===================================================================
RCS file: /cvs/src/libexec/ftpd/ftpd.c,v
retrieving revision 1.228
diff -u -p -r1.228 ftpd.c
--- ftpd.c      3 Jul 2019 03:24:04 -0000       1.228
+++ ftpd.c      13 Jan 2020 11:15:31 -0000
@@ -1124,7 +1124,7 @@ retrieve(enum ret_cmd cmd, char *name)
                fin = fopen(name, "r");
                st.st_size = 0;
        } else {
-               fin = ftpd_ls("-lgA", name, &pid);
+               fin = ftpd_ls(name, &pid);
                st.st_size = -1;
                st.st_blksize = BUFSIZ;
        }
@@ -1730,7 +1730,7 @@ statfilecmd(char *filename)
        int c;
        int atstart;
        pid_t pid;
-       fin = ftpd_ls("-lgA", filename, &pid);
+       fin = ftpd_ls(filename, &pid);
        if (fin == NULL) {
                reply(451, "Local resource failure");
                return;
Index: popen.c
===================================================================
RCS file: /cvs/src/libexec/ftpd/popen.c,v
retrieving revision 1.28
diff -u -p -r1.28 popen.c
--- popen.c     28 Jun 2019 13:32:53 -0000      1.28
+++ popen.c     14 Jan 2020 18:54:52 -0000
@@ -60,51 +60,45 @@
 #define MAX_GARGV      1000
 
 FILE *
-ftpd_ls(char *arg, char *path, pid_t *pidptr)
+ftpd_ls(const char *path, pid_t *pidptr)
 {
        char *cp;
        FILE *iop;
-       int argc = 0, gargc, pdes[2];
+       int argc = 0, pdes[2];
        pid_t pid;
-       char **pop, *argv[MAX_ARGV], *gargv[MAX_GARGV];
+       char **pop, *argv[MAX_ARGV];
 
        if (pipe(pdes) == -1)
                return (NULL);
 
        /* break up string into pieces */
        argv[argc++] = "/bin/ls";
-       if (arg != NULL)
-               argv[argc++] = arg;
-       if (path != NULL)
-               argv[argc++] = path;
-       argv[argc] = NULL;
-       argv[MAX_ARGV-1] = NULL;
+       argv[argc++] = "-lgA";
+       argv[argc++] = "--";
 
-       /* glob each piece */
-       gargv[0] = argv[0];
-       for (gargc = argc = 1; argv[argc]; argc++) {
+       /* glob that path */
+       if (path != NULL) {
                glob_t gl;
 
                memset(&gl, 0, sizeof(gl));
-               if (glob(argv[argc],
+               if (glob(path,
                    GLOB_BRACE|GLOB_NOCHECK|GLOB_QUOTE|GLOB_TILDE|GLOB_LIMIT,
                    NULL, &gl)) {
-                       if (gargc < MAX_GARGV-1) {
-                               gargv[gargc++] = strdup(argv[argc]);
-                               if (gargv[gargc -1] == NULL)
-                                       fatal ("Out of memory.");
-                       }
+                       argv[argc++] = strdup(path);
+                       if (argv[argc -1] == NULL)
+                               fatal ("Out of memory.");
 
                } else if (gl.gl_pathc > 0) {
-                       for (pop = gl.gl_pathv; *pop && gargc < MAX_GARGV-1; 
pop++) {
-                               gargv[gargc++] = strdup(*pop);
-                               if (gargv[gargc - 1] == NULL)
+                       for (pop = gl.gl_pathv; *pop && argc < MAX_GARGV-1; 
pop++) {
+                               argv[argc++] = strdup(*pop);
+                               if (argv[argc - 1] == NULL)
                                        fatal ("Out of memory.");
                        }
                }
                globfree(&gl);
        }
-       gargv[gargc] = NULL;
+       argv[argc] = NULL;
+       argv[MAX_ARGV-1] = NULL;
 
        iop = NULL;
 
@@ -128,15 +122,15 @@ ftpd_ls(char *arg, char *path, pid_t *pi
 
                /* reset getopt for ls_main */
                optreset = optind = 1;
-               exit(ls_main(gargc, gargv));
+               exit(ls_main(argc, argv));
        }
        /* parent; assume fdopen can't fail...  */
        iop = fdopen(pdes[0], "r");
        (void)close(pdes[1]);
        *pidptr = pid;
 
-pfree: for (argc = 1; gargv[argc] != NULL; argc++)
-               free(gargv[argc]);
+pfree: for (argc = 3; argv[argc] != NULL; argc++)
+               free(argv[argc]);
 
        return (iop);
 }