codeql icon indicating copy to clipboard operation
codeql copied to clipboard

isSanitizerGuard works incorrectly when the function name startwith "isValid"

Open oicu0619 opened this issue 1 year ago • 1 comments
trafficstars

i am try the the codeql query from https://codeql.github.com/docs/codeql-language-guides/analyzing-data-flow-in-javascript-and-typescript/ ; the code to analysis is copied from tutorial

const fs = require('fs'),
      path = require('path');

function readFileHelper(p) {     // #2
  if (!checkPath(p)){
    return;
  }
  fs.readFile(p,                 // #4
    'utf8', (err, data) => {
    if (err) throw err;
    console.log(data);
  });
}

readFileHelper(process.argv[2]); // #1

and the query is also from tutorial

/**
 * @name Find usage of request.body and request.query in Express.js
 * @description Finds variables assigned from request.body and request.query in Express.js applications.
 * @kind problem
 * @id js/express-request-body-query
 */
import javascript

class CheckPathSanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode {
  CheckPathSanitizerGuard() { this.getCalleeName() = "checkPath" }

  override predicate sanitizes(boolean outcome, Expr e) {
    outcome = true and
    e = getArgument(0).asExpr()
  }
}

class CommandLineFileNameConfiguration extends TaintTracking::Configuration {
  CommandLineFileNameConfiguration() { this = "CommandLineFileNameConfiguration" }

  override predicate isSource(DataFlow::Node source) {
    DataFlow::globalVarRef("process").getAPropertyRead("argv").getAPropertyRead() = source
  }

  override predicate isSink(DataFlow::Node sink) {
    DataFlow::moduleMember("fs", "readFile").getACall().getArgument(0) = sink
  }
  override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) {
    nd instanceof CheckPathSanitizerGuard
  }
}

from CommandLineFileNameConfiguration cfg, DataFlow::Node source, DataFlow::Node sink
where cfg.hasFlow(source, sink)
select source, "12312323" 

i use command line cli (2.18.3 newest version) to get the result

codeql database create codeqldb --language=javascript
codeql database analyze --rerun --threads 0 <dir of ql>/test2.ql --format=sarif-latest --output=1.sarif

It all works fine. Then i change the ql, this line

  override predicate sanitizes(boolean outcome, Expr e) {
    outcome = true and
    e = getArgument(0).asExpr()
  }

to

  override predicate sanitizes(boolean outcome, Expr e) {
    outcome = false and
    e = getArgument(0).asExpr()
  }

As expected, it does not sanitize the output. The wierd thing is, if i change the function name of "checkPath", to "isValid", the sanitizer works very wierd, it sanitize the output no matter outcome is true of false. code is like:

const fs = require('fs'),
      path = require('path');

function readFileHelper(p) {     // #2
  if (!isValid(p)){
    return;
  }
  fs.readFile(p,                 // #4
    'utf8', (err, data) => {
    if (err) throw err;
    console.log(data);
  });
}

readFileHelper(process.argv[2]); // #1

ql is like

/**
 * @name Find usage of request.body and request.query in Express.js
 * @description Finds variables assigned from request.body and request.query in Express.js applications.
 * @kind problem
 * @id js/express-request-body-query
 */
import javascript

class CheckPathSanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode {
  CheckPathSanitizerGuard() { this.getCalleeName() = "isValid" }

  override predicate sanitizes(boolean outcome, Expr e) {
    outcome = false and
    e = getArgument(0).asExpr()
  }
}

class CommandLineFileNameConfiguration extends TaintTracking::Configuration {
  CommandLineFileNameConfiguration() { this = "CommandLineFileNameConfiguration" }

  override predicate isSource(DataFlow::Node source) {
    DataFlow::globalVarRef("process").getAPropertyRead("argv").getAPropertyRead() = source
  }

  override predicate isSink(DataFlow::Node sink) {
    DataFlow::moduleMember("fs", "readFile").getACall().getArgument(0) = sink
  }
  override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) {
    nd instanceof CheckPathSanitizerGuard
  }
}

from CommandLineFileNameConfiguration cfg, DataFlow::Node source, DataFlow::Node sink
where cfg.hasFlow(source, sink)
select source, "12312323" 

The only thing changed is the function name. "isValid" is not some special thing in js i think. I try other function names, everything start with isValid works abnormal, like "isValidd" "isValidg", anything else works correctly, like "isVali"

oicu0619 avatar Sep 06 '24 02:09 oicu0619