github / github/codeql

Missing model for `.get()` function for `Map` in Unvalidated Dynamic Call

Aperta
#7,803 1 commento 0 reazioni 0 assegnatari Vedi su GitHub
JS question
Lingua principale
CodeQL
Stelle
10.1k
Fork
2.1k
Merge medio
2g 15h
PR unite (30g)
141

Descrizione

I have multiple questions/reports.
1.) The following code has the Unvalidated Dynamic Call vulnerability however it is missed by codeql.
```js
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`
```codeql
("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
```js
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?
https://github.com/github/codeql/blob/15c1ce722a4bcaa892e982c34ac28cd3d77e4a11/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll#L1065

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.