← Commit list

39c77c373

Denis Vlasenko

2008-03-24

FAILED Duration: 26s

Subject

lpd: much safer against malicious input. Does not fork anymore, as this is not needed.

Binary

No binary (build failed)

Architecture & Compiler Flags

Architecture: x86_64-linux-gnu Compiler: gcc (Ubuntu 11.4.0-1ubuntu1~22.04.3) 11.4.0

—

Build Log

(log yok)

Commit Diff GitHub →

printutils/lpd.c +120 -87

printutils/lpd.c+120 -87

@@ -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;
	}