From patchwork Tue Nov 21 01:30:04 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joseph Myers X-Patchwork-Id: 24395 Received: (qmail 80970 invoked by alias); 21 Nov 2017 01:30:40 -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 80839 invoked by uid 89); 21 Nov 2017 01:30:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KB_WAM_FROM_NAME_SINGLEWORD, RCVD_IN_DNSWL_NONE, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy=timestamps, persuade, plausible, NTP X-HELO: relay1.mentorg.com Date: Tue, 21 Nov 2017 01:30:04 +0000 From: Joseph Myers To: Subject: Fix p_secstodate overflow handling (bug 22463) Message-ID: User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 X-ClientProxiedBy: svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) To SVR-IES-MBX-03.mgc.mentorg.com (139.181.222.3) The resolv/res_debug.c function p_secstodate does: struct tm timebuf; time = __gmtime_r(&clock, &timebuf); time->tm_year += 1900; time->tm_mon += 1; sprintf(output, "%04d%02d%02d%02d%02d%02d", time->tm_year, time->tm_mon, time->tm_mday, time->tm_hour, time->tm_min, time->tm_sec); If __gmtime_r returns NULL (because the year overflows the range of int), this will dereference a null pointer. Otherwise, if the computed year does not fit in four characters, this will cause a buffer overrun of the fixed-size 15-byte buffer. With current GCC mainline, there is a compilation failure because of the possible buffer overrun. I couldn't find a specification for how this function is meant to behave. This patch makes the overflow cases use as the output string and set errno to EOVERFLOW (__gmtime_r will already have done so in the case of NULL return). I couldn't persuade GCC to recognize the ranges of the struct tm fields by adding explicit range checks with a call to __builtin_unreachable if outside the range, so having added a check for the year, this patch then disables the -Wformat-overflow= warning for the sprintf call. I do not know why this build failure (which I observed when testing a fix for the regcomp.c build failure) should be new with mainline GCC (that is, I don't know what GCC change might have introduced it, when the basic functionality for such warnings was already in GCC 7). I do not know if this is a security issue (that is, if there are plausible ways in which a date before -999 or after 9999 from an untrusted source might end up in this function). The system clock is arguably an untrusted source (in that e.g. NTP is insecure), but probably not to that extent (NTP can't communicate such wild timestamps). Tested with build-many-glibcs.py that, in conjunction with a fix for regcomp.c compilation, this restores the build for arm with mainline GCC. Also tested for x86_64. 2017-11-21 Joseph Myers [BZ #22463] * resolv/res_debug.c: Include . (p_secstodate): Use "" as output and set errno to EOVERFLOW if year would not fit in four characters. (p_secstodate) [__GNUC_PREREQ (7, 0)]: Disable -Wformat-overflow= for sprintf call. diff --git a/resolv/res_debug.c b/resolv/res_debug.c index 4114c2d..8a9ac77 100644 --- a/resolv/res_debug.c +++ b/resolv/res_debug.c @@ -107,6 +107,7 @@ #include #include #include +#include #ifdef SPRINTF_CHAR # define SPRINTF(x) strlen(sprintf/**/x) @@ -1064,11 +1065,28 @@ p_secstodate (u_long secs) { struct tm timebuf; time = __gmtime_r(&clock, &timebuf); + if (time == NULL + || time->tm_year > 9999 - 1900 + || time->tm_year < -999 - 1900) { + strcpy (output, ""); + __set_errno (EOVERFLOW); + return output; + } time->tm_year += 1900; time->tm_mon += 1; + /* The struct tm fields, given the above check of tm_year, + must have values that mean this sprintf exactly fills the + buffer. But as of GCC 8 of 2017-11-21, GCC cannot tell + that, even given range checks on all fields with + __builtin_unreachable called for out-of-range values. */ + DIAG_PUSH_NEEDS_COMMENT; +#if __GNUC_PREREQ (7, 0) + DIAG_IGNORE_NEEDS_COMMENT (8, "-Wformat-overflow="); +#endif sprintf(output, "%04d%02d%02d%02d%02d%02d", time->tm_year, time->tm_mon, time->tm_mday, time->tm_hour, time->tm_min, time->tm_sec); + DIAG_POP_NEEDS_COMMENT; return (output); } libresolv_hidden_def (__p_secstodate)