stb_vorbis crash when calling invalid file

[Youlean]

These lines could overflow when an invalid file is being used: https://github.com/nothings/stb/blob/5736b15f7ea0ffb08dd38af21067c314d6a3aae9/stb_vorbis.c#L1100 https://github.com/nothings/stb/blob/5736b15f7ea0ffb08dd38af21067c314d6a3aae9/stb_vorbis.c#L1116

Possible fix:

static int compute_codewords(Codebook *c, uint8 *len, int n, uint32 *values)
{
   int i,k,m=0;
   uint32 available[32];

   memset(available, 0, sizeof(available));
   // find the first entry
   for (k=0; k < n; ++k) if (len[k] < NO_CODE) break;
   if (k == n) { assert(c->sorted_entries == 0); return TRUE; }
   // add to the list
   add_entry(c, 0, k, m++, len[k], values);
   // add all available leaves
   for (i = 1; i <= len[k]; ++i) {

     if (i == 32) { return FALSE; }

     available[i] = 1U << (32 - i);
     // note that the above code treats the first case specially,
     // but it's really the same as the following code, so they
     // could probably be combined (except the initial code is 0,
     // and I use 0 in available[] to mean 'empty')
   }

   for (i=k+1; i < n; ++i) {
      uint32 res;
      int z = len[i], y;
      if (z == NO_CODE) continue;
      // find lowest available leaf (should always be earliest,
      // which is what the specification calls for)
      // note that this property, and the fact we can never have
      // more than one free leaf at a given level, isn't totally
      // trivial to prove, but it seems true and the assert never
      // fires, so!
      while (z > 0 && z < 32 && !available[z]) --z;
      if (z == 0 || z >= 32) { return FALSE; }
      res = available[z];
      assert(z >= 0 && z < 32);
      available[z] = 0;
      add_entry(c, bit_reverse(res), i, m++, len[i], values);
      // propogate availability up the tree
      if (z != len[i]) {
         assert(len[i] >= 0 && len[i] < 32);
         for (y=len[i]; y > z; --y) {
            assert(available[y] == 0);
            available[y] = res + (1 << (32-y));
         }
      }
   }

[Youlean]

Another bug: https://github.com/nothings/stb/blob/5736b15f7ea0ffb08dd38af21067c314d6a3aae9/stb_vorbis.c#L3908

c->multiplicands could be null

Possible fix:

         {
            float last=0;
            CHECK(f);

            int multiplicands_size = 0;
            if (c->multiplicands != NULL) multiplicands_size = sizeof(c->multiplicands[0]) * c->lookup_values;

            c->multiplicands = (codetype *) setup_malloc(f, multiplicands_size);
            if (c->multiplicands == NULL) { setup_temp_free(f, mults,sizeof(mults[0])*c->lookup_values); return error(f, VORBIS_outofmem); }
            for (j=0; j < (int) c->lookup_values; ++j) {
               float val = mults[j] * c->delta_value + c->minimum_value + last;
               c->multiplicands[j] = val;
               if (c->sequence_p)
                  last = val;
            }
         }

[nothings]

I don't see what the failure cases in any of these are.

1. https://github.com/nothings/stb/blob/5736b15f7ea0ffb08dd38af21067c314d6a3aae9/stb_vorbis.c#L1095
2. https://github.com/nothings/stb/blob/5736b15f7ea0ffb08dd38af21067c314d6a3aae9/stb_vorbis.c#L1095
3. sizeof() doesn't depend on runtime value

[Youlean]

Here i could be 32 available[i] = 1U << (32 - i);

[Youlean]

Also z could be 32 while (z > 0 && !available[z]) --z;

[nothings]

Can it, though? I quoted this line at you. Why do you think I did that?

assert(len[k] < 32); // no error return required, code reading lens checks this

[Youlean]

I have test files that can reproduce the issue. It is causing a buffer overflow in the latest msvc compiler.

EDIT: found your email. Will send it right now.

[N-R-K]

c->multiplicands could be null

Doesn't make a difference. sizeof is an operator that yields a result at compile time based on the type of the expression of the operand.

Maybe it won't be good to post it publicly as this could be a security vulnerability.

From what I've seen, stb library doesn't really deal with these type of things behind closed doors. Test cases are posted on the bug report publicly - which also makes it so others can also fix the issue and send a patch.

[Youlean]

You are right about the multiplicands. The problem is that sz in static void setup_malloc(vorb f, int sz) could sometimes be negative. That could happen if int sz overlows

[nothings]

None of the three test files ever triggered the quoted assert of len[k] < 32 in a debug build, or an equivalent test in a release build in my old MSVC. If they do in yours, you're going to have to root cause why len[k] is getting through there invalid, not rewrite the code to deal with an invalid len[k].

If you think the setup_malloc sz thing is still a valid bug, please open that in a new bug report so there's no confusion about which things are being reported for which bugs.

[Youlean]

Ah, sorry it seems like I was using a bit older version 1.14.This file should crash the latest version 1.22 in static void setup_free(vorb f, void p) I could set up the visual studio 2022 project with libFuzzer if you want to check it out.  On Wednesday, June 21, 2023 at 03:06:36 AM GMT+2, Sean Barrett @.***> wrote:

None of the three test files ever triggered the quoted assert of len[k] < 32 in a debug build, or an equivalent test in a release build.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[Youlean]

Forgot to mention, you might need the address sanitizer enabled if it doesn't.  On Wednesday, June 21, 2023 at 12:22:36 PM GMT+2, Julijan Nikolic @.***> wrote:

Ah, sorry it seems like I was using a bit older version 1.14.This file should crash the latest version 1.22 in static void setup_free(vorb f, void p) I could set up the visual studio 2022 project with libFuzzer if you want to check it out.  On Wednesday, June 21, 2023 at 03:06:36 AM GMT+2, Sean Barrett @.***> wrote:

None of the three test files ever triggered the quoted assert of len[k] < 32 in a debug build, or an equivalent test in a release build.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>