Skip to content

C++: Add a few more models #20769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MathiasVP merged 6 commits into from
Nov 8, 2025
Merged

C++: Add a few more models #20769

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0607100
C++: Add tests with missing flow.
MathiasVP Nov 6, 2025
1d5d272
C++: Add a few more models.
MathiasVP Nov 6, 2025
2b6a75f
C++: Accept test changes.
MathiasVP Nov 6, 2025
f0da0d9
C++: Add a few more models I noticed while here.
MathiasVP Nov 6, 2025
f163d01
C++: Clean up comments.
MathiasVP Nov 7, 2025
56b9566
C++: Fix mistakes in models of internal strcat and strcpy functions.
MathiasVP Nov 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions cpp/ql/lib/ext/cctype.model.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
extensions:
- addsTo:
pack: codeql/cpp-all
extensible: summaryModel
data: # namespace, type, subtypes, name, signature, ext, input, output, kind, provenance
- ["", "", False, "tolower", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
- ["std", "", False, "tolower", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
- ["", "", False, "toupper", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
- ["std", "", False, "toupper", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
7 changes: 7 additions & 0 deletions cpp/ql/lib/ext/iconv.model.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
extensions:
- addsTo:
pack: codeql/cpp-all
extensible: summaryModel
data: # namespace, type, subtypes, name, signature, ext, input, output, kind, provenance
- ["", "", False, "iconv", "", "", "Argument[**1]", "Argument[**3]", "value", "manual"]

8 changes: 5 additions & 3 deletions cpp/ql/lib/semmle/code/cpp/models/implementations/Memcpy.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ import semmle.code.cpp.models.interfaces.Taint
import semmle.code.cpp.models.interfaces.NonThrowing

/**
* The standard functions `memcpy`, `memmove` and `bcopy`; and the gcc variant
* `__builtin___memcpy_chk`.
* The standard functions `memcpy`, `memmove` and `bcopy`; and variants such as
* `__builtin___memcpy_chk` and `__builtin___memmove_chk`.
*/
private class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffectFunction,
AliasFunction, NonCppThrowingFunction
Expand All @@ -27,7 +27,9 @@ private class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffect
// bcopy(src, dest, num)
// mempcpy(dest, src, num)
// memccpy(dest, src, c, n)
this.hasGlobalName(["bcopy", mempcpy(), "memccpy", "__builtin___memcpy_chk"])
this.hasGlobalName([
"bcopy", mempcpy(), "memccpy", "__builtin___memcpy_chk", "__builtin___memmove_chk"
])
}

/**
Expand Down
5 changes: 3 additions & 2 deletions cpp/ql/lib/semmle/code/cpp/models/implementations/Memset.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ private class MemsetFunctionModel extends ArrayFunction, DataFlowFunction, Alias
this.hasGlobalOrStdName("wmemset")
or
this.hasGlobalName([
bzero(), "__builtin_memset", "__builtin_memset_chk", "RtlZeroMemory", "RtlSecureZeroMemory"
bzero(), "__builtin_memset", "__builtin_memset_chk", "__builtin___memset_chk",
"RtlZeroMemory", "RtlSecureZeroMemory"
])
}

Expand All @@ -32,7 +33,7 @@ private class MemsetFunctionModel extends ArrayFunction, DataFlowFunction, Alias
or
this.hasGlobalOrStdName("wmemset")
or
this.hasGlobalName(["__builtin_memset", "__builtin_memset_chk"])
this.hasGlobalName(["__builtin_memset", "__builtin_memset_chk", "__builtin___memset_chk"])
) and
result = 1
}
Expand Down
10 changes: 8 additions & 2 deletions cpp/ql/lib/semmle/code/cpp/models/implementations/Strcat.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid
"_mbsncat", // _mbsncat(dst, src, max_amount)
"_mbsncat_l", // _mbsncat_l(dst, src, max_amount, locale)
"_mbsnbcat", // _mbsnbcat(dest, src, count)
"_mbsnbcat_l" // _mbsnbcat_l(dest, src, count, locale)
"_mbsnbcat_l", // _mbsnbcat_l(dest, src, count, locale)
"__builtin___strcat_chk", // __builtin___strcat_chk (dest, src, magic);
"__builtin___strncat_chk" // __builtin___strncat_chk (dest, src, max_amount, magic);
])
}

Expand All @@ -56,7 +58,11 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid

override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
(
this.getName() = ["strncat", "wcsncat", "_mbsncat", "_mbsncat_l"] and
this.getName() =
[
"strncat", "wcsncat", "_mbsncat", "_mbsncat_l", "__builtin___strncat_chk",
"__builtin___strcat_chk"
Copy link
Copy Markdown
Contributor

@jketema jketema Nov 7, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@MathiasVP MathiasVP Nov 7, 2025

Choose a reason for hiding this comment

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

🤦 You're right!

Copy link
Copy Markdown
Contributor Author

@MathiasVP MathiasVP Nov 7, 2025

Choose a reason for hiding this comment

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

Fixed in 56b9566

] and
input.isParameter(2)
or
this.getName() = ["_mbsncat_l", "_mbsnbcat_l"] and
Expand Down
12 changes: 10 additions & 2 deletions cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid
"_mbsnbcpy", // _mbsnbcpy(dest, src, max_amount)
"stpcpy", // stpcpy(dest, src)
"stpncpy", // stpncpy(dest, src, max_amount)
"strlcpy" // strlcpy(dst, src, dst_size)
"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)
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
"__builtin___stpncpy_chk", // __builtin___stpncpy_chk(dest, src, max_amount, magic)
"__builtin___stpncpy_chk", // __builtin___stpncpy_chk(dest, src, max_amount, magic);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@jketema jketema Nov 6, 2025

Choose a reason for hiding this comment

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

I think we just want to get rid of the semi-colons, similarly for strcat.

Copy link
Copy Markdown
Contributor Author

@MathiasVP MathiasVP Nov 7, 2025

Choose a reason for hiding this comment

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

Fixed in f163d01

"__builtin___strncpy_chk" // __builtin___strncpy_chk (dest, src, max_amount, magic);
])
or
(
Expand Down Expand Up @@ -69,7 +73,11 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid
if this.isSVariant()
then result = 1
else (
this.getName().matches(["%ncpy%", "%nbcpy%", "%xfrm%", "strlcpy"]) and
this.getName()
.matches([
"%ncpy%", "%nbcpy%", "%xfrm%", "strlcpy", "__builtin___strcpy_chk",
"__builtin___stpcpy_chk", "__builtin___stpncpy_chk", "__builtin___strncpy_chk"
]) and
Copy link
Copy Markdown
Contributor

@jketema jketema Nov 7, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@MathiasVP MathiasVP Nov 7, 2025

Choose a reason for hiding this comment

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

Uh oh, you're right. Looks like this last commit went slightly too fast 😅 I'll fix this!

Copy link
Copy Markdown
Contributor Author

@MathiasVP MathiasVP Nov 7, 2025

Choose a reason for hiding this comment

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

Fixed in 56b9566

result = 2
)
}
Expand Down
20 changes: 20 additions & 0 deletions cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7988,6 +7988,26 @@ WARNING: module 'TaintTracking' has been deprecated and may be removed in future
| taint.cpp:841:21:841:35 | call to indirect_source | taint.cpp:843:16:843:17 | fp | |
| taint.cpp:842:11:842:12 | ref arg fp | taint.cpp:843:16:843:17 | fp | |
| taint.cpp:842:15:842:16 | | taint.cpp:842:11:842:12 | ref arg fp | TAINT |
| taint.cpp:851:10:851:15 | call to source | taint.cpp:852:18:852:18 | s | |
| taint.cpp:851:10:851:15 | call to source | taint.cpp:854:18:854:18 | s | |
| taint.cpp:852:10:852:16 | call to toupper | taint.cpp:853:7:853:7 | u | |
| taint.cpp:854:10:854:16 | call to tolower | taint.cpp:855:7:855:7 | l | |
| taint.cpp:861:24:861:27 | size | taint.cpp:866:16:866:19 | size | |
| taint.cpp:862:12:862:26 | call to indirect_source | taint.cpp:866:12:866:12 | s | |
| taint.cpp:863:7:863:9 | out | taint.cpp:864:12:864:14 | out | |
| taint.cpp:864:12:864:14 | out | taint.cpp:866:23:866:23 | p | |
| taint.cpp:864:12:864:14 | out | taint.cpp:867:8:867:8 | p | |
| taint.cpp:865:9:865:16 | size_out | taint.cpp:866:27:866:34 | size_out | |
| taint.cpp:866:11:866:12 | ref arg & ... | taint.cpp:866:12:866:12 | s [inner post update] | |
| taint.cpp:866:12:866:12 | s | taint.cpp:866:11:866:12 | & ... | |
| taint.cpp:866:15:866:19 | ref arg & ... | taint.cpp:866:16:866:19 | size [inner post update] | |
| taint.cpp:866:16:866:19 | size | taint.cpp:866:15:866:19 | & ... | |
| taint.cpp:866:22:866:23 | ref arg & ... | taint.cpp:866:23:866:23 | p [inner post update] | |
| taint.cpp:866:22:866:23 | ref arg & ... | taint.cpp:867:8:867:8 | p | |
| taint.cpp:866:23:866:23 | p | taint.cpp:866:22:866:23 | & ... | |
| taint.cpp:866:26:866:34 | ref arg & ... | taint.cpp:866:27:866:34 | size_out [inner post update] | |
| taint.cpp:866:27:866:34 | size_out | taint.cpp:866:26:866:34 | & ... | |
| taint.cpp:867:8:867:8 | p | taint.cpp:867:7:867:8 | * ... | TAINT |
| thread.cpp:10:27:10:27 | s | thread.cpp:10:27:10:27 | s | |
| thread.cpp:10:27:10:27 | s | thread.cpp:11:8:11:8 | s | |
| thread.cpp:14:26:14:26 | s | thread.cpp:15:8:15:8 | s | |
Expand Down
23 changes: 23 additions & 0 deletions cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,4 +842,27 @@ int f7(void)
fprintf(fp, "");
indirect_sink(fp); // $ ir MISSING: ast
return 0;
}

int toupper(int);
int tolower(int);

void test_toupper_and_tolower() {
int s = source();
int u = toupper(s);
sink(u); // $ ir MISSING: ast
int l = tolower(s);
sink(l); // $ ir MISSING: ast
}

typedef int iconv_t;
size_t iconv(iconv_t cd, char **, size_t *, char **, size_t *);

void test_iconv(size_t size) {
char* s = indirect_source();
char out[10];
char* p = out;
size_t size_out;
iconv(0, &s, &size, &p, &size_out);
sink(*p); // $ ast,ir
}
Loading
Loading