fix: four critical security/DoS bugs (unsigned overflow, LZW stack overflow, WebP parser)

CHANGELOG.md

@@ -16,6 +16,19 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Fixed
 
+- Security: `readUint32LE` in `src/utils/byte_utils.ts` now returns a proper unsigned 32-bit value
+  (`>>> 0`) — prevents signed-integer overflow in BMP, ICO, and TIFF decoders that could allow
+  crafted files to bypass bounds checks
+- Security: `readUint32LE` in `src/utils/webp_decoder.ts` removed in favour of the shared
+  `readUint32LE` from `src/utils/byte_utils.ts` (which includes the `>>> 0` fix) — prevents a large
+  WebP chunk size from wrapping to a negative value and silently short-circuiting the parse loop
+- Security: `readUint32` (big-endian) in `src/formats/png_base.ts` now returns a proper unsigned
+  32-bit value (`>>> 0`) — prevents a near-infinite parse loop on crafted PNGs with a high-bit chunk
+  length (CPU-based DoS)
+- Security: LZW decoder `output.push(...entry)` replaced with a `for`-loop in `src/utils/lzw.ts` and
+  `src/utils/gif_decoder.ts` — prevents call-stack overflow
+  (`RangeError: Maximum call stack
+  size exceeded`) on adversarial GIF input
 - PNG decoder: 16-bit per-channel images (bitDepth=16) now decode correctly; the pixel stride was
   using a fixed 8-bit offset (`x*4`, `x*3`, `x`) causing pixel-offset corruption in 16-bit RGBA,
   RGB, and grayscale images

TODO.md

