How to find CallNodes that break data flow?

[jghebre]

Hi 👋,

I'm trying to pinpoint CodeQLs limitations with finding vulnerabilities in certain vulnerable npm packages.
I've noticed that breaks in the call graph (CallNodes without resolved callees) seem to be a good place to start looking.

I have a query to find CallNodes without resolved callees, which works fairly well, except I've noticed that for some results data flow continues even though the callee is missing.

For instance path.join() is a CallNode without a resolved callee for cases in my data, except it does not break data flow.

for instance in this simple example:

const path = require('path');

function run() {
    let input = location.hash.substring(1);
    foo(path.join(input, ''));
}

function foo(input) {
    eval(input);
}

run();

path.join() does not break dataflow, the vulnerability is still caught. I've noticed that most if not all of these cases seem to be popular methods that I believe have rules for propagation in the libraries. I think path.join()s logic is here

whereas in this example:

const path = require('path'); // so CodeQL can handle callbacks

function getUserInput(input) {
    return input
}

function run(callback) {
    let input = location.hash.substring(1);

    //store callback func in array
    let callbacks = []
    callbacks.push(callback) 

    //call the callback
    foo(callbacks[0](input))
}

function foo(input) {
    eval(input);
}

run(getUserInput);

The CallNode callbacks[0](input) missing a callee causes data flow to stop and the vulnerability to be missed.

My issue is that I want to filter out CallNodes that are technically missed but still allow data flow like path.join(), since they are not relevant to the vulnerability being missed.

I've tried to solve this by checking if the CallNodes contain any flow edges emanating from them:

 predicate filtered_call(DataFlow::CallNode node) {
  node.getCalleeName() = "require" 
  or node.getReceiver().toString() = "console"
 }

 predicate missing_callee_with_flow_step(DataFlow::CallNode callee, DataFlow::Node next) {
   DataFlow::AdditionalFlowStep::step(callee, next)
  or DataFlow::SharedFlowStep::step(callee, next)
 }
 from DataFlow::CallNode node
 where not exists(node.getACallee(0))
 and not filtered_call(node)
 and not( missing_callee_with_flow_step(node, _))
 select 
   node, 
   "missing callee from call node " + node.toString() + 
   " | Callee Name: " + node.getCalleeName() + 
   " | found at line: " + node.getStartLine() + 
   " | column: " + node.getStartColumn() + 
   " | file: " + node.getFile().getAbsolutePath()

Unfortunately this doesn't seem to work well. I'm having a hard time finding a way to filter out these CallNodes.

I suppose I could manually filter out all these nodes but that would be very tedious and also inaccurate in cases where duplicate receiver & callee names exists.

My main question is what can I add to my query to filter out all CallNodes that still propagate data flow? or
How can I find all CallNodes that truly break data flow?

Thanks!

[rvermeulen]

@jghebre thanks for your question.

I will have a look if we can surface that information. It seems you want to exclude unresolved calls for which we have summaries that guide the global dataflow.

[rvermeulen]

Hi @jghebre,

Here is an update.
The following query should provide you with a good starting point.

SummarizedCallable will contain all the calls we summarize using models.
There is still the possibility that a call can be summarized in a configuration because it is specific to a vulnerability. For example the path.join call.
Since this is part of a global analysis we don't provide those by default for performance reasons, but you can import them by calling the isAdditionalFlowStep on their configuration. Make sure to check which version of isAdditionalFlowStep is used, we have one with and one without flow states.

Let me know if you have any further questions.

import javascript
import semmle.javascript.dataflow.FlowSummary
import semmle.javascript.security.dataflow.TaintedPathQuery as TPQ
import semmle.javascript.security.dataflow.CodeInjectionQuery as CIQ

class UnresolvedCallNode extends DataFlow::CallNode
{
  UnresolvedCallNode() {
    not exists(this.getACallee(0))
  }
}

class InterestingUnresolvedCallNode extends UnresolvedCallNode
{
  InterestingUnresolvedCallNode() {
    not(
      this.getCalleeName() = "require" or
      this.(DataFlow::CallNode).getReceiver().(DataFlow::ExprNode).asExpr().(VarAccess).getVariable().getName() = "console"
    )
  }
}

