JS: add new query: js/unclosed-stream #3682
Draft
Conversation
This file contains 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 |
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. |
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. |
|
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. |
|
Please retarget on |
esbena
force-pushed
the
js/hanging-stream
branch
from
d4cc58f to
e592e2b
Compare
|
(rebased on top of #4996) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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