github / github/codeql

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

Abierto
#7,803 1 comentario 0 reacciones 0 asignados Ver en GitHub
JS question
Lenguaje dominante
CodeQL
Estrellas
10.1k
Forks
2.1k
Merge medio
2 d 15 h
PR fusionados (30 d)
141

Descripción

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

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.