class InterestingUnresolvedCallWithSummary extends InterestingUnresolvedCallNode
{
  InterestingUnresolvedCallWithSummary() {
    any(SummarizedCallable summary).getACallSimple() = this
    or
    // Implemented with flow labels
    TPQ::TaintedPathConfig::isAdditionalFlowStep(_, _, this, _)
    or
    // Implemented without flow labels
    CIQ::CodeInjectionConfig::isAdditionalFlowStep(_, this)
    // Add other configurations of interest here.
  }
}

from InterestingUnresolvedCallNode unresolvedCallNode
where
  not unresolvedCallNode instanceof InterestingUnresolvedCallWithSummary
select unresolvedCallNode,
  "missing callee from call node " + unresolvedCallNode.toString() + " | Callee Name: " + unresolvedCallNode.getCalleeName() +
    " | found at line: " + unresolvedCallNode.getStartLine() + " | column: " + unresolvedCallNode.getStartColumn() + " | file: "
    + unresolvedCallNode.getFile().getAbsolutePath()

[jghebre]

Hi @rvermeulen , thank you, I think this is what I was looking for!

I have one more related question.

I have another method of finding these CallNodes without callees, instead of an alert the query is a path-problem for a security query, where the source remains the same but the sink is the CallNode in question. I think this manner is a more precise way of finding CallNodes that interrupt flow in security queries.

Here is an example of implementing this for CodeInjection.ql:

/**
 * @name Code injection CallNode Missing
 * @description Finding CallNodes without a callee in the path of Code Injection vulnerabilities
 * @kind path-problem
 * @problem.severity error
 * @security-severity 1.0
 * @precision high
 * @id js/code-injection
 * @tags security
 *       external/cwe/cwe-094
 *       external/cwe/cwe-095
 *       external/cwe/cwe-079
 *       external/cwe/cwe-116
 */

import javascript
import semmle.javascript.security.dataflow.CodeInjectionQuery
import CodeInjectionFlowNew::PathGraph
import queries.lib.InterestingUnresolveCallWithSummary


module CodeInjectionConfigNew implements DataFlow::ConfigSig {
    predicate isSource = CodeInjectionConfig::isSource/1;
    predicate isSink(DataFlow::Node sink) {
     sink instanceof InterestingUnresolvedCallWithSummary and
     not CodeInjectionConfig::isAdditionalFlowStep(_, sink)
  }
}

class ParamStep extends TaintTracking::SharedTaintStep {
  override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
      nodeTo instanceof DataFlow::InvokeNode
      // Must have no recognized callee
      and not exists(nodeTo.(DataFlow::InvokeNode).getACallee(0))
      // The `pred` is an argument to that invocation
      and nodeFrom = nodeTo.(DataFlow::InvokeNode).getAnArgument*()
  }
}

module CodeInjectionFlowNew = TaintTracking::Global<CodeInjectionConfigNew>;

from CodeInjectionFlowNew::PathNode source, CodeInjectionFlowNew::PathNode sink
where CodeInjectionFlowNew::flowPath(source, sink)
select sink, source, sink,
   "missing callee from call node " + sink.getNode().toString() + 
   " | Callee Name: " + sink.getNode().(DataFlow::InvokeNode).getCalleeName() + 
   " | found at line: " + sink.getNode().getStartLine() + 
   " | column: " + sink.getNode().getStartColumn() + 
   " | file: " + sink.getNode().getFile().getAbsolutePath()

I added a ParamStep because I noticed if the CallNode was missing a callee, CodeQL would not consider the flow to the parameter of that CallNode. Here I keep the source and the sink is an instance of the Class you made with a negation of the SummarizedCallables since I want to filter those out.

import javascript
import semmle.javascript.dataflow.FlowSummary
import semmle.javascript.security.dataflow.TaintedPathQuery as TPQ
import semmle.javascript.security.dataflow.CodeInjectionQuery as CIQ

class UnresolvedCallNode extends DataFlow::CallNode
{
  UnresolvedCallNode() {
    not exists(this.getACallee(0))
  }
}

class InterestingUnresolvedCallNode extends UnresolvedCallNode
{
  InterestingUnresolvedCallNode() {
    not(
      this.getCalleeName() = "require" or
      this.(DataFlow::CallNode).getReceiver().(DataFlow::ExprNode).asExpr().(VarAccess).getVariable().getName() = "console"
    )
  }
}

