classification
Title: Improve exceptions in aifc, sunau and wave
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: BT123, miss-islington, ned.deily, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2017-11-17 09:42 by BT123, last changed 2018-03-18 20:52 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
audio-testcase.wav BT123, 2017-11-17 09:42 open the audio file using wave.open function and will lead to divided by zero
Pull Requests
URL Status Linked Edit
PR 4437 BT123, 2017-11-17 09:42
PR 4440 closed BT123, 2017-11-17 10:00
PR 5951 merged serhiy.storchaka, 2018-03-01 15:52
PR 6139 merged miss-islington, 2018-03-18 09:01
Messages (15)
msg306420 - (view) Author: zhangdeyue (BT123) * Date: 2017-11-17 09:42
I found a bug in wave.py because there is no check for self._channel in _read_fmt_chunk function.
When I try to open a wav file which channel is zero, it will crash bacause of divided by zero in initfp function.
msg313079 - (view) Author: zhangdeyue (BT123) * Date: 2018-03-01 02:25
the bug still in the Python 3.7.0b2 - 2018-02-28
msg313080 - (view) Author: zhangdeyue (BT123) * Date: 2018-03-01 02:52
The CVE email:
The CVE ID is below. Please check whether the vulnerability still
exists in Python 3.6.4, and please inform the software maintainer that
the CVE ID has been assigned: https://bugs.python.org

Use CVE-2017-18207 for this vulnerability in Python.
msg313084 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-01 06:10
I have no idea why this was classified as a vulnerability. I don't think it can crash an application. If you have an example of crashing please provide it.

I would not classify this issue even as a bug. It is obvious that invalid files can cause an exception. It may be good to detect some of errors earlier and raise more specific exception (Error would be more appropriate here than ValueError). But in general validating the wave file is not the purpose of this module, and this task can't be performed without reading the whole file, not only the header.

