Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds CodeQL C++ dataflow models for Windows Strsafe.h “safe string” APIs, along with taint/source-sink test coverage updates to validate the new models.
Changes:
- Added a new extension model file defining
sourceModelentries forString*Gets*APIs andsummaryModelentries forString*Copy*,String*Cat*, andString*Printf*families. - Extended C++ taint and source/sink library tests with declarations and calls to Strsafe APIs.
- Updated expected test outputs to reflect the new modeled signatures/flows.
Show a summary per file
| File | Description |
|---|---|
| cpp/ql/lib/ext/Strsafe.model.yml | Adds Strsafe sourceModel and summaryModel entries. |
| cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp | Adds Strsafe function declarations and taint-propagation test cases. |
| cpp/ql/test/library-tests/dataflow/source-sink-tests/sources-and-sinks.cpp | Adds Strsafe StringCchGets* local source tests. |
| cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected | Updates expected signature extraction output for new APIs. |
| cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected | Updates expected local taint flow output for new tests. |
Copilot's findings
Comments suppressed due to low confidence (2)
cpp/ql/lib/ext/Strsafe.model.yml:82
- The
String*Cch/Cb*PrintfEx*models useArgument[*5..11], which again only captures dereferenced pointer arguments and truncates the variadic list. This can miss taint flowing from non-pointer variadic values (e.g. tainted integers) and any arguments beyond the covered range. Consider splitting this into format (Argument[*5]) plus both by-value and dereferenced ranges for the variadic args (e.g.Argument[6..13]andArgument[*6..13]).
# StringCchPrintfEx: (pszDest, cchDest, ppszDestEnd, pcchRemaining, dwFlags, pszFormat, ...)
- ["", "", False, "StringCchPrintfExA", "", "", "Argument[*5..11]", "Argument[*0]", "taint", "manual"]
- ["", "", False, "StringCchPrintfExW", "", "", "Argument[*5..11]", "Argument[*0]", "taint", "manual"]
# StringCbPrintfEx: (pszDest, cbDest, ppszDestEnd, pcbRemaining, dwFlags, pszFormat, ...)
- ["", "", False, "StringCbPrintfExA", "", "", "Argument[*5..11]", "Argument[*0]", "taint", "manual"]
- ["", "", False, "StringCbPrintfExW", "", "", "Argument[*5..11]", "Argument[*0]", "taint", "manual"]
cpp/ql/lib/ext/Strsafe.model.yml:94
- The
String*Cch/Cb*VPrintf*and*VPrintfEx*models currently only take taint from the format string (Argument[*2]/Argument[*5]) and ignore theva_list/argListparameter. That will miss taint flowing into the destination buffer via values supplied through theva_list. Consider adding a separate model that also propagates from theva_listargument (e.g.Argument[3]forStringCchVPrintf*andArgument[6]forStringCchVPrintfEx*) toArgument[*0], similar to how other*Vformatting APIs are modeled.
# StringCchVPrintf: (pszDest, cchDest, pszFormat, argList)
- ["", "", False, "StringCchVPrintfA", "", "", "Argument[*2]", "Argument[*0]", "taint", "manual"]
- ["", "", False, "StringCchVPrintfW", "", "", "Argument[*2]", "Argument[*0]", "taint", "manual"]
# StringCbVPrintf: (pszDest, cbDest, pszFormat, argList)
- ["", "", False, "StringCbVPrintfA", "", "", "Argument[*2]", "Argument[*0]", "taint", "manual"]
- ["", "", False, "StringCbVPrintfW", "", "", "Argument[*2]", "Argument[*0]", "taint", "manual"]
# StringCchVPrintfEx: (pszDest, cchDest, ppszDestEnd, pcchRemaining, dwFlags, pszFormat, argList)
- ["", "", False, "StringCchVPrintfExA", "", "", "Argument[*5]", "Argument[*0]", "taint", "manual"]
- ["", "", False, "StringCchVPrintfExW", "", "", "Argument[*5]", "Argument[*0]", "taint", "manual"]
# StringCbVPrintfEx: (pszDest, cbDest, ppszDestEnd, pcbRemaining, dwFlags, pszFormat, argList)
- ["", "", False, "StringCbVPrintfExA", "", "", "Argument[*5]", "Argument[*0]", "taint", "manual"]
- ["", "", False, "StringCbVPrintfExW", "", "", "Argument[*5]", "Argument[*0]", "taint", "manual"]
- Files reviewed: 6/6 changed files
- Comments generated: 2
jketema
reviewed
Apr 28, 2026
| - ["", "", False, "StringCbCatNExA", "", "", "Argument[*2]", "Argument[*0]", "taint", "manual"] | ||
| - ["", "", False, "StringCbCatNExW", "", "", "Argument[*2]", "Argument[*0]", "taint", "manual"] | ||
| # StringCchPrintf: (pszDest, cchDest, pszFormat, ...) | ||
| - ["", "", False, "StringCchPrintfA", "", "", "Argument[*2..8]", "Argument[*0]", "taint", "manual"] |
Contributor
There was a problem hiding this comment.
The 8 here is just a reasonable arbitrary upper bound, right?
Contributor
Author
There was a problem hiding this comment.
Correct. We should probably have a better way of doing this in MaD (i.e., some syntax for specifying "any vararg argument", but I haven't bothered implementing something for it yet.
Contributor
There was a problem hiding this comment.
Maybe we should have an internal issue for that?
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.
Pretty simple stuff. We just add source and flow summary models for https://learn.microsoft.com/en-us/windows/win32/menurc/strsafe-ovw