| Message ID | 1511962793-14533-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | cc683f7ed4a5bd8ce2c9b715581de727b04eb599 |
| Headers | show |
| Series | libio: Free backup area when it not required (BZ#22415) | expand |
Ping. On 29/11/2017 11:39, Adhemerval Zanella wrote: > Some libio operations fail to correctly free the backup area (created > by _IO_{w}default_pbackfail on unget{w}c) resulting in either invalid > buffer free operations or memory leaks. > > For instance, on the example provided by BZ#22415 a following > fputc after a fseek to rewind the stream issues an invalid free on > the buffer. It is because although _IO_file_overflow correctly > (from fputc) correctly calls _IO_free_backup_area, the > _IO_new_file_seekoff (called by fseek) updates the FILE internal > pointers without first free the backup area (resulting in invalid > values in the internal pointers). > > The wide version also shows an issue, but instead of accessing invalid > pointers it leaks the backup memory on fseek/fputwc operation. > > Checked on x86_64-linux-gnu and i686-linux-gnu. > > * libio/Makefile (tests): Add tst-bz22415. > (tst-bz22415-ENV): New rule. > (generated): Add tst-bz22415.mtrace and tst-bz22415.check. > (tests-special): Add tst-bz22415-mem.out. > ($(objpfx)tst-bz22415-mem.out): New rule. > * libio/fileops.c (_IO_new_file_seekoff): Call _IO_free_backup_area > in case of a successful seek operation. > * libio/wfileops.c (_IO_wfile_seekoff): Likewise. > (_IO_wfile_overflow): Call _IO_free_wbackup_area in case a write > buffer is required. > * libio/tst-bz22415.c: New test. > --- > ChangeLog | 14 ++++++++ > libio/Makefile | 11 ++++-- > libio/fileops.c | 3 ++ > libio/tst-bz22415.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > libio/wfileops.c | 4 +++ > 5 files changed, 127 insertions(+), 2 deletions(-) > create mode 100644 libio/tst-bz22415.c > > diff --git a/libio/Makefile b/libio/Makefile > index 9d09bd8..70b5530 100644 > --- a/libio/Makefile > +++ b/libio/Makefile > @@ -62,7 +62,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \ > bug-memstream1 bug-wmemstream1 \ > tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \ > tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ > - tst-ftell-append tst-fputws > + tst-ftell-append tst-fputws tst-bz22415 > ifeq (yes,$(build-shared)) > # Add test-fopenloc only if shared library is enabled since it depends on > # shared localedata objects. > @@ -151,9 +151,11 @@ tst_wprintf2-ARGS = "Some Text" > > test-fmemopen-ENV = MALLOC_TRACE=$(objpfx)test-fmemopen.mtrace > tst-fopenloc-ENV = MALLOC_TRACE=$(objpfx)tst-fopenloc.mtrace > +tst-bz22415-ENV = MALLOC_TRACE=$(objpfx)tst-bz22415.mtrace > > generated += test-fmemopen.mtrace test-fmemopen.check > generated += tst-fopenloc.mtrace tst-fopenloc.check > +generated += tst-bz22415.mtrace tst-bz22415.check > > aux := fileops genops stdfiles stdio strops > > @@ -167,7 +169,8 @@ shared-only-routines = oldiofopen oldiofdopen oldiofclose oldfileops \ > oldiofsetpos64 > > ifeq ($(run-built-tests),yes) > -tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-mem.out > +tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-mem.out \ > + $(objpfx)tst-bz22415-mem.out > ifeq (yes,$(build-shared)) > # Run tst-fopenloc-cmp.out and tst-openloc-mem.out only if shared > # library is enabled since they depend on tst-fopenloc.out. > @@ -218,3 +221,7 @@ $(objpfx)test-fmemopen-mem.out: $(objpfx)test-fmemopen.out > $(objpfx)tst-fopenloc-mem.out: $(objpfx)tst-fopenloc.out > $(common-objpfx)malloc/mtrace $(objpfx)tst-fopenloc.mtrace > $@; \ > $(evaluate-test) > + > +$(objpfx)tst-bz22415-mem.out: $(objpfx)tst-bz22415.out > + $(common-objpfx)malloc/mtrace $(objpfx)tst-bz22415.mtrace > $@; \ > + $(evaluate-test) > diff --git a/libio/fileops.c b/libio/fileops.c > index a9e948f..72a0cb5 100644 > --- a/libio/fileops.c > +++ b/libio/fileops.c > @@ -994,6 +994,9 @@ _IO_new_file_seekoff (_IO_FILE *fp, _IO_off64_t offset, int dir, int mode) > goto dumb; > } > } > + > + _IO_free_backup_area (fp); > + > /* At this point, dir==_IO_seek_set. */ > > /* If destination is within current buffer, optimize: */ > diff --git a/libio/tst-bz22415.c b/libio/tst-bz22415.c > new file mode 100644 > index 0000000..d7b23fe > --- /dev/null > +++ b/libio/tst-bz22415.c > @@ -0,0 +1,97 @@ > +/* Check static buffer handling with setvbuf (BZ #22415) > + > + Copyright (C) 2017 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>. */ > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <wchar.h> > +#include <mcheck.h> > + > +#include <support/check.h> > +#include <support/temp_file.h> > + > +static int > +do_test (void) > +{ > + mtrace (); > + > + char *temp_file; > + TEST_VERIFY_EXIT (create_temp_file ("tst-bz22145.", &temp_file)); > + > + char buf[BUFSIZ]; > + > + { > + /* Check if backup buffer is correctly freed and changing back > + to normal buffer does not trigger an invalid free in case of > + static buffer set by setvbuf. */ > + > + FILE *f = fopen (temp_file, "w+b"); > + TEST_VERIFY_EXIT (f != NULL); > + > + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0); > + TEST_VERIFY_EXIT (ungetc ('x', f) == 'x'); > + TEST_VERIFY_EXIT (fseek (f, 0L, SEEK_SET) == 0); > + TEST_VERIFY_EXIT (fputc ('y', f) == 'y'); > + > + TEST_VERIFY_EXIT (fclose (f) == 0); > + } > + > + { > + /* Check if backup buffer is correctly freed and changing back > + to normal buffer does not trigger an invalid free in case of > + static buffer set by setvbuf. */ > + > + FILE *f = fopen (temp_file, "w+b"); > + TEST_VERIFY_EXIT (f != NULL); > + > + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0); > + TEST_VERIFY_EXIT (ungetc ('x', f) == 'x'); > + TEST_VERIFY_EXIT (fputc ('y', f) == 'y'); > + > + TEST_VERIFY_EXIT (fclose (f) == 0); > + } > + > + { > + FILE *f = fopen (temp_file, "w+b"); > + TEST_VERIFY_EXIT (f != NULL); > + > + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0); > + TEST_VERIFY_EXIT (ungetwc (L'x', f) == L'x'); > + TEST_VERIFY_EXIT (fseek (f, 0L, SEEK_SET) == 0); > + TEST_VERIFY_EXIT (fputwc (L'y', f) == L'y'); > + > + TEST_VERIFY_EXIT (fclose (f) == 0); > + } > + > + { > + FILE *f = fopen (temp_file, "w+b"); > + TEST_VERIFY_EXIT (f != NULL); > + > + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0); > + TEST_VERIFY_EXIT (ungetwc (L'x', f) == L'x'); > + TEST_VERIFY_EXIT (fputwc (L'y', f) == L'y'); > + > + TEST_VERIFY_EXIT (fclose (f) == 0); > + } > + > + free (temp_file); > + > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/libio/wfileops.c b/libio/wfileops.c > index 8756b6f..91139e1 100644 > --- a/libio/wfileops.c > +++ b/libio/wfileops.c > @@ -421,6 +421,7 @@ _IO_wfile_overflow (_IO_FILE *f, wint_t wch) > if (f->_wide_data->_IO_write_base == 0) > { > _IO_wdoallocbuf (f); > + _IO_free_wbackup_area (f); > _IO_wsetg (f, f->_wide_data->_IO_buf_base, > f->_wide_data->_IO_buf_base, f->_wide_data->_IO_buf_base); > > @@ -850,6 +851,9 @@ _IO_wfile_seekoff (_IO_FILE *fp, _IO_off64_t offset, int dir, int mode) > goto dumb; > } > } > + > + _IO_free_wbackup_area (fp); > + > /* At this point, dir==_IO_seek_set. */ > > /* If destination is within current buffer, optimize: */ >
On Nov 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > * libio/Makefile (tests): Add tst-bz22415. > (tst-bz22415-ENV): New rule. > (generated): Add tst-bz22415.mtrace and tst-bz22415.check. > (tests-special): Add tst-bz22415-mem.out. > ($(objpfx)tst-bz22415-mem.out): New rule. > * libio/fileops.c (_IO_new_file_seekoff): Call _IO_free_backup_area > in case of a successful seek operation. > * libio/wfileops.c (_IO_wfile_seekoff): Likewise. > (_IO_wfile_overflow): Call _IO_free_wbackup_area in case a write > buffer is required. > * libio/tst-bz22415.c: New test. Ok. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
On 11/29/2017 02:39 PM, Adhemerval Zanella wrote: > Some libio operations fail to correctly free the backup area (created > by_IO_{w}default_pbackfail on unget{w}c) resulting in either invalid > buffer free operations or memory leaks. > > For instance, on the example provided by BZ#22415 a following > fputc after a fseek to rewind the stream issues an invalid free on > the buffer. Do we have to treat this as a security bug? I don't think so, but I'd still like a second opinion. Thanks, Florian
On 14/12/2017 09:57, Florian Weimer wrote: > On 11/29/2017 02:39 PM, Adhemerval Zanella wrote: >> Some libio operations fail to correctly free the backup area (created >> by_IO_{w}default_pbackfail on unget{w}c) resulting in either invalid >> buffer free operations or memory leaks. >> >> For instance, on the example provided by BZ#22415 a following >> fputc after a fseek to rewind the stream issues an invalid free on >> the buffer. > > Do we have to treat this as a security bug? I don't think so, but I'd still like a second opinion. > > Thanks, > Florian I am not well versed in exploit crafting, so how do we usually handle invalid free on user provided buffers?
On 12/14/2017 04:36 PM, Adhemerval Zanella wrote: > > > On 14/12/2017 09:57, Florian Weimer wrote: >> On 11/29/2017 02:39 PM, Adhemerval Zanella wrote: >>> Some libio operations fail to correctly free the backup area (created >>> by_IO_{w}default_pbackfail on unget{w}c) resulting in either invalid >>> buffer free operations or memory leaks. >>> >>> For instance, on the example provided by BZ#22415 a following >>> fputc after a fseek to rewind the stream issues an invalid free on >>> the buffer. >> >> Do we have to treat this as a security bug? I don't think so, but I'd still like a second opinion. >> >> Thanks, >> Florian > > I am not well versed in exploit crafting, so how do we usually handle > invalid free on user provided buffers? I'm not sure we have had many of those, so that we have a portfolio of bugs to look at for guidance. Do you think it is likely that the conditions to trigger this bug would only hold for one set of input data, and not for another? So that the invalid free only happens some of the time, and could be triggered by crafted input data? Obviously, this depends a lot on how these functions are used, but it looks not very likely to me that there are applications out there which have this kind of input dependency here. Thanks, Florian
On 14/12/2017 13:45, Florian Weimer wrote: > On 12/14/2017 04:36 PM, Adhemerval Zanella wrote: >> >> >> On 14/12/2017 09:57, Florian Weimer wrote: >>> On 11/29/2017 02:39 PM, Adhemerval Zanella wrote: >>>> Some libio operations fail to correctly free the backup area (created >>>> by_IO_{w}default_pbackfail on unget{w}c) resulting in either invalid >>>> buffer free operations or memory leaks. >>>> >>>> For instance, on the example provided by BZ#22415 a following >>>> fputc after a fseek to rewind the stream issues an invalid free on >>>> the buffer. >>> >>> Do we have to treat this as a security bug? I don't think so, but I'd still like a second opinion. >>> >>> Thanks, >>> Florian >> >> I am not well versed in exploit crafting, so how do we usually handle >> invalid free on user provided buffers? > > I'm not sure we have had many of those, so that we have a portfolio of bugs to look at for guidance. > > Do you think it is likely that the conditions to trigger this bug would only hold for one set of input data, and not for another? So that the invalid free only happens some of the time, and could be triggered by crafted input data? Obviously, this depends a lot on how these functions are used, but it looks not very likely to me that there are applications out there which have this kind of input dependency here. The bug is triggered independent of data, you need to basically use a non-default buffer with setvbuf, trigger the auxiliary buffer with ungetc and fseek after (so any operation following will see dangling pointers). Using codesearch.debian.net, it looks like there is at least 529 packages that have calls to both setvbuf and ungetc (I am considering fseek a common operation). So it reasonable to assume it indeed might happen in the wild. The good think is in my tests (and on bug reports) the invalid free is detected by glibc and aborted (so I am not sure how undefined it is).
diff --git a/libio/Makefile b/libio/Makefile index 9d09bd8..70b5530 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -62,7 +62,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \ bug-memstream1 bug-wmemstream1 \ tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \ tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ - tst-ftell-append tst-fputws + tst-ftell-append tst-fputws tst-bz22415 ifeq (yes,$(build-shared)) # Add test-fopenloc only if shared library is enabled since it depends on # shared localedata objects. @@ -151,9 +151,11 @@ tst_wprintf2-ARGS = "Some Text" test-fmemopen-ENV = MALLOC_TRACE=$(objpfx)test-fmemopen.mtrace tst-fopenloc-ENV = MALLOC_TRACE=$(objpfx)tst-fopenloc.mtrace +tst-bz22415-ENV = MALLOC_TRACE=$(objpfx)tst-bz22415.mtrace generated += test-fmemopen.mtrace test-fmemopen.check generated += tst-fopenloc.mtrace tst-fopenloc.check +generated += tst-bz22415.mtrace tst-bz22415.check aux := fileops genops stdfiles stdio strops @@ -167,7 +169,8 @@ shared-only-routines = oldiofopen oldiofdopen oldiofclose oldfileops \ oldiofsetpos64 ifeq ($(run-built-tests),yes) -tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-mem.out +tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-mem.out \ + $(objpfx)tst-bz22415-mem.out ifeq (yes,$(build-shared)) # Run tst-fopenloc-cmp.out and tst-openloc-mem.out only if shared # library is enabled since they depend on tst-fopenloc.out. @@ -218,3 +221,7 @@ $(objpfx)test-fmemopen-mem.out: $(objpfx)test-fmemopen.out $(objpfx)tst-fopenloc-mem.out: $(objpfx)tst-fopenloc.out $(common-objpfx)malloc/mtrace $(objpfx)tst-fopenloc.mtrace > $@; \ $(evaluate-test) + +$(objpfx)tst-bz22415-mem.out: $(objpfx)tst-bz22415.out + $(common-objpfx)malloc/mtrace $(objpfx)tst-bz22415.mtrace > $@; \ + $(evaluate-test) diff --git a/libio/fileops.c b/libio/fileops.c index a9e948f..72a0cb5 100644 --- a/libio/fileops.c +++ b/libio/fileops.c @@ -994,6 +994,9 @@ _IO_new_file_seekoff (_IO_FILE *fp, _IO_off64_t offset, int dir, int mode) goto dumb; } } + + _IO_free_backup_area (fp); + /* At this point, dir==_IO_seek_set. */ /* If destination is within current buffer, optimize: */ diff --git a/libio/tst-bz22415.c b/libio/tst-bz22415.c new file mode 100644 index 0000000..d7b23fe --- /dev/null +++ b/libio/tst-bz22415.c @@ -0,0 +1,97 @@ +/* Check static buffer handling with setvbuf (BZ #22415) + + Copyright (C) 2017 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <stdio.h> +#include <stdlib.h> +#include <wchar.h> +#include <mcheck.h> + +#include <support/check.h> +#include <support/temp_file.h> + +static int +do_test (void) +{ + mtrace (); + + char *temp_file; + TEST_VERIFY_EXIT (create_temp_file ("tst-bz22145.", &temp_file)); + + char buf[BUFSIZ]; + + { + /* Check if backup buffer is correctly freed and changing back + to normal buffer does not trigger an invalid free in case of + static buffer set by setvbuf. */ + + FILE *f = fopen (temp_file, "w+b"); + TEST_VERIFY_EXIT (f != NULL); + + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0); + TEST_VERIFY_EXIT (ungetc ('x', f) == 'x'); + TEST_VERIFY_EXIT (fseek (f, 0L, SEEK_SET) == 0); + TEST_VERIFY_EXIT (fputc ('y', f) == 'y'); + + TEST_VERIFY_EXIT (fclose (f) == 0); + } + + { + /* Check if backup buffer is correctly freed and changing back + to normal buffer does not trigger an invalid free in case of + static buffer set by setvbuf. */ + + FILE *f = fopen (temp_file, "w+b"); + TEST_VERIFY_EXIT (f != NULL); + + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0); + TEST_VERIFY_EXIT (ungetc ('x', f) == 'x'); + TEST_VERIFY_EXIT (fputc ('y', f) == 'y'); + + TEST_VERIFY_EXIT (fclose (f) == 0); + } + + { + FILE *f = fopen (temp_file, "w+b"); + TEST_VERIFY_EXIT (f != NULL); + + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0); + TEST_VERIFY_EXIT (ungetwc (L'x', f) == L'x'); + TEST_VERIFY_EXIT (fseek (f, 0L, SEEK_SET) == 0); + TEST_VERIFY_EXIT (fputwc (L'y', f) == L'y'); + + TEST_VERIFY_EXIT (fclose (f) == 0); + } + + { + FILE *f = fopen (temp_file, "w+b"); + TEST_VERIFY_EXIT (f != NULL); + + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0); + TEST_VERIFY_EXIT (ungetwc (L'x', f) == L'x'); + TEST_VERIFY_EXIT (fputwc (L'y', f) == L'y'); + + TEST_VERIFY_EXIT (fclose (f) == 0); + } + + free (temp_file); + + return 0; +} + +#include <support/test-driver.c> diff --git a/libio/wfileops.c b/libio/wfileops.c index 8756b6f..91139e1 100644 --- a/libio/wfileops.c +++ b/libio/wfileops.c @@ -421,6 +421,7 @@ _IO_wfile_overflow (_IO_FILE *f, wint_t wch) if (f->_wide_data->_IO_write_base == 0) { _IO_wdoallocbuf (f); + _IO_free_wbackup_area (f); _IO_wsetg (f, f->_wide_data->_IO_buf_base, f->_wide_data->_IO_buf_base, f->_wide_data->_IO_buf_base); @@ -850,6 +851,9 @@ _IO_wfile_seekoff (_IO_FILE *fp, _IO_off64_t offset, int dir, int mode) goto dumb; } } + + _IO_free_wbackup_area (fp); + /* At this point, dir==_IO_seek_set. */ /* If destination is within current buffer, optimize: */
Some libio operations fail to correctly free the backup area (created by _IO_{w}default_pbackfail on unget{w}c) resulting in either invalid buffer free operations or memory leaks. For instance, on the example provided by BZ#22415 a following fputc after a fseek to rewind the stream issues an invalid free on the buffer. It is because although _IO_file_overflow correctly (from fputc) correctly calls _IO_free_backup_area, the _IO_new_file_seekoff (called by fseek) updates the FILE internal pointers without first free the backup area (resulting in invalid values in the internal pointers). The wide version also shows an issue, but instead of accessing invalid pointers it leaks the backup memory on fseek/fputwc operation. Checked on x86_64-linux-gnu and i686-linux-gnu. * libio/Makefile (tests): Add tst-bz22415. (tst-bz22415-ENV): New rule. (generated): Add tst-bz22415.mtrace and tst-bz22415.check. (tests-special): Add tst-bz22415-mem.out. ($(objpfx)tst-bz22415-mem.out): New rule. * libio/fileops.c (_IO_new_file_seekoff): Call _IO_free_backup_area in case of a successful seek operation. * libio/wfileops.c (_IO_wfile_seekoff): Likewise. (_IO_wfile_overflow): Call _IO_free_wbackup_area in case a write buffer is required. * libio/tst-bz22415.c: New test. --- ChangeLog | 14 ++++++++ libio/Makefile | 11 ++++-- libio/fileops.c | 3 ++ libio/tst-bz22415.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++ libio/wfileops.c | 4 +++ 5 files changed, 127 insertions(+), 2 deletions(-) create mode 100644 libio/tst-bz22415.c -- 2.7.4