Bounds-check the bits field in legacy get_word3

[aizu-m]

UBSan (clang -fsanitize=array-bounds), decoding a crafted version-3 file:

src/unpack3.c:1472:21: runtime error: index -2 out of bounds for type 'const uint32_t[32]'
    #0 get_word3 unpack3.c:1472
    #1 unpack_samples3 unpack3.c:760
    #2 WavpackUnpackSamples unpack_utils.c:125

get_word3 takes the bits field straight from the version-3 header and uses it both as a getbits width and as an index into bitset/bitmask, but open_file3 never range-checks it. A negative value (e.g. 0xffff) walks off the front of the 32-entry tables. The sibling get_word1/get_old_word1 already guard their table index with & ~31, so I added the same guard here. Only reachable with ENABLE_LEGACY, which the fuzzer doesn't build, so I found it reading the code rather than fuzzing.

[dbry]

Thanks for this!

Would it be possible for you to attach or link to the crafted file that triggers this so I can add it to my regression test suite?

[dbry]

Actually, nevermind. I created one.
vers-30-lossy-bad-truncated.wv.gz

[dbry]

I've looked this over and think a better fix would be to sanitize the header on opening. Doing it in get_word3() would introduce a performance hit (which is not really a big deal here because this code should only be used to recover legacy files) and I think it's cleaner to detect a corrupt file as soon as possible. Here's my suggested change to unpack3_open.c:

--- a/src/unpack3_open.c
+++ b/src/unpack3_open.c
@@ -170,9 +170,9 @@ WavpackContext *open_file3 (WavpackContext *wpc, char *error)
 
     WavpackLittleEndianToNative (&wphdr, WavpackHeader3Format);
 
-    // make sure this is a version we know about
+    // make sure this is a version we know about (and valid)
 
-    if (strncmp (wphdr.ckID, "wvpk", 4) || wphdr.version < 1 || wphdr.version > 3) {
+    if (strncmp (wphdr.ckID, "wvpk", 4) || wphdr.version < 1 || wphdr.version > 3 || wphdr.bits < 0) {
         if (error) strcpy (error, "not a valid WavPack file!");
         return WavpackCloseFile (wpc);
     }

If you like you can create a fresh PR with this and I'll merge it, or I'll do it.

Thanks again!

[aizu-m]

Makes sense, catching it at open is cleaner and keeps the hot path untouched. I've pushed your version onto this branch rather than spinning up a fresh PR: get_word3 reverted, the wphdr.bits < 0 check added to open_file3. Confirmed the crafted file now bounces with "not a valid WavPack file!" instead of reaching get_word3. Should be ready to merge.

[dbry]

Turns out I made a mistake here. Version 1.0 files do not have the "bits" field (lossy introduced with 2.0) so I added an additional check here (2484abc). I discovered this by doing a wvunpack -vm on the legacy files in the test-suite, which is probably a good idea with all these changes, although it's not guaranteed to verify everything (for example, I dont think there are any mono files).

I assume you started with those files for your fuzzing, but if you were not aware of the test suite, it's here.