classification
Title: audioop: check that length is a multiple of the size
Type: security Stage: commit review
Components: Extension Modules Versions: Python 3.2, Python 3.1, Python 2.7, Python 2.6
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: haypo Nosy List: Arfrever, benjamin.peterson, haypo, mark.dickinson, pitrou
Priority: high Keywords: patch

Created on 2010-01-11 01:05 by haypo, last changed 2010-07-04 10:17 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
audioop_check_length-2.patch haypo, 2010-07-01 01:55
Messages (10)
msg97566 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2010-01-11 01:05
Most functions of audioop takes as input a byte string (audio data) and a size argument (number of bytes of a sample). Functions don't check that the byte string length is a multiple of the size. It leads to read and write from/to uninitialised memory and might crash.

Example on writing into uninitilized memory:

    $ python -c "import audioop; audioop.reverse('X', 2)"
    Fatal Python error: Inconsistent interned string state.
    Abandon

It allocates a string of 1 byte and write 2 bytes into this string => memory corruption.

Attached patch creates audioop_check_size() and audioop_check_parameters() functions.
msg108733 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * Date: 2010-06-26 16:32
http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2010-2089 claims that this issue is about security vulnerability. This problem seems to also affect at least Python 2.6.
msg108933 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-06-29 19:02
The patch looks fine to me.

- Please could you add some tests, to exercise the 'not a whole number of frames' errors?

- The patch obviously predates the grand reindenting, so its indentation needs fixing up

PEP 7 nits:

 - Please don't put spaces just inside the parens in an 'if' statement:  i.e., use "if (size != 1 ...)", not "if ( size != 1 ...)"  (I notice that the "if ( x == NULL )" style is already prevalent, though not universal, in the module, though.)

 - the 'else' clause of an if should be at the start of the line (i.e.,
on a new line below the closing brace of the 'if', if present)

Is there any particular reason that Python 3.1 is not included in the versions?
msg109027 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2010-07-01 01:55
@Mark: Here is the updated version of the patch including all of your remarks. I fixed 3 bugs in my patch: the checks of adpcm2lin(), alaw2lin() and audioop.ulaw2lin() were incomplete (len was not checked).

I added 3.1 to the version field.
msg109171 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-07-03 09:56
The new patch looks fine to me.

This is rather last minute for 2.7, and I'm very uncomfortable committing anything substantial this close to the release.  Still, if it's really a security vulnerability then it would be good to get it in.

For what it's worth, the code looks fine to me, and I've tested thoroughly;  I can't see any reasons this could cause problems.

Raising priority to release blocker just to alert Benjamin to the issue, and get his permission to go ahead (or not) before the release.
msg109172 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-03 10:24
The following error messages looks strange to me:

+    if (((len / size) & 1) != 0) {
+        PyErr_SetString(AudioopError, "not a whole number of frames");
+        return NULL;
+    }

Perhaps you meant "not an even number of frames"?
msg109173 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-07-03 10:36
Well, that would depend on how you define a 'frame', I guess.
msg109183 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2010-07-03 13:47
This issue is a security vulnerability referenced as CVE-2010-2089.

Fixed in 2.7 (r82492), 2.6 (r82494), 3.2 (r82495) and 3.1 (r82496).

--

> Perhaps you meant "not an even number of frames"?

Hum, no: the input data is a stereo sound track. A "frame" includes left and right channels.
msg109211 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-04 09:19
It seems you introduced a reference leak, Victor.
http://mail.python.org/pipermail/python-checkins/2010-July/094756.html
msg109212 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-07-04 10:17
Fixed in r82527 (py3k), r82528 (release31-maint).
History
Date User Action Args
2010-07-04 10:17:04mark.dickinsonsetstatus: open -> closed

messages: + msg109212
2010-07-04 09:19:44pitrousetstatus: closed -> open
priority: release blocker -> high
resolution: fixed -> accepted
messages: + msg109211
2010-07-03 13:48:15hayposetstatus: open -> closed
resolution: fixed
2010-07-03 13:47:50hayposetassignee: mark.dickinson -> haypo
messages: + msg109183
2010-07-03 12:27:24hayposetfiles: - audioop_check_length.patch
2010-07-03 10:36:17mark.dickinsonsetmessages: + msg109173
2010-07-03 10:24:48pitrousetnosy: + pitrou
messages: + msg109172
2010-07-03 10:11:23mark.dickinsonsetstage: commit review
2010-07-03 09:57:49mark.dickinsonsettype: crash -> security
2010-07-03 09:56:33mark.dickinsonsetpriority: normal -> release blocker

nosy: + benjamin.peterson
messages: + msg109171

assignee: mark.dickinson
2010-07-01 01:55:37hayposetfiles: + audioop_check_length-2.patch

messages: + msg109027
versions: + Python 3.1
2010-06-29 19:02:08mark.dickinsonsetnosy: + mark.dickinson
messages: + msg108933
2010-06-26 16:32:10Arfreversetmessages: + msg108733
versions: + Python 2.6
2010-06-25 22:06:45Arfreversetnosy: + Arfrever
2010-01-11 01:05:55haypocreate