github / github/codeql

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

Đang mở
#7,803 1 bình luận 0 reaction 0 người được giao Xem trên GitHub
JS question
Ngôn ngữ chính
CodeQL
Star
10.1k
Fork
2.1k
Merge trung bình
2 ngày 15 giờ
Pull request đã merge (30 ngày)
141

Mô tả

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

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.