Skip to content

Possible out-of-bounds read in PNG encoding from unchecked width/height vs buffer size #58

Description

@OvOhao

Possible out-of-bounds read in PNG encoding from unchecked width/height vs buffer size

I found a possible out-of-bounds read (heap over-read with information disclosure) in
PngEncoder::encode. The Png constructor (Png::New) accepts a JS Buffer, a width, and a
height as three independent arguments and validates only that the buffer is a Buffer and that
width/height are non-negative integers. It never checks that the buffer actually holds
width * height * channels bytes. The encoder then walks height rows of width * channels
bytes straight out of the buffer's backing store. If a caller passes a small buffer with large
width/height, the encoder reads far past the end of the buffer, and the over-read bytes are
compressed into the returned PNG and handed back to JS — leaking adjacent heap memory.

File: src/png_encoder.cpp (missing validation in src/png.cpp, Png::New)

Function: PngEncoder::encode

// src/png.cpp — Png::New: the ONLY input checks
if (!Buffer::HasInstance(args[0])) return VException("First argument must be Buffer.");
if (!args[1]->IsInt32())          return VException("Second argument must be integer width.");
if (!args[2]->IsInt32())          return VException("Third argument must be integer height.");
...
int w = args[1]->Int32Value();
int h = args[2]->Int32Value();
if (w < 0) return VException("Width smaller than 0.");
if (h < 0) return VException("Height smaller than 0.");
Png *png = new Png(w, h, buf_type, bits);          // no check: buffer length vs w*h*channels
png->handle_->SetHiddenValue(String::New("buffer"), args[0]);
// src/png_encoder.cpp — PngEncoder::encode reads w*h*channels from `data`
png_bytep *row_pointers = (png_bytep *)malloc(sizeof(png_bytep) * height);
switch (buf_type) {
case BUF_RGB:
case BUF_BGR:
    for (int i=0; i<height; i++)
        row_pointers[i] = data+3*i*width;          // row i points 3*i*width into the buffer
    break;
case BUF_GRAY:
    for (int i=0; i<height; i++)
        row_pointers[i] = data+(bits/8)*i*width;
    break;
default:
    for (int i=0; i<height; i++)
        row_pointers[i] = data+4*i*width;          // RGBA: 4*i*width
}
png_write_image(png_ptr, row_pointers);            // libpng reads width*channels per row
  1. Png::New stores the raw buffer pointer (data) together with width/height, checking
    only that width/height are >= 0. There is no relationship enforced between the buffer's
    byte length and width * height * channels.
  2. PngEncoder::encode builds height row pointers, each channels*i*width bytes into data,
    and png_write_image reads width*channels bytes from every row.
  3. Total bytes read from data = width * height * channels. When the JS buffer is smaller
    than that, libpng reads out of bounds.
  4. Those bytes are fed through png_chunk_producer into the output PNG, which is copied into a
    new Buffer and returned to the JS callback — turning the over-read into a memory-disclosure
    primitive. The same path exists in both PngEncodeSync and the async UV_PngEncode.

JS trigger (if applicable):

const png = require('./build/Release/png').Png;
// 4-byte buffer, but claim a 1000x1000 RGB image -> reads ~3 MB out of bounds
const p = new png(Buffer.alloc(4), 1000, 1000, 'rgb');
const leaked = p.encodeSync(); // returned PNG contains adjacent heap memory

Suggested fix: in Png::New, compute the required size
(bytesPerPixel(buf_type,bits) * width * height, with overflow checks) and reject the call with
a VException when Buffer::Length(args[0]) is smaller than that required size, before storing
the buffer.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions