Skip to content

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jun 11, 2020

For https://github.com/github/codeql-c-analysis-team/issues/88.

It looks like taint flows through the conversions with the AST taint lib, but not with the IR taint lib or DefaultTaintTracking - because only taint from a function call parameter to the return value is considered, and in the case of c_str() it comes from the object / qualifier. The issue originally came up with ExecTainted.ql which is DefaultTaintTracking.

This is not a regression as far as I'm aware.

@jbj do you think it's worth improving DefaultTaintTracking, or maybe moving the query across to semmle.code.cpp.dataflow.TaintTracking? Which of the taint libs have a long term future?

@geoffw0 geoffw0 requested a review from a team as a code owner June 11, 2020 16:55
@geoffw0 geoffw0 added the C++ label Jun 11, 2020
@rdmarsh2
Copy link
Contributor

The medium-term goal is to use semmle.code.cpp.ir.dataflow.DataFlow as the underlying implementation for everything. DefaultTaintTracking is a layer on top of that which we used to migrate the security pack queries away from their original library. I don't think we plan to do more work that's specific to DefaultTaintTracking, but it will benefit from improvements to semmle.code.cpp.ir.dataflow.DataFlow. For queries that could use more customization than DefaultTaintTracking allows, we could either use semmle.code.cpp.ir.dataflow.DataFlow directly or use semmle.code.cpp.ir.dataflow.TaintTracking.

#3587 should add those qualifier flows to all of the libraries I mentioned.

@jbj
Copy link
Contributor

jbj commented Jun 16, 2020

Since merging #3587 is imminent, I suggest we queue this PR behind it to avoid a semantic merge conflict.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 17, 2020

#3587 has been merged into master, and I've just merged master into this. The conflicts were in .expected files, but as far as I can tell are not consequential.

@jbj jbj merged commit 81d8dc1 into github:master Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants