png_set_quantize: validate palette count parameters

[gamedeveloper8136]

Summary

png_set_quantize() accepts an application-supplied num_palette and maximum_colors and uses them as allocation sizes, loop bounds, array indices, and a memcpy length — but validates neither. The internal arrays this function populates are sized for the PNG palette maximum, so invalid counts can produce out-of-bounds reads/writes and oversized copies.

Problem

• num_palette must be in [1, PNG_MAX_PALETTE_LENGTH] (256); maximum_colors must be positive.
• These values reach quantize_index[...] (a 256-byte array), png_malloc sizes, loop bounds, and memcpy(png_ptr->palette, palette, (unsigned int)num_palette * sizeof(png_color)) into a fixed 768-byte owned buffer.
• num_palette = maximum_colors is assigned at the end of the color-reduction path, so a non-positive maximum_colors propagates into the copy size.
• An invalid count can therefore cause a heap out-of-bounds write/read or a multi-gigabyte memcpy.

Exploitability note: the trigger requires an application to call the public API with invalid arguments (for example a >256 app-computed palette count, a zero, or a negative/uninitialized value). It is not directly reachable from a crafted PNG file, because file-derived palette counts are already capped at 256 during PLTE parsing.

Fix

• Validate num_palette and maximum_colors at entry.
• Reject invalid calls with png_warning() + early return, before PNG_QUANTIZE is enabled, so no internal state is created for invalid input.
• Behavior for all valid calls is unchanged.

Testing

• New pnggetset regression test covering num_palette = 257, -1, 0, maximum_colors = -1, and a valid (4,4) call asserted to produce no warning.
• Full pngtest, pngvalid (standard + transform), pngunknown, pngimage, and pngcp suites pass; UBSan clean; no new compiler warnings.

Security context

The unsafe code paths became reachable through interactions between earlier security fixes (the quantize_index array was fixed at 256 bytes by the CVE-2025-64505 fix, and the owned palette copy was added by the CVE-2026-33416 use-after-free fix); those fixes were correct for the defects they addressed, but the resulting code was not re-audited against out-of-range counts. This change is a hardening/validation fix consistent with the existing per-setter validation conventions.

[jbowler]

gamedeveloper, onlybug; can you provide a repro?

[jbowler]

In the documentation for png_set_quantize (from libpng(3), i.e. man 3 libpng) Glenn wrote:

If you pass a palette that is larger than maximum_colors the file will reduce the number of colors in the palette so it will fit into maximum_colors.

In your test case (line 842 of pnggetset.c) you test the "oversized palette" case by passing in 257 to both parameters, but 257 for num_palette should be valid; it's only if maximum_colors is greater than 1<<output_bit_depth (not PNG_MAX_PALETTE_LENGTH I think) there are should be a problem. The code at line 5134 (if (num_palette > maximum_colors) is meant to handle this but without a valid test case we can't be sure.

So the test needs to check maximum_palette, not num_palette. I'm not sure how the output bit depth gets set; that controls how many colors are possible, it should come from maximum_colors.

The png_warning and probably the two preceding return statements needs to be png_app_error because the app is going to assume the palette mapping works and most likely will allocate row buffers to match. The app will crash (well, in fact, libpng will crash) if png_set_quantize is skipped.

The (int) casts on PNG_MAX__PALETTE_LENGTH are spurious and should be removed; it's already an (int) and, anyway, the arithmetic conversions would make the test fail correctly even if it were 256U (so was an (unsigned int); a negative (int) is converted into a larger unsigned value).

I didn't check whether the other test cases, the (-1) ones, cause crashes or errors but it seems reasonable to filter them out right at the start with an app error. I think both values are self evidently bogus.

[gamedeveloper8136]

Thanks for the detailed review — all points addressed in the latest commit (png_set_quantize: address review feedback).

• num_palette > PNG_MAX_PALETTE_LENGTH is now accepted. Per the documented behavior, a palette larger than maximum_colors is reduced to fit. The regression test now covers the valid reduction cases (num_palette=257, maximum_colors=256) for both the median-cut and histogram paths, plus the (256, 256) boundary. This exposed the underlying defect you pointed at: with a fixed 256-entry quantize_index, the reduction code indexes it with input palette entries, so (257, 256) accessed quantize_index[256..] out of bounds. quantize_index is now sized for max(num_palette, PNG_MAX_PALETTE_LENGTH), keeping the pixel loop's 0..255 reads in bounds while making the reduction loops safe.

• Validation is now on maximum_colors (must be positive and no more than PNG_MAX_PALETTE_LENGTH, since the quantized output is an 8-bit palette), plus num_palette must be positive.

• png_app_error is used instead of png_warning + return, so the application is not left assuming the palette mapping was set up. The invalid-count cases in the test assert that an error is raised.

• (int) casts removed — PNG_MAX_PALETTE_LENGTH is already an int.

