Bug 21716

Summary: Crash in glibc's mktime in low-memory situations
Product: glibc Reporter: Johannes <sourceware>
Component: timeAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED FIXED    
Severity: normal CC: carlos, eggert, fweimer
Priority: P2 Flags: fweimer: security-
Version: 2.19   
Target Milestone: 2.29   
Host: Target:
Build: Last reconfirmed:
Attachments: patch for mktime low-memory problem
patch for tzset low-memory assert bug, and one other thing

Description Johannes 2017-07-05 20:16:15 UTC
Originally reported with all the details on the Debian bugtracker: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=867283

glibc uses the function __tzstring to allocate strings in various time-related places, but does not guard against out-of-memory situations sufficiently. __tzstring uses malloc internally and subsequently may return NULL if the memory allocation failed. The result of __tzstring should be verified so that later assumptions about strings being non-NULL are not violated. In this particular case, an assertion was violated and consequently caused a SIGABRT, which violates the API contract of mktime (it should return -1 on failure, and when used in C++ it is guaranteed to be exception-safe).

This bug was found with American Fuzzy Lop and libdislocator.
Comment 1 eggert 2018-09-02 09:31:23 UTC
Created attachment 11224 [details]
patch for mktime low-memory problem

This bug is in the assertion itself, not in the code. The assertion assumes that tzname[i] is non-null, an assumption false in low-memory situations. A null tzname[i] indicates merely that the name is unknown. Please try the attached patch, and see whether it works for you.
Comment 2 Johannes 2018-09-02 19:51:35 UTC
It seems like the patch might be incomplete? In my original report, the assertion that crashed was "assert (num_types == 1);", which is not handled by this patch.
Comment 3 eggert 2018-09-03 01:14:08 UTC
Created attachment 11225 [details]
patch for tzset low-memory assert bug, and one other thing

