False negative for JavaScript SQL injection

[wtfiwtz]

Description of the issue

I attempted to detect this vulnerability in our codebase but it doesn't get picked up. Similar to https://github.com/github/codeql/issues/7586 and https://github.com/github/codeql/pull/7591 but I can't quite see what changes are required to the CodeQL source (or how to analyse the data flows / evaluator logs).

const pg = require("pg");
const rl = require('readline');

// Introduce delibrate SQL injection to test CodeQL
rl.question('What is your name? ', ans => {
  console.log('Your name is', ans);

  const pgConn = new pg.Client({});
  pgConn.connect();

  pgConn.query(
    "SELECT * FROM users WHERE id = '" + ans + "'",
    (err, res) => {
      if (err) {
        console.log(err.stack);
      } else {
        console.log(res.rows[0]);
      }
      pgConn.end();
    }
  );

  rl.close();
});

codeql database create codeql --language=javascript-typescript
codeql database analyze codeql --rerun --verbose --format=csv --output codeql.csv codeql/javascript-queries@0.8.16:Security/CWE-089/SqlInjection.ql
codeql database analyze codeql --rerun --verbose --format=csv --output codeql.csv codeql/javascript-queries

Both the 2nd and 3rd commands give me an empty codeql.csv file (i.e. no issues detected).

Using `... ${...} ... ` or "" + ... concatenations didn't make any difference.

Is there any guidance on next steps to get this included in Github Advanced Security, etc?

I attempted this within a bigger repo with multiple AWS lambdas and it didn't pick it up there either.

Best regards, Nigel

[jketema]

Hi @wtfiwtz,

Thanks for your report. I've asked the Javascript team to take a look.

[jketema]

I had a chat with the Javascript team. The philosophy underlying almost all the Javascript queries is that only cases where a remote sources is involved are considered actual vulnerabilities. Consequently, local sources - like readline in your case - are ignored. We have been beta testing "threat models", as a way to adapt this if your specific context requires something different, but that approach is currently not available for Javascript. Hope this helps.

[wtfiwtz]

Thanks @jketema. My purpose for testing this out specifically regarding mono-repos but I did locate some code related to that within the CodeQL code base so they should be handled ok. The original examples had an environment variable or params.authorizationToken for an AWS API Gateway Authorization lambda as the injection parameter. I'll keep testing this a bit further and see if I can find anything else more appropriate that might be identified. We are a little bit jittery here because we've had some things picked up by manual code review lately... hence our motivation to upgrade to GH Advanced Security! Thanks for looking into it! Cheers, Nigel

[wtfiwtz]

This sample definitely works...

const express = require("express");
const pg = require("pg");

const app = express()
const port = 3000

const pgConn = new pg.Client({ connectionString: "socket:/tmp?db=test" });
pgConn.connect();

app.get('/', (req, resp) => {
    const name = req.query.name

    pgConn.query(
        "SELECT * FROM users WHERE id = '" + name + "'",
        (err, res) => {
            if (err) {
                console.log(err.stack);
            } else {
                console.log(res.rows[0]);
                resp.send(`Results: ${JSON.stringify(res.rows)}`);
            }
            // pgConn.end();
        }
    );
})

app.listen(port, () => {
    console.log(`Example app listening on port ${port}`)
})

I'm not sure if I needed to re-create the CodeQL database from scratch but it definitely detected it fine.

The CSV entry was:

"Database query built from user-controlled sources","Building a database query from user-controlled sources is vulnerable to insertion of malicious code by the user.","error","This query string depends on a [[""user-provided value""|""relative:///index.js:12:18:12:31""]].","/index.js","15","9","15","55"

Thanks for your assistance!

Cheers, Nigel

[jketema]

I'm not sure if I needed to re-create the CodeQL database from scratch but it definitely detected it fine.

Not sure what you mean by this, but if you changed your code you'll need to re-create the database. If you tested this through the CodeQL testing framework, this will be done automatically, otherwise you'll need to do this manually.

[jketema]

Your last example I do expect to be flagged up, as seems to be happening, as listening on a port can mean that the injected data is remote.

[wtfiwtz]

Thanks @jketema, it definitely was detected, as per the CSV result pasted there. Cheers!