classification
Title: asctime does not check its input
Type: crash Stage: resolved
Components: Extension Modules, Windows Versions: Python 3.2
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: belopolsky Nosy List: AmirHabibi, abbeyj, alexandre.vassalotti, belopolsky, lemburg, pitrou, ruseel, srid, vstinner
Priority: low Keywords: easy, patch

Created on 2009-07-30 23:56 by AmirHabibi, last changed 2010-10-01 14:23 by belopolsky. This issue is now closed.

Files
File name Uploaded Description Edit
asctime.patch abbeyj, 2009-08-02 20:04
issue6608.diff belopolsky, 2010-05-26 15:24 Untabified asctime.patch
issue6608-p3k.patch ruseel, 2010-09-25 03:31
Messages (22)
msg91115 - (view) Author: Amir Habibi (AmirHabibi) Date: 2009-07-30 23:56
I use:
Python 2.6.2 
(r262:71605, Apr 14 2009, 22:40:02) 
[MSC v.1500 32 bit (Intel)] on win32

import time
time.asctime((2009, 1, 1, 24, 0, 0, 0, 0, 0))

the 24 is a wrong parameter but it should'n't crash the engine. This may
be the side effect of a more serious problem though. An assertion may
fix it.
msg91120 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2009-07-31 02:08
Confirmed. On Windows, the out-of-range value triggers a debugging
assertion in the CRT library.
msg91157 - (view) Author: James Abbatiello (abbeyj) Date: 2009-08-01 04:00
Since there's no good way to disable the assertion (see issue4804),
checking the validity of the argument beforehand looks like an option. 
The checking that's currently being done in the strftime()
implementation looks useful but it is not enough.  The checking in the
MS implementation of asctime() is very strict and validates the entire
date, not just one field at a time.  So there's no way to print out
non-existant dates like (2009, 2, 31, 0, 0, 0, 0, 0, 0) -> 'Mon Feb 31
00:00:00 2009'.  

I don't know if anybody is relying on that kind of behavior.  If not
then the function could be limited to accept only valid dates. 
Alternatively we could just not call down to asctime() but instead
provide our own implementation using sprintf/strftime.
msg91199 - (view) Author: James Abbatiello (abbeyj) Date: 2009-08-02 20:04
Further investigation shows that MS asctime() doesn't like leap seconds
and causes an assertion when passing (2008, 12, 31, 23, 59, 60, 2, 366,
-1) -> 'Wed Dec 31 23:59:60 2008'.

Given that and since asctime() is such a simple function I think it
makes more sense to roll our own.  Attached is a rough patch which does
this.  It pulls the bounds checking used in strftime() out into its own
function so it can be used in both places.  Overriding ctime() is
necessary because Windows decided to use " %02d" instead of "%3d" to
print the day of the month.  Without overriding ctime() the output of
ctime(t) would be different from asctime(localtime(t)) and cause
test_conversion() to fail.  There is a USE_SYSTEM_ASCTIME switch to
decide whether to use the system asctime() or the internal one.
msg99235 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-02-11 18:59
Retrograding to critical after popular python-dev demand.
msg106533 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-26 15:23
I have untabified James' patch, ran the tests on the result, but did not otherwise review it.

There is a not-so-easy-to-find thread on python-dev discussing this issue under subject "Python 2.6.5".

Here is a relevant quote from  Martin v. Löwis:

"""
That would require that Barry actually *can* judge the issue at hand. In
the specific case, I would expect that Barry would defer the specifics
of the Windows issue to Windows experts, and then listen to what they
say.

I'm personally split whether the proposed patch is correct (i.e. whether
asctime really *can* be implemented in a cross-platform manner; any
definite ruling on that would be welcome). In the past, we had rather
taken approaches like disabling runtime assertions "locally"; not sure
whether such approaches would work for asctime as well.

In any case, I feel that the issue is not security-critical at all.
People just don't pass out-of-range values to asctime, but instead
typically pass the result of gmtime/localtime, which will not cause any
problems.
"""
msg106538 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-26 15:51
The patch as written causes buffer overflow for year >= 10,000:

>>> len(time.asctime( (10000, 1, 1, 0, 0, 0, 0, 1, -1)))
26
>>> len(time.asctime( (100000, 1, 1, 0, 0, 0, 0, 1, -1)))
27

while the buffer is only 26 characters:

