Fixes null pointer dereference in https://github.com/nothings/stb/issues/1452

[NBickford-NV]

Hi stb maintainers!

I just saw issue #1452, and put together this pull request to fix it. When stbi__pic_load_core returns NULL, this code now frees the allocated image and returns 0 immediately, instead of passing a null pointer to stbi__convert_format().

Thanks!

[musicinmybrain]

Thank you! I have just applied this as a downstream patch in Fedora Linux.

[NBickford-NV]

That's amazingly fast, thank you Ben! (I figured I'd send you an email later this morning since I remember you saw and applied the earlier stb_image patches, but you beat me to it!)

[NBickford-NV]

Tracking CVE numbers: this is a patch for https://nvd.nist.gov/vuln/detail/CVE-2023-43898 ((#1521).

[NBickford-NV]

Looks like this is also a patch for https://nvd.nist.gov/vuln/detail/CVE-2021-45340, which is how this bug appeared in libsixel (see libsixel/libsixel#73 and #1736).

[musicinmybrain]

@NBickford-NV You’ve studied this code more deeply than I am likely to have time to. Do you think #1736 is a wise change in addition to this PR? Is it easy enough to prove that there is nowhere else that stbi__convert_format could end up being called with a null data pointer?

[NBickford-NV]

@musicinmybrain Turns out, that's a really good question! I tried proving this and failed at stbi__load_gif_main. #1736 also protects against that. So while #1454 fixes one bug, #1736 is a more general solution.

My attempted proof that this is the only place where stbi__convert_format could be called with a null data pointer is below. This fails in case 6.

• stbi__convert_format is called in 8 places:
  1. In stbi__do_png, can (unsigned char *)result be NULL? If it's NULL, then p->out is NULL. That means z->out in stbi__parse_png() was NULL and stbi__parse_png() returned a non-zero value. There's a few things that could write to z->out:
     a. stbi__create_png_image; if it left a->out NULL, then it failed (so stbi__parse_png() returns 0) -- so this can't happen.
     b. stbi__expand_png_palette; this only sets a->out if it fails (so stbi__parse_png() returns 0) -- so this can't happen.
     c. So result can't be NULL.
  2. In stbi__bmp_load, can out be NULL? Control only reaches stbi__convert_format there if it doesn't exit at if (!out) return stbi__errpuc("outofmem", "Out of memory");, so this can't happen.
  3. In stbi__tga_load, tga_data can't be null due to a null check after allocation, like in stbi__bmp_load.
  4. In stbi__psd_load, out can't be null due to a null check after allocation, like in stbi__bmp_load.
  5. In stbi__pic_load, it used to be that result could be 0 if stbi__pic_load_core failed; this PR patches that. Since stbi__pic_load_core doesn't modify the result pointer itself (although it isn't passed in as an stbi_uc* const), result can only be null if it was null before stbi__pic_load_core. That can't happen because of the null check after the call to stbi__malloc_mad3.
  6. In stbi__load_gif_main, can out be null? This might be possible; we have to pass stbi__gif_test, but then have stbi__gif_load_next return either 0 (error) or s (end of animated gif). That's probably possible to do; make a file that starts with GIF8[79]a and then has a corrupt first frame.
  7. In stbi__gif_load, u can't be null because control's guarded by an if(u) check.
  8. In stbi__pnm_load, out can't be null due to a null check after allocation, like in stbi__bmp_load.

[NBickford-NV]

Okay, after deeper analysis, I think case 6 might be OK after all, but it's complex.

stbi__convert_format only crashes if out is null and both the width and height are greater than 0; if at least one is 0, it never reads from the source pointer. In stbi__load_gif_main, the width is layers * g.w, but layers is only nonzero if u is non-null at least once. And if u is non-null at least once, then control reaches out = (stbi_uc*)stbi__malloc( layers * stride ); and then the if (!out) check.

That is: if out is null, then layers must be 0, and so there's no crash. If layers were nonzero, then out would be a non-null pointer.

I had a go at reproducing this, and if you run the following code you'll notice that execution enters stbi__convert_format with a null pointer, but then doesn't crash because layers is 0. It leaves x and y uninitialized, though, and the only clue the image pointer isn't valid is that z is 0.

So, because that proof's nontrivial, it's probably a good idea to have #1736 as well!

#include "stb_image.h"

#include <stdlib.h>

int test_1454_gif()
{
	const stbi_uc data[] = { 'G', 'I', 'F', '8', '9', 'a', // Magic number
		1, 0, 1, 0, // Width and height
		0, 0, 0}; // flags, bgindex, ratio
	int size = (int)sizeof(data);

	int *delays = NULL;
	int x, y, z, comp;
	stbi_uc* image = stbi_load_gif_from_memory(
		(const stbi_uc*)data, size,
		&delays,
		&x, &y, &z,
		&comp, 3);

	if (image)
	{
		stbi_image_free(image);
	}

	if (delays)
	{
		stbi_image_free(delays);
	}
	
	return EXIT_SUCCESS;
}

[musicinmybrain]

Thank you for that very detailed analysis! I’m shipping #1736 as an additional downstream patch in Fedora’s stb package and referencing this discussion.

[hzeller]

Given the analysis, I wonder how we get @nothings attention to get both this #1454 and #1736 merged to fix a whole class of bugs.

They are also sufficiently trivial that the review is simple.

[nothings]

Given the analysis, I wonder how we get @nothings attention

I read every email I get, and I get an email for every comment in the repo. It's just a matter of me committing the time to a release cycle.