All changes in wave.py should be ported to aifc.py and sunau.py and needs tests.
msg313087 - (view) Author: zhangdeyue (BT123) * Date: 2018-03-01 09:37
ok, I found this bug when I use librosa-0.5.1 to read audio file in the audio-classification project -- an ASR project. (https://github.com/nextco/audio-classification)

In the project, librosa.load function read audio file, and it called wave.open function finally. But all of the functions don't validate the audio file which lead to the project dividing by zero. 

Although the bug is easy and small, I think the bug should be fixed because there are so many python-library(such as librosa, audioread) use it without validation in more and more popular ASR project.

My program backtrace is as follow:
https://github.com/BT123/testcasesForMyRequest/blob/master/wave-1.png
https://github.com/BT123/testcasesForMyRequest/blob/master/wave-2.png
msg313097 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-01 16:00
PR 5951 adds explicit checks for the number of channels and sample width when read audio files in aifc, sunau and wave and converts some struct.error to EOFError when the file is truncated in wave.

This change can break an existing code that currently successfully opens corrupted audio files and read only headers, but not audio samples. Therefore it is safer to not backport it, even if classify this issue as a bug.
msg313111 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-03-01 21:13
@zhangdeyue, can you please request that the CVE be unassigned?  I think we all agree this is *not* a security issue.
msg313121 - (view) Author: zhangdeyue (BT123) * Date: 2018-03-02 02:20
I agree that it is very small, but I still think it is indeed a security issue, because it can crash real world program when called by some library used in Deep Learning ASR project. 

Does a CVE assigned have any negative impact on you?
msg313123 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-03-02 03:43
> I agree that it is very small, but I still think it is indeed a security issue, because it can crash real world program when called by some library used in Deep Learning ASR project. 

That sounds like a programming error, not a security bug.  The case you describe causes a Python exception to be raised.  As noted in the Python Language Reference: "Exceptions are a means of breaking out of the normal flow of control of a code block in order to handle errors or other exceptional conditions."  Any program using Python libraries needs to be prepared to handle a wide variety of exception, particularly if the program is dealing with external data, like an arbitrary audio file.  If a program is failing because it fails to properly check for exceptions, like by using a "try" block, that's a bug in the program, not a security problem in Python.

> Does a CVE assigned have any negative impact on you?

Yes, because it implies that there is some security problem in Python that downstream vendors and users need to be concerned about and should expect some fix or other mediation from the responsible project.  That is not the case here.

Now, as Serhily noted, it might be nice if the exception produced a more meaningful message but changing that would not change the end result for a program: it will still see an exception and either need to handle it or be terminated like with any other Python exception.
msg313125 - (view) Author: zhangdeyue (BT123) * Date: 2018-03-02 04:14
I'm confused now. For any program which receive external file, to check the input file is necessary to do, isn't it? And program error lead to security bug, that's not right? 

The program itself check input file, catch and show some exceptions or asserts means that the error has been taken into account and the program is robust. 

However, I got the message from the system's check.
msg313230 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-03-04 23:43
> For any program which receive external file, to check the input file is necessary to do, isn't it?

Yes and no.  wave.py is doing checking and can raise various exceptions.  So a well-designed program has to be prepared to handle exceptions when calling wave.py.  The suggested fix would provide a more specific error message and exception, rather than a division by zero one, but the net effect to the caller of wave.py is the same.

> And program error lead to security bug, that's not right?

No. Just because a program can terminate due to an uncaught exception is not by itself considered a security error.  See https://www.python.org/news/security/ for a discussion. In particular, "The general rule is any attack worth reporting via the security address must allow an attacker to affect the confidentiality, integrity and availability of the Python application or its system for which the attacker does not already have the capability."  As things stand now, if an application is vulnerable to a denial-of-service attack due to a faulty wav file, it is a failure in that application to be handling possible exceptions from wave.py, not a security issue in Python itself.
msg314030 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-18 07:55
New changeset 134cb01cda50f02725575808130b05d2d776693f by Serhiy Storchaka in branch 'master':
bpo-32056: Improve exceptions in aifc, wave and sunau. (GH-5951)
https://github.com/python/cpython/commit/134cb01cda50f02725575808130b05d2d776693f
msg314033 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-18 09:16
Now open() in modules aifc, sunau and wave will raise only EOFError (if the file was truncated) or corresponding module error exception on corrupted files. aifc.open() can raise also OverflowError, but this is a different issue32978. And of course OSError, MemoryError, RecursionError and KeyboardInterrupt can be raised for causes not related to the correctness of the file.

I withdraw the part of my claim in msg313084. I have tested -- backporting this is safe, because some error always was raised in open(). But it can help existing user code which was not aware about wider set of exceptions (like in the original report). It can work in common case, and handle most corrupted files well, but fail in cases of specially created malicious data. Thus I think it is worth to backport this change at least to 3.7. What are your thoughts Ned?
msg314055 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-03-18 20:41
I'm OK with merging this for 3.7.0b3.
msg314057 - (view) Author: miss-islington (miss-islington) Date: 2018-03-18 20:50
New changeset 3c0a5a7c7ba8fbbc95dd1fe76cd7a1c0ce167371 by Miss Islington (bot) in branch '3.7':
bpo-32056: Improve exceptions in aifc, wave and sunau. (GH-5951)
https://github.com/python/cpython/commit/3c0a5a7c7ba8fbbc95dd1fe76cd7a1c0ce167371
History
Date User Action Args
2018-03-18 20:52:01serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-03-18 20:50:43miss-islingtonsetnosy: + miss-islington
messages: + msg314057
2018-03-18 20:41:00ned.deilysetmessages: + msg314055
versions: + Python 3.7
2018-03-18 09:16:54serhiy.storchakasetmessages: + msg314033
2018-03-18 09:01:20miss-islingtonsetpull_requests: + pull_request5896
2018-03-18 07:55:56serhiy.storchakasetmessages: + msg314030
2018-03-04 23:43:55ned.deilysetmessages: + msg313230
2018-03-02 04:14:47BT123setmessages: + msg313125
2018-03-02 03:43:44ned.deilysetmessages: + msg313123
2018-03-02 02:20:02BT123setmessages: + msg313121
2018-03-01 21:13:07ned.deilysetkeywords: - security_issue
nosy: + ned.deily
messages: + msg313111

2018-03-01 16:00:55serhiy.storchakasettitle: Improve exceptions in Lib/wave.py -> Improve exceptions in aifc, sunau and wave
messages: + msg313097
versions: + Python 3.8
2018-03-01 15:52:24serhiy.storchakasetkeywords: + patch
pull_requests: + pull_request5717
2018-03-01 09:37:53BT123setmessages: + msg313087
versions: - Python 3.8
2018-03-01 06:10:13serhiy.storchakasettype: crash -> enhancement
title: bug in Lib/wave.py -> Improve exceptions in Lib/wave.py
messages: + msg313084
versions: + Python 3.8, - Python 3.7
2018-03-01 02:55:16ned.deilysetkeywords: + security_issue, - patch
nosy: + serhiy.storchaka
2018-03-01 02:52:52BT123setmessages: + msg313080
2018-03-01 02:25:46BT123setmessages: + msg313079
versions: + Python 3.7, - Python 3.6
2017-11-17 10:00:14BT123setkeywords: + patch
stage: patch review
pull_requests: + pull_request4382
2017-11-17 09:42:26BT123create