[javascript] CodeQL query to detect Log Injection

[dellalibera]

Log Injection query is available in c# query but it is not available in javascript query.
I created a query to detect a log injection vulnerability in javascript code.

This query addresses the scenario where user controlled input is logged-as is.

Examples of scenarios detected:

const http = require('http');
const url = require('url');

const server = http.createServer((req, res) => {
    let q = url.parse(req.url, true);

    let username = q.query.username;
    console.info(`[INFO] User: ${username}`); // BAD

})

or

const http = require('http');
const url = require('url');

const logger = {
    log: console.log
}

const another_logger = console.log

const server = http.createServer((req, res) => {
    let q = url.parse(req.url, true);

    let username = q.query.username;
    logger.log('[INFO] User:', username); // BAD
    another_logger('[INFO] User:', username); // BAD
})

Any feedback is welcome.
I hope this will help.

[asgerf]

Hi @dellalibera, thanks for the contribution! We'll take a closer look at the query next week.

[esbena]

Thank you for another contribution.
Since we already have the query for C#, I think this should be straight-forward to merge. I have two super-minor comments for you to consider.

[esbena]

This is mostly a comment.

This last conjunct should cover the two cases above if

codeql/javascript/ql/src/semmle/javascript/frameworks/Logging.qll

Line 57 in 7a5aae7

this = console().getAMethodCall(name)

was console().getAMemberCall(name).

(I think console.log used to require a binding with Function.prototype.bind in order for it to work when invoked as a non-method call, but that appears to have changed now).

[dellalibera]

Thank you for your review and feedback.

Yes, if it would be console().getAMemberCall(name) I could avoid the first case
this = any(ConsoleSource console).getAMemberCall(getAStandardLoggerMethodName())

but not the second one.

The second check

      exists(DataFlow::SourceNode node, string propName |
        any(ConsoleSource console).getAPropertyRead() = node.getAPropertySource(propName) and
        this = node.getAPropertyRead(propName).getACall()
      )

detects cases where the console.log (for example) is assigned to a property of another object.
For example, consider the following case:

const my_logger = {
    log: console.log
}
my_logger.log("hi")

Though the Logging.dll will be updated to console().getAMemberCall(name), I will still miss the above case (please, do not hesitate to correct me if I am wrong).

[esbena]

I have opened #3767 to address the first case, so lets drop that case in this PR.

For the second case, we have a bit better machinery for tracking special values of interest through properties and even calls ("type tracking"), but lets just keep the second case as is.
Let me know if you want to try out the "type tracking" feature later.

[dellalibera]

Sorry for the late reply and thank you again for your feedback.
I am really happy that I inspire the other PR. Since now it is merged, I remove the first case, and leave as is the second one.

Let me know if there is something else I can do.

About the "type tracking" feature is something that I will investigate.

[esbena]

Thanks. I will merge when the tests go green.

[dellalibera]

Thanks @esbena. I am really happy that this PR is merged!