classification
Title: [2.7] time.asctime() crash with year > 9999 using musl C library
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: belopolsky, benjamin.peterson, christian.heimes, pitrou, serhiy.storchaka, vstinner
Priority: normal Keywords:

Created on 2017-09-04 19:22 by vstinner, last changed 2017-09-05 23:45 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 3293 merged vstinner, 2017-09-04 19:25
PR 3296 closed vstinner, 2017-09-04 20:03
Messages (12)
msg301246 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-04 19:22
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.
msg301247 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-09-04 19:27
What if this produces a different result and breaks some code on some platforms?
msg301251 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-04 20:05
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: https://github.com/python/cpython/pull/3296 raises a ValueError for year larger than 9999. It doesn't touch time.ctime() (yet?).
msg301255 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-04 20:31
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).
msg301257 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-04 20:40
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.
msg301259 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-09-04 20:57
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.
msg301264 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-04 21:17
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.
msg301265 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-04 21:19
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.
msg301303 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-05 10:31
Isn't this a duplicate of issue16137 which was closed with the resolution "won't fix"?
msg301333 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-05 16:51
> Isn't this a duplicate of issue16137 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 issue16137 crash on Windows, and indirectly it adds support for year > 9999 on all platforms which is a nice side effect.
msg301402 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-05 23:35
New changeset eeadf5fc231163ec97a8010754d9c995c7c14876 by Victor Stinner in branch '2.7':
bpo-31339: Rewrite time.asctime() and time.ctime() (#3293)
https://github.com/python/cpython/commit/eeadf5fc231163ec97a8010754d9c995c7c14876
msg301405 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-05 23:45
Ok, it's now fixed.
History
Date User Action Args
2017-09-05 23:45:56vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg301405

stage: resolved
2017-09-05 23:35:41vstinnersetmessages: + msg301402
2017-09-05 16:51:16vstinnersetmessages: + msg301333
2017-09-05 10:31:04serhiy.storchakasetnosy: + belopolsky, benjamin.peterson
messages: + msg301303
2017-09-04 21:19:20vstinnersetmessages: + msg301265
2017-09-04 21:17:06vstinnersetmessages: + msg301264
2017-09-04 20:57:32christian.heimessetnosy: + christian.heimes
messages: + msg301259
2017-09-04 20:40:00vstinnersetmessages: + msg301257
title: [2.7] time.asctime() buffer overflow for year > 9999 using mucl (C library) -> [2.7] time.asctime() crash with year > 9999 using musl C library
2017-09-04 20:31:11vstinnersetmessages: + msg301255
2017-09-04 20:05:54vstinnersetmessages: + msg301251
2017-09-04 20:03:33vstinnersetpull_requests: + pull_request3330
2017-09-04 19:27:07pitrousetnosy: + serhiy.storchaka, pitrou
messages: + msg301247
2017-09-04 19:25:18vstinnersetpull_requests: + pull_request3328
2017-09-04 19:22:54vstinnercreate