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

[Karel Zak <kzak@redhat.com>]

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