This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[ping6][PATCH][BZ15362] Fix fwrite() reading beyond end of buffer in error path
- From: Siddhesh Poyarekar <siddhesh at redhat dot com>
- To: Siddhesh Poyarekar <siddhesh dot poyarekar at gmail dot com>
- Cc: Eric Biggers <ebiggers3 at gmail dot com>, GNU C Library <libc-alpha at sourceware dot org>, 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: Mon, 7 Oct 2013 12:34:34 +0530
- Subject: [ping6][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>
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