Set behavior of sprintf-like functions with overlapping source and destination

According to ISO C99, passing the same buffer as source and destination
to sprintf, snprintf, vsprintf, or vsnprintf has undefined behavior.
Until the commit

  commit 4e2f43f842
  Author: Zack Weinberg <zackw@panix.com>
  Date:   Wed Mar 7 14:32:03 2018 -0500

      Use PRINTF_FORTIFY instead of _IO_FLAGS2_FORTIFY (bug 11319)

a call to sprintf or vsprintf with overlapping buffers, for instance
vsprintf (buf, "%sTEXT", buf), would append `TEXT' into buf, while a
call to snprintf or vsnprintf would override the contents of buf.
After the aforementioned commit, the behavior of sprintf and vsprintf
changed (so that they also override the contents of buf).

This patch reverts this behavioral change, because it will likely break
applications that rely on the previous behavior, even though it is
undefined by ISO C.  As noted by Szabolcs Nagy, this is used in SPEC2017
507.cactuBSSN_r/src/PUGH/PughUtils.c:

  sprintf(mess,"  Size:");
  for (i=0;i<dim+1;i++)
  {
      sprintf(mess,"%s %d",mess,pughGH->GFExtras[dim]->nsize[i]);
  }

More important to notice is the fact that the overwriting of the
destination buffer is not the only behavior affected by the refactoring.
Before the refactoring, sprintf and vsprintf would use _IO_str_jumps,
whereas __sprintf_chk and __vsprintf_chk would use _IO_str_chk_jumps.
After the refactoring, all use _IO_str_chk_jumps, which would make
sprintf and vsprintf report buffer overflows and terminate the program.
This patch also reverts this behavior, by installing the appropriate
jump table for each *sprintf functions.

Apart from reverting the changes, this patch adds a test case that has
the old behavior hardcoded, so that regressions are noticed if something
else unintentionally changes the behavior.

Tested for powerpc64le.
This commit is contained in:
Gabriel F. T. Gomes 2018-12-19 18:01:14 -02:00
parent d5c6df0b0e
commit 2d9837c1fb
8 changed files with 149 additions and 4 deletions

View File

@ -1,3 +1,17 @@
2019-01-02 Gabriel F. T. Gomes <gabriel@inconstante.eti.br>
* debug/sprintf_chk.c (___sprintf_chk): Use PRINTF_CHK.
* debug/vsprintf_chk.c (___vsprintf_chk): Likewise.
* libio/Makefile (tests): Add tst-sprintf-ub and
tst-sprintf-chk-ub.
(CFLAGS-tst-sprintf-ub.c): New variable.
(CFLAGS-tst-sprintf-chk-ub.c): Likewise.
* libio/iovsprintf.c (__vsprintf_internal): Only erase the
destination buffer and check for overflows in fortified mode.
* libio/libioP.h (PRINTF_CHK): New macro.
* libio/tst-sprintf-chk-ub.c: New file.
* libio/tst-sprintf-ub.c: Likewise.
2019-01-02 Florian Weimer <fweimer@redhat.com>
[BZ #24018]

View File

@ -29,6 +29,10 @@ ___sprintf_chk (char *s, int flag, size_t slen, const char *format, ...)
va_list ap;
int ret;
/* Regardless of the value of flag, let __vsprintf_internal know that
this is a call from *printf_chk. */
mode |= PRINTF_CHK;
if (slen == 0)
__chk_fail ();

View File

@ -25,6 +25,10 @@ ___vsprintf_chk (char *s, int flag, size_t slen, const char *format,
can only come from read-only format strings. */
unsigned int mode = (flag > 0) ? PRINTF_FORTIFY : 0;
/* Regardless of the value of flag, let __vsprintf_internal know that
this is a call from *printf_chk. */
mode |= PRINTF_CHK;
if (slen == 0)
__chk_fail ();

View File

@ -64,7 +64,8 @@ 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-bz22415 tst-fgetc-after-eof
tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \
tst-sprintf-ub tst-sprintf-chk-ub
tests-internal = tst-vtables tst-vtables-interposed tst-readline
@ -152,6 +153,10 @@ CFLAGS-oldtmpfile.c += -fexceptions
CFLAGS-tst_putwc.c += -DOBJPFX=\"$(objpfx)\"
# These test cases intentionally use overlapping arguments
CFLAGS-tst-sprintf-ub.c += -Wno-restrict
CFLAGS-tst-sprintf-chk-ub.c += -Wno-restrict
tst_wprintf2-ARGS = "Some Text"
test-fmemopen-ENV = MALLOC_TRACE=$(objpfx)test-fmemopen.mtrace

View File

@ -77,8 +77,18 @@ __vsprintf_internal (char *string, size_t maxlen,
sf._sbf._f._lock = NULL;
#endif
_IO_no_init (&sf._sbf._f, _IO_USER_LOCK, -1, NULL, NULL);
_IO_JUMPS (&sf._sbf) = &_IO_str_chk_jumps;
string[0] = '\0';
/* When called from fortified sprintf/vsprintf, erase the destination
buffer and try to detect overflows. When called from regular
sprintf/vsprintf, do not erase the destination buffer, because
known user code relies on this behavior (even though its undefined
by ISO C), nor try to detect overflows. */
if ((mode_flags & PRINTF_CHK) != 0)
{
_IO_JUMPS (&sf._sbf) = &_IO_str_chk_jumps;
string[0] = '\0';
}
else
_IO_JUMPS (&sf._sbf) = &_IO_str_jumps;
_IO_str_init_static_internal (&sf, string,
(maxlen == -1) ? -1 : maxlen - 1,
string);

View File

@ -705,9 +705,13 @@ extern int __vswprintf_internal (wchar_t *string, size_t maxlen,
PRINTF_FORTIFY, when set to one, indicates that fortification checks
are to be performed in input parameters. This is used by the
__*printf_chk functions, which are used when _FORTIFY_SOURCE is
defined to 1 or 2. Otherwise, such checks are ignored. */
defined to 1 or 2. Otherwise, such checks are ignored.
PRINTF_CHK indicates, to the internal function being called, that the
call is originated from one of the __*printf_chk functions. */
#define PRINTF_LDBL_IS_DBL 0x0001
#define PRINTF_FORTIFY 0x0002
#define PRINTF_CHK 0x0004
extern size_t _IO_getline (FILE *,char *, size_t, int, int);
libc_hidden_proto (_IO_getline)

View File

@ -0,0 +1,2 @@
#define _FORTIFY_SOURCE 2
#include <tst-sprintf-ub.c>

102
libio/tst-sprintf-ub.c Normal file
View File

@ -0,0 +1,102 @@
/* Test the sprintf (buf, "%s", buf) does not override buf.
Copyright (C) 2019 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 <stdarg.h>
#include <stdio.h>
#include <string.h>
#include <support/check.h>
enum
{
FUNCTION_FIRST,
FUNCTION_SPRINTF = FUNCTION_FIRST,
FUNCTION_SNPRINF,
FUNCTION_VSPRINTF,
FUNCTION_VSNPRINTF,
FUNCTION_LAST
};
static void
do_one_test (int function, char *buf, ...)
{
va_list args;
char *arg;
/* Expected results for fortified and non-fortified sprintf. */
#if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 1
const char *expected = "CD";
#else
const char *expected = "ABCD";
#endif
va_start (args, buf);
arg = va_arg (args, char *);
va_end (args);
switch (function)
{
/* The regular sprintf and vsprintf functions do not override the
destination buffer, even if source and destination overlap. */
case FUNCTION_SPRINTF:
sprintf (buf, "%sCD", buf);
TEST_COMPARE_STRING (buf, expected);
break;
case FUNCTION_VSPRINTF:
va_start (args, buf);
vsprintf (arg, "%sCD", args);
TEST_COMPARE_STRING (arg, expected);
va_end (args);
break;
/* On the other hand, snprint and vsnprint override overlapping
source and destination buffers. */
case FUNCTION_SNPRINF:
snprintf (buf, 3, "%sCD", buf);
TEST_COMPARE_STRING (buf, "CD");
break;
case FUNCTION_VSNPRINTF:
va_start (args, buf);
vsnprintf (arg, 3, "%sCD", args);
TEST_COMPARE_STRING (arg, "CD");
va_end (args);
break;
default:
support_record_failure ();
}
}
static int
do_test (void)
{
char buf[8];
int i;
/* For each function in the enum do:
- reset the buffer to the initial state "AB";
- call the function with buffer as source and destination;
- check for the desired behavior. */
for (i = FUNCTION_FIRST; i < FUNCTION_LAST; i++)
{
strncpy (buf, "AB", 3);
do_one_test (i, buf, buf);
}
return 0;
}
#include <support/test-driver.c>