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