From 23019269764e35ed8458e517f1897bd3c54820eb Mon Sep 17 00:00:00 2001 From: Oblivionsage Date: Sun, 15 Mar 2026 10:35:29 +0100 Subject: [PATCH] fix: Resolve use-after-free on `png_ptr->trans_alpha` The function `png_set_tRNS` sets `png_ptr->trans_alpha` to point at `info_ptr->trans_alpha` directly, so both structs share the same heap buffer. If the application calls `png_free_data(PNG_FREE_TRNS)`, or if `png_set_tRNS` is called a second time, the buffer is freed through `info_ptr` while `png_ptr` still holds a dangling reference. Any subsequent row read that hits the function `png_do_expand_palette` will dereference freed memory. The fix gives `png_struct` its own allocation instead of aliasing the `info_ptr` pointer. This was already flagged with a TODO in `png_handle_tRNS` ("horrible side effect ... Fix this.") but it was never addressed. Verified with AddressSanitizer. All 34 existing tests pass without regressions. CVE: CVE-2026-33416 Upstream-Status: Backport [https://github.com/pnggroup/libpng/commit/23019269764e35ed8458e517f1897bd3c54820eb] Comment: Refreshed hunk to match latest scarthgap Reviewed-by: Cosmin Truta Signed-off-by: Cosmin Truta Signed-off-by: Sourav Kumar Pramanik Signed-off-by: Zahir Hussain --- pngread.c | 11 +++++------ pngrutil.c | 4 ---- pngset.c | 31 +++++++++++++++++++------------ pngwrite.c | 6 ++++++ 4 files changed, 30 insertions(+), 22 deletions(-) diff --git a/pngread.c b/pngread.c index 01b731d8eb..0086edf6cf 100644 --- a/pngread.c +++ b/pngread.c @@ -968,12 +968,11 @@ png_read_destroy(png_structrp png_ptr) #if defined(PNG_tRNS_SUPPORTED) || \ defined(PNG_READ_EXPAND_SUPPORTED) || defined(PNG_READ_BACKGROUND_SUPPORTED) - if ((png_ptr->free_me & PNG_FREE_TRNS) != 0) - { - png_free(png_ptr, png_ptr->trans_alpha); - png_ptr->trans_alpha = NULL; - } - png_ptr->free_me &= ~PNG_FREE_TRNS; + /* png_ptr->trans_alpha is always independently allocated (not aliased + * with info_ptr->trans_alpha), so free it unconditionally. + */ + png_free(png_ptr, png_ptr->trans_alpha); + png_ptr->trans_alpha = NULL; #endif inflateEnd(&png_ptr->zstream); diff --git a/pngrutil.c b/pngrutil.c index 366379b991..a19507bf1b 100644 --- a/pngrutil.c +++ b/pngrutil.c @@ -1905,10 +1905,6 @@ png_handle_tRNS(png_structrp png_ptr, pn return; } - /* TODO: this is a horrible side effect in the palette case because the - * png_struct ends up with a pointer to the tRNS buffer owned by the - * png_info. Fix this. - */ png_set_tRNS(png_ptr, info_ptr, readbuf, png_ptr->num_trans, &(png_ptr->trans_color)); } diff --git a/pngset.c b/pngset.c index 4b78b8960c..47883684e4 100644 --- a/pngset.c +++ b/pngset.c @@ -990,28 +990,36 @@ png_set_tRNS(png_structrp png_ptr, png_i if (trans_alpha != NULL) { - /* It may not actually be necessary to set png_ptr->trans_alpha here; - * we do it for backward compatibility with the way the png_handle_tRNS - * function used to do the allocation. - * - * 1.6.0: The above statement is incorrect; png_handle_tRNS effectively - * relies on png_set_tRNS storing the information in png_struct - * (otherwise it won't be there for the code in pngrtran.c). - */ - png_free_data(png_ptr, info_ptr, PNG_FREE_TRNS, 0); if (num_trans > 0 && num_trans <= PNG_MAX_PALETTE_LENGTH) { - /* Changed from num_trans to PNG_MAX_PALETTE_LENGTH in version 1.2.1 */ + /* Allocate info_ptr's copy of the transparency data. */ info_ptr->trans_alpha = png_voidcast(png_bytep, png_malloc(png_ptr, PNG_MAX_PALETTE_LENGTH)); memcpy(info_ptr->trans_alpha, trans_alpha, (size_t)num_trans); - info_ptr->free_me |= PNG_FREE_TRNS; info_ptr->valid |= PNG_INFO_tRNS; + + + /* Allocate an independent copy for png_struct, so that the + * lifetime of png_ptr->trans_alpha is decoupled from the + * lifetime of info_ptr->trans_alpha. Previously these two + * pointers were aliased, which caused a use-after-free if + * png_free_data freed info_ptr->trans_alpha while + * png_ptr->trans_alpha was still in use by the row transform + * functions (e.g. png_do_expand_palette). + */ + png_free(png_ptr, png_ptr->trans_alpha); + png_ptr->trans_alpha = png_voidcast(png_bytep, + png_malloc(png_ptr, PNG_MAX_PALETTE_LENGTH)); + memcpy(png_ptr->trans_alpha, trans_alpha, (size_t)num_trans); + } + else + { + png_free(png_ptr, png_ptr->trans_alpha); + png_ptr->trans_alpha = NULL; } - png_ptr->trans_alpha = info_ptr->trans_alpha; } if (trans_color != NULL) diff --git a/pngwrite.c b/pngwrite.c index 5fc77d91f7..84af1e73fb 100644 --- a/pngwrite.c +++ b/pngwrite.c @@ -977,6 +977,12 @@ png_write_destroy(png_structrp png_ptr) png_ptr->chunk_list = NULL; #endif +#if defined(PNG_tRNS_SUPPORTED) + /* Free the independent copy of trans_alpha owned by png_struct. */ + png_free(png_ptr, png_ptr->trans_alpha); + png_ptr->trans_alpha = NULL; +#endif + /* The error handling and memory handling information is left intact at this * point: the jmp_buf may still have to be freed. See png_destroy_png_struct * for how this happens.