dbry / WavPack

WavPack encode/decode library, command-line programs, and several plugins
BSD 3-Clause "New" or "Revised" License
363 stars 66 forks source link

Review of past security vulnerabilities wrt v4.70.0 #95

Closed utkarsh2102 closed 3 years ago

utkarsh2102 commented 3 years ago

Hello @dbry,

I am incredibly thankful for your help for CVE-2020-35738 wrt 4.70.0. As mentioned there already, I am fixing all the pending security vulnerabilities for Debian ELTS which is at version 4.70.0.

I have carefully reviewed and backported all that I could find. Here's my take on this:

CVE ID Fixing Commit Backported?
CVE-2016-10169 https://github.com/dbry/WavPack/commit/4bc05fc490b66ef2d45b1de26abf1455b486b0dc Only a part of it, i.e., the hunk in src/read_words.c of the fixing commit. The initial hunk wasn't a part of the older version.
CVE-2018-19840 https://github.com/dbry/WavPack/commit/070ef6f138956d9ea9612e69586152339dbefe51 Backported completely.
CVE-2018-19841 https://github.com/dbry/WavPack/commit/bba5389dc598a92bdf2b297c3ea34620b6679b5b My assessment was the v4.70.0 was not affected as I couldn't find the vulnerable code. Let me know if this isn't the case.
CVE-2019-1010315 https://github.com/dbry/WavPack/commit/4c0faba32fddbd0745cbfaf1e1aeb3da5d35b9fc Vulnerable code is not present; DFF support was introduced later.
CVE-2019-1010317 https://github.com/dbry/WavPack/commit/f68a9555b548306c5b1ee45199ccdc4a16a6101b Vulnerable code is not present; CAF support was introduced later.
CVE-2019-1010319 https://github.com/dbry/WavPack/commit/33a0025d1d63ccd05d9dbaa6923d52b1446a62fe It looks like CLEAR (WaveHeader); was already in v4.70.0. So I guess this is not affecting that version. Right?
CVE-2019-11498 https://github.com/dbry/WavPack/commit/bc6cba3f552c44565f7f1e66dc1580189addb2b4 Vulnerable code is not present; DFF support was introduced later.
CVE-2020-35738 https://github.com/dbry/WavPack/commit/63f3ec70129843dd64e11aa4c21c4a1cf00c9f1c and https://github.com/dbry/WavPack/commit/89df160596132e3bd666322e1c20b2ebd4b92cd0 Backported already with your help! ❤️

If you have some time (and can take a quick look at this), could you help me verify that this is indeed correct? Based on this, I'll also backport to v5.0.0, where some of these are already fixed.

utkarsh2102 commented 3 years ago

Here's the entire diff that I've backported (fixing CVE-2016-10169, CVE-2018-19840, CVE-2019-1010319, and CVE-2020-35738).

--- a/src/words.c
+++ b/src/words.c
@@ -1146,6 +1146,10 @@

     low &= 0x7fffffff;
     high &= 0x7fffffff;
+
+    if (low > high)         // make sure high and low make sense
+        high = low;
+
     mid = (high + low + 1) >> 1;

     if (!c->error_limit)
--- a/src/wputils.c
+++ b/src/wputils.c
@@ -947,6 +947,21 @@
     int num_chans = config->num_channels;
     int i;

+    if (config->sample_rate <= 0) {
+        strcpy (wpc->error_message, "sample rate cannot be zero or negative!");
+        return FALSE;
+    }
+
+    if (num_chans <= 0 || num_chans > NEW_MAX_STREAMS * 2) {
+        strcpy (wpc->error_message, "invalid channel count!");
+        return FALSE;
+    }
+
+    if (config->block_samples && (config->block_samples < 16 || config->block_samples > 131072)) {
+        strcpy (wpc->error_message, "invalid custom block samples!");
+        return FALSE;
+    }
+
     wpc->total_samples = total_samples;
     wpc->config.sample_rate = config->sample_rate;
     wpc->config.num_channels = config->num_channels;
@@ -1096,10 +1111,10 @@
     else
         wpc->block_samples = wpc->config.sample_rate;

-    while (wpc->block_samples * wpc->config.num_channels > 150000)
+    while ((int64_t) wpc->block_samples * wpc->config.num_channels > 150000)
         wpc->block_samples /= 2;

-    while (wpc->block_samples * wpc->config.num_channels < 40000)
+    while ((int64_t) wpc->block_samples * wpc->config.num_channels < 40000)
         wpc->block_samples *= 2;

     if (wpc->config.block_samples) {
dbry commented 3 years ago

Oops, I guess I should have looked at this issue first. But some of my comments on the other issue still apply. This patch is fine.

As to CVE-2018-19841, yes, that code is checking the block checksums that were added with version 5.0.0, so no equivalent code existed before that.

utkarsh2102 commented 3 years ago

Oops, I guess I should have looked at this issue first.

Woah, this is my bad. There was a glitch while I was opening this issue and it now seems like it got opened twice by me. Apologies for this and the noise! 😞

As to CVE-2018-19841, yes, that code is checking the block checksums that were added with version 5.0.0, so no equivalent code existed before that.

Perfect, thank you so much for all your help again! I've fixed all the vulnerabilities in both Debian Jessie (v4.70.0) and Debian Stretch (v5.0.0).

The vulnerabilities in the later versions which are in the Debian archive are already fixed!