(In reply to Johannes from comment #2)
> In my original report, the
> assertion that crashed was "assert (num_types == 1);", which is not handled
> by this patch.

Ah, sorry, I was looking at the wrong assertion (which also has a problem, as it turns out). Please try the attached patch instead. The first hunk fixes a non-bug that I discovered while looking into the problem, so I plan to install these as two different patches, but I figure it's easier if you just see one patch.
Comment 4 Johannes 2018-09-09 10:28:13 UTC
Sadly I currently don't have a whole lot time to compile my own glibc (and the bug is very hard to trigger, because now that glibc changed, a different memory limit might be needed to trigger it), but I tried to read through tzfile.c and the patch for some more comments.
There are actually two instances of "assert (num_types == 1);" in the file (sorry for not being clear on that), and I suspect that both of them need to be guarded against this kind of OOM situation. The crash that I initially reported was in the assertion around line 750, while your patch seems to address the assertion at around line 440. My original analysis on the Debian bug tracker shows that there are three code paths that are expected to set the __tzname pointers to a non-null value before the assertion is reached, so it might be the case that of those instances need to be checked manually.
Comment 5 eggert 2018-09-09 14:47:35 UTC
(In reply to Johannes from comment #4)

> The crash that I initially
> reported was in the assertion around line 750, while your patch seems to
> address the assertion at around line 440.

The patch I sent should also address the assertion around line 750, because the patch arranges for later calls to __tzstring to not return NULL.
Comment 6 Sourceware Commits 2018-09-18 22:09:38 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  dab9c3488e86d5304f3e4b778933760374494a82 (commit)
       via  e4e4fde51a309801af5eed72d3494cbf4b7737aa (commit)
      from  d3a43e49f342c4663af0fff9d95000cfe26867c3 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=dab9c3488e86d5304f3e4b778933760374494a82

commit dab9c3488e86d5304f3e4b778933760374494a82
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Tue Sep 18 15:02:11 2018 -0700

    Simplify tzfile fstat failure code
    
    [BZ #21716]
    * time/tzfile.c (__tzfile_read): Simplify slightly.

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=e4e4fde51a309801af5eed72d3494cbf4b7737aa

commit e4e4fde51a309801af5eed72d3494cbf4b7737aa
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Tue Sep 18 15:02:10 2018 -0700

    Fix tzfile low-memory assertion failure
    
    [BZ #21716]
    * time/tzfile.c (__tzfile_read): Check for memory exhaustion
    when registering time zone abbreviations.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog     |   11 +++++++++++
 time/tzfile.c |    8 +++-----
 2 files changed, 14 insertions(+), 5 deletions(-)
Comment 7 eggert 2018-09-18 22:12:46 UTC
The patches are now installed in master so this bug report can be closed as FIXED.
Comment 8 Joseph Myers 2018-09-19 12:19:23 UTC
Note for future commits that if you use Bugzilla with an @gcc.gnu.org address it should give sufficient rights to mark a bug fixed and set the target milestone to the first mainline release in which it was fixed; this should generally be done by the committer.
Comment 9 eggert 2018-09-19 17:11:04 UTC
(In reply to Joseph Myers from comment #8)
> Note for future commits that if you use Bugzilla with an @gcc.gnu.org
> address it should give sufficient rights to mark a bug fixed and set the
> target milestone to the first mainline release in which it was fixed; this
> should generally be done by the committer.

Thanks, first I've heard that. I don't have a @gcc.gnu.org email address; how do I get one? I visited https://gcc.gnu.org to try to find out, but couldn't find anything. Also, should I start making all my glibc contributions from a gcc.gnu.org email address, or is it OK to use it only for bugzilla maintenance that is off-limits to ordinary users?
Comment 10 Florian Weimer 2018-09-19 17:15:32 UTC
(In reply to eggert from comment #9)
> (In reply to Joseph Myers from comment #8)
> > Note for future commits that if you use Bugzilla with an @gcc.gnu.org
> > address it should give sufficient rights to mark a bug fixed and set the
> > target milestone to the first mainline release in which it was fixed; this
> > should generally be done by the committer.
> 
> Thanks, first I've heard that. I don't have a @gcc.gnu.org email address;
> how do I get one? I visited https://gcc.gnu.org to try to find out, but
> couldn't find anything. Also, should I start making all my glibc
> contributions from a gcc.gnu.org email address, or is it OK to use it only
> for bugzilla maintenance that is off-limits to ordinary users?

We can also grant the necessary privileges to your eggert@cs.ucla.edu account if that works for you.
Comment 11 jsm-csl@polyomino.org.uk 2018-09-19 17:15:33 UTC
On Wed, 19 Sep 2018, eggert at cs dot ucla.edu wrote:

> Thanks, first I've heard that. I don't have a @gcc.gnu.org email address; how
> do I get one? I visited https://gcc.gnu.org to try to find out, but couldn't
> find anything. Also, should I start making all my glibc contributions from a
> gcc.gnu.org email address, or is it OK to use it only for bugzilla maintenance
> that is off-limits to ordinary users?

If you have commit access to any project on sourceware / gcc.gnu.org you 
automatically have an email address there.  See

https://sourceware.org/sourceware/accountinfo.html

for how to update where that forwards to.  It's only necessary to use it 
to get editbugs access for Bugzilla, not for any other contributions.
Comment 12 Carlos O'Donell 2018-09-20 02:17:35 UTC
(In reply to Florian Weimer from comment #10)
> (In reply to eggert from comment #9)
> > (In reply to Joseph Myers from comment #8)
> > > Note for future commits that if you use Bugzilla with an @gcc.gnu.org
> > > address it should give sufficient rights to mark a bug fixed and set the
> > > target milestone to the first mainline release in which it was fixed; this
> > > should generally be done by the committer.
> > 
> > Thanks, first I've heard that. I don't have a @gcc.gnu.org email address;
> > how do I get one? I visited https://gcc.gnu.org to try to find out, but
> > couldn't find anything. Also, should I start making all my glibc
> > contributions from a gcc.gnu.org email address, or is it OK to use it only
> > for bugzilla maintenance that is off-limits to ordinary users?
> 
> We can also grant the necessary privileges to your eggert@cs.ucla.edu
> account if that works for you.

I just went ahead and did that. Paul, your account should be able to edit bugs now, and as a member of the community you can grant that to someone else you trust.
Comment 13 Sourceware Commits 2018-11-12 12:45:46 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, release/2.28/master has been updated
       via  e7388e5134471ef965bd48bafc71ba71eb8bf017 (commit)
      from  9071be6b3f78da905ab2b6403933fe14d4482e47 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=e7388e5134471ef965bd48bafc71ba71eb8bf017

commit e7388e5134471ef965bd48bafc71ba71eb8bf017
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Tue Sep 18 15:02:10 2018 -0700

    Fix tzfile low-memory assertion failure
    
    [BZ #21716]
    * time/tzfile.c (__tzfile_read): Check for memory exhaustion
    when registering time zone abbreviations.
    
    (cherry picked from commit e4e4fde51a309801af5eed72d3494cbf4b7737aa)

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog     |    7 +++++++
 time/tzfile.c |    3 ++-
 2 files changed, 9 insertions(+), 1 deletions(-)