Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.

Bug 1444774

Summary: heap overflow security issue in date(1) and touch(1)
Product: [Fedora] Fedora Reporter: Pádraig Brady <p>
Component: coreutilsAssignee: Kamil Dudka <kdudka>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: urgent Docs Contact:
Priority: unspecified    
Version: 26CC: agk, Carolinewebb78, eggert, kdudka, omarandemad, security-response-team
Target Milestone: ---Keywords: Security
Target Release: ---   
Hardware: All   
OS: Unspecified   
Whiteboard:
Fixed In Version: coreutils-8.27-4.fc27 coreutils-8.27-5.fc26 Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2017-05-01 18:17:10 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
heap overflow mitigation
none
updated mitigation
none
proposed fix + test case
none
updated mitigation + test case none

Description Pádraig Brady 2017-04-24 09:22:42 UTC
Created attachment 1273560 [details]
heap overflow mitigation

coreutils >= 8.25 (Fedora 24) has a nasty arbitrary heap write issue that I presume can be exploited to run arbitrary code as the user running date,
just by setting a large TZ variable in the command line or in the environment.

Proposed fix attached.

This has not been discussed anywhere else yet.

Would it be possible to assign an CVE if required, as I'm not familiar with that process.

Note hwclock from the util-linux project recently moved to using parse_datetime from gnulib, but that is _not_ impacted as far as I can tell.

Comment 1 Kamil Dudka 2017-04-24 10:39:38 UTC
Thank you for contacting us, Pádraig!  Could you please report the security issue to secalert and tell them about this BZ?

The Product Security team will take care of the CVE assignment.  Alternatively, I could report it on your behalf but, if you communicate with them directly, it can speedup the process.

Thanks in advance!

Comment 2 Pádraig Brady 2017-04-24 15:40:51 UTC
OK cool. Note security-response-team are on the CC for this bug, so I presume secalert is different. I'm attaching a slightly updated patch to make things clearer and remove a theoretical overflow.

Comment 3 Pádraig Brady 2017-04-24 16:04:06 UTC
Created attachment 1273643 [details]
updated mitigation

Comment 4 Pádraig Brady 2017-04-24 16:33:44 UTC
A quick audit of the parse_datetime uses in util-linux and ostree show their adjustments avoid the issue.

Also recutils uses parse_datetime directly but references an old version before the issue was introduced.

Comment 5 Pádraig Brady 2017-04-24 17:52:10 UTC
BTW touch(1) is also impacted

Comment 6 Tomas Hoger 2017-04-25 12:59:53 UTC
You seem to have a good understanding of the issue, yet this report does not include details to indicate what the problem is.  Trying to figure out the issue from the patch, I assume the attack is to make zone_copy be more than tz->abbrs + ABBR_SIZE_MIN.  As a consequence, the following expression should be negative: tz->abbrs + ABBR_SIZE_MIN - zone_copy , but as it's computed as size_t, it wraps around to a large positive.  Hence code thinks there's enough space for zone_size bytes when it's not.

I've not yet tried to figure out if zone_copy > tz->abbrs + ABBR_SIZE_MIN is a problem of its own.

Note that the reproducer you shared does not trigger crash or valgrind-detectable error for me with coreutils-8.25-8.fc24.x86_64 or coreutils-8.25-16.fc25.x86_64.

Comment 7 Tomas Hoger 2017-04-25 13:15:24 UTC
It should also be noted that the affected code is in gnulib, which is bundled/embedded in coreutils.  The time_rz code was added to gnulib via this commit in Jul 2015:

http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=4bc76593d509827588081e32ebd75a5e34d1c374

Comment 8 Pádraig Brady 2017-04-25 17:05:54 UTC
@Tomas your analysis is correct, and corresponds to what I tried to describe in the patch commit message. It's interesting that the bugzilla patch preview strips this info.  Also one small point is that I think the issue is due to the promotion of signed to unsigned in the comparison, rather than wrapping. The consequence is the same in that arbitrary specified TZ is written over the heap.

It's very surprising that valgrind doesn't catch this.
Ah! Actually it only impacts coreutils >= 8.27, i.e. Fedora >= 26
The issue was latent since the gnulib commit you mentioned,
but only actually hit from parse_datetime since:
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=4e6e16b3f

