js - Detect sinks introduced through implicit globals

[DSimsek000]

I am trying to reproduce GHSA-m7p2-ghfh-pjvx and came up with the following query:

/**
 * @name Command injection
 * @description Using externally controlled strings in a command line may allow a malicious
 *              user to change the meaning of the command.
 * @severity high
 * @kind path-problem
 * @precision high
 * @id js/command-line-injection
 * @tags external/cwe/cwe-088
 */

import javascript
import semmle.javascript.dataflow.TaintTracking
import semmle.javascript.security.dataflow.CommandInjectionQuery

class Config extends DataFlow::Configuration {
  Config() { this = "CommandInjection" }

  override predicate isSource(DataFlow::Node source) {
    exists(Function f | f.getName() = "crunch" | f.getAParameter() = source.asExpr())
  }

  override predicate isSink(DataFlow::Node sink) {
    sink instanceof Sink
    or
    isIndirectCommandArgument(sink, _)
  }
}

import DataFlow::PathGraph

from Config dataflow, DataFlow::PathNode source, DataFlow::PathNode sink
where dataflow.hasFlowPath(source, sink)
select sink, source, sink, "This command depends on $@.", source.getNode(), ""

However, it does not find the path or any sinks.

Adding

const { exec } = require("shelljs")

to the beginning of aapts/index.js solves this.

It seems the issue is the missing sink definition for shelljs/global, which does not export any members but instead adds them to the global object https://github.com/shelljs/shelljs/blob/master/global.js#L4.

Are there recommended strategies in CodeQL for detecting command sinks introduced through implicit globals, like those in shelljs/global?

[mbg]

Hi @DSimsek000,

For all libraries, we rely on having models that summarise them. For shelljs, you can find the implementation of these models in https://github.com/github/codeql/blob/main/javascript/ql/lib/semmle/javascript/frameworks/ShellJS.qll

It doesn't seem like this implementation deals with functions that shelljs can make available globally. This makes sense, because it would be difficult to determine that some function that exists globally is a particular one that comes from shelljs and it would be unreasonable to make assumptions about this for all codebases.

Nonetheless, you can add your own, custom models which assume that e.g. a call to exec always refers to the shelljs function if you can safely make that assumption about your codebase.

In particular, rather than using the existing Sink implementation that your query currently uses, you could replace that with your own Sink class which includes calls to any global function called exec.

[DSimsek000]

This makes sense, because it would be difficult to determine that some function that exists globally is a particular one that comes from shelljs and it would be unreasonable to make assumptions about this for all codebases.

Thats a fair point. Thanks for your answer and suggestions!