Fix Attempt to free an uninitialized memory pointer in vorbis_deinit (GHSL-2023-169/CVE-2023-45679)

[JarLob]

A crafted file may trigger memory allocation failure in start_decoder at [1]. In that case the function returns early [2], but some of the pointers in f->comment_list are left initialized [3].

   f->comment_list_length = get32_packet(f);
   f->comment_list = NULL;
   if (f->comment_list_length > 0)
   {
      f->comment_list = (char**) setup_malloc(f, sizeof(char*) * (f->comment_list_length)); // [3]
      if (f->comment_list == NULL)                  return error(f, VORBIS_outofmem);
   }

   for(i=0; i < f->comment_list_length; ++i) {
      len = get32_packet(f);
      f->comment_list[i] = (char*)setup_malloc(f, sizeof(char) * (len+1)); // [1] OOM
      if (f->comment_list[i] == NULL)               return error(f, VORBIS_outofmem); // [2]

      for(j=0; j < len; ++j) {
         f->comment_list[i][j] = get8_packet(f);
      }
      f->comment_list[i][len] = (char)'\0';
   }

Later setup_free is called on these pointers in vorbis_deinit [4].

static void vorbis_deinit(stb_vorbis *p)
{
   int i,j;

   setup_free(p, p->vendor);
   for (i=0; i < p->comment_list_length; ++i) {
      setup_free(p, p->comment_list[i]); // [4]
   }

Impact

This issue may lead to code execution.

Resources

To reproduce the issue:

1. Make ASAN build of the following program:

#include "../stb_vorbis.c"
#include <stdint.h>

int main(int argc, char* argv[])
{
    const uint8_t data[] = {0x4f,0x67,0x67,0x53,0x00,0x7a,0x18,0xfe,0xa9,0x00,0x53,
                            0x00,0xe3,0xb5,0x21,0x68,0x00,0x00,0x00,0x00,0x00,0x00,
                            0x6b,0x0e,0xa0,0x75,0x01,0x1e,0x01,0x76,0x6f,0x72,0x62,
                            0x69,0x73,0x00,0x00,0x00,0x00,0x01,0xfb,0x07,0xae,0x69,
                            0x73,0x00,0x00,0x00,0x00,0x2e,0x09,0x3c,0xff,0x30,0x00,
                            0x01,0xa9,0xf9,0x4f,0x67,0x67,0x53,0x00,0x00,0x00,0x7e,
                            0x79,0x6f,0x42,0x0c,0xc5,0x97,0x21,0x68,0x00,0x00,0x01,
                            0x00,0x00,0x00,0x6f,0x11,0x00,0x00,0x00,0x03,0x76,0x6f,
                            0x72,0x62,0x69,0x73,0x00,0x00,0x00,0x00,0x2e};
    size_t size = sizeof(data);

    int chan, samplerate;
    short *output;
    int samples = stb_vorbis_decode_memory(data, size, &chan, &samplerate, &output);
    if (samples >= 0)
        free(output);
    return 0;
}

2. Run the program with an instruction that allocator may fail (otherwise ASAN will quit early with AddressSanitizer: requested allocation size ... exceeds maximum supported size): ASAN_OPTIONS=allocator_may_return_null=1 <program name> to hit the error.

AddressSanitizer:DEADLYSIGNAL
=================================================================
==217447==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x00000041f6d4 bp 0x000000000000 sp 0x7ffd3e146c60 T0)
==217447==The signal is caused by a READ memory access.
    #0 0x41f6d4 in atomic_compare_exchange_strong<__sanitizer::atomic_uint8_t> /src/llvm-project/compiler-rt/lib/asan/../sanitizer_common/sanitizer_atomic_clang.h:81:10
    #1 0x41f6d4 in AtomicallySetQuarantineFlagIfAllocated /src/llvm-project/compiler-rt/lib/asan/asan_allocator.cpp:610:10
    #2 0x41f6d4 in __asan::Allocator::Deallocate(void*, unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType) /src/llvm-project/compiler-rt/lib/asan/asan_allocator.cpp:685:10
    #3 0x49e5d5 in free /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:53:3
    #4 0x4dcedb in setup_free(stb_vorbis*, void*) tests/../stb_vorbis.c:966:4
    #5 0x4dbe57 in vorbis_deinit(stb_vorbis*) tests/../stb_vorbis.c:4214:7
    #6 0x4f9638 in stb_vorbis_open_memory tests/../stb_vorbis.c:5122:4
    #7 0x4fbfb1 in stb_vorbis_decode_memory tests/../stb_vorbis.c:5390:20

[sezero]

This #1557 and next #1558 must be combined into the following, and #1558 must be discarded.

diff --git a/stb_vorbis.c b/stb_vorbis.c
index 3e5c250..3dbfb55 100644
--- a/stb_vorbis.c
+++ b/stb_vorbis.c
@@ -3662,7 +3662,11 @@ static int start_decoder(vorb *f)
    if (f->comment_list_length > 0)
    {
       f->comment_list = (char**) setup_malloc(f, sizeof(char*) * (f->comment_list_length));
-      if (f->comment_list == NULL)                  return error(f, VORBIS_outofmem);
+      if (f->comment_list == NULL) {
+         f->comment_list_length = 0;
+         return error(f, VORBIS_outofmem);
+      }
+      memset(f->comment_list, 0, sizeof(char*) * f->comment_list_length);
    }
 
    for(i=0; i < f->comment_list_length; ++i) {