github / github/codeql

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

オープン
#7,803 コメント 1 件 リアクション 0 件 担当者 0 名 GitHub で見る
JS question
主要言語
CodeQL
スター
10.1k
フォーク
2.1k
平均マージ
2日 15時間
マージ済み PR(30日)
141

説明

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

コントリビューションガイド

コントリビューションガイドを開く

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。