SQL injections are no longer detected

[ronnn]

Hi,

I think the change introduced by 54fdb3d removes more findings than it should.

We're using njsscan in our GitLab pipelines and have a project that we use to test GitLab updates to verify that vulnerabilities (like SQL injections) are detected by our pipelines. The expected SQL injection is no longer detected.

To me it looks like the added pattern-inside restrictions will lead to a lot of undetected injections in "complex" use cases. For example, if the require calls are wrapped in another module, the pattern doesn't match anymore. Moreover, if the injection is affecting code that is using other libraries (in our case oracledb) it won't be detected.

Example:

const oracle = require('oracledb');

const param = process.argv.slice(2);

async function run() {

  let connection;

  try {
    // Get a non-pooled connection
    connection = await oracledb.getConnection(dbConfig);

    const result = await connection.query("SELECT * from foobar WHERE foo_id = " + param);

    console.log(result.rows);

  } catch (err) {
    console.error(err);
  } finally {
    if (connection) {
      try {
        // Connections should always be released when not needed
        await connection.close();
      } catch (err) {
        console.error(err);
      }
    }
  }
}

run();

njsscan 0.2.9 detects the injection, but 0.3.1 does not.

Let me know what you think.

Regards,
Ronald

[ajinabraham]

Hi @ronnn

The changes were made to avoid a lot of false positives we were seeing.
Added oracledb to the rule to cover this example.
Semgrep that we use internally cannot perform inter file tainting for cases like wrapped calls.

[ronnn]

Hi Ajin,

thank you for adding oracledb. And I see the problem with the false positives. You basically have this with all scanning tools.

In my opinion this is still not the best way to handle the false positives. If developers are relying on the result of the scans but the real injection cases are filtered out by the new patterns it's not a good design for a library that should increase security.

Adding other/new libraries (e.g. there's knex, sequelize, sqlite3... you get the idea) will never work, as you would need to monitor npm continuously. Moreover, at least from my experience, some projects are using imports/wrappers to decouple the implementation or just to add helper functions.

I would vote for removing the pattern-inside and let the users add // njsscan-ignore: node_nosqli_injection whenever needed. Better have a false positive injection that is documented to be a no-issue by adding the comment than having real injections that are not detected.

Thanks again and let me know what you think.

Regards,
Ronald

[ajinabraham]

From a SAST maintainer perspective, there is a priority on making the rules balanced and less noisy. False positives can occur, but noisy rules are inconvenience for a developer trying to integrate a security tool which adds friction to adoption. We also have community feedback on few of our rules. See: #84 and the rule node_sqli_injection received significant negative feedback which made us to QA this rule.

[ronnn]

Hi Ajin,

got you. Would you be open to a PR that adds an option to the .njsscan file that restores the more restrictive check?

Regards,
Ronald

[ajinabraham]

Can you brief how you are planning to implement this?

[ronnn]

Hi Ajin,

To summarize:

1. New option in the .njsscan file. Without the setting the default would be false. Something like:

  node_nosqli_injection-ignore-require: true

I can also do this more generic if you want so that it can be extended easily in the future:

  customize-rules:
    node_nosqli_injection:
      ignore-require: true

2. Adapt code to follow the setting similar to the changes of add severity-filter feature #70

My naming ideas are obviously open for discussions. 😅 Let me know what you think.

Regards,
Ronald

[ronnn]

Hey Ajin,

what do you think? I don't want to waste your and my time by creating a PR that wouldn't be merged anyway.

Regards.
Ronald

[ajinabraham]

Apologies for the delayed response, I got carried away due to other priorities.

I prefer something natural like

rule-configurations:
    rule-name:
        high-confidence: false

Before you start working on this, I don't think this is as straightforward as severity filter as that is a post scan feature. This has to be pre-scan feature, and the scan logic lives in libsast. This will require you to update/monkey patch libsast/semgrep to update the rule file content in memory. Requires a lot of work for what you are trying to achieve. If this is something you just need in your local setup, the easiest method will be to replace the rule file after installing njsscan.