classification
Title: audioop: incorrect integer overflow checks
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.2, Python 3.1, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: mark.dickinson, thoger
Priority: normal Keywords: patch

Created on 2010-05-10 13:43 by thoger, last changed 2010-05-11 13:11 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
python2.6-audioop-int-overflows.diff thoger, 2010-05-10 13:43 Proposed patch for python 2.6
Messages (7)
msg105434 - (view) Author: Tomas Hoger (thoger) Date: 2010-05-10 13:43
SVN commit r64114 added integer overflow checks to multiple modules.  Checks added to audioop module are incorrect and can still be bypassed:

http://svn.python.org/view/python/trunk/Modules/audioop.c?r1=64114&r2=64113

- audioop_tostereo - should be fine, but relies on undefined behaviour
- audioop_lin2lin - undetected overflow: size=1, size2=4, len=0x40000001
- audioop_ratecv - undetected overflow: nchannels=0x5fffffff (32bit)
- audioop_ulaw2lin - undetected overflow: size=4, len=0x40000001
- audioop_alaw2lin - same as audioop_ulaw2lin
- audioop_adpcm2lin - undetected overflow: size=4, len=0x20000001

Most of these are triggered by large fragment as an input.

Attached patch replaces checks added in r64114 by checks using INT_MAX.
msg105437 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-10 14:44
Thanks for the patch.  I agree that undefined behaviour (e.g., from signed overflow) should be avoided where at all possible.

Do you have any Python examples that failed to trigger the overflow on your platform?  If so, it would be useful to add them to Lib/test/test_audioop.py as extra testcases.

One other question:  is there something about the formats that audioop is dealing with that limits sizes to INT_MAX (rather than PY_SSIZE_T_MAX, for example)?  I'm not really familiar with audio formats.

As an aside, I also find it strange that the code raises MemoryError in these cases, since these exceptions can be raised even when there's plenty of memory available.  IMO MemoryError should only be raised as a result of a failed attempt to allocate memory from the system.  Some other exception---perhaps OverflowError---would seem more appropriate here.
msg105438 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-10 14:48
Unless you have an explicit exploit, I think the 'type' should be 'behavior' rather than 'security'.
msg105441 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-10 15:05
Okay, it looks to me as though all those 'int' lengths should really be 'Py_ssize_t'.  I don't think that's something we can change in 2.6, though;  probably not in 2.7 either, since we're getting too close to the release.  It can and should be changed in py3k, though.

I'll review this patch and (assuming it looks good) apply it to the 2.x branches.  It would be great to have some tests, though.
msg105443 - (view) Author: Tomas Hoger (thoger) Date: 2010-05-10 15:21
> Do you have any Python examples that failed to trigger the overflow
> on your platform?

No, I've not really tried to create some, as I found it while looking into similar checks added to rgbimg module (which is dead and removed upstream now) in the same commit r64114.

Having another close look, I can reproduce crash with lin2lin:
  audioop.lin2lin("A"*0x40000001, 1, 4)

ratecv may cause issues too.  Other cases use for loop with multiplication product as an upper bound, so the integer overflow should be harmless in those case.

> is there something about the formats that audioop is dealing
> with that limits sizes to INT_MAX (rather than PY_SSIZE_T_MAX,
> for example)?

I've started looking into this on oldish python 2.4, where PyString_FromStringAndSize accepts int size, rather than Py_ssize_t.  Rest of the audioop code was using ints too.  It's possible it is ok to more to size_t in current python version.
msg105449 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-10 17:16
Yes, writing portable tests turns out to be tricky;  I also don't want to write tests based on int that'll fail when/if Py_ssize_t is substituted.

Applied the patch (with a couple of minor changes) in r81045 through r81048.  I'll open another issue for the int->Py_ssize_t changes.

Thanks again for the patch!
msg105509 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-11 13:11
Fixed one more bogus overflow check in r81079 through r81082.
History
Date User Action Args
2010-05-11 13:11:38mark.dickinsonsetmessages: + msg105509
2010-05-10 17:16:39mark.dickinsonsetstatus: open -> closed
messages: + msg105449

assignee: mark.dickinson
resolution: fixed
stage: test needed -> resolved
2010-05-10 15:21:33thogersetmessages: + msg105443
2010-05-10 15:05:15mark.dickinsonsetmessages: + msg105441
2010-05-10 14:48:07mark.dickinsonsettype: security -> behavior
stage: test needed
messages: + msg105438
versions: + Python 3.1, Python 2.7, Python 3.2
2010-05-10 14:44:37mark.dickinsonsetnosy: + mark.dickinson
messages: + msg105437
2010-05-10 13:43:26thogercreate