From patchwork Thu Dec 20 21:54:09 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Gabriel F. T. Gomes" X-Patchwork-Id: 1017076 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=sourceware.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=libc-alpha-return-98675-incoming=patchwork.ozlabs.org@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=inconstante.eti.br Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="kwAuPDlk"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43LQX85KZTz9sCQ for ; Fri, 21 Dec 2018 08:54:32 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id:mime-version :content-type; q=dns; s=default; b=tE9ho2qqD7o4vdDF0f7bYDP42GoZu xkJ8TD7JMfWUMBHe/FUqdKZKv3h4gpBKBRur0Mc9MmIeaqGD01J5HH7tHEAfNfe2 XN8KQEaurdxb44NwNmz816bfoA02ybRrl8nA7gDNXg5MVJbyTk/8/c48jIkN6ZBp VIoF51eQS1S1mo= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id:mime-version :content-type; s=default; bh=Ezv77iQ/dBBLsVGcXx1wldEeqNQ=; b=kwA uPDlks8R5RtaejsaaEU3x/8+MFECMsmtRmMc9Ij3iuxeZug7QhV9IDaylZdZCUR7 MgvV2NcULZn10W1rlP1QARKGxVRsrMgpQvZSMUlVmiSJyE7x9/Psw4pGUMvP3mt8 xZL5vROC8eHA53P2T8yeX1XBPFTyxAk5+Z5VEo6U= Received: (qmail 113131 invoked by alias); 20 Dec 2018 21:54:26 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 113106 invoked by uid 89); 20 Dec 2018 21:54:25 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, KAM_SHORT, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=erase, overflows, behavioral X-HELO: mo19.mail-out.ovh.net From: "Gabriel F. T. Gomes" To: Subject: [PATCH] Set behavior of sprintf-like functions with overlapping source and destination Date: Thu, 20 Dec 2018 19:54:09 -0200 Message-ID: <20181220215409.8971-1-gabriel@inconstante.eti.br> MIME-Version: 1.0 X-Ovh-Tracer-Id: 1119707459917369027 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: 0 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgedtkedrudejfedgudehhecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemucehtddtnecu 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 4e2f43f842ef5e253cc23383645adbaa03cedb86 Author: Zack Weinberg 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;iGFExtras[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. * 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. --- debug/sprintf_chk.c | 4 ++ debug/vsprintf_chk.c | 4 ++ libio/Makefile | 7 +++- libio/iovsprintf.c | 14 ++++++- libio/libioP.h | 6 ++- libio/tst-sprintf-chk-ub.c | 2 + libio/tst-sprintf-ub.c | 102 +++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 libio/tst-sprintf-chk-ub.c create mode 100644 libio/tst-sprintf-ub.c diff --git a/debug/sprintf_chk.c b/debug/sprintf_chk.c index 649e8ab4d5..dccdec92c3 100644 --- a/debug/sprintf_chk.c +++ b/debug/sprintf_chk.c @@ -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 (); diff --git a/debug/vsprintf_chk.c b/debug/vsprintf_chk.c index c1b1a8da4f..3db6f624de 100644 --- a/debug/vsprintf_chk.c +++ b/debug/vsprintf_chk.c @@ -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 (); diff --git a/libio/Makefile b/libio/Makefile index 8239fba828..e1b0bf629d 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -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 diff --git a/libio/iovsprintf.c b/libio/iovsprintf.c index 08e4002625..f58519d172 100644 --- a/libio/iovsprintf.c +++ b/libio/iovsprintf.c @@ -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); diff --git a/libio/libioP.h b/libio/libioP.h index 958ef9bffe..e5f8d2381d 100644 --- a/libio/libioP.h +++ b/libio/libioP.h @@ -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) diff --git a/libio/tst-sprintf-chk-ub.c b/libio/tst-sprintf-chk-ub.c new file mode 100644 index 0000000000..77ec64229a --- /dev/null +++ b/libio/tst-sprintf-chk-ub.c @@ -0,0 +1,2 @@ +#define _FORTIFY_SOURCE 2 +#include diff --git a/libio/tst-sprintf-ub.c b/libio/tst-sprintf-ub.c new file mode 100644 index 0000000000..35c6711122 --- /dev/null +++ b/libio/tst-sprintf-ub.c @@ -0,0 +1,102 @@ +/* Test the sprintf (buf, "%s", buf) does not override buf. + Copyright (C) 2018 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 + . */ + +#include +#include +#include + +#include + +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