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

[Jann Horn <jann@thejh.net>]

[-- 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 --]