• Non-positive counts (num_palette <= 0, maximum_colors <= 0) are filtered at the start with an application error.

Re-validated: pnggetset (incl. the reworked regression cases) PASS, full pngtest/pngvalid/pngunknown/pngimage/pngcp suite PASS, UBSan clean, 0 warnings with -Wall -Wextra -Wconversion. Guard-page verification: (257,256) and (300,256) complete without fault (previously SIGSEGV), and the invalid cases raise the app error without any out-of-bounds access.

[jbowler]

I can't do the line-by-line reviews that I used to be able to do; I assume I've been locked out by the WC^3. So this is going to be difficult:

      index_size = PNG_MAX_PALETTE_LENGTH;
      if (num_palette > PNG_MAX_PALETTE_LENGTH)
         index_size = (png_alloc_size_t)num_palette;

The code looks wrong, intuitively wrong to me. Maybe I just died and went to heathen, but, Shirley, those three lines should just be "index_size = num_palette"?

Ah, never mind; Wednesday. My power is out tomorrow. Whatever.

[gamedeveloper8136]

Good question — it's not index_size = num_palette, and I've confirmed that empirically.

quantize_index has two readers:

1. The pixel loop in png_do_quantize reads quantize_lookup[*sp] where *sp is an 8-bit palette index straight from the image data (0..255). For a malformed file that index can exceed num_palette. That is precisely CVE-2025-64505 — the array used to be sized num_palette, and an out-of-range index read past it. The fix (6a528eb) set it to PNG_MAX_PALETTE_LENGTH (256) so every 8-bit index is covered. So the array must be at least 256 elements regardless of num_palette.

2. The reduction code (the num_palette > maximum_colors branch, which is the documented "reduce a larger palette to fit" behavior) indexes quantize_index with input palette entries, i.e. up to num_palette - 1. So the array must be at least num_palette elements.

Hence size = max(num_palette, PNG_MAX_PALETTE_LENGTH).

I built both variants and fed them a 1x1 palette PNG with PLTE=2 entries and a pixel index of 250, under a guard-page allocator:

• index_size = num_palette (3): pixel loop runs, reads quantize_index[250] → SIGSEGV.
• max(num_palette, 256): same input completes cleanly; (num_palette=257, maximum_colors=256) reduction also completes.

I've also pushed a commit that documents this in the comment next to the sizing, so the "why not just num_palette" is self-explanatory in the code.

[jbowler]

Good question — it's not index_size = num_palette, and I've confirmed that empirically.

That's a very good explanation. It doesn't fix the bug. I read your explanation and treated it at face value (sorry, I'm getting to used to thinking like everyone is an AI). What you say is correct (at face value) but the issue is that is not what the API documentation says and the bug cannot be in the API because that invalidates existing users of the documented API.

The fix is not to change the API but, either, to document the API correctly (call an API with undocumented parameters and you own the bug) or to implement the API like it suggests.

Bear in mind I'm just a dude here; the high and mighty are on vacation. Regardless of how much of the code I wrote everyone of us needs to put our own oxygen mask on first: I won't comment any more. I believe you are competent so you need to work out how to fix it.

[gamedeveloper8136]

Thanks @jbowler. Addressed by strictly enforcing the API bounds:

1. Both num_palette and maximum_colors are now required to be in [1, PNG_MAX_PALETTE_LENGTH] (256). Any call with num_palette > 256, num_palette <= 0, maximum_colors > 256, or maximum_colors <= 0 raises an application error (png_app_error) and returns early without enabling quantize.
2. quantize_index is reverted to a fixed allocation of PNG_MAX_PALETTE_LENGTH (256 bytes) covering all 8-bit palette indices (0..255).
3. Updated pnggetset.c regression tests to assert an application error for num_palette > 256 and verify valid palette reduction when num_palette = 256, maximum_colors = 128.

[onlybugs05]

@jbowler he is the core maintainer Not me

…

On Fri, Aug 28, 2026 at 10:39 PM gamedeveloper8136 ***@***.***> wrote: @gamedeveloper8136 <https://github.com/gamedeveloper8136> requested your review on: pnggroup/libpng#907 <#907> png_set_quantize: validate palette count parameters. — Reply to this email directly, view it on GitHub <#907?email_source=notifications&email_token=B5NFMBYF6LW7HDMGD4VDHIT5MG4GHA5CNFSNUABQM5UWIORPF5TWS5BNNB2WEL2JONZXKZKFOZSW45CON52GSZTJMNQXI2LPNYXTGMBRG43TSMJRGE4TNJTSMVQXG33OWBZGK5TJMV3V64TFOF2WK43UMVSKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#event-30177911196>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/B5NFMBZPHF55UPYQSXV3QHD5MG4GHAVCNFSNUABDKJSXA33TNF2G64TZHMZTONRYGAZDKO2JONZXKZJ3GUYTEMRUGUZDCMRWUF3AE> . You are receiving this because your review was requested.Message ID: ***@***.***>