JavaScript: Add new query UnvalidatedDynamicMethodCall.#555
Conversation
…reporting. This query now only flags user-controlled property and header writes, method calls are handled by the new unsafe/unvalidated method call queries.
|
Ping @mc-semmle for doc review. I'd like to get this merged today if at all possible so it is ready for LGTM testing day. If you don't have time today I can put up a separate doc review PR later. |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
LGTM. I think this can be merged today.
The try-block comment is not critical, but a small discussion would be good.
The qhelp examples are missing the usual GOOD/BAD annotations, and ditto for OK/NOT OK for their twins in the query-tests directory.
| } | ||
|
|
||
| app.get('/perform/:action/:payload', function(req, res) { | ||
| let action = actions[req.params.action]; |
|
|
||
| app.get('/perform/:action/:payload', function(req, res) { | ||
| if (actions.has(req.params.action)) { | ||
| let action = actions.get(req.params.action); |
| if (actions.hasOwnProperty(req.params.action)) { | ||
| let action = actions[req.params.action]; | ||
| if (typeof action === 'function') { | ||
| res.end(action(req.params.payload)); |
| CalleeAsSink() { | ||
| this = invk.getCallee().flow() and | ||
| // don't flag invocations inside a try-catch | ||
| not invk.getASuccessor() instanceof CatchClause |
There was a problem hiding this comment.
This is meant to whitelist calls that guarded with a dedicated try-catch right?
The whitelist seems brittle to me, and it does not capture the "dedicatedness" very well: the check only holds if the call is the last expression in the try-block, and it holds regardless of the size of the try-block.
Could we perhaps check if the enclosing try-statement is "small" instead ?
There was a problem hiding this comment.
the check only holds if the call is the last expression in the try-block
Oh? It's meant to hold for anything inside the try block. Why does it only hold for the last expression?
This is meant to whitelist calls that guarded with a dedicated try-catch right?
I hadn't thought about it in terms of dedicatedness. It's more that the query help emphasises the possibility of the call throwing an unhandled runtime exception, and this whitelists the case where there is, in fact, a handler. It's not a complete whitelist, of course (not inter-procedural, and it gets confused by intervening finally blocks), but I think in practice it's probably good enough.
There was a problem hiding this comment.
Oh? It's meant to hold for anything inside the try block. Why does it only hold for the last expression?
Ehh, nevermind. The successor that you use is the exceptional successor. I was thinking that the ordinary successor to the last expression of a try block would be the catch-clause, but that would be silly.
I am happy with this as it is then. We can experiment with dedicatedness later.
|
GOOD/BAD annotations added. |
Promote weak crypto key from experimental
2 participants
This complements the Unsafe dynamic method access query to flag cases of method name injection that can result in a crash (as opposed to code injection). Such cases are less problematic (hence the new query has severity "warning"), but still worth flagging.
I've further restricted the "Remote property injection" query to no longer flag any dynamic method calls, so there should be no overlap in results.
Running on our default benchmarks yields good results (internal link); query runtimes look reasonable for a security query.
As discussed with @pavgust, I am making this a hotfix since it is customer-motivated and fixes what to an end-user would look like a regression from 1.18, where remote property injection results were shown by default.