diff: simplify globbing in ftpd(8)

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

diff: simplify globbing in ftpd(8)

Jan Klemkow
Hi,

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.

Tested it manually and by regression test.

OK?

Bye,
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 13 Jan 2020 12:07:14 -0000
@@ -60,51 +60,44 @@
 #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";
 
- /* 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 +121,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 = 2; argv[argc] != NULL; argc++)
+ free(argv[argc]);
 
  return (iop);
 }

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: diff: simplify globbing in ftpd(8)

Todd C. Miller-3
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.

 - todd

Reply | Threaded
Open this post in threaded view
|

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.

Oh, I missed that.  And will it checkout first.

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: diff: simplify globbing in ftpd(8)

Jan Klemkow
In reply to this post by Todd C. Miller-3
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);
 }