stb_vorbis CVE fixes #706

sezero · 2023-12-11T07:06:45Z

The following 4 patches are based on PR submissions at mainstream.
I modified most of the patches and notified the patch author about
them.

(1) Fix CVE-2023-45679 and CVE-2023-45680:
Based on the patches by Jaroslav Lobačevski (@JarLob) submitted
to mainstream at: nothings/stb#1557 and nothings/stb#1558
GHSL-2023-169/CVE-2023-45679: Attempt to free an uninitialized memory pointer in vorbis_deinit()
GHSL-2023-170/CVE-2023-45680: Null pointer dereference in vorbis_deinit()

(2) Fix CVE-2023-45681 (integer overflow):
Based on patch by Jaroslav Lobačevski (@JarLob) submitted to
mainstream at nothings/stb#1559
GHSL-2023-171/CVE-2023-45681: Out of bounds heap buffer write

(3) Fix CVE-2023-45676 and CVE-2023-45677 (integer overflow in setup_malloc()):
Based on the patches by Jaroslav Lobačevski (@JarLob) submitted
to mainstream at: nothings/stb#1554 and nothings/stb#1555
GHSL-2023-166/CVE-2023-45676: Multi-byte write heap buffer overflow in start_decoder()
GHSL-2023-167/CVE-2023-45677: Heap buffer out of bounds write in start_decoder()

(4) Fix CVE-2023-45682:
Based on patch by Jaroslav Lobačevski (@JarLob) submitted to
mainstream at nothings/stb#1560
GHSL-2023-172/CVE-2023-45682: Wild address read in vorbis_decode_packet_rest()

(5) apply CVE-2023-45676/CVE-2023-45677 fix to setup_temp_malloc(),
and fix make_block_array() for it along the way.

(The first two patches don't actually affect us because we disable the comments
support, but I included them here for completeness..)

@AliceLR: Please review.

@JarLob

Based on the patches by Jaroslav Lobačevski (@JarLob) submitted to mainstream at: nothings/stb#1557 and nothings/stb#1558 GHSL-2023-169/CVE-2023-45679: Attempt to free an uninitialized memory pointer in vorbis_deinit() GHSL-2023-170/CVE-2023-45680: Null pointer dereference in vorbis_deinit()

@JarLob

Based on patch by Jaroslav Lobačevski (@JarLob) submitted to mainstream at nothings/stb#1559 GHSL-2023-171/CVE-2023-45681: Out of bounds heap buffer write

@JarLob

Based on the patches by Jaroslav Lobačevski (@JarLob) submitted to mainstream at: nothings/stb#1554 and nothings/stb#1555 GHSL-2023-166/CVE-2023-45676: Multi-byte write heap buffer overflow in start_decoder() GHSL-2023-167/CVE-2023-45677: Heap buffer out of bounds write in start_decoder()

@JarLob

Based on patch by Jaroslav Lobačevski (@JarLob) submitted to mainstream at nothings/stb#1560 GHSL-2023-172/CVE-2023-45682: Wild address read in vorbis_decode_packet_rest()

... and fix make_block_array for it along the way.

codecov · 2023-12-11T07:08:31Z

Codecov Report

Merging #706 (8dbdf93) into master (cc62388) will increase coverage by 0%.
Report is 25 commits behind head on master.
The diff coverage is 93%.

Additional details and impacted files

@@          Coverage Diff           @@
##           master    #706   +/-   ##
======================================
  Coverage      82%     82%           
======================================
  Files         171     171           
  Lines       31229   31271   +42     
======================================
+ Hits        25751   25793   +42     
  Misses       5478    5478

Files	Coverage Δ
src/common.h	`100% <ø> (ø)`
src/depackers/mmcmp.c	`74% <100%> (ø)`
src/loaders/amf_load.c	`70% <100%> (+4%)`	⬆️
src/loaders/iff.c	`87% <100%> (ø)`
src/loaders/mtm_load.c	`86% <100%> (ø)`
src/loaders/stm_load.c	`85% <100%> (+<1%)`	⬆️
src/loaders/vorbis.c	`72% <100%> (ø)`
src/loaders/xm_load.c	`85% <100%> (ø)`
src/player.c	`93% <100%> (-<1%)`	⬇️
src/scan.c	`97% <100%> (+<1%)`	⬆️
... and 4 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48eeb01...8dbdf93. Read the comment docs.

sezero · 2023-12-12T12:50:09Z

BTW, I forked stb and created a branch for stb_vorbis updates here:
https://github.com/sezero/stb/tree/stb_vorbis-sezero

I think I applied everything that's already applied in SDL_mixer,
SDL_sound, and libxmp (excluding changes specific to those libs) in
a one-thing-at-a-time manner. If there's something missing, either
in there or in libxmp, drop a note..

AliceLR · 2023-12-13T23:17:57Z

src/loaders/vorbis.c

-  // libxmp hack: decode work buffer (used in inverse_mdct and decode_residues)
+  // hack: decode work buffer (used in inverse_mdct and decode_residues)


If these patches have been merged upstream, the "libxmp hack" comments can be removed entirely. They aren't part of my upstream patches, they're just documentation I added here to make it clear that they haven't been merged yet. I don't think obfuscating this intent further is a good idea though.

AliceLR · 2023-12-13T23:19:47Z

src/loaders/vorbis.c

-      f->comment_list = (char**) setup_malloc(f, sizeof(char*) * (f->comment_list_length));
-      if (f->comment_list == NULL)                  return error(f, VORBIS_outofmem);
+      if (INT_MAX / sizeof(char*) < f->comment_list_length)
+          goto no_comment;


