JS: torrent content as a remote flow source#2324
Merged
Merged
Conversation
max-schaefer
approved these changes
Nov 14, 2019
Contributor
max-schaefer
left a comment
There was a problem hiding this comment.
LGTM, assuming the evaluation works out (but I don't see why it wouldn't).
Contributor
Author
|
Uninteresting evaluation. Good to go. |
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
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.
This PR makes us treat most content of torrents parsed by
parse-torrentas untrusted. Such torrent content was the taint source in CVE-2019-15782 (we do not flag this vulnerability yet, we are missing a flow step elsewhere).Implementationwise, I would have liked
parseTorrent(...)to be a labelledRemoteFlowSource, and then let the flow label mechanism ensure that we only propagate through certain properties of the parsed torrent, but that requires unreasonable adjustments in allConfigurations.Instead, I have used type tracking to track the parsed torrent, and then used certain property reads of that tracked torrent as ordinary
RemoteFlowSources. This leads to sub-optimal sources in the path-problem alerts, but it is the same pattern as the one we use forRequestInputAccess: we do not provide a link to the request object itself, but rather a use of it.Generally speaking, this new source is related to the
js/zipslipsources where we model that something is expected to be provided by external parties and thus should not be trusted. We should investigate such sources more thoroughly, and perhaps even deprecatejs/zipslipin favor of ajs/path-injectionwith zip-file content as aRemoteFlowSource.Evaluation
parse-torrentas a dependency.