-
Notifications
You must be signed in to change notification settings - Fork 2k
C++: Model secure versions of scanf as flow sources
#21856
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
2902a19
16235d7
5add24b
5f10a88
19781e5
e6c5f94
25d2039
a33af09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * Added flow source models for `scanf_s` and related functions. | ||
| * Added a `Call` column to `LocalFlowSourceFunction::hasLocalFlowSource` and `RemoteFlowSourceFunction::hasRemoteFlowSource`. The old predicates without a `Call` column continue to be supported. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,15 @@ abstract class ScanfFunction extends Function { | |
| * (rather than a `char*`). | ||
| */ | ||
| predicate isWideCharDefault() { exists(this.getName().indexOf("wscanf")) } | ||
|
|
||
| /** Holds if this is one of the `scanf_s` variants. */ | ||
| predicate isSVariant() { | ||
| exists(string name | name = this.getName() | | ||
| name.matches("%\\_s") | ||
| or | ||
| name.matches("%\\_s\\_l") | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -34,8 +43,12 @@ class Scanf extends ScanfFunction instanceof TopLevelFunction { | |
| Scanf() { | ||
| this.hasGlobalOrStdOrBslName("scanf") or // scanf(format, args...) | ||
| this.hasGlobalOrStdOrBslName("wscanf") or // wscanf(format, args...) | ||
| this.hasGlobalOrStdOrBslName("scanf_s") or // scanf_s(format, args...) | ||
| this.hasGlobalOrStdOrBslName("wscanf_s") or // wscanf_s(format, args...) | ||
| this.hasGlobalName("_scanf_l") or // _scanf_l(format, locale, args...) | ||
| this.hasGlobalName("_wscanf_l") | ||
| this.hasGlobalName("_wscanf_l") or // _wscanf_l(format, locale, args...) | ||
| this.hasGlobalName("_scanf_s_l") or // _scanf_s_l(format, locale, args...) | ||
| this.hasGlobalName("_wscanf_s_l") // _wscanf_s_l(format, locale, args...) | ||
| } | ||
|
|
||
| override int getInputParameterIndex() { none() } | ||
|
|
@@ -50,8 +63,12 @@ class Fscanf extends ScanfFunction instanceof TopLevelFunction { | |
| Fscanf() { | ||
| this.hasGlobalOrStdOrBslName("fscanf") or // fscanf(src_stream, format, args...) | ||
| this.hasGlobalOrStdOrBslName("fwscanf") or // fwscanf(src_stream, format, args...) | ||
| this.hasGlobalOrStdOrBslName("fscanf_s") or // fscanf_s(src_stream, format, args...) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could add
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in a33af09 |
||
| this.hasGlobalOrStdOrBslName("fwscanf_s") or // fwscanf_s(src_stream, format, args...) | ||
| this.hasGlobalName("_fscanf_l") or // _fscanf_l(src_stream, format, locale, args...) | ||
| this.hasGlobalName("_fwscanf_l") | ||
| this.hasGlobalName("_fwscanf_l") or // _fwscanf_l(src_stream, format, locale, args...) | ||
| this.hasGlobalName("_fscanf_s_l") or // _fscanf_s_l(src_stream, format, locale, args...) | ||
| this.hasGlobalName("_fwscanf_s_l") // _fwscanf_s_l(src_stream, format, locale, args...) | ||
| } | ||
|
|
||
| override int getInputParameterIndex() { result = 0 } | ||
|
|
@@ -66,8 +83,12 @@ class Sscanf extends ScanfFunction instanceof TopLevelFunction { | |
| Sscanf() { | ||
| this.hasGlobalOrStdOrBslName("sscanf") or // sscanf(src_stream, format, args...) | ||
| this.hasGlobalOrStdOrBslName("swscanf") or // swscanf(src, format, args...) | ||
| this.hasGlobalOrStdOrBslName("sscanf_s") or // sscanf_s(src, format, args...) | ||
| this.hasGlobalOrStdOrBslName("swscanf_s") or // swscanf_s(src, format, args...) | ||
| this.hasGlobalName("_sscanf_l") or // _sscanf_l(src, format, locale, args...) | ||
| this.hasGlobalName("_swscanf_l") | ||
| this.hasGlobalName("_swscanf_l") or // _swscanf_l(src, format, locale, args...) | ||
| this.hasGlobalName("_sscanf_s_l") or // _sscanf_s_l(src, format, locale, args...) | ||
| this.hasGlobalName("_swscanf_s_l") // _swscanf_s_l(src, format, locale, args...) | ||
| } | ||
|
|
||
| override int getInputParameterIndex() { result = 0 } | ||
|
|
@@ -97,6 +118,14 @@ class Snscanf extends ScanfFunction instanceof TopLevelFunction { | |
| int getInputLengthParameterIndex() { result = 1 } | ||
| } | ||
|
|
||
| private predicate isCharLike(Type t) { t instanceof CharType or t instanceof Wchar_t } | ||
|
|
||
| private predicate isStringLike(Type t) { | ||
| isCharLike(t.(PointerType).getBaseType()) | ||
| or | ||
| isCharLike(t.(ArrayType).getBaseType()) | ||
| } | ||
|
|
||
| /** | ||
| * A call to one of the `scanf` functions. | ||
| */ | ||
|
|
@@ -130,14 +159,40 @@ class ScanfFunctionCall extends FunctionCall { | |
| */ | ||
| predicate isWideCharDefault() { this.getScanfFunction().isWideCharDefault() } | ||
|
|
||
| bindingset[this, k] | ||
| pragma[inline_late] | ||
| private predicate isSizeArgument(int k) { | ||
| // The first vararg is never the size argument since a size argument must | ||
| // always follow a string buffer argument. | ||
| k > 0 and | ||
| isStringLike(this.getArgument(this.getScanfFunction().getNumberOfParameters() + k - 1) | ||
| .getUnspecifiedType()) | ||
| } | ||
|
MathiasVP marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * Gets the output argument at position `n` in the vararg list of this call. | ||
| * | ||
| * The range of `n` is from `0` to `this.getNumberOfOutputArguments() - 1`. | ||
|
MathiasVP marked this conversation as resolved.
|
||
| */ | ||
| Expr getOutputArgument(int n) { | ||
| result = this.getArgument(this.getTarget().getNumberOfParameters() + n) and | ||
| n >= 0 | ||
| exists(ScanfFunction target | target = this.getScanfFunction() | | ||
| // If this is an S variant then every string buffer argument has a | ||
| // corresponding size argument immediately following it, so we need to | ||
| // skip over those size arguments when counting the output arguments. | ||
| if target.isSVariant() | ||
| then | ||
| result = | ||
| rank[n + 1](Expr arg, int k | | ||
| k >= 0 and | ||
| arg = this.getArgument(target.getNumberOfParameters() + k) and | ||
| not this.isSizeArgument(k) | ||
| | | ||
| arg order by k | ||
| ) | ||
| else ( | ||
| n >= 0 and result = this.getArgument(target.getNumberOfParameters() + n) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,3 +131,112 @@ void test_strsafe_gets() { | |
| StringCchGetsExA(dest, sizeof(dest), &end, &remaining, 0); // $ local_source | ||
| } | ||
| } | ||
|
|
||
| int scanf_s(const char *format, ...); | ||
| int fscanf_s(FILE *stream, const char *format, ...); | ||
|
|
||
| void test_scanf_s(FILE *stream) { | ||
| { | ||
| int n1, n2; | ||
| scanf_s( | ||
| "%d %d", | ||
| &n1, // $ local_source | ||
| &n2); // $ local_source | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the argument Same question for the cases below.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, yes. I just wanted to have two outputs, but I forgot to fixup the formatting string 🤦 Fixed in e6c5f94 |
||
| } | ||
|
|
||
| { | ||
| int n; | ||
| fscanf_s(stream, "%d", &n); // $ remote_source | ||
| } | ||
|
|
||
| { | ||
| int n1, n2; | ||
| char buf[256]; | ||
| scanf_s("%d %s %d", | ||
| &n1, // $ local_source | ||
| buf, // $ local_source | ||
| 256, | ||
| &n2); // $ local_source | ||
| } | ||
|
|
||
| { | ||
| int n1, n2; | ||
| char buf[256]; | ||
| fscanf_s(stream, "%d %s %d", | ||
| &n1, // $ remote_source | ||
| buf, // $ remote_source | ||
| 256, | ||
| &n2); // $ remote_source | ||
| } | ||
| } | ||
|
|
||
| typedef void *locale_t; | ||
|
|
||
| int wscanf_s(const wchar_t *format, ...); | ||
| int _scanf_s_l(const char *format, locale_t locale, ...); | ||
| int _wscanf_s_l(const wchar_t *format, locale_t locale, ...); | ||
| int fwscanf_s(FILE *stream, const wchar_t *format, ...); | ||
| int _fscanf_s_l(FILE *stream, const char *format, locale_t locale, ...); | ||
| int _fwscanf_s_l(FILE *stream, const wchar_t *format, locale_t locale, ...); | ||
|
|
||
| void test_additional_scanf_s_variants(FILE *stream, locale_t locale) { | ||
| { | ||
| int n1, n2; | ||
| wchar_t buf[256]; | ||
| wscanf_s(L"%d %s %d", | ||
| &n1, // $ local_source | ||
| buf, // $ local_source | ||
| 256, | ||
| &n2); // $ local_source | ||
| } | ||
|
|
||
| { | ||
| int n1, n2; | ||
| char buf[256]; | ||
| _scanf_s_l("%d %s %d", locale, | ||
| &n1, // $ local_source | ||
| buf, // $ local_source | ||
| 256, | ||
| &n2); // $ local_source | ||
| } | ||
|
|
||
| { | ||
| int n1, n2; | ||
| wchar_t buf[256]; | ||
| _wscanf_s_l(L"%d %s %d", locale, | ||
| &n1, // $ local_source | ||
| buf, // $ local_source | ||
| 256, | ||
| &n2); // $ local_source | ||
| } | ||
|
|
||
| { | ||
| int n1, n2; | ||
| wchar_t buf[256]; | ||
| fwscanf_s(stream, L"%d %s %d", | ||
| &n1, // $ remote_source | ||
| buf, // $ remote_source | ||
| 256, | ||
| &n2); // $ remote_source | ||
| } | ||
|
|
||
| { | ||
| int n1, n2; | ||
| char buf[256]; | ||
| _fscanf_s_l(stream, "%d %s %d", locale, | ||
| &n1, // $ remote_source | ||
| buf, // $ remote_source | ||
| 256, | ||
| &n2); // $ remote_source | ||
| } | ||
|
|
||
| { | ||
| int n1, n2; | ||
| wchar_t buf[256]; | ||
| _fwscanf_s_l(stream, L"%d %s %d", locale, | ||
| &n1, // $ remote_source | ||
| buf, // $ remote_source | ||
| 256, | ||
| &n2); // $ remote_source | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| | test.c:18:2:18:6 | call to scanf | 0 | s | 0 | 0 | | ||
| | test.c:19:2:19:7 | call to fscanf | 0 | s | 10 | 10 | | ||
| | test.c:19:2:19:7 | call to fscanf | 1 | i | 0 | 0 | | ||
| | test.c:20:2:20:7 | call to sscanf | 0 | s | 0 | 0 | | ||
| | test.c:21:2:21:8 | call to swscanf | 0 | s | 10 | 10 | | ||
| | test.c:19:2:19:6 | call to scanf | 0 | s | 0 | 0 | | ||
| | test.c:20:2:20:7 | call to fscanf | 0 | s | 10 | 10 | | ||
| | test.c:20:2:20:7 | call to fscanf | 1 | i | 0 | 0 | | ||
| | test.c:21:2:21:7 | call to sscanf | 0 | s | 0 | 0 | | ||
| | test.c:22:2:22:8 | call to swscanf | 0 | s | 10 | 10 | | ||
| | test.c:23:2:23:8 | call to scanf_s | 0 | d | 0 | 0 | | ||
| | test.c:23:2:23:8 | call to scanf_s | 1 | s | 0 | 0 | | ||
| | test.c:23:2:23:8 | call to scanf_s | 2 | d | 0 | 0 | |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| | ms.cpp:17:3:17:8 | call to sscanf | 0 | 1 | ms.cpp:17:24:17:30 | %I64i | non-wide | | ||
| | test.c:18:2:18:6 | call to scanf | 0 | 0 | test.c:18:8:18:11 | %s | non-wide | | ||
| | test.c:19:2:19:7 | call to fscanf | 0 | 1 | test.c:19:15:19:23 | %10s %i | non-wide | | ||
| | test.c:20:2:20:7 | call to sscanf | 0 | 1 | test.c:20:19:20:28 | %*i%s%*s | non-wide | | ||
| | test.c:21:2:21:8 | call to swscanf | 0 | 1 | test.c:21:21:21:26 | %10s | wide | | ||
| | test.c:19:2:19:6 | call to scanf | 0 | 0 | test.c:19:8:19:11 | %s | non-wide | | ||
| | test.c:20:2:20:7 | call to fscanf | 0 | 1 | test.c:20:15:20:23 | %10s %i | non-wide | | ||
| | test.c:21:2:21:7 | call to sscanf | 0 | 1 | test.c:21:19:21:28 | %*i%s%*s | non-wide | | ||
| | test.c:22:2:22:8 | call to swscanf | 0 | 1 | test.c:22:21:22:26 | %10s | wide | | ||
| | test.c:23:2:23:8 | call to scanf_s | 0 | 0 | test.c:23:10:23:19 | %d %s %d | non-wide | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could add
_scanf_s_l,wscanf_sand_wscanf_s_l.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Fixed in 25d2039.