Repository navigation
Conversation
… using 'char*' instead of 'const char*' which meant it would not type check when attempting to use string literals as keys (if we had been using a real implementation of those containers).
This commit should be read with `?w=1`.
…nce it in models. Ideally, we should have a synthetic fields in dataflow for this instead. But this PR is large enough as it is, and this won't be a problem in real code as the member will always exist.
…ing test with missing flow through 'SetQueryParameters'.
…ue in a map is stored in an EleentContent and a pair::second content we only need to add an implicit read of pair::second. We could consider adding an implicit read of pair::first for symmetry.
…pair's constructor which has always bothered me since we dont depend on the implementations of any models in any other tests.
…in the forward pruning phase before constructing the IPA type.
… all integral types, and one from incorrectly picking the non-indirect node for a glvalue expression. While here, we also add MaD pretty-printing to this query as this will be relevant later.
…the underlying instruction is of type glvalue.
…e<->expr mapping didn't work for this case, but with the changes in this PR we can now just use asExpr. As a bonus, we now get proper result deduplication (which is handled by asExpr) which removes a duplicated result.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The changes omit assignment-return and iterator-pointee taint flows and weaken the cleartext queries’ barriers for non-character integral values.
4 open issues
What changed in this PR
Migrates C++ container and string flow modeling from handwritten QL taint models to models-as-data, improving field and indirection precision.
Changes:
- Adds associative-container, pair, and string-view summaries, and switches vector emplacement to constructor forwarding.
- Supports member typedefs in forwarding models, optimizes constructor matching, and adjusts glvalue expression-node mapping.
- Updates cleartext queries, Azure models, command-concatenation handling, and regression tests.
| File | Description |
|---|---|
| cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected | Updated as part of this pull request. |
| cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/test2.cpp | Updated as part of this pull request. |
| cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/CleartextFileWrite.qlref | Updated as part of this pull request. |
| cpp/ql/test/query-tests/Security/CWE/CWE-311/semmle/tests/CleartextFileWrite.expected | Updated as part of this pull request. |
| cpp/ql/test/query-tests/Security/CWE/CWE-078/semmle/ExecTainted/ExecTainted.expected | Updated as part of this pull request. |
| cpp/ql/test/library-tests/dataflow/taint-tests/string.cpp | Updated as part of this pull request. |
| cpp/ql/test/library-tests/dataflow/taint-tests/stl.h | Updated as part of this pull request. |
| cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected | Updated as part of this pull request. |
| cpp/ql/test/library-tests/dataflow/external-models/validatemodels.expected | Updated as part of this pull request. |
| cpp/ql/test/library-tests/dataflow/external-models/test.cpp | Updated as part of this pull request. |
| cpp/ql/test/library-tests/dataflow/external-models/sources.expected | Updated as part of this pull request. |
| cpp/ql/test/library-tests/dataflow/external-models/sinks.expected | Updated as part of this pull request. |
| cpp/ql/test/library-tests/dataflow/external-models/flow.ext.yml | Updated as part of this pull request. |
| cpp/ql/test/library-tests/dataflow/external-models/azure.cpp | Updated as part of this pull request. |
| cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql | Updated as part of this pull request. |
| cpp/ql/src/Security/CWE/CWE-311/CleartextFileWrite.ql | Updated as part of this pull request. |
| cpp/ql/src/Security/CWE/CWE-311/ClearText.qll | Updated as part of this pull request. |
| cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql | Updated as part of this pull request. |
| cpp/ql/lib/semmle/code/cpp/models/implementations/StdString.qll | Updated as part of this pull request. |
| cpp/ql/lib/semmle/code/cpp/models/implementations/StdPair.qll | Updated as part of this pull request. |
| cpp/ql/lib/semmle/code/cpp/models/implementations/StdMap.qll | Updated as part of this pull request. |
| cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/TaintTrackingUtil.qll | Updated as part of this pull request. |
| cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ExprNodes.qll | Updated as part of this pull request. |
| cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll | Updated as part of this pull request. |
| cpp/ql/lib/ext/std.vector.model.yml | Updated as part of this pull request. |
| cpp/ql/lib/ext/std.unordered_map.model.yml | Updated as part of this pull request. |
| cpp/ql/lib/ext/std.string_view.model.yml | Updated as part of this pull request. |
| cpp/ql/lib/ext/std.pair.model.yml | Updated as part of this pull request. |
| cpp/ql/lib/ext/std.map.model.yml | Updated as part of this pull request. |
| cpp/ql/lib/ext/bsl.vector.model.yml | Updated as part of this pull request. |
| cpp/ql/lib/ext/bsl.unordered_map.model.yml | Updated as part of this pull request. |
| cpp/ql/lib/ext/bsl.pair.model.yml | Updated as part of this pull request. |
| cpp/ql/lib/ext/bsl.map.model.yml | Updated as part of this pull request. |
| cpp/ql/lib/ext/azure.core.model.yml | Updated as part of this pull request. |
Review effort: Max (auto)
30 tasks
…ccept test changes.
…licated std::string declarations in 'azure.cpp' and 'bslx.cpp'.
This branch has not been deployed
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 adds MaD summaries for
std::map,std::unordered_map,std::string,std::pair, andstd::string_view.Commit-by-commit review strongly recommended!
This work brings together a bunch of PRs that has been merged over the last couple of weeks/months(/years):
Fields to defineFieldContent#20901 which fixed a performance problem that was blocking this workReturnValues #22061 which allows us to model writes intostd::strings properlyThere are a lot of moving parts here. Here are the highlights:
stdcontainer tests. These were not a problem with the old models, but they cause missing forward-related flow (since we use the argument/parameter types to identify the right constructor to forward to).std::map::emplaceconstructed a value of typestd::map<Key, Value>::value_type.stdcontainer implementations, so they obviously exist in the database.cpp/cleartext-storage-filerevealed two problems in that query (and the other CWE-311 query):chars which meant that a copying loop iterating over each char in astd::stringwould be blocked.asExprwhich meant that we tracked the reference when the sensitive variable was a reference, and otherwise we tracked the value.asExprandasIndirectExprshould have a result when the expression is a glvalue. I've added a rather long explanation for why the new behavior is what we want.--check-databases) unhappy about duplicated declarations breaking ODR. So I also refactored those tests at the very end.DCA is rather uneventful. A couple of new results:
cpp/return-c-str-of-std-stringresult is a false positive which comes from the existing query logic.cpp/non-constant-formatresult are FPs which are now removed. The sources in those query results are all allocations of internal STL buffers which we now exclude from dataflow since we have precise MaD models of the enclosing functions.Diffed query result tuple countstable are all from results now being deduplicated better:cpp/command-line-injectionones are similar to what we've seen in our query testscpp/tainted-format-stringones are because we now track indirection flow properly throughstd::map. For example we'd previously get a result for bothdataand*datahere because we conflated the pointer and the pointee when we insert into (and pull out) thestd::map, and now we only get a result for*dataas expected.