Determine if a function is called with a specific value as parameter based on the value of another variable

[esarafianou]

Hi - I’m pretty new to CodeQL and I haven’t been able to determine if a case I have in mind can be written as a CodeQL query. I want to write a query that for a specific function (validate in the example) returns if there are cases where c will be called if a is false.

Example code that the query should find and return

function validate(a, c) {
  if (a) {
    console.log('error')	
  }
  c(1)
}

Example code that is correct and the query should not mark and return

function validate(a, c) {
  if (a) {
    throw new Error('error')	
  }
  c(1)
}

Is it possible to write such a query?

[asgerf]

Hi,

It sounds like to you can use ConditionGuardNode. It's a control-flow node that occurs in places where it's known that a certain expression is true or false. We can use .dominates() to check whether it dominates another node ("dominates" means all paths to the other node must first go through the guard node).

Here's a query that flags the first example but not the second:

import javascript

Function validateFn() {
    result.getName() = "validate"
}

ConditionGuardNode guardNode() {
    result.getTest() = validateFn().getParameterByName("a").getVariable().getAnAccess() and
    result.getOutcome() = false
}

predicate isGuarded(ControlFlowNode node) {
    guardNode().dominates(node.getBasicBlock())
}

CallExpr sensitiveCall() {
    result.getCallee() = validateFn().getParameterByName("c").getVariable().getAnAccess()
}

from CallExpr call
where call = sensitiveCall()
  and not isGuarded(call)
select call

[esarafianou]

Hi @asgerf , thanks a lot for the response, it was really helpful. I was working today on transforming this to my exact scenario but I faced some issues. So in my scenario the code looks more like:

function validate(a, c) {
  let foo = a.b
  if (foo && <some_other_expression>) {
    console.log('error') or throw Error('error')
  }
  c(1)
}

And the query needs to return if there are code flows that violate the following principle:

c should be called only if a.b exists (is not undefined).

In this example the a.b value goes to the if statement via the foo variable but ideally I wouldn't want to hardcode this assumption

To access the a.b I used the getASuccessor():

ConditionGuardNode guardNode(ControlFlowNode b) {
    b = validateFn().getParameterByName("a").getVariable().getAnAccess().getASuccessor()
    and b.toString() = "b" and
    result.getTest() = b.getAccess() and
    result.getOutcome() = false
}

predicate isGuarded(ControlFlowNode node) {
    guardNode(_).dominates(node.getBasicBlock())
}

Now the code doesn't compile because a ControlFlowNode has no getAccess() predicate but I wrote it to illustrate what I tried. I couldn't find a way to move from a ControlFlowNode to another Type suitable for the getTest() function.

[asgerf]

getAccess is only available on variables - it worked for the simple case, but in general what we're after is a way to ask if one expression can flow into another.

For this we should switch to the DataFlow module. With this library we don't need things like getVariable and getAnAccess, instead there are methods like flowsTo, flowsToExpr, and getAPropertyRead which follow indirections through variables automatically.

There's a tutorial and a cheat sheet for using this library.

import javascript

DataFlow::FunctionNode validateFn() {
    result.getName() = "validate"
}

ConditionGuardNode guardNode() {
    validateFn().getParameter(0).getAPropertyRead("b").flowsToExpr(result.getTest()) and
    result.getOutcome() = true
}

predicate isGuarded(ControlFlowNode node) {
    guardNode().dominates(node.getBasicBlock())
}

DataFlow::CallNode sensitiveCall() {
    result = validateFn().getParameter(1).getACall()
}

from DataFlow::CallNode call
where call = sensitiveCall()
  and not isGuarded(call.asExpr())
select call

Note that your example doesn't actually satisfy the criteria regardless of whether you use throw in the if, but here's one that's correctly recognized as safe:

function validate(a, c) {
    let foo = a.b
    if (!foo || something()) {
        throw 'error';
    }
    c(1)
}

[esarafianou]

Hi @asgerf - thanks, that helped a lot!. I was trying to better understand how ConditionGuardNode and this query work and I wrote some minimal cases here: https://github.com/esarafianou/test6/blob/master/index.js.

I first run the following query:

import javascript

ConditionGuardNode guardNode(DataFlow::FunctionNode validateFn) {
    validateFn.getParameter(0).getAPropertyRead("b").flowsToExpr(result.getTest())
}

from DataFlow::FunctionNode validateFn
select guardNode(validateFn)

With this I wanted to understand what are the guardNodes to which a.b flows. It turns out that if we have something like:

let foo = !a.b
if (foo)

codeQL doesn't understand that a.b flows into the if. That's what my test function secure2h is testing. The query I wrote above doesn't return line 82 as a GuardNode.

Another thing I tested is the case where the sensitive call is made from a function other than the validateFn one even if this other function is embedded. This is what my test function secure2f is testing.

I ran the following query. This is a slightly modified version of yours with the changes being that I get the validateFn name in the select and my sensitiveCall is c(null) (argument must be null):

import javascript

ConditionGuardNode guardNode(DataFlow::FunctionNode validateFn) {
    validateFn.getParameter(0).getAPropertyRead("b").flowsToExpr(result.getTest()) and
    result.getOutcome() = true
}

predicate isGuarded(ControlFlowNode node, DataFlow::FunctionNode validateFn) {
    guardNode(validateFn).dominates(node.getBasicBlock())
}


DataFlow::CallNode sensitiveCall(DataFlow::FunctionNode validateFn) {
    result = validateFn.getParameter(1).getACall()
}

from DataFlow::CallNode call, DataFlow::FunctionNode validateFn
where call = sensitiveCall(validateFn)
  and call.getArgument(0).asExpr() instanceof NullLiteral
  and not isGuarded(call.asExpr(), validateFn)
  select call, validateFn.getName()

Running this against by test6 repo, I get the secure2h and secure2f as insecure ( I also get the secure2g but this is similar to secure2h so let's ignore it).

I'm wondering if both cases (let foo = !a.b; if (foo) and calling the sensitive call from another function) are limitations of CodeQL flow tracking or if there's yet another way I can also track these cases. For the second case I guess it's the limitation of intra-procedural(?)

[asgerf]

We can use a recursive helper predicate to backtrack the condition through ! operators:

/**
 * Gets a data-flow node whose boolean-value must be `outcome` in order for
 * `guard` to be reached.
 */
DataFlow::Node getConditionSource(ConditionGuardNode guard, boolean outcome) {
  result = guard.getTest().flow() and
  outcome = guard.getOutcome()
  or
  result = getConditionSource(guard, outcome).getAPredecessor()
  or
  result = getConditionSource(guard, outcome.booleanNot()).asExpr().(LogicalNotExpr).getOperand().flow()
}

Now we can match guard nodes more precisely:

ConditionGuardNode guardNode() {
  validateFn().getParameter(0).getAPropertyRead("b") = getConditionSource(result, true)
}

Finally we can handle inner functions in some cases by checking if the surrounding function itself is guarded:

predicate isGuarded(ControlFlowNode node) {
  guardNode().dominates(node.getBasicBlock())
  or
  // Check if surrounding function is guarded
  isGuarded(node.getContainer())
}

DataFlow::CallNode sensitiveCall() {
  result = validateFn().getParameter(1).getACall() and
  result.getArgument(0).asExpr() instanceof NullLiteral
}