Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds taint flow models for character conversion functions (toupper, tolower) and the iconv function, along with support for additional GCC builtin variants of string manipulation functions. The changes improve taint tracking capabilities for these commonly used functions.
- Adds YAML models for
toupper,tolower, andiconvfunctions to enable taint tracking - Includes test cases demonstrating taint flow through these functions
- Extends existing string function models to include GCC builtin
__builtin___*_chkvariants
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cpp/ql/lib/ext/cctype.model.yml | Defines taint flow models for toupper/tolower functions |
| cpp/ql/lib/ext/iconv.model.yml | Defines value flow model for iconv function |
| cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp | Adds test cases for toupper, tolower, and iconv taint tracking |
| cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected | Auto-generated expected test output |
| cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected | Auto-generated expected test output |
| cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll | Adds support for GCC builtin strcpy/stpcpy/strncpy/stpncpy variants |
| cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll | Adds support for GCC builtin strcat/strncat variants |
| cpp/ql/lib/semmle/code/cpp/models/implementations/Memset.qll | Adds support for GCC builtin memset variant |
| cpp/ql/lib/semmle/code/cpp/models/implementations/Memcpy.qll | Adds support for GCC builtin memmove variant and updates documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "strlcpy", // strlcpy(dst, src, dst_size) | ||
| "__builtin___strcpy_chk", // __builtin___strcpy_chk (dest, src, magic); | ||
| "__builtin___stpcpy_chk", // __builtin___stpcpy_chk (dest, src, magic); | ||
| "__builtin___stpncpy_chk", // __builtin___stpncpy_chk(dest, src, max_amount, magic) |
There was a problem hiding this comment.
The comment for __builtin___stpncpy_chk is missing a semicolon at the end, unlike the other builtin function comments on lines 40, 41, and 43. Add a semicolon for consistency.
| "__builtin___stpncpy_chk", // __builtin___stpncpy_chk(dest, src, max_amount, magic) | |
| "__builtin___stpncpy_chk", // __builtin___stpncpy_chk(dest, src, max_amount, magic); |
There was a problem hiding this comment.
I think we just want to get rid of the semi-colons, similarly for strcat.
jketema
left a comment
jketema
left a comment
There was a problem hiding this comment.
This does not seem to be quite right. Comments below.
| this.getName() = | ||
| [ | ||
| "strncat", "wcsncat", "_mbsncat", "_mbsncat_l", "__builtin___strncat_chk", | ||
| "__builtin___strcat_chk" |
There was a problem hiding this comment.
I'm not sure what __builtin___strcat_chk (without the n) is doing here. This seems to be from taint flow from the numeric argument of strncat.
| this.getName() | ||
| .matches([ | ||
| "%ncpy%", "%nbcpy%", "%xfrm%", "strlcpy", "__builtin___strcpy_chk", | ||
| "__builtin___stpcpy_chk", "__builtin___stpncpy_chk", "__builtin___strncpy_chk" | ||
| ]) and |
There was a problem hiding this comment.
It seems that __builtin___stpncpy_chk and __builtin___strncpy_chk are already covered by %ncpy%.
The __builtin___strcpy_chk and __builtin___stpcpy_chk functions don't seem to belong here, as they don't have a size argument.
There was a problem hiding this comment.
Uh oh, you're right. Looks like this last commit went slightly too fast 😅 I'll fix this!
|
DCA looks good. Results changes all look like good changes 🎉 |
2 participants
Just a couple more models that I encountered a need for while doing some Microsoft work