class InterestingUnresolvedCallWithSummary extends InterestingUnresolvedCallNode
{
  InterestingUnresolvedCallWithSummary() {
     not any(SummarizedCallable summary).getACallSimple() = this
  }
}

and a negation of CodeInjections addtionalFlowSteps.

This works okay but it records calls with flow emanating from them.

for instance for this code:

const path = require('path');


function passthrough(input) {
    return input;
}

function run(callback) {
    let input = location.hash.substring(1);

    // call the passthrough function
    same_input = passthrough(path.join(input, ''))

    // store callback func in array
    let callbacks = []
    callbacks.push(callback)
    
    foo = callbacks[0];

    placeholder = foo(same_input);


}


function foo(input) {
    eval(input);
}

run(foo);

It picks up paths to:

• substring()
• path.join()
• foo()

foo() is the one I want here because it truly stops the dataflow.

I think I could address this by adding other Configurations isAdditionalFlowSteps you mentioned.

For instance negating TaintedPathConfig::isAdditionalFlowStep() in isSink like so:

  predicate isSink(DataFlow::Node sink) {
     sink instanceof InterestingUnresolvedCallWithSummary and
     not CIQ::CodeInjectionConfig::isAdditionalFlowStep(_, sink) and
     not TPQ::TaintedPathConfig::isAdditionalFlowStep(_, _, sink, _)
  }

fixes this issue for CodeInjection, it filters out substring() and path.join() and only shows foo().
I was wondering if there is a more direct way of determining, for a Configuration, if there are any flow steps after the CallNode other than this method.

My only concern with this method is that flow continues for CodeInjection Configuration through path.join() and substring() with CodeInjectionConfig::isAdditionalFlowStep() negated, and requires TaintedPathConfig::isAdditionalFlowStep() to filter out those calls. I'm not sure how that is happening.
I think it would require me adding isAdditionalFlowSteps() for every security Configuration which I worry could be imprecise.

My main question is there a more direct way, given a configuration, to determine no flow propagates from a Node?
If not, how can I determine which Configurations isAdditionalFlowSteps() apply for a security query?

Thanks again!

---

[rvermeulen]

Before I go through your follow-up question I was wondering if https://github.com/github/codeql/blob/main/javascript/ql/src/Security/CWE-020/UntrustedDataToExternalAPI.ql is what you are looking for.

We use this to debug missing cases, and in hindsight this seems what you are looking for. Let me know if that is, or isn't, the case.

[jghebre]

Thanks for the suggestion, I was not aware of that query, it was very helpful in another aspect of my project but not so much this one because I'm not really concerned with external APIs modeling.
I have a method for building additional flow steps rules based on the results of certain queries I've made. The goal is to bridge the gaps in dataflow and find more results.

The flow steps are built as a result of the queries I run to find these potential "gaps" in dataflow. One of these gaps I've found are CallNodes without resolved callees. The problem is that CodeQL has alot of CallNodes without resolved callees, and many of these do not actually result in any gaps in dataflow.
I am trying to be more precise in this case about the steps I build by restricting my queries to the CallNodes that have no resolved callees and no propagating flow.

One of the queries I'm using is a path-problem query where the sink is a CallNode without a resolved callee and the source is a regular source for a security query. I'm was hoping there was a way in QL to specify that a CallNode has no flow propagation based on a Configuration.

I tried DataFlow::AdditionalFlowStep::step, DataFlow::localFlowStep and DataFlow::SharedFlowStep::step but these don't seem to work. Adding the SummarizedCallable worked great for cutting down CallNodes I wasn't concerned with. I think adding in other Configuration's isAdditionalFlowStep should help even more, I'm still trying to determine which one I should add.

Sorry for the verbose response, you've been extremely helpful thank you!

[rvermeulen]

I believe the query considers function calls that cannot be resolved (what typically means the callee is in a dependency) and filters out those that are summarized, but it has been a while since I looked at it.

Good to hear the SummarizedCallable is useful. The next step is indeed to look at the additional flow steps to include. Unfortunately that is going to be a bit of manual work unless you parse out the configurations.

[jghebre]

Closing since SummarizedCallable and isAdditionalFlowStep work well enough for my usecase. Thanks again!