DataFlow can't pass taint flow for functions not found defined in Node.js?

[u2400]

For example, the SQL injection in the code in the example below does not get the scan result with the following codeql statement.

javascript code:

const route = require('koa-route');
const Koa = require('koa');
const mysql = require('mysql2');
const app = new Koa();
const xss = require('xss');

const connection = mysql.createConnection({
    host: 'localhost',
    user: 'root',
    database: 'test'
});

app.use(route.get('/test1', (context, param1) => {
    param1 = xss(param1)
    connection.query(
        `SELECT * FROM \`table\` WHERE \`name\` =` + param1,
    );
}));

codeql scan rule:

/**
 * @name sql-injection
 * @kind path-problem
 * @precision high
 * @id js/sql-injection
 */

import javascript
import semmle.javascript.security.dataflow.SqlInjection
import DataFlow::PathGraph
import udc.SourceClass

class CostumSqlInjectionConfig extends SqlInjection::Configuration {
  override predicate isSource(DataFlow::Node n) {
    n instanceof udc::Source
    or n instanceof SqlInjection::Source
  }
}

from CostumSqlInjectionConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight,
DataFlow::Node sourceNode
where
    cfg.hasFlowPath(source, sink)
    and highlight = sink.getNode()
    and sourceNode = source.getNode()
select highlight, source, sink, "This Sql query depends on $@.", sourceNode
, sourceNode.toString()

I currently believe that the localFlowStep function in Node.js does not take into account functions defined in the npm library, and does not make conservative assumptions about undefined functions.

https://github.com/github/codeql/blob/main/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll#L1536

Is this an unintended situation? I have currently fixed the issue by modifying the query rules under my local DataFlow namespace.

But codeql's internal library doesn't give pr guidelines, so I open issue for now.

[hmakholm]

Thanks for your suggestion.

Even though assuming a data flow through the unknown code does seem to make sense in this case, our experience is that if we enable it across the board, it results in an unacceptable amount of false-positive alerts.

We feel that would do more harm than good -- even if we also get more true positive alerts, too many false positives would lead many developers to stop taking CodeQL alerts seriously, with the net effect of fewer true positives actually being acted on.

Our strategy for dealing with the problem currently centers around improved explicit modeling of external libraries. There are several strands of work, ranging from experiments with extracting models from the library's code when it's open source anyway (this turns out not to work as well as one could have hoped for), to providing easier ways for the community to write and contribute models for the libraries that matter to them.

[u2400]

Thanks for your suggestion.

Even though assuming a data flow through the unknown code does seem to make sense in this case, our experience is that if we enable it across the board, it results in an unacceptable amount of false-positive alerts.

We feel that would do more harm than good -- even if we also get more true positive alerts, too many false positives would lead many developers to stop taking CodeQL alerts seriously, with the net effect of fewer true positives actually being acted on.

Our strategy for dealing with the problem currently centers around improved explicit modeling of external libraries. There are several strands of work, ranging from experiments with extracting models from the library's code when it's open source anyway (this turns out not to work as well as one could have hoped for), to providing easier ways for the community to write and contribute models for the libraries that matter to them.

Thank you very much for your reply

I understand and agree with your point of view. But how do I solve this problem while synchronizing with the official core library code?

It is not uncommon for security filtering components to be misused.

[asgerf]

Hi, and sorry for the prolonged silence on this issue. 👋

Should be fixed by #12424

[asgerf]

Thanks for reporting the false negative @u2400.

I'm closing this issue as the concrete example should be fixed as of #12424, but feel free to open another issue if you find other problems like this (we'll try to resolve it quicker next time).