lpd: much safer against malicious input. Does not fork anymore,
authorDenis Vlasenko <vda.linux@googlemail.com>
Mon, 24 Mar 2008 01:52:52 +0000 (01:52 -0000)
committerDenis Vlasenko <vda.linux@googlemail.com>
Mon, 24 Mar 2008 01:52:52 +0000 (01:52 -0000)
as this is not needed.

printutils/lpd.c

index fe89593..cac8813 100644 (file)
@@ -64,7 +64,7 @@ static char *sane(char *str)
        char *s = str;
        char *p = s;
        while (*s) {
-               if (isalnum(*s) || '-' == *s) {
+               if (isalnum(*s) || '-' == *s || '_' == *s) {
                        *p++ = *s;
                }
                s++;
@@ -73,100 +73,85 @@ static char *sane(char *str)
        return str;
 }
 
-/* vfork() disables some optimizations. Moving its use
- * to minimal, non-inlined function saves bytes */
-static NOINLINE void vfork_close_stdio_and_exec(char **argv)
+// we can use leaky setenv since we are about to exec or exit
+static void exec_helper(char **filenames, char **argv) ATTRIBUTE_NORETURN;
+static void exec_helper(char **filenames, char **argv)
 {
-       if (vfork() == 0) {
-               // CHILD
-               // we are the helper. we wanna be silent.
-               // this call reopens stdio fds to "/dev/null"
-               // (no daemonization is done)
-               bb_daemonize_or_rexec(DAEMON_DEVNULL_STDIO | DAEMON_ONLY_SANITIZE, NULL);
-               BB_EXECVP(*argv, argv);
-               _exit(127);
-       }
-}
+       char *p, *q;
+       char var[2];
 
-static void exec_helper(const char *fname, char **argv)
-{
-       char *p, *q, *file;
-       char *our_env[12];
-       int env_idx;
-
-       // read control file
-       file = q = xmalloc_open_read_close(fname, NULL);
-       // delete control file
-       unlink(fname);
+       // read and delete ctrlfile
+       q = xmalloc_open_read_close(filenames[0], NULL);
+       unlink(filenames[0]);
+       // provide datafile name
+       xsetenv("DATAFILE", filenames[1]);
        // parse control file by "\n"
-       env_idx = 0;
        while ((p = strchr(q, '\n')) != NULL
         && isalpha(*q)
-        && env_idx < ARRAY_SIZE(our_env)
        ) {
                *p++ = '\0';
                // here q is a line of <SYM><VALUE>
                // let us set environment string <SYM>=<VALUE>
-               // N.B. setenv is leaky!
-               // We have to use putenv(malloced_str),
-               // and unsetenv+free (in parent)
-               our_env[env_idx] = xasprintf("%c=%s", *q, q+1);
-               putenv(our_env[env_idx]);
-               env_idx++;
+               var[0] = *q++;
+               var[1] = '\0';
+               xsetenv(var, q);
                // next line, plz!
                q = p;
        }
-       free(file);
-
-       vfork_close_stdio_and_exec(argv);
-
-       // PARENT (or vfork error)
-       // clean up...
-       while (--env_idx >= 0) {
-               *strchrnul(our_env[env_idx], '=') = '\0';
-               unsetenv(our_env[env_idx]);
-       }
+       // we are the helper, we wanna be silent.
+       // this call reopens stdio fds to "/dev/null"
+       // (no daemonization is done)
+       bb_daemonize_or_rexec(DAEMON_DEVNULL_STDIO | DAEMON_ONLY_SANITIZE, NULL);
+       BB_EXECVP(*argv, argv);
+       exit(0);
 }
 
 static char *xmalloc_read_stdin(void)
 {
-       size_t max = 4 * 1024; /* more than enough for commands! */
+       // SECURITY:
+       size_t max = 4 * 1024; // more than enough for commands!
        return xmalloc_reads(STDIN_FILENO, NULL, &max);
 }
 
 int lpd_main(int argc, char *argv[]) MAIN_EXTERNALLY_VISIBLE;
 int lpd_main(int argc ATTRIBUTE_UNUSED, char *argv[])
 {
-       int spooling;
+       int spooling = spooling; // for compiler
+       int seen;
        char *s, *queue;
+       char *filenames[2];
 
-       // read command
-       s = xmalloc_read_stdin();
+       // goto spool directory
+       if (*++argv)
+               xchdir(*argv++);
+
+       // error messages of xfuncs will be sent over network
+       xdup2(STDOUT_FILENO, STDERR_FILENO);
 
+       filenames[0] = NULL; // ctrlfile name
+       filenames[1] = NULL; // datafile name
+
+       // read command
+       s = queue = xmalloc_read_stdin();
        // we understand only "receive job" command
-       if (2 != *s) {
+       if (2 != *queue) {
  unsupported_cmd:
                printf("Command %02x %s\n",
                        (unsigned char)s[0], "is not supported");
-               return EXIT_FAILURE;
+               goto err_exit;
        }
 
-       // goto spool directory
-       if (*++argv)
-               xchdir(*argv++);
-
-       // parse command: "\x2QUEUE_NAME\n"
-       queue = s + 1;
-       *strchrnul(s, '\n') = '\0';
-
+       // parse command: "2 | QUEUE_NAME | '\n'"
+       queue++;
        // protect against "/../" attacks
+       // *strchrnul(queue, '\n') = '\0'; - redundant, sane() will do
        if (!*sane(queue))
                return EXIT_FAILURE;
 
        // queue is a directory -> chdir to it and enter spooling mode
-       spooling = chdir(queue) + 1; /* 0: cannot chdir, 1: done */
-
-       xdup2(STDOUT_FILENO, STDERR_FILENO);
+       spooling = chdir(queue) + 1; // 0: cannot chdir, 1: done
+       seen = 0;
+       // we don't free(queue), we might need it later
 
        while (1) {
                char *fname;
@@ -176,30 +161,65 @@ int lpd_main(int argc ATTRIBUTE_UNUSED, char *argv[])
                int expected_len, real_len;
 
                // signal OK
-               write(STDOUT_FILENO, "", 1);
+               safe_write(STDOUT_FILENO, "", 1);
 
                // get subcommand
+               // valid s must be of form: "SUBCMD | LEN | space | FNAME"
+               // N.B. we bail out on any error
                s = xmalloc_read_stdin();
-               if (!s)
-                       return EXIT_SUCCESS; // probably EOF
+               if (!s) { // (probably) EOF
+                       if (spooling /* && 6 != spooling - always true */) {
+                               // we didn't see both ctrlfile & datafile!
+                               goto err_exit;
+                       }
+                       // one of only two non-error exits
+                       return EXIT_SUCCESS;
+               }
+
+               // validate input.
                // we understand only "control file" or "data file" cmds
                if (2 != s[0] && 3 != s[0])
                        goto unsupported_cmd;
-
+               if (seen & (s[0] - 1)) {
+                       printf("Duplicated subcommand\n");
+                       goto err_exit;
+               }
+               seen &= (s[0] - 1); // bit 1: ctrlfile; bit 2: datafile
+               // get filename
                *strchrnul(s, '\n') = '\0';
-               // valid s must be of form: SUBCMD | LEN | SP | FNAME
-               // N.B. we bail out on any error
                fname = strchr(s, ' ');
                if (!fname) {
-                       printf("Command %02x %s\n",
-                               (unsigned char)s[0], "lacks filename");
-                       return EXIT_FAILURE;
+// bad_fname:
+                       printf("No or bad filename\n");
+                       goto err_exit;
                }
                *fname++ = '\0';
+//             // s[0]==2: ctrlfile, must start with 'c'
+//             // s[0]==3: datafile, must start with 'd'
+//             if (fname[0] != s[0] + ('c'-2))
+//                     goto bad_fname;
+               // get length
+               expected_len = bb_strtou(s + 1, NULL, 10);
+               if (errno || expected_len < 0) {
+                       printf("Bad length\n");
+                       goto err_exit;
+               }
+               if (2 == s[0] && expected_len > 16 * 1024) {
+                       // SECURITY:
+                       // ctrlfile can't be big (we want to read it back later!)
+                       printf("File is too big\n");
+                       goto err_exit;
+               }
+
+               // open the file
                if (spooling) {
                        // spooling mode: dump both files
                        // job in flight has mode 0200 "only writable"
-                       fd = xopen3(sane(fname), O_CREAT | O_WRONLY | O_TRUNC | O_EXCL, 0200);
+                       sane(fname);
+                       fd = open3_or_warn(fname, O_CREAT | O_WRONLY | O_TRUNC | O_EXCL, 0200);
+                       if (fd < 0)
+                               goto err_exit;
+                       filenames[s[0] - 2] = xstrdup(fname);
                } else {
                        // non-spooling mode:
                        // 2: control file (ignoring), 3: data file
@@ -207,35 +227,48 @@ int lpd_main(int argc ATTRIBUTE_UNUSED, char *argv[])
                        if (3 == s[0])
                                fd = xopen(queue, O_RDWR | O_APPEND);
                }
-               expected_len = xatoi_u(s + 1);
+
+               // copy the file
                real_len = bb_copyfd_size(STDIN_FILENO, fd, expected_len);
-               if (spooling && real_len != expected_len) {
-                       unlink(fname); // don't keep corrupted files
+               if (real_len != expected_len) {
                        printf("Expected %d but got %d bytes\n",
                                expected_len, real_len);
-                       return EXIT_FAILURE;
+                       goto err_exit;
                }
-               // chmod completely downloaded file as "readable+writable" ...
+               // get ACK and see whether it is NUL (ok)
+               if (safe_read(STDIN_FILENO, s, 1) != 1 || s[0] != 0) {
+                       // don't send error msg to peer - it obviously
+                       // don't follow the protocol, so probably
+                       // it can't understand us either
+                       goto err_exit;
+               }
+
                if (spooling) {
+                       // chmod completely downloaded file as "readable+writable"
                        fchmod(fd, 0600);
-                       // ... and accumulate dump state.
+                       // accumulate dump state
                        // N.B. after all files are dumped spooling should be 1+2+3==6
                        spooling += s[0];
                }
+               free(s);
                close(fd); // NB: can do close(-1). Who cares?
 
-               // are all files dumped? -> spawn spool helper
+               // spawn spool helper and exit if all files are dumped
                if (6 == spooling && *argv) {
-                       fname[0] = 'c'; // pass control file name
-                       exec_helper(fname, argv);
+                       // signal OK
+                       safe_write(STDOUT_FILENO, "", 1);
+                       // does not return (exits 0)
+                       exec_helper(filenames, argv);
                }
-               // get ACK and see whether it is NUL (ok)
-               if (read(STDIN_FILENO, s, 1) != 1 || s[0] != 0) {
-                       // don't send error msg to peer - it obviously
-                       // don't follow the protocol, so probably
-                       // it can't understand us either
-                       return EXIT_FAILURE;
-               }
-               free(s);
-       } /* while (1) */
+       } // while (1)
+
+ err_exit:
+       // don't keep corrupted files
+       if (spooling) {
+               if (filenames[0])
+                       unlink(filenames[0]);
+               if (filenames[1])
+                       unlink(filenames[1]);
+       }
+       return EXIT_FAILURE;
 }