@@ -55,8 +55,8 @@ instead of returning a clean error. The same pattern appears in `gif_decoder.ts:
 In multi-frame TIFF encoding, pixel data is compressed once in the first loop (lines 582–596) and
 written to the output. The second loop (IFD-writing phase) compresses each frame's data **again**
 with a fresh encoder/compressor to obtain `StripByteCounts`. For deflate, the OS-level zlib may
-produce a different output length on the second call, writing an `StripByteCounts` tag that does
-not match the actual strip, making the TIFF **unreadable by most viewers**.
+produce a different output length on the second call, writing an `StripByteCounts` tag that does not
+match the actual strip, making the TIFF **unreadable by most viewers**.
 
 **Fix:** Cache the compressed result from the first loop (e.g. `compressedFrames[i]`) and reuse
 `.length` in the second loop.
@@ -81,9 +81,9 @@ wrong buffer size.
 ### H3 — `src/formats/png.ts:59-70` — No bounds check before reading chunk data in `decode()`
 
 The decode loop reads a 4-byte chunk `length` field without first verifying that at least 8 bytes
-remain at `pos`. For a truncated PNG, individual byte reads return `undefined`, silently coercing
-to `0`. A very large `length` (e.g. `0x7FFFFFFF`) yields `data.slice(pos, pos + length)` returning
-an under-sized slice that is silently mis-parsed with no error thrown.
+remain at `pos`. For a truncated PNG, individual byte reads return `undefined`, silently coercing to
+`0`. A very large `length` (e.g. `0x7FFFFFFF`) yields `data.slice(pos, pos + length)` returning an
+under-sized slice that is silently mis-parsed with no error thrown.
 
 `extractMetadata()` already has the guard `if (pos + 8 > data.length) break;`; `decode()` should
 too.
@@ -108,9 +108,10 @@ the `>>> 0` fix there (C1).
 
 ### M1 — `src/formats/pcx.ts:52-54` — Missing `validateImageDimensions` call; unchecked allocation
 
-PCX decode computes `new Uint8Array(height * scanlineLength)` and `new Uint8Array(width * height * 4)`
-using dimensions derived directly from the header, with only a manual `> 0` check. A crafted PCX
-with `width=65535, height=65535` would attempt to allocate ~17 GB of memory.
+PCX decode computes `new Uint8Array(height * scanlineLength)` and
+`new Uint8Array(width * height * 4)` using dimensions derived directly from the header, with only a
+manual `> 0` check. A crafted PCX with `width=65535, height=65535` would attempt to allocate ~17 GB
+of memory.
 
 **Fix:** Add `validateImageDimensions(width, height)` after the dimension calculation on lines
 49-50, consistent with all other decoders in the project.
@@ -120,9 +121,9 @@ with `width=65535, height=65535` would attempt to allocate ~17 GB of memory.
 ### M2 — `src/formats/bmp.ts:260-261` — `readUint32LE` used for signed `xPelsPerMeter`/`yPelsPerMeter` fields in `extractMetadata`
 
 The BMP spec defines `biXPelsPerMeter` / `biYPelsPerMeter` as signed `LONG` fields. `decode()`
-correctly uses `readInt32LE`, but `extractMetadata()` uses `readUint32LE` for the same fields.
-For a BMP with a negative DPI value, `extractMetadata` returns a large positive DPI value,
-inconsistent with `decode()`.
+correctly uses `readInt32LE`, but `extractMetadata()` uses `readUint32LE` for the same fields. For a
+BMP with a negative DPI value, `extractMetadata` returns a large positive DPI value, inconsistent
+with `decode()`.
 
 **Fix:** Use `readInt32LE` for DPI fields in `extractMetadata`, matching `decode()`.
 
@@ -142,19 +143,19 @@ Standard JPEG libraries use a precomputed cosine table (or the AAN 1D IDCT) and
 ### M4 — `src/utils/image_processing.ts:718-767` — `medianFilter` allocates 4 arrays per pixel in the inner loop
 
 `rValues`, `gValues`, `bValues`, `aValues` are declared as fresh `number[]` inside the pixel loop.
-For a 1000×1000 image with `radius=1` this creates 4 million short-lived arrays, causing GC
-pressure and significant slowdown.
+For a 1000×1000 image with `radius=1` this creates 4 million short-lived arrays, causing GC pressure
+and significant slowdown.
 
-**Fix:** Declare the four arrays once outside the pixel loop and reset `length = 0` at the start
-of each iteration.
+**Fix:** Declare the four arrays once outside the pixel loop and reset `length = 0` at the start of
+each iteration.
 
 ---
 
 ### M5 — `src/utils/lzw.ts:74` — LZW `decompress` accumulates output in a `number[]`, using ~9× peak memory
 
 The decoder collects decoded bytes in a JS `number[]` (8 bytes per element), then converts to
-`new Uint8Array(output)` at the end. Peak memory is 9× the final output size. For large GIFs this
-is significant.
+`new Uint8Array(output)` at the end. Peak memory is 9× the final output size. For large GIFs this is
+significant.
 
 **Fix:** Pre-allocate a `Uint8Array` sized to `width × height` (the expected GIF pixel count) and
 write directly into it, or use a `Uint8Array`-backed dynamic buffer.
@@ -163,9 +164,9 @@ write directly into it, or use a `Uint8Array`-backed dynamic buffer.
 
 ### M6 — `src/formats/tiff.ts:275` and `encodeFrames:562` — TIFF encoder accumulates output in a `number[]`
 
-Both single-frame and multi-frame TIFF encoding builds the full output as `number[]` then
-converts to `Uint8Array`. For a 4K RGBA image (4096×2160) the intermediate array holds ~35 million
-JS numbers (~280 MB), followed by a ~35 MB `Uint8Array`. Peak memory is roughly 9× the final file.
+Both single-frame and multi-frame TIFF encoding builds the full output as `number[]` then converts
+to `Uint8Array`. For a 4K RGBA image (4096×2160) the intermediate array holds ~35 million JS numbers
+(~280 MB), followed by a ~35 MB `Uint8Array`. Peak memory is roughly 9× the final file.
 
 **Fix:** Build the output as a list of `Uint8Array` chunks and concatenate them at the end with a
 single `new Uint8Array(totalLength)` copy.
@@ -199,8 +200,8 @@ reliability.
 ### L2 — `src/formats/pam.ts:136-144` — PAM decoder silently rejects valid DEPTH values without documentation
 
 The PAM format supports DEPTH 1 (grayscale), 2 (grayscale+alpha), and 3 (RGB). The implementation
-only handles DEPTH=4 (RGBA) and MAXVAL=255, throwing a generic error for everything else. This is
-an intentional limitation but is not documented in the JSDoc, so callers receive a confusing error.
+only handles DEPTH=4 (RGBA) and MAXVAL=255, throwing a generic error for everything else. This is an
+intentional limitation but is not documented in the JSDoc, so callers receive a confusing error.
 
 **Fix:** Document the DEPTH/MAXVAL constraints in the JSDoc and consider a more descriptive error
 message such as `"Only DEPTH=4 MAXVAL=255 PAM files are supported"`.
@@ -209,8 +210,8 @@ message such as `"Only DEPTH=4 MAXVAL=255 PAM files are supported"`.
 
 ### L3 — `src/utils/image_processing.ts:106-135` — `adjustBrightness` rebuilds the LUT on every call
 
-The 767-entry brightness lookup table depends only on `amount`. It is re-created on every call.
-For batch-processing many frames at the same brightness level this is unnecessary work.
+The 767-entry brightness lookup table depends only on `amount`. It is re-created on every call. For
+batch-processing many frames at the same brightness level this is unnecessary work.
 
 **Fix:** Accept the LUT as an optional pre-built parameter, or cache by `amount` with a simple
 `Map`.
@@ -224,8 +225,8 @@ const bgR = colorTable[backgroundColorIndex * 3] || 0;
 ```
 
 If `backgroundColorIndex * 3` is out of range, `colorTable[oob]` returns `undefined`, and
