Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions cpp/ql/lib/change-notes/2026-05-15-secure-scanf.md
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.
65 changes: 60 additions & 5 deletions cpp/ql/lib/semmle/code/cpp/commons/Scanf.qll
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)
}
}

/**
Expand All @@ -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...)
Comment thread
geoffw0 marked this conversation as resolved.
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() }
Expand All @@ -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...)
Comment thread
geoffw0 marked this conversation as resolved.
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 }
Expand All @@ -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 }
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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())
}
Comment thread
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`.
Comment thread
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)
)
)
}

/**
Expand Down
27 changes: 20 additions & 7 deletions cpp/ql/lib/semmle/code/cpp/models/implementations/Scanf.qll
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ abstract private class ScanfFunctionModel extends ArrayFunction, TaintFunction,
(
if exists(this.getLengthParameterIndex())
then result = this.getLengthParameterIndex() + 2
else result = 2
else
if exists(this.(ScanfFunction).getInputParameterIndex())
then result = 2
else result = 1
)
}

Expand Down Expand Up @@ -69,23 +72,33 @@ abstract private class ScanfFunctionModel extends ArrayFunction, TaintFunction,
}
}

private predicate hasFlowSource(
ScanfFunction func, ScanfFunctionCall call, FunctionOutput output, string description
) {
exists(int n, Expr arg |
call.getScanfFunction() = func and
call.getOutputArgument(_) = arg and
call.getArgument(n) = arg and
output.isParameterDeref(n) and
description = "value read by " + func.getName()
)
}

/**
* The standard function `scanf` and its assorted variants
*/
private class ScanfModel extends ScanfFunctionModel, LocalFlowSourceFunction instanceof Scanf {
override predicate hasLocalFlowSource(FunctionOutput output, string description) {
output.isParameterDeref(any(int i | i >= this.getArgsStartPosition())) and
description = "value read by " + this.getName()
override predicate hasLocalFlowSource(Call call, FunctionOutput output, string description) {
hasFlowSource(this, call, output, description)
}
}

/**
* The standard function `fscanf` and its assorted variants
*/
private class FscanfModel extends ScanfFunctionModel, RemoteFlowSourceFunction instanceof Fscanf {
override predicate hasRemoteFlowSource(FunctionOutput output, string description) {
output.isParameterDeref(any(int i | i >= this.getArgsStartPosition())) and
description = "value read by " + this.getName()
override predicate hasRemoteFlowSource(Call call, FunctionOutput output, string description) {
hasFlowSource(this, call, output, description)
}

override predicate hasSocketInput(FunctionInput input) {
Expand Down
24 changes: 22 additions & 2 deletions cpp/ql/lib/semmle/code/cpp/models/interfaces/FlowSource.qll
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,17 @@ abstract class RemoteFlowSourceFunction extends Function {
/**
* Holds if remote data described by `description` flows from `output` of a call to this function.
*/
abstract predicate hasRemoteFlowSource(FunctionOutput output, string description);
predicate hasRemoteFlowSource(FunctionOutput output, string description) {
this.hasRemoteFlowSource(_, output, description)
}

/**
* Holds if remote data described by `description` flows from `output` of `call` to this function.
*/
predicate hasRemoteFlowSource(Call call, FunctionOutput output, string description) {
call.getTarget() = this and
this.hasRemoteFlowSource(output, description)
}

/**
* Holds if remote data from this source comes from a socket or stream
Expand All @@ -35,7 +45,17 @@ abstract class LocalFlowSourceFunction extends Function {
/**
* Holds if data described by `description` flows from `output` of a call to this function.
*/
abstract predicate hasLocalFlowSource(FunctionOutput output, string description);
predicate hasLocalFlowSource(FunctionOutput output, string description) {
this.hasLocalFlowSource(_, output, description)
}

/**
* Holds if data described by `description` flows from `output` of `call` to this function.
*/
predicate hasLocalFlowSource(Call call, FunctionOutput output, string description) {
call.getTarget() = this and
this.hasLocalFlowSource(output, description)
}
}

/** A library function that sends data over a network connection. */
Expand Down
5 changes: 2 additions & 3 deletions cpp/ql/lib/semmle/code/cpp/security/FlowSources.qll
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ private class RemoteModelSource extends RemoteFlowSource {

RemoteModelSource() {
exists(CallInstruction call, RemoteFlowSourceFunction func, FunctionOutput output |
call.getStaticCallTarget() = func and
func.hasRemoteFlowSource(output, sourceType) and
func.hasRemoteFlowSource(call.getConvertedResultExpression(), output, sourceType) and
this = callOutput(call, output)
)
}
Expand All @@ -46,7 +45,7 @@ private class LocalModelSource extends LocalFlowSource {
LocalModelSource() {
exists(CallInstruction call, LocalFlowSourceFunction func, FunctionOutput output |
call.getStaticCallTarget() = func and
func.hasLocalFlowSource(output, sourceType) and
func.hasLocalFlowSource(call.getConvertedResultExpression(), output, sourceType) and
this = callOutput(call, output)
)
}
Expand Down
5 changes: 1 addition & 4 deletions cpp/ql/src/Security/CWE/CWE-020/ExternalAPIsSpecific.qll
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,7 @@ class ExternalApiDataNode extends DataFlow::Node {
/** A configuration for tracking flow from `RemoteFlowSource`s to `ExternalApiDataNode`s. */
private module UntrustedDataToExternalApiConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) {
exists(RemoteFlowSourceFunction remoteFlow |
remoteFlow = source.asExpr().(Call).getTarget() and
remoteFlow.hasRemoteFlowSource(_, _)
)
any(RemoteFlowSourceFunction remoteFlow).hasRemoteFlowSource(source.asExpr(), _, _)
}

predicate isSink(DataFlow::Node sink) { sink instanceof ExternalApiDataNode }
Expand Down
3 changes: 1 addition & 2 deletions cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,8 @@ class Recv extends SendRecv instanceof RemoteFlowSourceFunction {
}

override Expr getDataExpr(Call call) {
call.getTarget() = this and
exists(FunctionOutput output, int arg |
super.hasRemoteFlowSource(output, _) and
super.hasRemoteFlowSource(call, output, _) and
output.isParameterDeref(arg) and
result = call.getArgument(arg)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
geoffw0 marked this conversation as resolved.
}

{
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
}
}
13 changes: 8 additions & 5 deletions cpp/ql/test/library-tests/scanf/scanfFormatLiteral.expected
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 |
9 changes: 5 additions & 4 deletions cpp/ql/test/library-tests/scanf/scanfFunctionCall.expected
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 |
Loading
Loading