[2.7] time.asctime() crash with year > 9999 using musl C library

[vstinner]

BPO	31339
Nosy	@abalkin, @pitrou, @vstinner, @tiran, @benjaminp, @serhiy-storchaka
PRs	bpo-31339: Rewrite time.asctime() and time.ctime() #3293 bpo-31339: time.asctime() raise on year > 9999 #3296

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-09-05.23:45:56.410>
created_at = <Date 2017-09-04.19:22:54.516>
labels = ['type-security', 'library']
title = '[2.7] time.asctime() crash with year > 9999 using musl C library'
updated_at = <Date 2017-09-05.23:45:56.409>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2017-09-05.23:45:56.409>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-09-05.23:45:56.410>
closer = 'vstinner'
components = ['Library (Lib)']
creation = <Date 2017-09-04.19:22:54.516>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 31339
keywords = []
message_count = 12.0
messages = ['301246', '301247', '301251', '301255', '301257', '301259', '301264', '301265', '301303', '301333', '301402', '301405']
nosy_count = 6.0
nosy_names = ['belopolsky', 'pitrou', 'vstinner', 'christian.heimes', 'benjamin.peterson', 'serhiy.storchaka']
pr_nums = ['3293', '3296']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue31339'
versions = ['Python 2.7']

[vstinner]

Attached PR backports (and adapts) the _asctime() function from the master branch to Python 2.7, so time.asctime() and time.ctime() don't call asctime() and ctime() functions of the external C library, but use portable and safe C code.

[pitrou]

What if this produces a different result and breaks some code on some platforms?

[vstinner]

Antoine: "What if this produces a different result and breaks some code on some platforms?"

No idea. It would call that a regression.

I wrote another much simpler change: #3296 raises a ValueError for year larger than 9999. It doesn't touch time.ctime() (yet?).

[vstinner]

To be honest, I'm not sure that the issue must be categorized as a security vulnerability, nor that it should be fixed. I opened an issue to discuss if it's worth it.

To be clear: Python 3 is safe (at least since Python 3.3, I didn't check older Python 3.x released).

[vstinner]

Oh, I forgot to describe the bug.

asctime() (and ctime()?) do crash on year > 9999 with musl. musl uses an hardcoded buffer of 26 bytes:
http://git.musl-libc.org/cgit/musl/tree/src/time/__asctime.c

musl developers consider that the caller must reject year larger than 9999:

	/* ISO C requires us to use the above format string,
	 * even if it will not fit in the buffer. Thus asctime_r
	 * is \_supposed_ to crash if the fields in tm are too large.
	 * We follow this behavior and crash "gracefully" to warn
	 * application developers that they may not be so lucky
	 * on other implementations (e.g. stack smashing..).
	 \*/
	a_crash();

I disagree. asctime() must either return a longer string, or return an error. But not crash.

[tiran]

I'm ok to replace the asctime with our own implementation that can handle year >9999. Please include range checks for year, wday and month, though.

[vstinner]

Ok, there are 3 choices:

• Do nothing: musl crash on year > 9999, glibc supports year > 9999, behaviour is likely undefined on other libc
• PR 3296: Reject year > 9999 on all platforms: can be seen as a Python 2.7 regression when running with glibc
• PR 3293: Reimplement time.asctime() and time.ctime() to not depend on the libc anymore, as done in Python 3

My favorite option is now "PR 3293: Reimplement time.asctime()". Yeah, there is a risk of getting a behaviour change on funky libc with funky values, but IMHO it's worth it.

[vstinner]

Christian Heimes: "I'm ok to replace the asctime with our own implementation that can handle year >9999. Please include range checks for year, wday and month, though."

Done.

By the way, I discussed with Alex Gaynor on #python-dev to define the severity of the bug. It was said that it's a medium security issue, it's a denial of service which can be triggered through user data.

[serhiy-storchaka]

Isn't this a duplicate of bpo-16137 which was closed with the resolution "won't fix"?

[vstinner]

Isn't this a duplicate of bpo-16137 which was closed with the resolution "won't fix"?

This issue seems to be a crash on Windows using negative year. You proposed a patch using asctime_s() instead of asctime(). So the fix is specific to Windows.

This issue is now qualified as a security vulnerability (msg301255), so we have to fix it.

My proposed PR 3293 fixes the crash on musl, it should fix bpo-16137 crash on Windows, and indirectly it adds support for year > 9999 on all platforms which is a nice side effect.

[vstinner]

New changeset eeadf5f by Victor Stinner in branch '2.7':
bpo-31339: Rewrite time.asctime() and time.ctime() (bpo-3293)
eeadf5f

[vstinner]

Ok, it's now fixed.