[issue6608] asctime causing python to crash

[Alexander Belopolsky]

Alexander Belopolsky  added the comment:

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.

--
stage: patch review -> needs patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6608] asctime causing python to crash

[Alexander Belopolsky]

Alexander Belopolsky  added the comment:

The patch as written causes buffer overflow for year >= 10,000:

>>> len(time.asctime( (1, 1, 1, 0, 0, 0, 0, 1, -1)))
26
>>> len(time.asctime( (10, 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.

--
components: +Extension Modules, Windows -Distutils
stage: needs patch -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6608] asctime causing python to crash

[Alexander Belopolsky]

Changes by Alexander Belopolsky :


Added file: http://bugs.python.org/file17468/issue6608.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6608] asctime causing python to crash

[Alexander Belopolsky]

Alexander Belopolsky  added the comment:

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.
"""

--
assignee:  -> belopolsky
components: +Distutils -Extension Modules, Windows
nosy: +belopolsky

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6608] asctime causing python to crash

[Antoine Pitrou]

Antoine Pitrou  added the comment:

Retrograding to critical after popular python-dev demand.

--
nosy: +pitrou
priority: release blocker -> critical

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6608] asctime causing python to crash

[Sridhar Ratnakumar]

Changes by Sridhar Ratnakumar :


--
nosy: +srid

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6608] asctime causing python to crash

[Antoine Pitrou]

Changes by Antoine Pitrou :


--
nosy: +brett.cannon, lemburg
priority: normal -> release blocker

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6608] asctime causing python to crash

[James Abbatiello]

James Abbatiello  added the comment:

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.

--
keywords: +patch
Added file: http://bugs.python.org/file14628/asctime.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6608] asctime causing python to crash

[James Abbatiello]

James Abbatiello  added the comment:

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.

--
nosy: +abbeyj

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6608] asctime causing python to crash

[Alexandre Vassalotti]

Alexandre Vassalotti  added the comment:

Confirmed. On Windows, the out-of-range value triggers a debugging
assertion in the CRT library.

--
components: +Extension Modules, Windows -None
nosy: +alexandre.vassalotti
priority:  -> normal
stage:  -> needs patch
versions: +Python 2.7, Python 3.1, Python 3.2

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue6608] asctime causing python to crash

[Amir Habibi]

New submission from Amir Habibi :

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.

--
components: None
messages: 91115
nosy: AmirHabibi
severity: normal
status: open
title: asctime causing python to crash
type: crash
versions: Python 2.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com