[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: I'll fix it on #25029. This thread is already too long for my liking. -- resolution: -> fixed status: open -> closed superseder: -> MemoryError in test_strptime ___ Python tracker _

[issue24917] time_strftime() Buffer Over-read

STINNER Victor added the comment: Oh, tracemalloc sees the memory peak of 8 GB too: $ ./python -X tracemalloc -i -m test -v test_strptime ... SystemExit: True >>> import tracemalloc; tracemalloc.get_traced_memory()[1] / 1024.**2 8201.658247947693 Memory peak: 8201.7 MB (8.2 GB!). --

[issue24917] time_strftime() Buffer Over-read

STINNER Victor added the comment: The change c31dad22c80d introduced a regression: issue #25029. -- resolution: fixed -> status: closed -> open ___ Python tracker ___ __

[issue24917] time_strftime() Buffer Over-read

Roundup Robot added the comment: New changeset c31dad22c80d by Steve Dower in branch '3.5': Issue #24917: time_strftime() buffer over-read. https://hg.python.org/cpython/rev/c31dad22c80d New changeset f185917498ca by Steve Dower in branch '3.4': Issue #24917: time_strftime() buffer over-read. ht

[issue24917] time_strftime() Buffer Over-read

Changes by Larry Hastings : -- resolution: -> fixed stage: commit review -> resolved status: open -> closed ___ Python tracker ___ __

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: (I meant, just normal pull request. I did your two pull requests right in a row and got my wires crossed.) -- ___ Python tracker ___ _

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: Backout pull request merged, please forward-merge, thanks! -- ___ Python tracker ___ ___ Python-bugs

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: Good enough for me. -- ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://m

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: Okay, I think *I* reproduced it. 1) I pulled your cpython350 fork down locally. 2) I updated to your checkin that fixed the bug. (c31dad22c80d) 3) I reverted the change to Modules/timemodule.c to put the bug back: % hg cat -r 97393 Modules/timemodule.c > Mo

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: I wasn't able to repro the crash at all, even with the debugging flags that make this sort of issue more prominent. It relies on a very precise layout of multiple objects in memory, or possibly a specific sequence of allocations/deallocations, as well as a format

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: Steve, did you confirm that the test triggers the array bounds bug when the patch *isn't* applied? I want to confirm both that a) the test exercises the bug, and b) the fix fixes the bug. I assume you ran the test suite with the patch applied, so that covers

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: New PR: https://bitbucket.org/larry/cpython350/pull-requests/19/issue-24917-time_strftime-buffer-over-read -- ___ Python tracker ___ _

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: How's this for the test (I added an explanatory comment, since it may look like it isn't testing anything to someone who isn't familiar with the issue): def test_strftime_format_check(self): # Test that strftime does not crash on invalid format strings

[issue24917] time_strftime() Buffer Over-read

eryksun added the comment: With MSVC, if errno is cleared before calling strftime, then when buflen == 0 you know whether it's an invalid format string (EINVAL) or maxsize is too small (ERANGE). There's no need to guess. Oddly, only EINVAL is documented, even though setting ERANGE has been in

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: Having slept on it, I agree with Steve. We should make the minimal change necessary in order to not crash. However, it still needs a regression test. The test can use JohnLeitch's proposed test as a good starting point, but it must accept either success or

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: FWIW, those macros prevent a certain class of "undefined behavior," which in this case is to terminate the process rather than return an error code. They don't do anything to prevent crashes due to exploitation or malicious code. The place for more robust formatt

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: Historically, time and os modules have been considered low level, thin wrappers over system libc functions. Users of these modules are the proverbial "consenting adults" who should understand their power and the associated risks. The place to provide a

[issue24917] time_strftime() Buffer Over-read

John Leitch added the comment: First, let me begin by saying I believe this patch will fix the buffer over-read, which is a good step forward. However, after giving the matter more thought, and at the risk of wearing out my welcome, I am of the belief that relying on the CRT to handle malforme

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: New patch attached that just breaks out of the scanning loop and lets the system CRT handle invalid format strings. Fixes the condition that was suppressing some errors on Windows. Also fixed the PEP 7 issues around the changed code (I believe). No new test beca

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: Larry: agreed, no crashing. We'll get the trivial fix in that doesn't raise any error, and should probably stop suppressing the exception in the situation eryksun describes. That will correctly expose the platform's strftime semantics. Unless you want to write mo

