Bug 1444774 - heap overflow security issue in date(1) and touch(1)
heap overflow security issue in date(1) and touch(1)
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: coreutils (Show other bugs)
26
All Unspecified
unspecified Severity urgent
: ---
: ---
Assigned To: Kamil Dudka
Fedora Extras Quality Assurance
: Security
Depends On:
Blocks: CVE-2017-7476
  Show dependency treegraph
 
Reported: 2017-04-24 05:22 EDT by Pádraig Brady
Modified: 2017-05-01 14:17 EDT (History)
3 users (show)

See Also:
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 14:17:10 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
heap overflow mitigation (1.74 KB, patch)
2017-04-24 05:22 EDT, Pádraig Brady
no flags Details | Diff
updated mitigation (2.20 KB, patch)
2017-04-24 12:04 EDT, Pádraig Brady
no flags Details | Diff
proposed fix + test case (3.36 KB, patch)
2017-04-25 17:45 EDT, Paul Eggert
no flags Details | Diff
updated mitigation + test case (3.93 KB, patch)
2017-04-26 11:46 EDT, Pádraig Brady
no flags Details | Diff

  None (edit)
Description Pádraig Brady 2017-04-24 05:22:42 EDT
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 06:39:38 EDT
Thank you for contacting us, Pádraig!  Could you please report the security issue to secalert@redhat.com 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 11:40:51 EDT
OK cool. Note security-response-team@redhat.com are on the CC for this bug, so I presume secalert@redhat.com 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 12:04 EDT
Created attachment 1273643 [details]
updated mitigation
Comment 4 Pádraig Brady 2017-04-24 12:33:44 EDT
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 13:52:10 EDT
BTW touch(1) is also impacted
Comment 6 Tomas Hoger 2017-04-25 08:59:53 EDT
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 09:15:24 EDT
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 13:05:54 EDT
@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 13:15:14 EDT
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 16:23:39 EDT
(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 17:45 EDT
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 19:55:52 EDT
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 03:39:17 EDT
(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 11:46 EDT
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 11:49:19 EDT
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-26 23:46:09 EDT
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 03:28:41 EDT
Thank you very much!  I will take care of the backport for Fedora.
Comment 18 Kamil Dudka 2017-04-27 04:02:45 EDT
downstream commit:

https://src.fedoraproject.org/cgit/rpms/coreutils.git/commit/?id=e0567d54
Comment 19 Fedora Update System 2017-04-27 04:28:10 EDT
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-27 20:23:13 EDT
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 08:26:05 EDT
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-29 23:51:59 EDT
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 14:17:10 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.