diff --git a/change-notes/1.25/analysis-cpp.md b/change-notes/1.25/analysis-cpp.md index 43a5d9e9f8d1..7ab98ffe859a 100644 --- a/change-notes/1.25/analysis-cpp.md +++ b/change-notes/1.25/analysis-cpp.md @@ -41,4 +41,4 @@ The following changes in version 1.25 affect C/C++ analysis in all applications. }; ``` * The security pack taint tracking library (`semmle.code.cpp.security.TaintTracking`) now considers that equality checks may block the flow of taint. This results in fewer false positive results from queries that use this library. - +* The length of a tainted string (such as the return value of a call to `strlen` or `strftime` with tainted parameters) is no longer itself considered tainted by the `models` library. This leads to fewer false positive results in queries that use any of our taint libraries. diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll index d13a6b58d830..75b8641d449b 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll @@ -33,8 +33,7 @@ private predicate predictableInstruction(Instruction instr) { * Note that the list itself is not very principled; it consists of all the * functions listed in the old security library's [default] `isPureFunction` * that have more than one argument, but are not in the old taint tracking - * library's `returnArgument` predicate. In addition, `strlen` is included - * because it's also a special case in flow to return values. + * library's `returnArgument` predicate. */ predicate predictableOnlyFlow(string name) { name = "strcasestr" or @@ -43,7 +42,6 @@ predicate predictableOnlyFlow(string name) { name = "strchrnul" or name = "strcmp" or name = "strcspn" or - name = "strlen" or // special case name = "strncmp" or name = "strndup" or name = "strnlen" or diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll index c831a8bf3570..8e1739fe6a74 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll @@ -20,9 +20,7 @@ class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideE name = "strpbrk" or name = "strcmp" or name = "strcspn" or - name = "strlen" or name = "strncmp" or - name = "strnlen" or name = "strrchr" or name = "strspn" or name = "strtod" or @@ -30,16 +28,7 @@ class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideE name = "strtol" or name = "strtoll" or name = "strtoq" or - name = "strtoul" or - name = "wcslen" - ) - or - hasGlobalName(name) and - ( - name = "_mbslen" or - name = "_mbslen_l" or - name = "_mbstrlen" or - name = "_mbstrlen_l" + name = "strtoul" ) ) } @@ -90,6 +79,52 @@ class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideE } } +class StrLenFunction extends AliasFunction, ArrayFunction, SideEffectFunction { + StrLenFunction() { + exists(string name | + hasGlobalOrStdName(name) and + ( + name = "strlen" or + name = "strnlen" or + name = "wcslen" + ) + or + hasGlobalName(name) and + ( + name = "_mbslen" or + name = "_mbslen_l" or + name = "_mbstrlen" or + name = "_mbstrlen_l" + ) + ) + } + + override predicate hasArrayInput(int bufParam) { + getParameter(bufParam).getUnspecifiedType() instanceof PointerType + } + + override predicate hasArrayWithNullTerminator(int bufParam) { + getParameter(bufParam).getUnspecifiedType() instanceof PointerType + } + + override predicate parameterNeverEscapes(int i) { + getParameter(i).getUnspecifiedType() instanceof PointerType + } + + override predicate parameterEscapesOnlyViaReturn(int i) { none() } + + override predicate parameterIsAlwaysReturned(int i) { none() } + + override predicate hasOnlySpecificReadSideEffects() { any() } + + override predicate hasOnlySpecificWriteSideEffects() { any() } + + override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { + getParameter(i).getUnspecifiedType() instanceof PointerType and + buffer = true + } +} + class PureFunction extends TaintFunction, SideEffectFunction { PureFunction() { exists(string name | diff --git a/cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll b/cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll index e20dfd83efde..65836d285adc 100644 --- a/cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll @@ -1,5 +1,7 @@ /* * Support for tracking tainted data through the program. + * + * Prefer to use `semmle.code.cpp.dataflow.TaintTracking` when designing new queries. */ import semmle.code.cpp.ir.dataflow.DefaultTaintTracking diff --git a/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll b/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll index a24820b277f9..06cf4c456ce1 100644 --- a/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll +++ b/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll @@ -1,4 +1,8 @@ /** + * DEPRECATED: we now use `semmle.code.cpp.ir.dataflow.DefaultTaintTracking`, + * which is based on the IR but designed to behave similarly to this old + * libarary. + * * Provides the implementation of `semmle.code.cpp.security.TaintTracking`. Do * not import this file directly. */ diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp index 974ea6040e8c..90bbb1ca0cd3 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp @@ -136,3 +136,24 @@ void test1() sink(buffer); // tainted [NOT DETECTED] } } + +// ---------- + +size_t strlen(const char *s); +size_t wcslen(const wchar_t *s); + +void test2() +{ + char *s = string::source(); + wchar_t *ws = wstring::source(); + int i; + + sink(strlen(s)); + sink(wcslen(ws)); + + i = strlen(s) + 1; + sink(i); + + sink(s[strlen(s) - 1]); // tainted + sink(ws + (wcslen(ws) / 2)); // tainted +} 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 c7d58f99966f..2ce7d426eea5 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -111,6 +111,26 @@ | format.cpp:135:39:135:45 | ref arg & ... | format.cpp:135:40:135:45 | buffer [inner post update] | | | format.cpp:135:39:135:45 | ref arg & ... | format.cpp:136:8:136:13 | buffer | | | format.cpp:135:40:135:45 | buffer | format.cpp:135:39:135:45 | & ... | | +| format.cpp:147:12:147:25 | call to source | format.cpp:151:14:151:14 | s | | +| format.cpp:147:12:147:25 | call to source | format.cpp:154:13:154:13 | s | | +| format.cpp:147:12:147:25 | call to source | format.cpp:157:7:157:7 | s | | +| format.cpp:147:12:147:25 | call to source | format.cpp:157:16:157:16 | s | | +| format.cpp:148:16:148:30 | call to source | format.cpp:152:14:152:15 | ws | | +| format.cpp:148:16:148:30 | call to source | format.cpp:158:7:158:8 | ws | | +| format.cpp:148:16:148:30 | call to source | format.cpp:158:20:158:21 | ws | | +| format.cpp:154:6:154:11 | call to strlen | format.cpp:154:6:154:18 | ... + ... | TAINT | +| format.cpp:154:6:154:18 | ... + ... | format.cpp:154:2:154:18 | ... = ... | | +| format.cpp:154:6:154:18 | ... + ... | format.cpp:155:7:155:7 | i | | +| format.cpp:154:18:154:18 | 1 | format.cpp:154:6:154:18 | ... + ... | TAINT | +| format.cpp:157:7:157:7 | s | format.cpp:157:7:157:22 | access to array | TAINT | +| format.cpp:157:9:157:14 | call to strlen | format.cpp:157:9:157:21 | ... - ... | TAINT | +| format.cpp:157:9:157:21 | ... - ... | format.cpp:157:7:157:22 | access to array | TAINT | +| format.cpp:157:21:157:21 | 1 | format.cpp:157:9:157:21 | ... - ... | TAINT | +| format.cpp:158:7:158:8 | ws | format.cpp:158:7:158:27 | ... + ... | TAINT | +| format.cpp:158:7:158:27 | ref arg ... + ... | format.cpp:158:7:158:8 | ws [inner post update] | | +| format.cpp:158:13:158:18 | call to wcslen | format.cpp:158:13:158:26 | ... / ... | TAINT | +| format.cpp:158:13:158:26 | ... / ... | format.cpp:158:7:158:27 | ... + ... | TAINT | +| format.cpp:158:26:158:26 | 2 | format.cpp:158:13:158:26 | ... / ... | TAINT | | stl.cpp:67:12:67:17 | call to source | stl.cpp:71:7:71:7 | a | | | stl.cpp:68:16:68:20 | 123 | stl.cpp:68:16:68:21 | call to basic_string | TAINT | | stl.cpp:68:16:68:21 | call to basic_string | stl.cpp:72:7:72:7 | b | | 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 e873ff919152..312a46e979e9 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected @@ -8,6 +8,8 @@ | format.cpp:100:8:100:13 | buffer | format.cpp:99:30:99:43 | call to source | | format.cpp:105:8:105:13 | buffer | format.cpp:104:31:104:45 | call to source | | format.cpp:110:8:110:14 | wbuffer | format.cpp:109:38:109:52 | call to source | +| format.cpp:157:7:157:22 | access to array | format.cpp:147:12:147:25 | call to source | +| format.cpp:158:7:158:27 | ... + ... | format.cpp:148:16:148:30 | call to source | | stl.cpp:71:7:71:7 | a | stl.cpp:67:12:67:17 | call to source | | stl.cpp:73:7:73:7 | c | stl.cpp:69:16:69:21 | call to source | | stl.cpp:75:9:75:13 | call to c_str | stl.cpp:69:16:69:21 | call to source | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected index 63ab09a46823..41176c5caa20 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected @@ -1,3 +1,6 @@ +| format.cpp:157:7:157:22 | (int)... | format.cpp:147:12:147:25 | call to source | +| format.cpp:157:7:157:22 | access to array | format.cpp:147:12:147:25 | call to source | +| format.cpp:158:7:158:27 | ... + ... | format.cpp:148:16:148:30 | call to source | | stl.cpp:71:7:71:7 | (const char *)... | stl.cpp:67:12:67:17 | call to source | | stl.cpp:71:7:71:7 | a | stl.cpp:67:12:67:17 | call to source | | taint.cpp:8:8:8:13 | clean1 | taint.cpp:4:27:4:33 | source1 |