[issue24917] time_strftime() Buffer Over-read

Changes by Larry Hastings : -- priority: release blocker -> deferred blocker ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue24917] time_strftime() Buffer Over-read

eryksun added the comment: > MSVC seems somewhat inconsistent about its response: > >>> strftime('aaa%') > '' That's not due to MSVC. It's setting errno to EINVAL. The problem is that time_strftime is testing (buflen > 0 || i >= 256 * fmtlen). The initial value of the outbuf size i is 1024, so

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: I'm going to back this out of 3.5.0rc3. I'm still willing to discuss accepting it into 3.5.0 final in some form, but for now I don't want to hold up the release. Steve: it should never be possible to crash the Python interpreter with well-formed Python code.

[issue24917] time_strftime() Buffer Over-read

John Leitch added the comment: Yes, this is a user-mode read, but I disagree with the assertion that it's not possible to use this to disclose memory. While it isn't as critical as something that outright dumps memory, there is logic that throws exceptions based on values encountered while rea

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: Yeah, I came in late and in too much of a rush too :( Backed out the broken changes, and now I'm off to bed. Not convinced there's anything that needs fixing here, and IIRC we argued through the different platform behaviours on the issue when I removed the null c

[issue24917] time_strftime() Buffer Over-read

Roundup Robot added the comment: New changeset 73227e857d6b by Steve Dower in branch '3.5': Issue #24917: Backed out changeset 09b62202d9b7 https://hg.python.org/cpython/rev/73227e857d6b New changeset 743924064dc8 by Steve Dower in branch 'default': Issue #24917: Backed out changeset 09b62202d9b

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: Oh, maybe it was all like that before. Sorry, I was in a hurry and not in a charitable frame of mind. -- ___ Python tracker ___ __

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: I'll get my commits out of the way to make the buildbots green again. -- ___ Python tracker ___ ___ Pyt

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: I have convinced myself that disallowing trailing % marks on all platforms is the right thing to do. Whether or not the underlying platform tolerates it, it is never *valid* input to strftime. I have a patch incubating at home. I'll put it up for review when

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: Having now read over this whole issue, I don't actually see where the security vulnerability is (on Windows at least). This is a user-mode read, so it can only access memory in the same process, and it doesn't display it anywhere. The worst that can happen is tha

[issue24917] time_strftime() Buffer Over-read

John Leitch added the comment: If it's so wildly inconsistent, it's my opinion that Python should perform its own validation to achieve better cross-platform support. The alternative is playing a never ending game of whack-a-mole, or just accepting that format strings may cause exceptions in s

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: Whoops, must have done a bad copy-paste to get that DECREF in there (I couldn't apply the patch directly because it didn't come from an HG repo, so I had to do it by hand). MSVC seems somewhat inconsistent about its response: >>> strftime('aaa%') '' >>> strftime(

[issue24917] time_strftime() Buffer Over-read

John Leitch added the comment: Yikes--your comment prompted me to look at the check-in, and it seems my patch wasn't properly applied. The curly braces got tweaked, which is minor as you stated, but more importantly the AIX code should not decref format. That could introduce problems bigger th

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: I'll fix the PEP 7 stuff (mostly inherited). MSVC can also handle '%#' and '%' as format strings (producing '' in both cases). If that matches libc behaviour, I see no reason to raise a ValueError here apart from consistency with previous Python releases. If con

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: Sorry, maybe you inherited those violations. I was in a hurry and not in a charitable frame of mind. -- ___ Python tracker ___ ___

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: The starting curly brace goes on the same line as the statement starting the block. Keywords followed by a left parenthesis get a space between the keyword and the parenthesis. It's a small matter, I'm really much more interested in reconciling the behavior

[issue24917] time_strftime() Buffer Over-read

John Leitch added the comment: Is there a way to see what style guidelines have been violated? The only thing I can think of is the curly braces in the Windows check, but I was following the conventions of the surrounding code. -- ___ Python tracker

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: The tests from this patch fail on Linux. - First: There is no trailing % test on Linux, and glibc's strftime() happily ignores a trailing %, so no ValueError is raised. Python should do either one or the other of the following: 1) Python should enforce no

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: I applied the AIX fix to 3.4 (introduced in #19634). Python 3.2 and 3.3 aren't affected. -- assignee: -> steve.dower resolution: -> fixed stage: patch review -> commit review status: open -> closed ___ Python tracker

[issue24917] time_strftime() Buffer Over-read

Roundup Robot added the comment: New changeset 09b62202d9b7 by Steve Dower in branch '3.5': Issue #24917: time_strftime() Buffer Over-read. Patch by John Leitch. https://hg.python.org/cpython/rev/09b62202d9b7 New changeset 8b81b7ad2d0a by Steve Dower in branch '3.5': Issue #24917: Moves NEWS ent

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: Pull request accepted. Please forward-merge. Thanks! -- ___ Python tracker ___ ___ Python-bugs-lis

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: > Rather than debating about how various platforms handle malformed > format strings for strftime(), and whether or not they crash, we > should simply prevent the native strftime() function from seeing > them in the first place. The crash is nothing to do with t

[issue24917] time_strftime() Buffer Over-read

Steve Dower added the comment: I'll do the PR. -- ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.

[issue24917] time_strftime() Buffer Over-read

eryksun added the comment: > Rather than debating about how various platforms handle malformed > format strings for strftime(), and whether or not they crash, we > should simply prevent the native strftime() function from seeing > them in the first place. Of course the check needs to be resto

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: Rather than debating about how various platforms handle malformed format strings for strftime(), and whether or not they crash, we should simply prevent the native strftime() function from seeing them in the first place. I'd like the "v3" patch in 3.5.0rc3. C

[issue24917] time_strftime() Buffer Over-read

eryksun added the comment: > On Windows, the C lib strftime() segfaults with a trailing '%', > just as it does with unsupported format codes. No, it doesn't segfault; it invokes the invalid parameter handler (IPH). This could always be configured for the whole process, but with VC 14 it can al

[issue24917] time_strftime() Buffer Over-read

Marc-Andre Lemburg added the comment: On 05.09.2015 03:49, Alexander Belopolsky wrote: > > Alexander Belopolsky added the comment: > > Hmm, on Mac OSX "%" and "A%" are valid format strings: > time.strftime("%") > '%' time.strftime("A%") > 'A%' > > Mark's experiments show that on Win

[issue24917] time_strftime() Buffer Over-read

John Leitch added the comment: I plucked the error message from the % operator: >>> '%' % 'foo' Traceback (most recent call last): File "", line 1, in ValueError: incomplete format >>> '%z' % 'foo' Traceback (most recent call last): File "", line 1, in ValueError: unsupported format charac

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: Hmm, on Mac OSX "%" and "A%" are valid format strings: >>> time.strftime("%") '%' >>> time.strftime("A%") 'A%' Mark's experiments show that on Windows they are not. What about the other platforms affected by the patch? I am concerned about the bottom pa

[issue24917] time_strftime() Buffer Over-read

John Leitch added the comment: Attached is a revised patch. -- Added file: http://bugs.python.org/file40367/time_strftime_Buffer_Over-read_v2.patch ___ Python tracker ___ __

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: > if there's a risk I'm overlooking I'd like to better understand it, > and the relevant Python documentation should be updated. I don't think there is any special risk that you are overlooking other than a documented fact that Python's strftime is a thin

[issue24917] time_strftime() Buffer Over-read

Larry Hastings added the comment: Yes, I'd like this fix in 3.5.0. One bit of feedback on the patch: outbuf is a char * (or wchar_t *), therefore outbuf[1] is a char (or wchar_t). You shouldn't compare it to NULL. I'm not sure we still support any compilers that define NULL as (void *)0, but

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: As far as I know, the practical consequence of "security" classification for the issue is how many affected older versions will be patched. I am keeping that and the 3.2 - 3.5 versions range. The priority may affect whether this will make it into 3.5.0.

[issue24917] time_strftime() Buffer Over-read

John Leitch added the comment: When I get a bit of slackspace (probably tomorrow afternoon/evening) I can test on the spectrum of versions to confirm the issue is in >= 3.2. I'll also look into improving our automation so all future reports can have the appropriate versions flagged. Regarding

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: FWIW, the patch looks good to me, but it needs to be reviewed by a Windows developer. -- components: +Extension Modules ___ Python tracker __

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: I will keep the "security" classification, but will not increase the priority. Accepting format strings from untrusted sources is a vulnerability in and by itself and in most cases those strings are literals. --

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: John, There is no doubt that this is a bona fide bug. I was just hoping that someone with a Windows development machine would help figuring out the affected versions. >From the hg history, it looks like the faulty code is present in all versions >star

[issue24917] time_strftime() Buffer Over-read

John Leitch added the comment: It very well may apply to versions apart from 3.5. Our test environment is quite complex and unfriendly to working with multiple versions of Python. Plus, we're strapped for time, so we tend to file under the version we're currently targeting and defer to those b

[issue24917] time_strftime() Buffer Over-read

Mark Lawrence added the comment: Okay, so presumably this applies to all supported versions and not just 3.5? Can you also remove the reproducer and supply specific instructions on how to build the code so that the problem can be reproduced. -- components: +Windows nosy: +paul.moore,

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: Sorry for a bad guess. John's advise makes much more sense than mine. -- ___ Python tracker ___ _

[issue24917] time_strftime() Buffer Over-read

Mark Lawrence added the comment: Python prompt or script makes no difference. C:\Users\Mark\Desktop>py -2.6 time_strftime_Buffer_Over-read.py Traceback (most recent call last): File "time_strftime_Buffer_Over-read.py", line 2, in strftime("AA%"*0x1) ValueError: Invalid format string

[issue24917] time_strftime() Buffer Over-read

John Leitch added the comment: > I have tried the reproducer on Windows 10 with 2.6, 2.7, 3.3, 3.4, 3.5 and > 3.6. In every case I got this. What you are observing is due to the arrangement and contents of process memory. With a simple repro (such as the one provided), there's a good chance

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: > In every case I got [ValueError] You shouldn't have. I am no Windows expert, but I suspect something is wrong with your use of the command line. Please try it at the Python prompt or put the code in a script. --

[issue24917] time_strftime() Buffer Over-read

Mark Lawrence added the comment: I have tried the reproducer on Windows 10 with 2.6, 2.7, 3.3, 3.4, 3.5 and 3.6. In every case I got this. C:\py -3.4 -c"from time import *;strftime('AA%'*0x1)" Traceback (most recent call last): File "", line 1, in ValueError: Invalid format string

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: > Would you like us to report this and future vulnerabilities to CERT? If it affects released versions, I think this is the right thing to do. Note that I don't know what PSF policy is on this is or whether they even have one. -- __

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: This is happening in windows-only code, so not being a windows user I cannot reproduce it. It does not look like something new. This code was last touched in #10653. Can someone confirm that only 3.5 is affected? -- nosy: +haypo _

[issue24917] time_strftime() Buffer Over-read

John Leitch added the comment: Currently, no. Would you like us to report this and future vulnerabilities to CERT? -- ___ Python tracker ___

[issue24917] time_strftime() Buffer Over-read

Changes by Alexander Belopolsky : -- stage: -> patch review ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: ht

[issue24917] time_strftime() Buffer Over-read

Alexander Belopolsky added the comment: Is there a CERT report associated with this vulnerability? -- ___ Python tracker ___ ___ Pytho

[issue24917] time_strftime() Buffer Over-read

Changes by John Leitch : -- nosy: +belopolsky, lemburg ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://

[issue24917] time_strftime() Buffer Over-read

Changes by Bryce Darling : -- nosy: +brycedarling ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.

[issue24917] time_strftime() Buffer Over-read

Changes by John Leitch : Added file: http://bugs.python.org/file40229/time_strftime_Buffer_Over-read.py ___ Python tracker ___ ___ Python-bugs

[issue24917] time_strftime() Buffer Over-read

New submission from John Leitch: Python 3.5 suffers from a vulnerability caused by the behavior of the time_strftime() function. When called, the function loops over the format string provided, using strchr to search for each instance of '%'. After finding a '%', it continues to search two cha