Skip to content

Possible out-of-bounds write when the options string has more than 32 tokens in ParseArgs #34

Description

@OvOhao

Possible out-of-bounds write when the options string has more than 32 tokens in ParseArgs

I found a possible stack out-of-bounds write in PQCompress::ParseArgs. The caller (PQCompress::Invoke) declares a fixed 32-element pointer array char *cmd_argv[32] and allocates a buffer for each of those 32 slots, then hands it to ParseArgs, which splits the options string on spaces and writes one token into argv[argc] for each token, incrementing argc with no upper bound. If the options string (the second argument to new Pngquant(buffer, args)) contains more than ~30 space-separated tokens, argc reaches and exceeds 32, so argv[32], argv[33], ... read past the end of the 32-element cmd_argv array; those out-of-bounds slots contain uninitialized stack data that is then used as a destination pointer for strncpy, producing a write to an arbitrary/garbage address (crash or memory corruption).

File: src/PQCompress.cc

Function: PQCompress::ParseArgs (array supplied by PQCompress::Invoke)

napi_value PQCompress::Invoke(napi_env env, napi_callback_info info) {
  ...
  char *cmd_argv[32];                                   // fixed 32 slots
  ...
  strcpy(c_args, "-f pngqaunt ");
  strcat(c_args, obj->_args);                           // obj->_args is caller-controlled
  for (i = 0; i < 32; i++) { cmd_argv[i] = (char *)malloc(33); }
  cmd_argc = ParseArgs(c_args, 32, cmd_argv);
  ...
}

int PQCompress::ParseArgs(const char *args, int n, char *argv[]) {
  int i, len = (int)strlen(args), k = 0, argc = 0, is_new = 0;
  char *buf = (char *) malloc (n);
  ...
  for (i = 0; i < len + 1; i++) {
    if (args[i] != ' ' && i < len) {
      if (k < n) { buf[k++] = args[i]; is_new = 1; }
    } else {
      if (is_new == 1) {
        strncpy(argv[argc], buf, k);   // argv[argc] with argc unbounded -> OOB when argc >= 32
        memset(buf, '\0', k);
        k = 0;
        argc++;                        // no check against the array capacity (32)
        is_new = 0;
      }
    }
  }
  free(buf);
  return argc;
}
  1. cmd_argv has exactly 32 valid entries (indices 0..31), each a malloc(33) buffer.
  2. ParseArgs receives n = 32, but n is used only to cap the per-token scratch buffer length, not the number of tokens.
  3. For each whitespace-delimited token it executes strncpy(argv[argc], buf, k) and then argc++, with no comparison of argc against the array size.
  4. obj->_args originates from the JS string passed to the constructor (napi_get_value_string_utf8 into a 1024-byte buffer), so an options string with 32 or more tokens drives argc to 32+.
  5. At argc == 32, argv[32] reads a stack slot beyond cmd_argv, yielding an uninitialized pointer; strncpy then copies the token bytes into that address — an out-of-bounds/wild write.

This code is the addon's own glue (src/PQCompress.cc), not the vendored pngquant/ library, and there is no bound check anywhere on the token count.

JS trigger (if applicable):

const { Pngquant } = require('pngquant-native');
const png = require('fs').readFileSync('in.png');
const manyTokens = Array(40).fill('-v').join(' ');   // >32 tokens
new Pngquant(png, manyTokens).compress();            // OOB write in ParseArgs

Suggested fix: pass the array capacity into ParseArgs and stop (or error) once argc reaches it, e.g. if (argc >= maxArgs) break; before writing argv[argc]; also guard cmd_argv[cmd_argc - 1] in the error path against cmd_argc == 0.

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