classification
Title: PyOS_vsnprintf() underflow leads to memory corruption
Type: security Stage:
Components: Interpreter Core Versions: Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: amaury.forgeotdarc, belopolsky, gregory.p.smith, jnferguson, psss
Priority: normal Keywords: patch

Created on 2008-04-08 15:55 by jnferguson, last changed 2009-04-14 13:54 by psss. This issue is now closed.

Messages (12)
msg65174 - (view) Author: Justin Ferguson (jnferguson) Date: 2008-04-08 15:55
The PyOS_vsnprintf() contains the caveat that the length parameter
cannot be zero, however this is only enforced via assert() which is
compiled out. As a result if the length parameter is zero then the
function will underflow and write a null byte to invalid memory.

 53 int
 54 PyOS_vsnprintf(char *str, size_t size, const char  *format, va_list va)
 55 {
 56         int len;  /* # bytes written, excluding \0 */
 57 #ifndef HAVE_SNPRINTF
 58         char *buffer;
 59 #endif
 60         assert(str != NULL);
 61         assert(size > 0);
 62         assert(format != NULL);
 [...]
 65         len = vsnprintf(str, size, format, va);
 [...]
 91         str[size-1] = '\0';
 92         return len;
 93 }
msg65184 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-04-08 16:21
I think that programming errors against the python API are best checked
with asserts: I develop in development mode (with asserts enabled), then
I want my released program to run at full speed.

Other thoughts?
msg65186 - (view) Author: Justin Ferguson (jnferguson) Date: 2008-04-08 16:26
I can generally agree with that, and I admit I haven't verified all of
the code paths here- theres several hundred of them leading into this
function, are you positive all of them are safe? (seems like it would be
easier to just move the check into an if than sitting down and verifying
that all XXX hundred code paths are safe).

In the other bug, I have verified code paths into it, for instance test
the misallocation poc in 2586 as an example
msg65226 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-04-09 01:11
As long as snprintf is used with a fixed size buffer using an idiom

 snprintf(buffer, sizeof(buffer), ..)

there is no issue because sizeof(buffer) cannot be zero.  AFAICT, this 
is how python uses PyOS_vsnprintf wrapper.

On the other hand, may this is a good opportunity to revisit the 
decision to make  PyOS_vsnprintf semantics different from C99 vsnprintf.

C99 defines snprintf semantics as follows:

int snprintf(char *restrict s, size_t n,
       const char *restrict format, ...);

The snprintf() function shall be equivalent to sprintf(), with the 
addition of the n argument which states the size of the buffer referred 
to by s. If n is zero, nothing shall be written and s may be a null 
pointer. Otherwise, output bytes beyond the n-1st shall be discarded 
instead of being written to the array, and a null byte is written at the 
end of the bytes actually written into the array.

<http://www.opengroup.org/onlinepubs/000095399/functions/printf.html>
msg65227 - (view) Author: Justin Ferguson (jnferguson) Date: 2008-04-09 01:21
I do agree with your point about snprintf(..., sizeof(x), ...)-- my
single biggest point (and maybe i'm just not seeing it), is that there
appears to be no good reason for having this caveat and in turn its
essentially just code waiting to break; with as commonly used of a
function as it is, it's really a matter of when and not so much if.

While no one seems to ever use it this way, don't forget that a good
alternative to asprintf() is calling sprintf() with a length of zero to
get the length (in compliant implementations), allocating the memory and
then calling it again.
msg65228 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-04-09 01:37
On Tue, Apr 8, 2008 at 9:21 PM, Justin Ferguson <report@bugs.python.org> wrote:

> ..
>  While no one seems to ever use it this way, don't forget that a good
>  alternative to asprintf() is calling sprintf() with a length of zero to
>  get the length (in compliant implementations), allocating the memory and
>  then calling it again.

Remember that PyOS_vsnprintf was introduced back in 2001 when
(according to the comments in the file) not all platforms provided c99
compliant implementations.  If you can verify that the situation has
changes for the supported platforms, I think you will have a good case
for making the wrapper c99 compliant.
msg65242 - (view) Author: Justin Ferguson (jnferguson) Date: 2008-04-09 17:16
Actually, I'm not sure things are any better today- even the same
operating system but different versions have inconsistencies, for
instance on some versions of RHEL the vsnprintf() can fail during
unicode conversion. MSVCRT still returns -1 on truncation, et cetera.

That said, theres plenty of other implementations that manage this
without the potential of underflowing a buffer
msg65246 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-04-09 17:54
On Wed, Apr 9, 2008 at 1:16 PM, Justin Ferguson <report@bugs.python.org> wrote:
..
>  That said, theres plenty of other implementations that manage this
>  without the potential of underflowing a buffer
>

Do you have in mind something like the following?

===================================================================
--- Python/mysnprintf.c (revision 62211)
+++ Python/mysnprintf.c (working copy)
@@ -88,6 +88,7 @@
        PyMem_FREE(buffer);
 Done:
 #endif
-       str[size-1] = '\0';
+       if (size > 0)
+               str[size-1] = '\0';
        return len;
 }

I would be +0 on such change.
msg65254 - (view) Author: Justin Ferguson (jnferguson) Date: 2008-04-09 18:47
Yep, that works for me.
msg67399 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-05-26 22:10
Fixed in trunk r63734 using Alexander's suggested fix.

I will backport this to release25-maint.
msg67620 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-06-02 00:08
Fixed in release25-maint r63883.
msg85969 - (view) Author: Petr Splichal (psss) Date: 2009-04-14 13:54
Justin, is there any reproducer available for this issue?
Thanks!
History
Date User Action Args
2009-04-14 13:54:38pssssetnosy: + psss
messages: + msg85969
2008-06-02 00:08:34gregory.p.smithsetstatus: open -> closed
resolution: fixed
messages: + msg67620
2008-05-26 22:10:25gregory.p.smithsetmessages: + msg67399
2008-05-25 09:10:27gregory.p.smithsetpriority: normal
assignee: gregory.p.smith
keywords: + patch
nosy: + gregory.p.smith
2008-04-09 18:47:52jnfergusonsetmessages: + msg65254
2008-04-09 17:54:17belopolskysetmessages: + msg65246
2008-04-09 17:16:30jnfergusonsetmessages: + msg65242
2008-04-09 01:37:22belopolskysetmessages: + msg65228
2008-04-09 01:21:17jnfergusonsetmessages: + msg65227
2008-04-09 01:11:55belopolskysetnosy: + belopolsky
messages: + msg65226
2008-04-08 16:26:03jnfergusonsetmessages: + msg65186
2008-04-08 16:21:23amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg65184
2008-04-08 15:55:22jnfergusonsetcomponents: + Interpreter Core, - Distutils
2008-04-08 15:55:09jnfergusoncreate