This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[ping6][PATCH][BZ15362] Fix fwrite() reading beyond end of buffer in error path


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]