From 69cd6039eb3b1e5559121e9d2e7fe2299a814fb7 Mon Sep 17 00:00:00 2001 From: nowhere Date: Sat, 30 Dec 2017 18:52:36 +0100 Subject: [PATCH 1/5] Make parsing WAV data more robust Check that we have enough data in the input instead of happily reading out of bounds memory. This fixes the most common problem of crashing on bad data which doesn't look like WAV at all, but doesn't fix problems with parsing input which does look like WAV, but is incorrect -- this will be done in subsequent commits. --- src/unix/sound.cpp | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/unix/sound.cpp b/src/unix/sound.cpp index ae38289009..2227b117d1 100644 --- a/src/unix/sound.cpp +++ b/src/unix/sound.cpp @@ -651,14 +651,6 @@ bool wxSound::LoadWAV(const void* data_, size_t length, bool copyData) waveformat.uiBlockAlign = wxUINT16_SWAP_ON_BE(waveformat.uiBlockAlign); waveformat.uiBitsPerSample = wxUINT16_SWAP_ON_BE(waveformat.uiBitsPerSample); - // get the sound data size - wxUint32 ul; - memcpy(&ul, &data[FMT_INDEX + waveformat.uiSize + 12], 4); - ul = wxUINT32_SWAP_ON_BE(ul); - - if ( length < ul + FMT_INDEX + waveformat.uiSize + 16 ) - return false; - if (memcmp(data, "RIFF", 4) != 0) return false; if (memcmp(&data[WAVE_INDEX], "WAVE", 4) != 0) @@ -675,6 +667,24 @@ bool wxSound::LoadWAV(const void* data_, size_t length, bool copyData) waveformat.ulAvgBytesPerSec / waveformat.uiBlockAlign) return false; + // get file size from header + wxUint32 chunkSize; + memcpy(&chunkSize, &data[4], 4); + chunkSize = wxUINT32_SWAP_ON_BE(chunkSize); + + // ensure file length is at least length in header + if (chunkSize > length - 8) + return false; + + // get the sound data size + wxUint32 ul; + memcpy(&ul, &data[FMT_INDEX + waveformat.uiSize + 12], 4); + ul = wxUINT32_SWAP_ON_BE(ul); + + // ensure we actually have at least that much data in the input + if (ul > length - FMT_INDEX - waveformat.uiSize - 16) + return false; + m_data = new wxSoundData; m_data->m_channels = waveformat.uiChannels; m_data->m_samplingRate = waveformat.ulSamplesPerSec; From 45e8d13e1304f39fdf763ec06a9443d3a4df06f3 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Dec 2017 17:28:27 +0100 Subject: [PATCH 2/5] Add format sub-chunk size check to WAV parsing This fixes a crash due to reading beyond the buffer bounds when checking for "data" if WAVEFORMAT::uiSize was too big. --- src/unix/sound.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/unix/sound.cpp b/src/unix/sound.cpp index 2227b117d1..53e3f31240 100644 --- a/src/unix/sound.cpp +++ b/src/unix/sound.cpp @@ -657,6 +657,12 @@ bool wxSound::LoadWAV(const void* data_, size_t length, bool copyData) return false; if (memcmp(&data[FMT_INDEX], "fmt ", 4) != 0) return false; + + // Check that the format chunk size is correct: it must be 16 for PCM, + // which is the only format we handle. + if (waveformat.uiSize != 16) + return false; + if (memcmp(&data[FMT_INDEX + waveformat.uiSize + 8], "data", 4) != 0) return false; From 932f384c87959790fc47fe1099c8ac54530c1b3d Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Dec 2017 17:37:21 +0100 Subject: [PATCH 3/5] Avoid division by 0 when parsing WAV data Don't divide by waveformat.uiBlockAlign which could be 0, but rather multiply by it and verify that we get the expected result. This is more robust, as it prevents crashes on malformed input and also slightly more efficient even for correct input. --- src/unix/sound.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/unix/sound.cpp b/src/unix/sound.cpp index 53e3f31240..e0913ee65d 100644 --- a/src/unix/sound.cpp +++ b/src/unix/sound.cpp @@ -669,8 +669,8 @@ bool wxSound::LoadWAV(const void* data_, size_t length, bool copyData) if (waveformat.uiFormatTag != WAVE_FORMAT_PCM) return false; - if (waveformat.ulSamplesPerSec != - waveformat.ulAvgBytesPerSec / waveformat.uiBlockAlign) + if (waveformat.ulAvgBytesPerSec != + waveformat.ulSamplesPerSec * waveformat.uiBlockAlign) return false; // get file size from header From 61c7cf9a9c5d743fee16094f491fa83c03391eb8 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Dec 2017 17:45:01 +0100 Subject: [PATCH 4/5] Fix yet another division by 0 when parsing WAV data Ensure that the size of one sample (including all the channels) is non zero before dividing by it. --- src/unix/sound.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/unix/sound.cpp b/src/unix/sound.cpp index e0913ee65d..9697c36af4 100644 --- a/src/unix/sound.cpp +++ b/src/unix/sound.cpp @@ -673,6 +673,13 @@ bool wxSound::LoadWAV(const void* data_, size_t length, bool copyData) waveformat.ulSamplesPerSec * waveformat.uiBlockAlign) return false; + // We divide by this number below to obtain the number of samples, so it + // definitely can't be 0. + wxUint32 const sampleSize = + waveformat.uiChannels * waveformat.uiBitsPerSample / 8; + if (!sampleSize) + return false; + // get file size from header wxUint32 chunkSize; memcpy(&chunkSize, &data[4], 4); @@ -695,7 +702,7 @@ bool wxSound::LoadWAV(const void* data_, size_t length, bool copyData) m_data->m_channels = waveformat.uiChannels; m_data->m_samplingRate = waveformat.ulSamplesPerSec; m_data->m_bitsPerSample = waveformat.uiBitsPerSample; - m_data->m_samples = ul / (m_data->m_channels * m_data->m_bitsPerSample / 8); + m_data->m_samples = ul / sampleSize; m_data->m_dataBytes = ul; if (copyData) From c010efc172907253570126d5a13c70230b2abbda Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 30 Dec 2017 18:47:43 +0100 Subject: [PATCH 5/5] Avoid integer overflow when computing the sample size While it seems to be harmless in this particular case, it still prevents testing this code with UBSAN by triggering it here, so check that multiplication doesn't overflow. --- src/unix/sound.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/unix/sound.cpp b/src/unix/sound.cpp index 9697c36af4..355fc8673d 100644 --- a/src/unix/sound.cpp +++ b/src/unix/sound.cpp @@ -673,10 +673,16 @@ bool wxSound::LoadWAV(const void* data_, size_t length, bool copyData) waveformat.ulSamplesPerSec * waveformat.uiBlockAlign) return false; - // We divide by this number below to obtain the number of samples, so it - // definitely can't be 0. - wxUint32 const sampleSize = - waveformat.uiChannels * waveformat.uiBitsPerSample / 8; + // We divide by the sample size below to obtain the number of samples, so + // it definitely can't be 0. Also take care to avoid integer overflow when + // computing it. + unsigned tmp = waveformat.uiChannels; + if (tmp >= 0x10000) + return false; + + tmp *= waveformat.uiBitsPerSample; + + wxUint32 const sampleSize = tmp / 8; if (!sampleSize) return false;