| Message ID | e08b126d-fb6a-8b6b-676c-c5815da86f80@cs.ucla.edu |
|---|---|
| State | New |
| Series | "Re: parse-datetime test failure" |
| Headers | show |
From e2739ba6310893be93d01a23cbfed8d8dfb08966 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Wed, 11 Nov 2020 19:20:42 -0800 Subject: [PATCH 3/3] time_rz: simplify CVE-2017-7476 fix * lib/time_rz.c: Do not include limits.h; I think it was included under the mistaken impression that limits.h defines SIZE_MAX. (SIZE_MAX): Remove. (save_abbr): Put string length into a ptrdiff_t variable, so that the size comparison works naturally. This fixes CVE-2017-7476 in a cleaner way. --- ChangeLog | 8 ++++++++ lib/time_rz.c | 15 ++------------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/ChangeLog b/ChangeLog index 530c9a661..fe298605e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,13 @@ 2020-11-11 Paul Eggert <eggert@cs.ucla.edu> + time_rz: simplify CVE-2017-7476 fix + * lib/time_rz.c: Do not include limits.h; I think it was included + under the mistaken impression that limits.h defines SIZE_MAX. + (SIZE_MAX): Remove. + (save_abbr): Put string length into a ptrdiff_t variable, + so that the size comparison works naturally. This + fixes CVE-2017-7476 in a cleaner way. + parse-datetime: streamline overflow checking When parse-datetime.y’s overflow code was written, INT_ADD_WRAPV did not work for unsigned destinations, and since time_t might diff --git a/lib/time_rz.c b/lib/time_rz.c index c58e6831b..a33b8078b 100644 --- a/lib/time_rz.c +++ b/lib/time_rz.c @@ -27,7 +27,6 @@ #include <time.h> #include <errno.h> -#include <limits.h> #include <stdbool.h> #include <stddef.h> #include <stdlib.h> @@ -36,10 +35,6 @@ #include "flexmember.h" #include "time-internal.h" -#ifndef SIZE_MAX -# define SIZE_MAX ((size_t) -1) -#endif - /* The approximate size to use for small allocation requests. This is the largest "small" request for the GNU C library malloc. */ enum { DEFAULT_MXFAST = 64 * sizeof (size_t) / 4 }; @@ -125,14 +120,8 @@ save_abbr (timezone_t tz, struct tm *tm) { if (! (*zone_copy || (zone_copy == tz->abbrs && tz->tz_is_set))) { - size_t zone_size = strlen (zone) + 1; - size_t zone_used = zone_copy - tz->abbrs; - if (SIZE_MAX - zone_used < zone_size) - { - errno = ENOMEM; - return false; - } - if (zone_used + zone_size < ABBR_SIZE_MIN) + ptrdiff_t zone_size = strlen (zone) + 1; + if (zone_size < tz->abbrs + ABBR_SIZE_MIN - zone_copy) extend_abbrs (zone_copy, zone, zone_size); else { -- 2.25.1
On Wed, Nov 11, 2020 at 07:38:00PM -0800, Paul Eggert wrote: > On 11/11/20 8:20 AM, Bruno Haible wrote: > >It works fine on Alpine Linux 3.7 (32-bit, 64-bit) and 3.9 (64-bit). > > > >On Alpine Linux 3.10 and 3.12 (64-bit) it fails: > >../../gltests/test-parse-datetime.c:448: assertion 'result.tv_sec == 1 * 60 * 60 + 2 * 60 + 3 && result.tv_nsec == 123456789' failed > >Aborted > > > >So, to me it looks like a regression between Alpine Linux 3.9 and 3.10. > > It's arguably a bug in the test case, since Alpine uses musl libc > which does not support time zone abbreviations longer than 6 bytes, > whereas the test case uses an time zone abbreviation of 2000 bytes > (to test a bug in an old Gnulib version when running on GNU/Linux). > POSIX does not define behavior if you go over the limit. > > I worked around the problem by changing the test case to not go over > the limit as determined by sysconf (_SC_TZNAME_MAX), in the first > attached patch. Plus I refactored and/or slightly improved the > Gnulib overflow checking while I was in the neighborhood (last two > attached patches). > > Arguably this is a quality-of-implementation issue here, since > Alpine and/or musl goes beserk with long timezone abbreviations > whereas every other implementation I know of either works or > silently substitutes localtime or UTC (which is good enough for this > test case). But I'll leave that issue to the Alpine and/or musl libc > folks. > > I'll cc this to the musl bug reporting list. Although the Gnulib > test failure has been fixed, it may be the symptom of a more-severe > bug in musl. For those new to the problem, this thread starts here: > > https://lists.gnu.org/r/bug-gnulib/2020-11/msg00039.html Thanks. I believe you've just re-discovered a known bug that's fixed in musl commit 33338ebc853d37c80f0f236cc7a92cb0acc6aace, which will be included in the upcoming 1.2.2 release. Rich
On 11/11/20 8:07 PM, Rich Felker wrote: > Thanks. I believe you've just re-discovered a known bug that's fixed > in musl commit 33338ebc853d37c80f0f236cc7a92cb0acc6aace, which will be > included in the upcoming 1.2.2 release. Yes, thanks, that looks exactly right. It's *so* nice to have bugs fixed before I even report them! Here's a URL for the musl patch, for the record: https://git.musl-libc.org/cgit/musl/commit/?id=33338ebc853d37c80f0f236cc7a92cb0acc6aace