JS: Should SanitizedChar apply when the sanitized value can be an array, as in the systeminformation sanitizer bypassed by CVE-2021-21315? #23123

Description of the issue

js/shell-command-constructed-from-input treats s[i] as sanitized when it is guarded by comparisons against each character in getAShellChar() and i is a number. I'm wondering whether this is intended when s is not known to be a string. An exported function receives whatever its caller passes, and request values from Express query strings or JSON bodies can be arrays. If s is an array, s[i] is a whole element, which is not equal to any of the single characters, so an element like "x; touch /tmp/marker" passes the check unchanged.

Affected sanitizer

SanitizedChar requires that the guards in blocksCharInAccess exclude every character of getAShellChar() and that the property name has type number (L377). Nothing requires the base to be a string or the element to be a single character.

The sanitizer was added in #4921 to get a true negative for CVE-2020-26274. The query test's yetAnohterSanitizer is the sanitizeShellString function of systeminformation 4.31.1 (lib/util.js), the release that fixed CVE-2020-26274, and the test expects no alert for its result (L409). This sanitizer was then bypassed with arrays: CVE-2021-21315 (systeminformation < 5.3.1, in CISA's Known Exploited Vulnerabilities catalog) advises to "only allow strings, reject any arrays. String sanitation works as expected." The fix in 5.3.1 added typeof ... !== "string" checks to the affected functions (for services() in 1e3b479), and 5.6.3 changed sanitizeShellString itself to drop elements longer than one character (af9b8a5). CVE-2021-21388 (< 5.6.4) gives the same workaround.

Steps to reproduce

index.js, the main module of a package ("main": "index.js" in package.json), with the sanitizer copied from the query test:

const cp = require("child_process");

// sanitizeShellString from systeminformation 4.31.1 (lib/util.js), the same code as
// yetAnohterSanitizer in the CodeQL query test (UnsafeShellCommandConstruction/lib/lib.js).
function sanitizeShellString(str) {
  const s = str || '';
  let result = '';
  for (let i = 0; i <= 2000; i++) {
    if (!(s[i] === undefined ||
      s[i] === '>' ||
      s[i] === '<' ||
      s[i] === '*' ||
      s[i] === '?' ||
      s[i] === '[' ||
      s[i] === ']' ||
      s[i] === '|' ||
      s[i] === '˚' ||
      s[i] === '$' ||
      s[i] === ';' ||
      s[i] === '&' ||
      s[i] === '(' ||
      s[i] === ')' ||
      s[i] === ']' ||
      s[i] === '#' ||
      s[i] === '\\' ||
      s[i] === '\t' ||
      s[i] === '\n' ||
      s[i] === '\'' ||
      s[i] === '`' ||
      s[i] === '"')) {
      result = result + s[i];
    }
  }
  return result;
}

module.exports.greet = function (name) {
  return cp.execSync("echo hello " + sanitizeShellString(name)).toString(); // not reported
};

module.exports.greetUnsanitized = function (name) {
  return cp.execSync("echo hello " + name).toString(); // reported
};

js/shell-command-constructed-from-input (and javascript-security-extended.qls) reports only line 42. At runtime (Node.js v24.18.0):

• greet("x; touch /tmp/marker") runs echo hello x touch /tmp/marker; the string is sanitized as intended and no file is created.
• greet(["x; touch /tmp/marker", ""]), the value that querystring.parse("name=x%3B%20touch%20%2Ftmp%2Fmarker&name=") returns, runs echo hello x; touch /tmp/marker, and the file is created.

The same happens on the vulnerable code itself: on systeminformation 5.3.0 (affected by CVE-2021-21315), js/shell-command-constructed-from-input reports nothing. With sanitizeShellString replaced by return str;, it reports lib/processes.js:164 and lib/processes.js:261 in services(), one of the functions named in the advisory, among others.

Question

Is it intended that SanitizedChar applies when the base of s[i] may be an array? Would it be reasonable to require that the base is known to be a string, for example because of a typeof s === "string" check or a conversion with String(...)?

Environment

• CodeQL CLI 2.27.4 (codeql/javascript-queries 2.5.0, codeql/javascript-all 3.0.0)
• CodeQL repository commit for the links above: fbf9fca5ce7c3b64c2f5d719ea0444577850cb4a (the files are unchanged since 2.27.4)
• Node.js v24.18.0 on Linux/amd64