This is a pointless spaghetti usage of goto that saves one entire line of code...

AliceLR · 2023-12-13T23:22:34Z

src/loaders/vorbis.c

@@ -4225,7 +4235,6 @@ static int start_decoder(vorb *f)
      int i,max_part_read=0;
      for (i=0; i < f->residue_count; ++i) {
         Residue *r = f->residue_config + i;
-         /* libxmp hack: https://github.com/nothings/stb/pull/1487 */


Yeah, no, the divergences from upstream should remain documented.

AliceLR · 2023-12-13T23:22:45Z

src/loaders/vorbis.c

-// libxmp hack: replaced signed overflow clamps with unsigned overflow (UBSan).
-// Reported upstream: https://github.com/nothings/stb/issues/1168.
-


AliceLR · 2023-12-13T23:24:33Z

I didn't spot any problems with the CVE patches aside from that tacky goto. I don't really like that you're removing documentation containing links to open patches to upstream, though.

AliceLR · 2023-12-13T23:39:30Z

I think I applied everything that's already applied in SDL_mixer, SDL_sound, and libxmp (excluding changes specific to those libs) in a one-thing-at-a-time manner. If there's something missing, either in there or in libxmp, drop a note..

It looks like you got everything...

edit: I see you've got all the patch URLs in your fork commits—if we switch to using that as an upstream then getting rid of all the "libxmp hack" comments might actually be fine, since the burden of merging with stb would be on your fork.

sezero · 2023-12-14T03:53:53Z

@AliceLR:

I didn't spot any problems with the CVE patches aside from that tacky goto. I don't really like that you're removing documentation containing links to open patches to upstream, though.

edit: I see you've got all the patch URLs in your fork commits—if we switch to using that as an upstream then getting rid of all the "libxmp hack" comments might actually be fine, since the burden of merging with stb would be on your fork.

That was the idea yes: the mainstream seems is really slow or uninterested in patches, and that's why I removed the extra comments.

Feel free to push to my branch to revise

AliceLR

That was the idea yes: the mainstream seems is really slow or uninterested in patches, and that's why I removed the extra comments.

Feel free to push to my branch to revise

Okay, I think it's fine for now as long as they're documented somewhere.

sezero added 5 commits December 11, 2023 05:50

stb_vorbis: fix CVE-2023-45681 (integer overflow.)

421d6d1

Based on patch by Jaroslav Lobačevski (@JarLob) submitted to mainstream at nothings/stb#1559 GHSL-2023-171/CVE-2023-45681: Out of bounds heap buffer write

stb_vorbis: fix CVE-2023-45682

0223cb0

Based on patch by Jaroslav Lobačevski (@JarLob) submitted to mainstream at nothings/stb#1560 GHSL-2023-172/CVE-2023-45682: Wild address read in vorbis_decode_packet_rest()

stb_vorbis: apply CVE-2023-45676/CVE-2023-45677 fix to setup_temp_malloc

c7a5374

... and fix make_block_array for it along the way.

shortened several libxmp patch notes.

8dbdf93

sezero force-pushed the stbvorbis_cve branch from 067ce46 to ec62bbf Compare December 11, 2023 09:10

sezero requested a review from AliceLR December 11, 2023 09:20

sezero force-pushed the stbvorbis_cve branch 2 times, most recently from 13a9fca to 8dbdf93 Compare December 12, 2023 12:40

AliceLR reviewed Dec 13, 2023

View reviewed changes

AliceLR requested changes Dec 13, 2023

View reviewed changes

AliceLR approved these changes Dec 14, 2023

View reviewed changes

AliceLR merged commit c9c6dc9 into libxmp:master Dec 14, 2023
16 checks passed

sezero deleted the stbvorbis_cve branch December 14, 2023 06:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stb_vorbis CVE fixes #706

stb_vorbis CVE fixes #706

sezero commented Dec 11, 2023

codecov bot commented Dec 11, 2023 •

edited

sezero commented Dec 12, 2023 •

edited

AliceLR Dec 13, 2023 •

edited

AliceLR Dec 13, 2023

AliceLR Dec 13, 2023

AliceLR Dec 13, 2023

AliceLR commented Dec 13, 2023 •

edited

AliceLR commented Dec 13, 2023 •

edited

sezero commented Dec 14, 2023 •

edited

AliceLR left a comment

		// libxmp hack: decode work buffer (used in inverse_mdct and decode_residues)
		// hack: decode work buffer (used in inverse_mdct and decode_residues)

		// libxmp hack: replaced signed overflow clamps with unsigned overflow (UBSan).
		// Reported upstream: https://github.com/nothings/stb/issues/1168.

stb_vorbis CVE fixes #706

stb_vorbis CVE fixes #706

Conversation

sezero commented Dec 11, 2023

codecov bot commented Dec 11, 2023 • edited

Codecov Report

sezero commented Dec 12, 2023 • edited

AliceLR Dec 13, 2023 • edited

Choose a reason for hiding this comment

AliceLR Dec 13, 2023

Choose a reason for hiding this comment

AliceLR Dec 13, 2023

Choose a reason for hiding this comment

AliceLR Dec 13, 2023

Choose a reason for hiding this comment

AliceLR commented Dec 13, 2023 • edited

AliceLR commented Dec 13, 2023 • edited

sezero commented Dec 14, 2023 • edited

AliceLR left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 11, 2023 •

edited

sezero commented Dec 12, 2023 •

edited

AliceLR Dec 13, 2023 •

edited

AliceLR commented Dec 13, 2023 •

edited

AliceLR commented Dec 13, 2023 •

edited

sezero commented Dec 14, 2023 •

edited