diff --git a/change-notes/1.24/analysis-cpp.md b/change-notes/1.24/analysis-cpp.md index 94256075a88..aa4bbdf5887 100644 --- a/change-notes/1.24/analysis-cpp.md +++ b/change-notes/1.24/analysis-cpp.md @@ -40,4 +40,4 @@ The following changes in version 1.24 affect C/C++ analysis in all applications. * The taint tracking library (`semmle.code.cpp.dataflow.TaintTracking`) has had the following improvements: * The library now models data flow through `strdup` and similar functions. - \ No newline at end of file + * The library now models data flow through formatting functions such as `sprintf`. diff --git a/cpp/ql/src/semmle/code/cpp/Parameter.qll b/cpp/ql/src/semmle/code/cpp/Parameter.qll index 8b391101c6c..1fbd8b0f071 100644 --- a/cpp/ql/src/semmle/code/cpp/Parameter.qll +++ b/cpp/ql/src/semmle/code/cpp/Parameter.qll @@ -163,5 +163,8 @@ class Parameter extends LocalScopeVariable, @parameter { * An `int` that is a parameter index for some function. This is needed for binding in certain cases. */ class ParameterIndex extends int { - ParameterIndex() { exists(Parameter p | this = p.getIndex()) } + ParameterIndex() { + exists(Parameter p | this = p.getIndex()) or + exists(Call c | exists(c.getArgument(this))) // permit indexing varargs + } } diff --git a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll index 130c067f70c..7c035072916 100644 --- a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll +++ b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll @@ -6,7 +6,8 @@ * `FormattingFunction` to match the flow within that function. */ -import semmle.code.cpp.Function +import semmle.code.cpp.models.interfaces.ArrayFunction +import semmle.code.cpp.models.interfaces.Taint private Type stripTopLevelSpecifiersOnly(Type t) { result = stripTopLevelSpecifiersOnly(t.(SpecifiedType).getBaseType()) @@ -39,7 +40,7 @@ private Type getAFormatterWideTypeOrDefault() { /** * A standard library function that uses a `printf`-like formatting string. */ -abstract class FormattingFunction extends Function { +abstract class FormattingFunction extends ArrayFunction, TaintFunction { /** Gets the position at which the format parameter occurs. */ abstract int getFormatParameterIndex(); @@ -133,4 +134,33 @@ abstract class FormattingFunction extends Function { * Gets the position of the buffer size argument, if any. */ int getSizeParameterIndex() { none() } + + override predicate hasArrayWithNullTerminator(int bufParam) { + bufParam = getFormatParameterIndex() + } + + override predicate hasArrayWithVariableSize(int bufParam, int countParam) { + bufParam = getOutputParameterIndex() and + countParam = getSizeParameterIndex() + } + + override predicate hasArrayWithUnknownSize(int bufParam) { + bufParam = getOutputParameterIndex() and + not exists(getSizeParameterIndex()) + } + + override predicate hasArrayInput(int bufParam) { bufParam = getFormatParameterIndex() } + + override predicate hasArrayOutput(int bufParam) { bufParam = getOutputParameterIndex() } + + override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { + exists(int arg | + ( + arg = getFormatParameterIndex() or + arg >= getFirstFormatArgumentIndex() + ) and + input.isParameterDeref(arg) and + output.isParameterDeref(getOutputParameterIndex()) + ) + } } diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp new file mode 100644 index 00000000000..2080707f17f --- /dev/null +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp @@ -0,0 +1,134 @@ + +typedef unsigned long size_t; +typedef struct {} FILE; + +int snprintf(char *s, size_t n, const char *format, ...); +int sprintf(char *s, const char *format, ...); +int swprintf(wchar_t *s, size_t n, const wchar_t *format, ...); + +typedef void *va_list; +#define va_start(ap, parmN) +#define va_end(ap) +#define va_arg(ap, type) ((type)0) + +int vsnprintf(char *s, size_t n, const char *format, va_list arg); + +int mysprintf(char *s, size_t n, const char *format, ...) +{ + va_list args; + va_start(args, format); + vsnprintf(s, n, format, args); + va_end(args); +} + +int sscanf(const char *s, const char *format, ...); + +// ---------- + +int source(); +void sink(...) {}; + +namespace string +{ + char *source(); +}; + +namespace wstring +{ + wchar_t *source(); +}; + +// ---------- + +void test1() +{ + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%s", "Hello.")); + sink(buffer); + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%s", string::source())); + sink(buffer); // tainted + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, string::source(), "Hello.")); + sink(buffer); // tainted + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%s %s %s", "a", "b", string::source())); + sink(buffer); // tainted + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%.*s", 10, string::source())); + sink(buffer); // tainted + } + + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%i", 0)); + sink(buffer); + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%i", source())); + sink(buffer); // tainted + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%.*s", source(), "Hello.")); + sink(buffer); // tainted + } + + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%p", string::source())); + sink(buffer); // tainted (debatable) + } + + { + char buffer[256] = {0}; + sink(sprintf(buffer, "%s", string::source())); + sink(buffer); // tainted + } + { + char buffer[256] = {0}; + sink(sprintf(buffer, "%ls", wstring::source())); + sink(buffer); // tainted + } + { + wchar_t wbuffer[256] = {0}; + sink(swprintf(wbuffer, 256, L"%s", wstring::source())); + sink(wbuffer); // tainted + } + { + char buffer[256] = {0}; + sink(mysprintf(buffer, 256, "%s", string::source())); + sink(buffer); // tainted [NOT DETECTED - implement UserDefinedFormattingFunction.getOutputParameterIndex()] + } + + { + int i = 0; + sink(sscanf("123", "%i", &i)); + sink(i); + } + { + int i = 0; + sink(sscanf(string::source(), "%i", &i)); + sink(i); // tainted [NOT DETECTED] + } + { + char buffer[256] = {0}; + sink(sscanf("Hello.", "%s", &buffer)); + sink(buffer); + } + { + char buffer[256] = {0}; + sink(sscanf(string::source(), "%s", &buffer)); + sink(buffer); // tainted [NOT DETECTED] + } +} diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index 10a25da812b..648159b4944 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -3,6 +3,108 @@ | file://:0:0:0:0 | p#0 | file://:0:0:0:0 | p#0 | | | file://:0:0:0:0 | p#0 | file://:0:0:0:0 | p#0 | | | file://:0:0:0:0 | p#0 | file://:0:0:0:0 | p#0 | | +| format.cpp:16:21:16:21 | s | format.cpp:20:13:20:13 | s | | +| format.cpp:16:31:16:31 | n | format.cpp:20:16:20:16 | n | | +| format.cpp:16:46:16:51 | format | format.cpp:20:19:20:24 | format | | +| format.cpp:18:10:18:13 | args | format.cpp:20:27:20:30 | args | | +| format.cpp:46:21:46:24 | {...} | format.cpp:47:17:47:22 | buffer | | +| format.cpp:46:21:46:24 | {...} | format.cpp:48:8:48:13 | buffer | | +| format.cpp:46:23:46:23 | 0 | format.cpp:46:21:46:24 | {...} | TAINT | +| format.cpp:47:17:47:22 | ref arg buffer | format.cpp:48:8:48:13 | buffer | | +| format.cpp:47:30:47:33 | %s | format.cpp:47:17:47:22 | ref arg buffer | TAINT | +| format.cpp:47:36:47:43 | Hello. | format.cpp:47:17:47:22 | ref arg buffer | TAINT | +| format.cpp:51:21:51:24 | {...} | format.cpp:52:17:52:22 | buffer | | +| format.cpp:51:21:51:24 | {...} | format.cpp:53:8:53:13 | buffer | | +| format.cpp:51:23:51:23 | 0 | format.cpp:51:21:51:24 | {...} | TAINT | +| format.cpp:52:17:52:22 | ref arg buffer | format.cpp:53:8:53:13 | buffer | | +| format.cpp:52:30:52:33 | %s | format.cpp:52:17:52:22 | ref arg buffer | TAINT | +| format.cpp:52:36:52:49 | call to source | format.cpp:52:17:52:22 | ref arg buffer | TAINT | +| format.cpp:56:21:56:24 | {...} | format.cpp:57:17:57:22 | buffer | | +| format.cpp:56:21:56:24 | {...} | format.cpp:58:8:58:13 | buffer | | +| format.cpp:56:23:56:23 | 0 | format.cpp:56:21:56:24 | {...} | TAINT | +| format.cpp:57:17:57:22 | ref arg buffer | format.cpp:58:8:58:13 | buffer | | +| format.cpp:57:30:57:43 | call to source | format.cpp:57:17:57:22 | ref arg buffer | TAINT | +| format.cpp:57:48:57:55 | Hello. | format.cpp:57:17:57:22 | ref arg buffer | TAINT | +| format.cpp:61:21:61:24 | {...} | format.cpp:62:17:62:22 | buffer | | +| format.cpp:61:21:61:24 | {...} | format.cpp:63:8:63:13 | buffer | | +| format.cpp:61:23:61:23 | 0 | format.cpp:61:21:61:24 | {...} | TAINT | +| format.cpp:62:17:62:22 | ref arg buffer | format.cpp:63:8:63:13 | buffer | | +| format.cpp:62:30:62:39 | %s %s %s | format.cpp:62:17:62:22 | ref arg buffer | TAINT | +| format.cpp:62:42:62:44 | a | format.cpp:62:17:62:22 | ref arg buffer | TAINT | +| format.cpp:62:47:62:49 | b | format.cpp:62:17:62:22 | ref arg buffer | TAINT | +| format.cpp:62:52:62:65 | call to source | format.cpp:62:17:62:22 | ref arg buffer | TAINT | +| format.cpp:66:21:66:24 | {...} | format.cpp:67:17:67:22 | buffer | | +| format.cpp:66:21:66:24 | {...} | format.cpp:68:8:68:13 | buffer | | +| format.cpp:66:23:66:23 | 0 | format.cpp:66:21:66:24 | {...} | TAINT | +| format.cpp:67:17:67:22 | ref arg buffer | format.cpp:68:8:68:13 | buffer | | +| format.cpp:67:30:67:35 | %.*s | format.cpp:67:17:67:22 | ref arg buffer | TAINT | +| format.cpp:67:38:67:39 | 10 | format.cpp:67:17:67:22 | ref arg buffer | TAINT | +| format.cpp:67:42:67:55 | call to source | format.cpp:67:17:67:22 | ref arg buffer | TAINT | +| format.cpp:72:21:72:24 | {...} | format.cpp:73:17:73:22 | buffer | | +| format.cpp:72:21:72:24 | {...} | format.cpp:74:8:74:13 | buffer | | +| format.cpp:72:23:72:23 | 0 | format.cpp:72:21:72:24 | {...} | TAINT | +| format.cpp:73:17:73:22 | ref arg buffer | format.cpp:74:8:74:13 | buffer | | +| format.cpp:73:30:73:33 | %i | format.cpp:73:17:73:22 | ref arg buffer | TAINT | +| format.cpp:73:36:73:36 | 0 | format.cpp:73:17:73:22 | ref arg buffer | TAINT | +| format.cpp:77:21:77:24 | {...} | format.cpp:78:17:78:22 | buffer | | +| format.cpp:77:21:77:24 | {...} | format.cpp:79:8:79:13 | buffer | | +| format.cpp:77:23:77:23 | 0 | format.cpp:77:21:77:24 | {...} | TAINT | +| format.cpp:78:17:78:22 | ref arg buffer | format.cpp:79:8:79:13 | buffer | | +| format.cpp:78:30:78:33 | %i | format.cpp:78:17:78:22 | ref arg buffer | TAINT | +| format.cpp:78:36:78:41 | call to source | format.cpp:78:17:78:22 | ref arg buffer | TAINT | +| format.cpp:82:21:82:24 | {...} | format.cpp:83:17:83:22 | buffer | | +| format.cpp:82:21:82:24 | {...} | format.cpp:84:8:84:13 | buffer | | +| format.cpp:82:23:82:23 | 0 | format.cpp:82:21:82:24 | {...} | TAINT | +| format.cpp:83:17:83:22 | ref arg buffer | format.cpp:84:8:84:13 | buffer | | +| format.cpp:83:30:83:35 | %.*s | format.cpp:83:17:83:22 | ref arg buffer | TAINT | +| format.cpp:83:38:83:43 | call to source | format.cpp:83:17:83:22 | ref arg buffer | TAINT | +| format.cpp:83:48:83:55 | Hello. | format.cpp:83:17:83:22 | ref arg buffer | TAINT | +| format.cpp:88:21:88:24 | {...} | format.cpp:89:17:89:22 | buffer | | +| format.cpp:88:21:88:24 | {...} | format.cpp:90:8:90:13 | buffer | | +| format.cpp:88:23:88:23 | 0 | format.cpp:88:21:88:24 | {...} | TAINT | +| format.cpp:89:17:89:22 | ref arg buffer | format.cpp:90:8:90:13 | buffer | | +| format.cpp:89:30:89:33 | %p | format.cpp:89:17:89:22 | ref arg buffer | TAINT | +| format.cpp:89:36:89:49 | call to source | format.cpp:89:17:89:22 | ref arg buffer | TAINT | +| format.cpp:94:21:94:24 | {...} | format.cpp:95:16:95:21 | buffer | | +| format.cpp:94:21:94:24 | {...} | format.cpp:96:8:96:13 | buffer | | +| format.cpp:94:23:94:23 | 0 | format.cpp:94:21:94:24 | {...} | TAINT | +| format.cpp:95:16:95:21 | ref arg buffer | format.cpp:96:8:96:13 | buffer | | +| format.cpp:95:24:95:27 | %s | format.cpp:95:16:95:21 | ref arg buffer | TAINT | +| format.cpp:95:30:95:43 | call to source | format.cpp:95:16:95:21 | ref arg buffer | TAINT | +| format.cpp:99:21:99:24 | {...} | format.cpp:100:16:100:21 | buffer | | +| format.cpp:99:21:99:24 | {...} | format.cpp:101:8:101:13 | buffer | | +| format.cpp:99:23:99:23 | 0 | format.cpp:99:21:99:24 | {...} | TAINT | +| format.cpp:100:16:100:21 | ref arg buffer | format.cpp:101:8:101:13 | buffer | | +| format.cpp:100:24:100:28 | %ls | format.cpp:100:16:100:21 | ref arg buffer | TAINT | +| format.cpp:100:31:100:45 | call to source | format.cpp:100:16:100:21 | ref arg buffer | TAINT | +| format.cpp:104:25:104:28 | {...} | format.cpp:105:17:105:23 | wbuffer | | +| format.cpp:104:25:104:28 | {...} | format.cpp:106:8:106:14 | wbuffer | | +| format.cpp:104:27:104:27 | 0 | format.cpp:104:25:104:28 | {...} | TAINT | +| format.cpp:105:17:105:23 | ref arg wbuffer | format.cpp:106:8:106:14 | wbuffer | | +| format.cpp:105:31:105:35 | %s | format.cpp:105:17:105:23 | ref arg wbuffer | TAINT | +| format.cpp:105:38:105:52 | call to source | format.cpp:105:17:105:23 | ref arg wbuffer | TAINT | +| format.cpp:109:21:109:24 | {...} | format.cpp:110:18:110:23 | buffer | | +| format.cpp:109:21:109:24 | {...} | format.cpp:111:8:111:13 | buffer | | +| format.cpp:109:23:109:23 | 0 | format.cpp:109:21:109:24 | {...} | TAINT | +| format.cpp:110:18:110:23 | ref arg buffer | format.cpp:111:8:111:13 | buffer | | +| format.cpp:115:10:115:11 | 0 | format.cpp:116:29:116:29 | i | | +| format.cpp:115:10:115:11 | 0 | format.cpp:117:8:117:8 | i | | +| format.cpp:116:28:116:29 | ref arg & ... | format.cpp:117:8:117:8 | i | | +| format.cpp:116:29:116:29 | i | format.cpp:116:28:116:29 | & ... | | +| format.cpp:120:10:120:11 | 0 | format.cpp:121:40:121:40 | i | | +| format.cpp:120:10:120:11 | 0 | format.cpp:122:8:122:8 | i | | +| format.cpp:121:39:121:40 | ref arg & ... | format.cpp:122:8:122:8 | i | | +| format.cpp:121:40:121:40 | i | format.cpp:121:39:121:40 | & ... | | +| format.cpp:125:21:125:24 | {...} | format.cpp:126:32:126:37 | buffer | | +| format.cpp:125:21:125:24 | {...} | format.cpp:127:8:127:13 | buffer | | +| format.cpp:125:23:125:23 | 0 | format.cpp:125:21:125:24 | {...} | TAINT | +| format.cpp:126:31:126:37 | ref arg & ... | format.cpp:127:8:127:13 | buffer | | +| format.cpp:126:32:126:37 | buffer | format.cpp:126:31:126:37 | & ... | | +| format.cpp:130:21:130:24 | {...} | format.cpp:131:40:131:45 | buffer | | +| format.cpp:130:21:130:24 | {...} | format.cpp:132:8:132:13 | buffer | | +| format.cpp:130:23:130:23 | 0 | format.cpp:130:21:130:24 | {...} | TAINT | +| format.cpp:131:39:131:45 | ref arg & ... | format.cpp:132:8:132:13 | buffer | | +| format.cpp:131:40:131:45 | buffer | format.cpp:131:39:131:45 | & ... | | | taint.cpp:4:27:4:33 | source1 | taint.cpp:6:13:6:19 | source1 | | | taint.cpp:4:40:4:45 | clean1 | taint.cpp:5:8:5:13 | clean1 | | | taint.cpp:4:40:4:45 | clean1 | taint.cpp:6:3:6:8 | clean1 | | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected index a507f6b7b83..46146094e53 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected @@ -1,3 +1,13 @@ +| format.cpp:53:8:53:13 | buffer | format.cpp:52:36:52:49 | call to source | +| format.cpp:58:8:58:13 | buffer | format.cpp:57:30:57:43 | call to source | +| format.cpp:63:8:63:13 | buffer | format.cpp:62:52:62:65 | call to source | +| format.cpp:68:8:68:13 | buffer | format.cpp:67:42:67:55 | call to source | +| format.cpp:79:8:79:13 | buffer | format.cpp:78:36:78:41 | call to source | +| format.cpp:84:8:84:13 | buffer | format.cpp:83:38:83:43 | call to source | +| format.cpp:90:8:90:13 | buffer | format.cpp:89:36:89:49 | call to source | +| format.cpp:96:8:96:13 | buffer | format.cpp:95:30:95:43 | call to source | +| format.cpp:101:8:101:13 | buffer | format.cpp:100:31:100:45 | call to source | +| format.cpp:106:8:106:14 | wbuffer | format.cpp:105:38:105:52 | call to source | | taint.cpp:8:8:8:13 | clean1 | taint.cpp:4:27:4:33 | source1 | | taint.cpp:16:8:16:14 | source1 | taint.cpp:12:22:12:27 | call to source | | taint.cpp:17:8:17:16 | ++ ... | taint.cpp:12:22:12:27 | call to source | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected index 1d875493db0..659ea724637 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected @@ -1,3 +1,13 @@ +| format.cpp:53:8:53:13 | format.cpp:52:36:52:49 | AST only | +| format.cpp:58:8:58:13 | format.cpp:57:30:57:43 | AST only | +| format.cpp:63:8:63:13 | format.cpp:62:52:62:65 | AST only | +| format.cpp:68:8:68:13 | format.cpp:67:42:67:55 | AST only | +| format.cpp:79:8:79:13 | format.cpp:78:36:78:41 | AST only | +| format.cpp:84:8:84:13 | format.cpp:83:38:83:43 | AST only | +| format.cpp:90:8:90:13 | format.cpp:89:36:89:49 | AST only | +| format.cpp:96:8:96:13 | format.cpp:95:30:95:43 | AST only | +| format.cpp:101:8:101:13 | format.cpp:100:31:100:45 | AST only | +| format.cpp:106:8:106:14 | format.cpp:105:38:105:52 | AST only | | taint.cpp:41:7:41:13 | taint.cpp:35:12:35:17 | AST only | | taint.cpp:42:7:42:13 | taint.cpp:35:12:35:17 | AST only | | taint.cpp:43:7:43:13 | taint.cpp:37:22:37:27 | AST only | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected index 4500779812b..de52927e1e8 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected @@ -4,6 +4,7 @@ | test.c:32:20:32:25 | call to malloc | This allocation does not include space to null-terminate the string. | | test.c:49:20:49:25 | call to malloc | This allocation does not include space to null-terminate the string. | | test.cpp:24:35:24:40 | call to malloc | This allocation does not include space to null-terminate the string. | +| test.cpp:45:28:45:33 | call to malloc | This allocation does not include space to null-terminate the string. | | test.cpp:63:28:63:33 | call to malloc | This allocation does not include space to null-terminate the string. | | test.cpp:71:28:71:33 | call to malloc | This allocation does not include space to null-terminate the string. | | test.cpp:79:28:79:33 | call to malloc | This allocation does not include space to null-terminate the string. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp index 5be98851b3d..b0db8dea6ef 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp @@ -41,7 +41,7 @@ void good1(wchar_t *wstr) { } void bad3(char *str) { - // BAD -- zero-termination proved by sprintf (as destination) [NOT DETECTED] + // BAD -- zero-termination proved by sprintf (as destination) char *buffer = (char *)malloc(strlen(str)); sprintf(buffer, "%s", str); free(buffer);