Skip to content

Fix integer overflow in ImagingAllocateArray and OOB read in ImagingDestroyArray - #10395

Open
Mengxi-Xu wants to merge 3 commits into
python-pillow:mainfrom
Mengxi-Xu:main
Open

Mengxi-Xu wants to merge 3 commits into
python-pillow:mainfrom
Mengxi-Xu:main

Conversation

@Mengxi-Xu

Copy link
Copy Markdown
Contributor

Extended Description

ImagingAllocateArray() computes the block count in 32-bit signed arithmetic:

blocks_count = (im->ysize + lines_per_block - 1) / lines_per_block;

With a large arena block size (~2 GiB) and a specific image height, the addition overflows and wraps negative, so calloc() allocates (near) zero elements while the allocation loop still writes 16-byte ImagingMemoryBlock entries into it — a heap out-of-bounds write (confirmed by UBSan/ASan).

Independently, ImagingDestroyArray() terminates its cleanup loop on a NULL pointer instead of blocks_count, so whenever the count is wrong for any reason, it reads past the array and passes whatever it finds to free() (confirmed with valgrind against the official wheel).

This PR fixes both, independently:

  1. Compute the block count in 64 bits with an INT_MAX range check, failing cleanly with a memory error on overflow. No behavior change for any valid input.
  2. Bound the destroy loop by im->blocks_count, keeping the NULL early-exit, so a miscalculated count can never turn into an out-of-bounds read followed by free().

Both are defensive robustness fixes.Both defects were found while fuzzing and are described in the
security advisory (GHSA-xq7j-jqgf-54xq):
https://github.com/python-pillow/Pillow/security/advisories/GHSA-xq7j-jqgf-54xq

## Extended Description

`ImagingAllocateArray()` computes the block count in 32-bit signed
arithmetic:

    blocks_count = (im->ysize + lines_per_block - 1) / lines_per_block;

With a large arena block size (~2 GiB) and a specific image height, the
addition overflows and wraps negative, so `calloc()` allocates (near)
zero elements while the allocation loop still writes 16-byte
`ImagingMemoryBlock` entries into it — a heap out-of-bounds write
(confirmed by UBSan/ASan).

Independently, `ImagingDestroyArray()` terminates its cleanup loop on a
NULL pointer instead of `blocks_count`, so whenever the count is wrong
for any reason, it reads past the array and passes whatever it finds to
`free()` (confirmed with valgrind against the official wheel).

This PR fixes both, independently:

1. Compute the block count in 64 bits with an `INT_MAX` range check,
   failing cleanly with a memory error on overflow. No behavior change
   for any valid input.
2. Bound the destroy loop by `im->blocks_count`, keeping the NULL
   early-exit, so a miscalculated count can never turn into an
   out-of-bounds read followed by `free()`.

Both are defensive robustness fixes; triggering requires the host to
raise the block size via `set_block_size()` or `PILLOW_BLOCK_SIZE`,
which is not attacker-controllable.
Comment thread src/libImaging/Storage.c
#include "Imaging.h"
#include <string.h>
#include <limits.h>
#include <stdint.h>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these headers added?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INT_MAX (used in the new range check) comes from <limits.h>, and int64_t from <stdint.h>. I added both explicitly rather than relying on them being pulled in indirectly — but happy to drop either one if they're already guaranteed elsewhere.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants