Shared: restrict flow after using implicit read #17262
Merged
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
asgerf
force-pushed
the
shared/implicit-read
branch
from
d550718 to
5879490
Compare
This was referenced Aug 22, 2024
Merged
asgerf
force-pushed
the
shared/implicit-read
branch
from
5879490 to
babac33
Compare
asgerf
force-pushed
the
shared/implicit-read
branch
from
babac33 to
781e753
Compare
This reveals that some tests were passing for the wrong reasons. See github#17275
The 'first' field is seen as a TaintInheritingContent, which means any read step for 'first' becomes a taint step too. This type of taint step does not permit an implicit read before it, because it wasn't contributed by a configuration. So there is no way for the taint to get out of the collection content before the taint step through '.first'. The test previously passed because an implicit read at once of the earlier sinks could follow use-use flow down to the receiver of .first, allowing it to escape the collection content.
asgerf
force-pushed
the
shared/implicit-read
branch
from
781e753 to
8df7fbf
Compare
aschackmull
approved these changes
Aug 23, 2024
|
Are the dca runs up-to-date and ok? If so, then this looks ready to merge. |
|
Yes, the DCA runs are up to date. (The C++ report is messed up due to a failure in JavaScriptCore, but that's also failing on nightly) |
asgerf
added a commit
to asgerf/codeql
that referenced
this pull request
Aug 27, 2024
Shows the effect of github#17262
|
This will need to be backed out. The C++ DCA failure on Wireshark is a serious regression where we now get a timeout. |
This was referenced Aug 27, 2024
geoffw0
reviewed
Aug 28, 2024
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.
After using an implicit read, flow can now only terminate in a sink or continue along edges contributed by
Config:isAdditionalFlowStep, but not via other kinds of steps, like simple local flow steps.This is achieved by treating the implicit read node as a sink if its original node was a sink, and copying any relevant outgoing steps from the original node onto the implicit read node.