github / github/codeql

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

Offen
#7,803 1 Kommentar 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
JS question
Vorherrschende Sprache
CodeQL
Sterne
10.1k
Forks
2.1k
Ø Merge
2 T. 15 Std.
Gemergte PRs (30 T.)
141

Beschreibung

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

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.