valgrind _does_ notice the issue as I double checked with:

valgrind ~/t/date-8.27 -d \
  $(printf 'TZ="aaa%020daaaaaaaaaaaaaaaaaaaaaaaaaaab%089d"')

Note that the a....b part can be expanded to write whatever,
though I suppose arbitrary binary info would be ignored earlier
in the process somewhat mitigating the issue.

Given this impacts just coreutils >= 8.27 I guess we can release a public fix sooner rather than later. (I also notice 8.27 is not in debian yet).
I also notice that tar uses parse_datetime, but tar-1.29 uses the older version not directly impacted.

Comment 9 Pádraig Brady 2017-04-25 17:15:14 UTC
CCing Paul Eggert who is the originator of this code,
and who would know better about other released users of this localtime_rz() issue.

Paul note this is embargoed for now and I'll apply the fix to gnulib when deemed appropriate. tar will also need to bump their gnulib reference to avoid releasing with the issue.

Comment 10 Tomas Hoger 2017-04-25 20:23:39 UTC
(In reply to Pádraig Brady from comment #8)
> @Tomas your analysis is correct, and corresponds to what I tried to describe
> in the patch commit message. It's interesting that the bugzilla patch
> preview strips this info.

The fancy diff viewer only shows code changes and hides the commit message:

https://bugzilla.redhat.com/attachment.cgi?id=1273643&action=diff

Think link shows the raw patch including commit message:

https://bugzilla.redhat.com/attachment.cgi?id=1273643

I should have been more careful to check the latter link too.


Regarding embargo - you can make this public whenever you need to.  Fedora does not have mechanisms to produce embargoed builds while flaws are non-public.

Comment 11 Paul Eggert 2017-04-25 21:45:14 UTC
Created attachment 1274031 [details]
proposed fix + test case

Ouch. Sorry about that. Although we should fix it ASAP, my kneejerk reaction is that the bug is not serious enough to embargo.

The proposed patch has some minor glitches in the overflow case. I'm attaching a revised Gnulib patch, which also adds a test for this bug; what do you think?

Comment 12 Pádraig Brady 2017-04-25 23:55:52 UTC
I was going to add a test after the fix was discussed finalized.
Can you explain the minor glitches in my patch?
I much prefer my commit message at least :)
cheers

Comment 13 Paul Eggert 2017-04-26 07:39:17 UTC
(In reply to Pádraig Brady from comment #12)

> Can you explain the minor glitches in my patch?

It doesn't set errno on failure like it should, and it consumes one more machine instruction (to subtract a value from SIZE_MAX) than it needs to.

Please feel free to improve the commit message; I didn't spend a lot of time on it.

Comment 14 Pádraig Brady 2017-04-26 15:46:16 UTC
Created attachment 1274288 [details]
updated mitigation + test case

Thanks. I've set the errno on theoretical overflow case.
Note I prefer the more explicit broken down code
rather than wondering about what may happen in overflow cases.
I've added your test, thanks.

Comment 15 Pádraig Brady 2017-04-26 15:49:19 UTC
Given there are no more flagged impacted projects I hope to apply this to gnulib tomorrow. Fedora coreutils can patch now (which I realize also publicizes the issue, albeit less directly).

Comment 16 Pádraig Brady 2017-04-27 03:46:09 UTC
Final patch now pushed upstream at:
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=94e01571

Comment 17 Kamil Dudka 2017-04-27 07:28:41 UTC
Thank you very much!  I will take care of the backport for Fedora.

Comment 18 Kamil Dudka 2017-04-27 08:02:45 UTC
downstream commit:

https://src.fedoraproject.org/cgit/rpms/coreutils.git/commit/?id=e0567d54

Comment 19 Fedora Update System 2017-04-27 08:28:10 UTC
coreutils-8.27-4.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-54c2e9124e

Comment 20 Fedora Update System 2017-04-28 00:23:13 UTC
coreutils-8.27-4.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-54c2e9124e

Comment 21 Fedora Update System 2017-04-28 12:26:05 UTC
coreutils-8.27-5.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-b17d54561b

Comment 22 Fedora Update System 2017-04-30 03:51:59 UTC
coreutils-8.27-5.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-b17d54561b

Comment 23 Fedora Update System 2017-05-01 18:17:10 UTC
coreutils-8.27-5.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.