| msg301246 - (view) |
Author: STINNER Victor (vstinner) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2017-09-05 10:31 |
| Isn't this a duplicate of which was closed with the resolution "won't fix"? |
|
|
| msg301333 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-09-05 16:51 |
| > Isn't this a duplicate of 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 (), so we have to fix it. My proposed PR 3293 fixes the crash on musl, it should fix 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) * |
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) * |
Date: 2017-09-05 23:45 |
| Ok, it's now fixed. |
|
|