Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isSanitizerGuard works incorrectly when the function name startwith "isValid" #17393

Open
oicu0619 opened this issue Sep 6, 2024 · 1 comment
Labels
JS question Further information is requested

Comments

@oicu0619
Copy link

oicu0619 commented Sep 6, 2024

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 oicu0619 added the question Further information is requested label Sep 6, 2024
@sidshank sidshank added the JS label Sep 27, 2024
@erik-krogh
Copy link
Contributor

Yes, what you're seeing is a bug and it's an unfortunate limitation of how sanitizers work with our current dataflow library.

The reason why is a bit complicated, but the short version is that the dataflow library can't differentiate your sanitizer and this build-in sanitizer, and it ends up merging the two in an bad way.

The good news is that we're working to fix this limitation (and other limitations) by migrating the JavaScript analysis to use the same shared dataflow library that the other languages are using.
I'm not sure when that dataflow migration will land, but it will probably be a few months.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants