Skip to content

C++: Add Strsafe.h models#21764

Merged
MathiasVP merged 5 commits intomainfrom
add-strsafe.h-models
Apr 28, 2026
Merged

C++: Add Strsafe.h models#21764
MathiasVP merged 5 commits intomainfrom
add-strsafe.h-models

Conversation

@MathiasVP
Copy link
Copy Markdown
Contributor

Pretty simple stuff. We just add source and flow summary models for https://learn.microsoft.com/en-us/windows/win32/menurc/strsafe-ovw

Copilot AI review requested due to automatic review settings April 28, 2026 09:54
@MathiasVP MathiasVP requested a review from a team as a code owner April 28, 2026 09:54
@github-actions github-actions Bot added the C++ label Apr 28, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sourceModel entries for String*Gets* APIs and summaryModel entries for String*Copy*, String*Cat*, and String*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 use Argument[*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] and Argument[*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 the va_list/argList parameter. That will miss taint flowing into the destination buffer via values supplied through the va_list. Consider adding a separate model that also propagates from the va_list argument (e.g. Argument[3] for StringCchVPrintf* and Argument[6] for StringCchVPrintfEx*) to Argument[*0], similar to how other *V formatting 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

Comment thread cpp/ql/lib/ext/Strsafe.model.yml
Comment thread cpp/ql/lib/ext/Strsafe.model.yml
- ["", "", 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"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 8 here is just a reasonable arbitrary upper bound, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have an internal issue for that?

Copy link
Copy Markdown
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MathiasVP MathiasVP merged commit 1ba9601 into main Apr 28, 2026
15 of 16 checks passed
@MathiasVP MathiasVP deleted the add-strsafe.h-models branch April 28, 2026 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants