nothings / stb

stb single-file public domain libraries for C/C++
https://twitter.com/nothings
Other
26.22k stars 7.68k forks source link

stb_vorbis needs STB_VORBIS_MAX_PACKET_SIZE option with sane default #937

Open ghost opened 4 years ago

ghost commented 4 years ago

stb_vorbis needs a STB_VORBIS_MAX_PACKET_SIZE option with sane default, like this:

#ifndef STB_VORBIS_MAX_PACKET_SIZE
#define STB_VORBIS_MAX_PACKET_SIZE (1024 * 1024 * 10)
#endif

Because while an ideal world according to specs, any packet size goes, in practice this means stb_vorbis currently will:

  1. happily try to allocate up to 2GiB of memory even for tiny ogg files if they're faulty or malicious,
  2. crash and likely corrupt its own memory (=could allow code execution) if a broken or malicious ogg file specifies a packet size between 2GiB and 4GiB, since the following function casts uint32 to sint32 with no overflow check:

    static int get32_packet(vorb *f)
    {
      uint32 x;
      x = get8_packet(f);
      x += get8_packet(f) << 8;
      x += get8_packet(f) << 16;
      x += (uint32) get8_packet(f) << 24;
      return x;
    }

    The specs also recommend any packets to remain in 2-4KiB. While a max-size of considerably more might still be necessary for metadata like images (which is why I suggested a 10MiB default) it will otherwise likely never be used in any valid file. And a user who wants to be sure even files with giant ogg packet sizes will still work no matter the memory consumption, can then still use #define STB_VORBIS_MAX_PACKET_SIZE (1024 * 1024 * 1024), or -DSTB_VORBIS_MAX_PACKET_SIZE=1073741824, or similar.

I would also recommend that test files are added with faulty large packets to ensure the limits actually work fine without a complete crash.

This issue can also amplify other bugs, e.g. I ran into this because stb_vorbis failed to parse a likely perfectly valid file's header, ended up in wrong data, picked up a faulty >2GiB packet size in the wrong spot, and then self-destructed bringing down my program. It would be highly preferable if that use case just returned a parse failure and allowed me to resume operation. (I can email this file to someone interested, due to copyright reasons I don't think I am allowed to attach it here.) Anyway, that is kind of a separate bug and the main issue IMHO really is the failure to enforce any sane packet sizes.

rygorous commented 3 years ago

As far as I can tell this is really just relevant for these handful comment/vendor fields in the Vorbis header; should just fix get32_packet to return a uint32 and then add a configurable threshold on the amount of metadata kept by stb_vorbis. By itself this doesn't need to be treated as an error either, if it's above the threshold we can just try to skip over the metadata and not keep it in the stb_vorbis object.

The main reason to fix this soon-ish is that the current behavior with its integer overflow is definitely exploitable.