This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[ping7][PATCH][BZ15362] Fix fwrite() reading beyond end of buffer in error path
- From: Siddhesh Poyarekar <siddhesh at redhat dot com>
- To: libc-alpha at sourceware dot org
- Cc: Eric Biggers <ebiggers3 at gmail dot com>, carlos at redhat dot com, "Joseph S. Myers" <joseph at codesourcery dot com>, Andreas Jaeger <aj at suse dot com>, Roland McGrath <roland at hack dot frob dot com>
- Date: Fri, 11 Oct 2013 16:59:07 +0530
- Subject: [ping7][PATCH][BZ15362] Fix fwrite() reading beyond end of buffer in error path
- Authentication-results: sourceware.org; auth=none
- References: <20130922020321 dot GA9977 at zzz dot kirk dot macalester dot edu> <CAAHN_R15-MSp65h=gNEimj14Aa0f24jvGJqswCZEhyh0foCZUw at mail dot gmail dot com> <20131007070434 dot GC3178 at spoyarek dot pnq dot redhat dot com>
Ping!
On Mon, Oct 07, 2013 at 12:34:34PM +0530, Siddhesh Poyarekar wrote:
> Ping!
>
> On Tue, Oct 01, 2013 at 10:46:32AM +0530, Siddhesh Poyarekar wrote:
> > Pushing this forward since it has been pending since pre-2.18. This
> > patch has been in Fedora since the first time it was posted and we
> > haven't seen any problems reported since. Does anyone have any
> > objections if I commit this patch?
> >
> > Siddhesh
> >
> > On 22 September 2013 07:33, Eric Biggers <ebiggers3@gmail.com> wrote:
> > > Pinging my patch for BZ15362 yet again.
> > >
> > > Patch included, but it is the same as posted at
> > > https://sourceware.org/ml/libc-alpha/2013-04/msg00566.html except rebased on
> > > master and retested, and ChangeLog date updated.
> > >
> > > This patch was already reviewed and pinged a while back by Siddhesh Poyarekar
> > > (see https://sourceware.org/ml/libc-alpha/2013-06/msg00625.html), but no one
> > > else reviewed it.
> > >
> > > Note that I classified this as a critical bug when I opened it since it involves
> > > an invalid memory access being made. Even worse, invalid/unexpected data may be
> > > passed to the write() system call which could constitute an information
> > > disclosure vulnerability, although this can only occur following a write error
> > > on the same file descriptor.
> > >
> > > -----
> > >
> > > Partially revert commits 2b766585f9b4ffabeef2f36200c275976b93f2c7 and
> > > de2fd463b1c0310d75084b6d774fb974075a4ad9, which were intended to fix BZ#11741
> > > but caused another, likely worse bug, namely that fwrite() and fputs() could,
> > > in an error path, read data beyond the end of the specified buffer, and
> > > potentially even write this data to the file.
> > >
> > > Fix BZ#11741 properly by checking the return value from _IO_padn() in
> > > stdio-common/vfprintf.c.
> > >
> > > diff --git a/ChangeLog b/ChangeLog
> > > index 8f264da..952fcff 100644
> > > --- a/ChangeLog
> > > +++ b/ChangeLog
> > > @@ -1,3 +1,14 @@
> > > +2013-09-12 Eric Biggers <ebiggers3@gmail.com>
> > > +
> > > + [BZ #15362]
> > > + * libio/fileops.c: Revert problematic fixes for [BZ #11741]
> > > + * libio/iofwrite.c: Likewise.
> > > + * libio/iofwrite_u.c: Likewise.
> > > + * libio/iopadn.c: Likewise.
> > > + * libio/iowpadn.c: Likewise.
> > > + * stdio-common/vfprintf.c: Fix [BZ #11741] properly by checking whether
> > > + _IO_padn() returned the full count written.
> > > +
> > > 2013-09-11 Jia Liu <proljc@gmail.com>
> > >
> > > * sunrpc/rpc/types.h [__APPLE_CC__]: Define __u_char_defined and
> > > diff --git a/libio/fileops.c b/libio/fileops.c
> > > index e92f85b..c58e860 100644
> > > --- a/libio/fileops.c
> > > +++ b/libio/fileops.c
> > > @@ -1245,13 +1245,12 @@ _IO_new_file_write (f, data, n)
> > > _IO_ssize_t n;
> > > {
> > > _IO_ssize_t to_do = n;
> > > - _IO_ssize_t count = 0;
> > > while (to_do > 0)
> > > {
> > > - count = (__builtin_expect (f->_flags2
> > > - & _IO_FLAGS2_NOTCANCEL, 0)
> > > - ? write_not_cancel (f->_fileno, data, to_do)
> > > - : write (f->_fileno, data, to_do));
> > > + _IO_ssize_t count = (__builtin_expect (f->_flags2
> > > + & _IO_FLAGS2_NOTCANCEL, 0)
> > > + ? write_not_cancel (f->_fileno, data, to_do)
> > > + : write (f->_fileno, data, to_do));
> > > if (count < 0)
> > > {
> > > f->_flags |= _IO_ERR_SEEN;
> > > @@ -1263,7 +1262,7 @@ _IO_new_file_write (f, data, n)
> > > n -= to_do;
> > > if (f->_offset >= 0)
> > > f->_offset += n;
> > > - return count < 0 ? count : n;
> > > + return n;
> > > }
> > >
> > > _IO_size_t
> > > @@ -1323,13 +1322,11 @@ _IO_new_file_xsputn (f, data, n)
> > > _IO_size_t block_size, do_write;
> > > /* Next flush the (full) buffer. */
> > > if (_IO_OVERFLOW (f, EOF) == EOF)
> > > - /* If nothing else has to be written or nothing has been written, we
> > > - must not signal the caller that the call was even partially
> > > - successful. */
> > > - return (to_do == 0 || to_do == n) ? EOF : n - to_do;
> > > + /* If nothing else has to be written we must not signal the
> > > + caller that everything has been written. */
> > > + return to_do == 0 ? EOF : n - to_do;
> > >
> > > - /* Try to maintain alignment: write a whole number of blocks.
> > > - dont_write is what gets left over. */
> > > + /* Try to maintain alignment: write a whole number of blocks. */
> > > block_size = f->_IO_buf_end - f->_IO_buf_base;
> > > do_write = to_do - (block_size >= 128 ? to_do % block_size : 0);
> > >
> > > diff --git a/libio/iofwrite.c b/libio/iofwrite.c
> > > index 81596a6..66542ea 100644
> > > --- a/libio/iofwrite.c
> > > +++ b/libio/iofwrite.c
> > > @@ -42,12 +42,12 @@ _IO_fwrite (buf, size, count, fp)
> > > if (_IO_vtable_offset (fp) != 0 || _IO_fwide (fp, -1) == -1)
> > > written = _IO_sputn (fp, (const char *) buf, request);
> > > _IO_release_lock (fp);
> > > - /* We are guaranteed to have written all of the input, none of it, or
> > > - some of it. */
> > > - if (written == request)
> > > + /* We have written all of the input in case the return value indicates
> > > + this or EOF is returned. The latter is a special case where we
> > > + simply did not manage to flush the buffer. But the data is in the
> > > + buffer and therefore written as far as fwrite is concerned. */
> > > + if (written == request || written == EOF)
> > > return count;
> > > - else if (written == EOF)
> > > - return 0;
> > > else
> > > return written / size;
> > > }
> > > diff --git a/libio/iofwrite_u.c b/libio/iofwrite_u.c
> > > index 4a9d6ca..18dc6d0 100644
> > > --- a/libio/iofwrite_u.c
> > > +++ b/libio/iofwrite_u.c
> > > @@ -44,12 +44,12 @@ fwrite_unlocked (buf, size, count, fp)
> > > if (_IO_fwide (fp, -1) == -1)
> > > {
> > > written = _IO_sputn (fp, (const char *) buf, request);
> > > - /* We are guaranteed to have written all of the input, none of it, or
> > > - some of it. */
> > > - if (written == request)
> > > + /* We have written all of the input in case the return value indicates
> > > + this or EOF is returned. The latter is a special case where we
> > > + simply did not manage to flush the buffer. But the data is in the
> > > + buffer and therefore written as far as fwrite is concerned. */
> > > + if (written == request || written == EOF)
> > > return count;
> > > - else if (written == EOF)
> > > - return 0;
> > > }
> > >
> > > return written / size;
> > > diff --git a/libio/iopadn.c b/libio/iopadn.c
> > > index cc93c0f..5ebbcf4 100644
> > > --- a/libio/iopadn.c
> > > +++ b/libio/iopadn.c
> > > @@ -59,7 +59,7 @@ _IO_padn (fp, pad, count)
> > > w = _IO_sputn (fp, padptr, PADSIZE);
> > > written += w;
> > > if (w != PADSIZE)
> > > - return w == EOF ? w : written;
> > > + return written;
> > > }
> > >
> > > if (i > 0)
> > > diff --git a/libio/iowpadn.c b/libio/iowpadn.c
> > > index d94db71..5600f37 100644
> > > --- a/libio/iowpadn.c
> > > +++ b/libio/iowpadn.c
> > > @@ -65,7 +65,7 @@ _IO_wpadn (fp, pad, count)
> > > w = _IO_sputn (fp, (char *) padptr, PADSIZE);
> > > written += w;
> > > if (w != PADSIZE)
> > > - return w == EOF ? w : written;
> > > + return written;
> > > }
> > >
> > > if (i > 0)
> > > diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
> > > index fb22f69..8cd7a85 100644
> > > --- a/stdio-common/vfprintf.c
> > > +++ b/stdio-common/vfprintf.c
> > > @@ -90,13 +90,13 @@
> > > do { \
> > > if (width > 0) \
> > > { \
> > > - unsigned int d = _IO_padn (s, (Padchar), width); \
> > > - if (__glibc_unlikely (d == EOF)) \
> > > + _IO_ssize_t written = _IO_padn (s, (Padchar), width); \
> > > + if (__glibc_unlikely (written != width)) \
> > > { \
> > > done = -1; \
> > > goto all_done; \
> > > } \
> > > - done_add (d); \
> > > + done_add (written); \
> > > } \
> > > } while (0)
> > > # define PUTC(C, F) _IO_putc_unlocked (C, F)
> > > @@ -119,13 +119,13 @@
> > > do { \
> > > if (width > 0) \
> > > { \
> > > - unsigned int d = _IO_wpadn (s, (Padchar), width); \
> > > - if (__glibc_unlikely (d == EOF)) \
> > > + _IO_ssize_t written = _IO_wpadn (s, (Padchar), width); \
> > > + if (__glibc_unlikely (written != width)) \
> > > { \
> > > done = -1; \
> > > goto all_done; \
> > > } \
> > > - done_add (d); \
> > > + done_add (written); \
> > > } \
> > > } while (0)
> > > # define PUTC(C, F) _IO_putwc_unlocked (C, F)
> >
> >
> >
> > --
> > http://siddhesh.in