Description
I have multiple questions/reports.
1.) The following code has the Unvalidated Dynamic Call vulnerability however it is missed by codeql.
var actions = new Map();
actions.put("play", function(data) {
// ...
});
actions.put("pause", function(data) {
// ...
});
app.get('/perform/:action/:payload2', function(req, res) {
let action = actions.get(req.params.action);
res.end(action(req.params.payload));
});This seems to be because of the missing model for the .get function of Map.
To handle the object indexing case, based occurrence of PropRead, the isAdditionalFlowStep predicate of configuration assigns tgtlabel to MaybeFromProto or MaybeNonFunction. However, a similar case is missing for Map .get method. The following is a (perhaps dirty and potentially wrong) way to implement the missing case that should be added to the isAdditionalTaintStep
("get" = dst.getAstNode().(MethodCallExpr).getMethodName() and
src.asExpr() = dst.getAstNode().(MethodCallExpr).getAnArgument() and
srclabel.isTaint() and dstlabel instanceof MaybeNonFunction)2.) I wanted to clarify whether the following code is expected to be safe without the FunctionCheck sanitizer guard. Even after adding the .get model, the following code still doesn't get reported as vulnerable
var express = require('express');
var app = express();
var actions = new Map();
actions.put("play", function play(data) {
// ...
});
actions.put("pause", function pause(data) {
// ...
});
app.get('/perform/:action/:payload', function(req, res) {
if (actions.has(req.params.action)) {
let action = actions.get(req.params.action);
// GOOD: `action` is either the `play` or the `pause` function from above
res.end(action(req.params.payload));
} else {
res.end("Unsupported action.");
}
});I suspect it might be because WhitelistContainmentCallSanitizer below sanitizes both labels. Is that correct?