-`undefined || 0` silently returns 0 (black). This is functionally safe for most images but hides
-a potential header-parsing error. An explicit bounds check and warning would aid debugging.
+`undefined || 0` silently returns 0 (black). This is functionally safe for most images but hides a
+potential header-parsing error. An explicit bounds check and warning would aid debugging.
 
 ---
 
@@ -242,9 +243,8 @@ All tests use well-formed images or programmatically generated valid inputs. The
 truncated files, wrong-length headers, or near-limit dimensions that exercise the security
 validation paths in `validateImageDimensions` and the format decoders.
 
-**Fix:** Add a dedicated `test/security.test.ts` (or equivalent) with crafted inputs for each of
-the issues above, asserting that a clean `Error` is thrown rather than a crash or silent
-mis-decode.
+**Fix:** Add a dedicated `test/security.test.ts` (or equivalent) with crafted inputs for each of the
+issues above, asserting that a clean `Error` is thrown rather than a crash or silent mis-decode.
 
 ---
 
@@ -257,35 +257,35 @@ enough to stress the call stack. This makes it easy to reintroduce the bug.
 
 ### L8 — `docs/` — TIFF multi-frame encode limitations are undocumented
 
-The API docs for `encodeFrames` do not warn that large frame counts with `deflate` compression
-incur 2× CPU cost (H1), that the full output is built in memory, or that the encoded file may be
+The API docs for `encodeFrames` do not warn that large frame counts with `deflate` compression incur
+2× CPU cost (H1), that the full output is built in memory, or that the encoded file may be
 unreadable due to the `StripByteCounts` mismatch.
 
 ---
 
 ## Summary
 
-| ID | File | Lines | Category | Severity |
-|----|------|-------|----------|----------|
-| C1 | `src/utils/byte_utils.ts` | 21–24 | Security / DoS | **Critical** |
-| C2 | `src/formats/png_base.ts` | 15–17 | Security / DoS | **Critical** |
-| C3 | `src/utils/lzw.ts` | 100, 122 | Security / DoS | **Critical** |
-| H1 | `src/formats/tiff.ts` | 654–664 | Correctness (data corruption) | **High** |
-| H2 | `src/formats/ico.ts` | 145 | Correctness | **High** |
-| H3 | `src/formats/png.ts` | 59–70 | Correctness | **High** |
-| H4 | `src/utils/webp_decoder.ts` | 31–34 | Correctness / Duplication | **High** |
-| M1 | `src/formats/pcx.ts` | 52–54 | Security / DoS | **Medium** |
-| M2 | `src/formats/bmp.ts` | 260–261 | Correctness | **Medium** |
-| M3 | `src/utils/jpeg_decoder.ts` | 961–996 | Performance | **Medium** |
-| M4 | `src/utils/image_processing.ts` | 718–767 | Performance | **Medium** |
-| M5 | `src/utils/lzw.ts` | 74 | Memory | **Medium** |
-| M6 | `src/formats/tiff.ts` | 275, 562 | Memory | **Medium** |
-| M7 | `src/utils/image_processing.ts` | 303 | Documentation | **Medium** |
-| L1 | `src/formats/png.ts` | — | Correctness | **Low** |
-| L2 | `src/formats/pam.ts` | 136–144 | API / Documentation | **Low** |
-| L3 | `src/utils/image_processing.ts` | 106–135 | Performance | **Low** |
-| L4 | `src/utils/gif_decoder.ts` | 342–344 | Robustness | **Low** |
-| L5 | `src/formats/tiff.ts` | 656–657 | Code quality | **Low** |
-| L6 | `test/` | — | Test coverage | **Low** |
-| L7 | `test/` | — | Test coverage | **Low** |
-| L8 | `docs/` | — | Documentation | **Low** |
+| ID | File                            | Lines    | Category                      | Severity     |
+| -- | ------------------------------- | -------- | ----------------------------- | ------------ |
+| C1 | `src/utils/byte_utils.ts`       | 21–24    | Security / DoS                | **Critical** |
+| C2 | `src/formats/png_base.ts`       | 15–17    | Security / DoS                | **Critical** |
+| C3 | `src/utils/lzw.ts`              | 100, 122 | Security / DoS                | **Critical** |
+| H1 | `src/formats/tiff.ts`           | 654–664  | Correctness (data corruption) | **High**     |
+| H2 | `src/formats/ico.ts`            | 145      | Correctness                   | **High**     |
+| H3 | `src/formats/png.ts`            | 59–70    | Correctness                   | **High**     |
+| H4 | `src/utils/webp_decoder.ts`     | 31–34    | Correctness / Duplication     | **High**     |
+| M1 | `src/formats/pcx.ts`            | 52–54    | Security / DoS                | **Medium**   |
+| M2 | `src/formats/bmp.ts`            | 260–261  | Correctness                   | **Medium**   |
+| M3 | `src/utils/jpeg_decoder.ts`     | 961–996  | Performance                   | **Medium**   |
+| M4 | `src/utils/image_processing.ts` | 718–767  | Performance                   | **Medium**   |
+| M5 | `src/utils/lzw.ts`              | 74       | Memory                        | **Medium**   |
+| M6 | `src/formats/tiff.ts`           | 275, 562 | Memory                        | **Medium**   |
+| M7 | `src/utils/image_processing.ts` | 303      | Documentation                 | **Medium**   |
+| L1 | `src/formats/png.ts`            | —        | Correctness                   | **Low**      |
+| L2 | `src/formats/pam.ts`            | 136–144  | API / Documentation           | **Low**      |
+| L3 | `src/utils/image_processing.ts` | 106–135  | Performance                   | **Low**      |
+| L4 | `src/utils/gif_decoder.ts`      | 342–344  | Robustness                    | **Low**      |
+| L5 | `src/formats/tiff.ts`           | 656–657  | Code quality                  | **Low**      |
+| L6 | `test/`                         | —        | Test coverage                 | **Low**      |
+| L7 | `test/`                         | —        | Test coverage                 | **Low**      |
+| L8 | `docs/`                         | —        | Documentation                 | **Low**      |

