classification
Title: S_unpack_from() Read Access Violation
Type: security Stage:
Components: Extension Modules Versions: Python 2.5
process
Status: closed Resolution: works for me
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, gvanrossum, jnferguson
Priority: normal Keywords:

Created on 2008-04-08 16:09 by jnferguson, last changed 2009-06-03 21:42 by amaury.forgeotdarc. This issue is now closed.

Messages (5)
msg65178 - (view) Author: Justin Ferguson (jnferguson) Date: 2008-04-08 16:09
The S_unpack_from() function in Modules/_struct.c does not adequately
validate its arguments, potentially causing an out-of-bounds read
access. It should be noted that the check at line 1561 is inadequate for
obscene values of offset. Finally, because they're not really important
and I really don't want to type them all up-- you guys might want to go
through your code-- especially the modules and look for constructs where
an empty string will cause memory to be uninitialized-- look at the
audioop module for examples of what I mean-- the only thing that
actually saved you guys from overflows there was that the loops you
write with use the same variable. 

1533 static PyObject *
1534 s_unpack_from(PyObject *self, PyObject *args, PyObject *kwds)
1535 {
1536         static char *kwlist[] = {"buffer", "offset", 0};
1537 #if (PY_VERSION_HEX < 0x02050000)
1538         static char *fmt = "z#|i:unpack_from";
1539 #else
1540         static char *fmt = "z#|n:unpack_from";
1541 #endif
1542         Py_ssize_t buffer_len = 0, offset = 0;
[...]
1547 
1548         if (!PyArg_ParseTupleAndKeywords(args, kwds, fmt, kwlist,
1549                                          &buffer, &buffer_len,
&offset))
1550                 return NULL;
[...]
1558         if (offset < 0)
1559                 offset += buffer_len;
1560 
1561         if (offset < 0 || (buffer_len - offset) < soself->s_size) {
[...]
1566         }
1567         return s_unpack_internal(soself, buffer + offset);
1568 }
msg65373 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-04-11 21:06
What do you mean by "obscene" values? Do you have an example of actual
values where the check at line 1561 does not do the right thing?

-- just trying to understand where the problem is.
msg65380 - (view) Author: Justin Ferguson (jnferguson) Date: 2008-04-11 22:40
What I was originally thinking was if offset was larger than buf_len,
that would cause the check at 1561 to fail due to the subtraction. That
said, I'm not sure what type its being compared against so I need to
check this further, let me get back to you, I may have made a mistake
and this may be !bug.
msg70757 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-08-05 19:30
Ping? Patch?
msg88832 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-06-03 21:42
All expressions are of type Py_ssize_t, which is signed.
buffer_len is positive; the subtraction (buffer_len - offset) can 
overflow only if offset is a (large) negative number, but then the first 
part of the test is already fulfilled.

Closing unless more evidence is given.
History
Date User Action Args
2009-06-03 21:42:45amaury.forgeotdarcsetstatus: open -> closed
resolution: works for me
messages: + msg88832
2008-08-05 19:30:15gvanrossumsetnosy: + gvanrossum
messages: + msg70757
2008-04-11 22:40:11jnfergusonsetmessages: + msg65380
2008-04-11 21:06:44amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg65373
2008-04-08 16:09:34jnfergusoncreate