Improve exceptions in aifc, sunau and wave

[BT123]

BPO	32056
Nosy	@ned-deily, @serhiy-storchaka, @BT123, @miss-islington
PRs	[3.6] Add check for channels of wav file in Lib/wave.py #4437 bpo-32056: Add check for channels of wav file in wave module #4440 bpo-32056: Improve exceptions in aifc, wave and sunau. #5951 [3.7] bpo-32056: Improve exceptions in aifc, wave and sunau. (GH-5951) #6139
Files	audio-testcase.wav: open the audio file using wave.open function and will lead to divided by zero

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-03-18.20:52:01.286>
created_at = <Date 2017-11-17.09:42:26.780>
labels = ['3.7', '3.8', 'type-feature', 'library']
title = 'Improve exceptions in aifc, sunau and wave'
updated_at = <Date 2018-03-18.20:52:01.285>
user = 'https://github.com/BT123'

bugs.python.org fields:

activity = <Date 2018-03-18.20:52:01.285>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2018-03-18.20:52:01.286>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2017-11-17.09:42:26.780>
creator = 'BT123'
dependencies = []
files = ['47270']
hgrepos = []
issue_num = 32056
keywords = ['patch']
message_count = 15.0
messages = ['306420', '313079', '313080', '313084', '313087', '313097', '313111', '313121', '313123', '313125', '313230', '314030', '314033', '314055', '314057']
nosy_count = 4.0
nosy_names = ['ned.deily', 'serhiy.storchaka', 'BT123', 'miss-islington']
pr_nums = ['4437', '4440', '5951', '6139']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue32056'
versions = ['Python 3.7', 'Python 3.8']

[BT123]

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.

[BT123]

the bug still in the Python 3.7.0b2 - 2018-02-28

[BT123]

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.

[serhiy-storchaka]

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.

[BT123]

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

[serhiy-storchaka]

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.

[ned-deily]

@zhangdeyue, can you please request that the CVE be unassigned? I think we all agree this is *not* a security issue.

[BT123]

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?

[ned-deily]

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.

[BT123]

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.

[ned-deily]

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.

[serhiy-storchaka]

New changeset 134cb01 by Serhiy Storchaka in branch 'master':
bpo-32056: Improve exceptions in aifc, wave and sunau. (GH-5951)
134cb01

[serhiy-storchaka]

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 bpo-32978. 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?

[ned-deily]

I'm OK with merging this for 3.7.0b3.

[miss-islington]

New changeset 3c0a5a7 by Miss Islington (bot) in branch '3.7':
bpo-32056: Improve exceptions in aifc, wave and sunau. (GH-5951)
3c0a5a7