src/formats/png_base.ts

@@ -13,8 +13,8 @@ export abstract class PNGBase {
    * Read a 32-bit unsigned integer (big-endian)
    */
   protected readUint32(data: Uint8Array, offset: number): number {
-    return (data[offset] << 24) | (data[offset + 1] << 16) |
-      (data[offset + 2] << 8) | data[offset + 3];
+    return ((data[offset] << 24) | (data[offset + 1] << 16) |
+      (data[offset + 2] << 8) | data[offset + 3]) >>> 0;
   }
 
   /**

src/utils/byte_utils.ts

@@ -19,8 +19,8 @@ export function readUint16LE(data: Uint8Array, offset: number): number {
  * Read a 32-bit unsigned integer in little-endian format
  */
 export function readUint32LE(data: Uint8Array, offset: number): number {
-  return data[offset] | (data[offset + 1] << 8) |
-    (data[offset + 2] << 16) | (data[offset + 3] << 24);
+  return (data[offset] | (data[offset + 1] << 8) |
+    (data[offset + 2] << 16) | (data[offset + 3] << 24)) >>> 0;
 }
 
 /**

src/utils/gif_decoder.ts

@@ -72,7 +72,7 @@ export class GIFDecoder {
       const blockSize = this.readByte();
       if (blockSize === 0) break;
       const blockData = this.readBytes(blockSize);
-      blocks.push(...blockData);
+      for (const b of blockData) blocks.push(b);
     }
     return new Uint8Array(blocks);
   }

src/utils/lzw.ts

@@ -97,7 +97,7 @@ export class LZWDecoder {
 
       if (code < this.dict.length && this.dict[code]) {
         const entry = this.dict[code];
-        output.push(...entry);
+        for (const b of entry) output.push(b);
 
         if (this.prevCode !== null && this.prevCode < this.dict.length) {
           const prevEntry = this.dict[this.prevCode];
@@ -119,7 +119,7 @@ export class LZWDecoder {
           this.dict[this.nextCode] = newEntry;
           this.nextCode++;
 
-          output.push(...newEntry);
+          for (const b of newEntry) output.push(b);
         }
       }
 

src/utils/webp_decoder.ts

@@ -21,18 +21,14 @@
  */
 
 import { validateImageDimensions } from "./security.ts";
+import { readUint32LE } from "./byte_utils.ts";
 import type { ImageDecoderOptions } from "../types.ts";
 
 // Helper to read little-endian values
 function readUint24LE(data: Uint8Array, offset: number): number {
   return data[offset] | (data[offset + 1] << 8) | (data[offset + 2] << 16);
 }
 
-function readUint32LE(data: Uint8Array, offset: number): number {
-  return data[offset] | (data[offset + 1] << 8) |
-    (data[offset + 2] << 16) | (data[offset + 3] << 24);
-}
-
 // Huffman tree node
 interface HuffmanNode {
   symbol?: number;