JS: add new query: js/unclosed-stream #3682
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
erik-krogh reviewed Jun 11, 2020
erik-krogh previously approved these changes Jun 12, 2020
esbena force-pushed the js/hanging-stream branch from e2725dd to 445c294 Compare esbena added the Awaiting evaluation 
asgerf reviewed Jun 18, 2020
| escape = any(DataFlow::CallNode c).getAnArgument() | ||
| ) and | ||
| // no return value | ||
| (this.getFunction() instanceof ArrowFunctionExpr or not exists(this.getAReturn())) and |
Contributor
There was a problem hiding this comment.
We could exclude arrow functions with an explicit return, like () => { ...; return x }
Suggested change
| (this.getFunction() instanceof ArrowFunctionExpr or not exists(this.getAReturn())) and | |
| (this.getFunction().getBody() instanceof Expr or not exists(this.getAReturn())) and |
Comment on lines +10 to +12
| Callback-based input streams generate calls until they are empty or | ||
| closed explicitly, and they will take up system resources until | ||
| that point in time. |
Contributor
There was a problem hiding this comment.
If we rephrase this in terms of events, we can avoid inventing non-standard terms:
Suggested change
| Callback-based input streams generate calls until they are empty or | |
| closed explicitly, and they will take up system resources until | |
| that point in time. | |
| Input streams in Node.js emit events until they are empty or | |
| closed explicitly, and they will take up system resources until | |
| that point in time. |
Contributor Author
| Hmm. I am not sure we should merge this yet, it should be generalized more first. There are simply no remotely interesting results anywhere on LGTM.com to make it worthwhile the computation time at the moment. |
Contributor
| Please retarget on |
esbena force-pushed the js/hanging-stream branch from d4cc58f to e592e2b Compare 
Contributor Author
| (rebased on top of #4996) |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
CVE-2019-10742: TP/TN
This query is a bit heuristic, but it seems to work out in practice (as in: it only flags the CVE above, and nothing else).
The idea of the query is to look for stream handlers that can be excepted to be invoked as long as there is more data in the stream, once we have such a handler, we check if it "terminates" its enclosing promise without also closing the stream.
Such a termination is problematic since once the enclosing promise is terminated, it seems unlikely that something can close the stream as the stream was "created inside" the promise.
Caveat: this is only truly problematic when an attacker keeps feeding data into the stream, which is exactly why the CVE was assigned.
TODOs later:
AsyncTerminatableFunctionto handle callback stylegetACalleewith the one in JS: add initial version of ServerCrash.ql #3672