diff --git a/CHANGELOG.md b/CHANGELOG.md index 79b6c53..2cfcb38 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/TODO.md b/TODO.md index c1e7cc4..8b9089f 100644 --- a/TODO.md +++ b/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** | diff --git a/src/formats/png_base.ts b/src/formats/png_base.ts index abc0d0a..2a435e1 100644 --- a/src/formats/png_base.ts +++ b/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; } /** diff --git a/src/utils/byte_utils.ts b/src/utils/byte_utils.ts index 7438481..2cb0132 100644 --- a/src/utils/byte_utils.ts +++ b/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; } /** diff --git a/src/utils/gif_decoder.ts b/src/utils/gif_decoder.ts index e16f3da..b2fff96 100644 --- a/src/utils/gif_decoder.ts +++ b/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); } diff --git a/src/utils/lzw.ts b/src/utils/lzw.ts index 4bb470c..3c14bc0 100644 --- a/src/utils/lzw.ts +++ b/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); } } diff --git a/src/utils/webp_decoder.ts b/src/utils/webp_decoder.ts index 4fd2403..aaa3143 100644 --- a/src/utils/webp_decoder.ts +++ b/src/utils/webp_decoder.ts @@ -21,6 +21,7 @@ */ import { validateImageDimensions } from "./security.ts"; +import { readUint32LE } from "./byte_utils.ts"; import type { ImageDecoderOptions } from "../types.ts"; // Helper to read little-endian values @@ -28,11 +29,6 @@ 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;