+       static char result[26];
+
+       sprintf(result, "%.3s %.3s%3d %.2d:%.2d:%.2d %d\n",

This can be fixed in multiple ways: changing the year format to %.4d, using PyString_Format, or restricting the year to 4 decimal digits in check_bounds.

A nit pick: you can save some static storage by making wday_name and mon_name and possibly increase performance of asctime 2d arrays instead of arrays of pointers to null-terminated strings.  See http://www.opengroup.org/onlinepubs/009695399/functions/asctime.html .

Just as Martin, I am split on whether the patch is correct.  The fact that it is almost a copy of POSIX reference implementation gives some confidence, but that confidence is taken away by the reference implementation having a buffer overflow bug.

I am also not sure that all systems produce the same end of line character.  I would like to hear from Windows experts.
msg106958 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-06-03 15:06
Here is a quote from the relevant CERT advisory (MSC33-C):

"""
This function is supposed to output a character string of 26 positions at most, including the terminating zero. If we count the length indicated by the format directives we arrive at 25. Taking into account the terminating zero, the array size of the string appears sufficient.

However, this implementation assumes that the values of the struct tm data in timeptr are within normal ranges, and does nothing to enforce this. If any of the values print more characters than expected, the sprintf() function may overflow the result array. For instance, if tm_year has the value 12345, then 27 characters (including the terminating null character) are printed, resulting in a buffer overflow.

The asctime() function primarily exists for compatibility with older implementations. Also, the asctime() function does not support localized date and time formats. The POSIX standard developers decided to mark the asctime() function obsolescent even though they are in C99 because of the possibility of buffer overflow.

C99 also provides the strftime() function which can be used to avoid these problems.
""" https://www.securecoding.cert.org/confluence/display/seccode/MSC33-C.+Do+not+pass+invalid+data+to+the+asctime%28%29+function

(I am changing the stage back to "needs patch" because the current patch  is vulnerable to buffer overflow.)

I think it is best to leave the code as is and possibly add a warning in documentation that passing hand-crafted timetuple is unsafe on some systems and that locale aware strftime("%c", ..) is preferable to asctime.
msg107570 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-06-11 19:04
Downgrading further.  If anyone has interest in supplying a patch, please step in.  Otherwise I plan to add a note to documentation and leave the code as is.
msg107572 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-06-11 19:18
Hmm... it's still a crash, though. I really think this should be fixed. Crashing on invalid input is bad.
msg107596 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2010-06-11 22:35
How about checking the preconditions before calling asctime()? If the check fails, then we can raise an exception without crashing.
msg107605 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-06-11 23:13
That's what CERT recommends.  Their code can be reused as is:

int validate_tm(struct tm* time) {
  /* 
   * The range of valid values of the tm_sec member is [0, 60] 
   * inclusive (to allow for leap seconds).
   */
  if (time->tm_sec < 0 || time->tm_sec > 60) return 0;
  if (time->tm_min < 0 || time->tm_min >= 60) return 0;
  if (time->tm_hour < 0 || time->tm_hour >= 24) return 0;
  if (time->tm_mday <= 0 || time->tm_mday > 31) return 0;
  if (time->tm_mon < 0 || time->tm_mon >= 12) return 0;
  /* While other years are legit, they may overflow asctime()'s buffer */
  if (time->tm_year < -999 || time->tm_year > 9999) return 0;
  if (time->tm_wday < 0 || time->tm_wday >= 7) return 0;
  if (time->tm_yday < 0 || time->tm_yday >= 366) return 0;
  return 1;
}
msg108574 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-06-25 00:13
I've just noticed that time_strftime already has the range checks for tm structure fields.  These checks can be separated in a function and shared with asctime.  Marking this as "easy".  See also issue897625.
msg117196 - (view) Author: MunSic JEONG (ruseel) Date: 2010-09-23 15:16
As alexandre.vassalotti pointed in msg107596, I added precondition check.

 * extracted the range check from time_strftime as "is_valid_tm".
 * time_asctime, time_strftime call "is_valid_tm"

 * testcase for both asctime and strftime (abbeyj's work)

I splited patch just for unittest files(Lib/test/test_time.py) and (Modules/timemodule.c)
msg117206 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-09-23 16:14
The patch looks good to me.  Just a few nitpicks: the local convention seems to be no underscores in helper functions such as gettmarg.  I would call is_valid_tm, "checktm" instead.  Also, predicate-like naming (is_...) suggests a function without side-effects, while is_valid_tm sets an error when it gets an invalid tm.  Also, please add missing comments above gettmarg and is_valid_tm.  Finally, please start the function body with a "{" on a new line.  Thank you for your submission.
msg117254 - (view) Author: MunSic JEONG (ruseel) Date: 2010-09-24 01:23
Thank you for a kind guidance.

I uploaded patch with 

 - function name "checktm" 
 - add comment above gettmarg
 - move comment in "checktm" to (above function signature)
  - change comment little bit to mention issue6608
 - "{" on new line
msg117312 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-09-24 17:25
MunSic,

Your patch does not apply cleanly to the py3k branch.  Please not that new features can only go to py3k.  While some may argue that this issue is a bug, I think it is more likely that it will only be accepted for 3.2.  In any case, a py3k patch should be the starting point.  Could you post a patch agains the py3k branch?
msg117313 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-09-24 17:30
Hmm, it looks like issue6608-timemodule-2nd.patch is a patch on top of your previous patches.  I'll check if I can apply last three patches in order.
msg117315 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-09-24 17:36
No, it looks like the starting patch is against 2.7.  We need a py3k patch.  Meanwhile, I've noticed a couple of grammatical mistakes in comments:

"strftime(), asctime() does not" -> "strftime() and asctime() do not"
"fixes bug #897625, #6608" -> "fixes bugs #897625 and #6608"
msg117348 - (view) Author: MunSic JEONG (ruseel) Date: 2010-09-25 03:31
I uploaded a patch against py3k branch.

Sorry for many "Added file" and "Removed file" mails. I am learning customs Trac now.
msg117776 - (view) Author: MunSic JEONG (ruseel) Date: 2010-10-01 02:48
belopolsky
 
I am little worried about my words. I made patch against 2.7-maint branch. But I recognized that patch is not following rules, because of your guidance. Then I uploaded another patch against py3k branch again. and issue6608-p3k.patch could be applied to py3k branch cleanly. If there were any boldness in my words, I'm sorry.

and I'm ready to address any thing left in this issue. :)
msg117804 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-10-01 14:23
Committed with minor changes in r85137.   Thanks for the patch!
History
Date User Action Args
2010-10-01 14:23:55belopolskysetstatus: open -> closed
resolution: accepted
messages: + msg117804

stage: needs patch -> resolved
2010-10-01 02:48:35ruseelsetmessages: + msg117776
2010-09-25 03:31:05ruseelsetfiles: + issue6608-p3k.patch

messages: + msg117348
2010-09-25 03:24:29ruseelsetfiles: - issue6608-timemodule-2nd.patch
2010-09-25 03:24:25ruseelsetfiles: - issue6608-testcase.patch
2010-09-25 03:24:20ruseelsetfiles: - issue6608-timemodule.patch
2010-09-24 17:36:30belopolskysetmessages: + msg117315
stage: patch review -> needs patch
2010-09-24 17:30:09belopolskysetmessages: + msg117313
2010-09-24 17:25:06belopolskysetmessages: + msg117312
stage: needs patch -> patch review
2010-09-24 02:35:03brett.cannonsetnosy: - brett.cannon
2010-09-24 01:23:58ruseelsetfiles: + issue6608-timemodule-2nd.patch

messages: + msg117254
2010-09-23 16:14:38belopolskysetmessages: + msg117206
2010-09-23 15:17:30ruseelsetfiles: + issue6608-testcase.patch
2010-09-23 15:17:10ruseelsetfiles: - issue6608-timemodule.patch
2010-09-23 15:16:57ruseelsetfiles: + issue6608-timemodule.patch
2010-09-23 15:16:39ruseelsetfiles: + issue6608-timemodule.patch

nosy: + ruseel
messages: + msg117196

keywords: + patch
2010-06-25 00:13:56belopolskysetkeywords: + easy, - patch

messages: + msg108574
2010-06-11 23:16:01vstinnersetnosy: + vstinner
2010-06-11 23:13:18belopolskysetmessages: + msg107605
2010-06-11 22:35:24alexandre.vassalottisetmessages: + msg107596
2010-06-11 19:18:20pitrousettype: behavior -> crash
messages: + msg107572
2010-06-11 19:04:54belopolskysetpriority: critical -> low
versions: - Python 2.6, Python 3.1, Python 2.7
title: asctime causing python to crash -> asctime does not check its input
messages: + msg107570

type: crash -> behavior
2010-06-03 15:06:03belopolskysetmessages: + msg106958
stage: patch review -> needs patch
2010-05-26 15:51:05belopolskysetnosy: lemburg, brett.cannon, belopolsky, pitrou, alexandre.vassalotti, srid, abbeyj, AmirHabibi
messages: + msg106538
components: + Extension Modules, Windows, - Distutils
stage: needs patch -> patch review
2010-05-26 15:24:57belopolskysetfiles: + issue6608.diff
2010-05-26 15:23:47belopolskysetnosy: + belopolsky
messages: + msg106533

assignee: belopolsky
components: + Distutils, - Extension Modules, Windows
2010-02-11 18:59:21pitrousetpriority: release blocker -> critical
nosy: + pitrou
messages: + msg99235

2010-02-10 17:58:27sridsetnosy: + srid
2010-02-09 10:56:23pitrousetpriority: normal -> release blocker
nosy: + lemburg, brett.cannon
2009-08-02 20:04:48abbeyjsetfiles: + asctime.patch
keywords: + patch
messages: + msg91199
2009-08-01 04:00:39abbeyjsetnosy: + abbeyj
messages: + msg91157
2009-07-31 02:08:14alexandre.vassalottisetpriority: normal

components: + Extension Modules, Windows, - None
versions: + Python 3.1, Python 2.7, Python 3.2
nosy: + alexandre.vassalotti

messages: + msg91120
stage: needs patch
2009-07-30 23:56:47AmirHabibicreate