diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Pure.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Pure.qll index 915bcbce6556..a715a865cb01 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Pure.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Pure.qll @@ -1,5 +1,6 @@ import semmle.code.cpp.models.interfaces.ArrayFunction import semmle.code.cpp.models.interfaces.Taint +import semmle.code.cpp.models.interfaces.DataFlow import semmle.code.cpp.models.interfaces.Alias import semmle.code.cpp.models.interfaces.SideEffect @@ -8,7 +9,7 @@ import semmle.code.cpp.models.interfaces.SideEffect * guaranteed to be side-effect free. */ private class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, - SideEffectFunction + SideEffectFunction, DataFlowFunction { PureStrFunction() { this.hasGlobalOrStdOrBslName([ @@ -25,23 +26,48 @@ private class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunctio this.getParameter(bufParam).getUnspecifiedType() instanceof PointerType } + /** Holds if `i` is a locale parameter that does not carry taint. */ + private predicate isLocaleParameter(ParameterIndex i) { + this.getName().matches("%\\_l") and i + 1 = this.getNumberOfParameters() + } + override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { + // For these functions we add taint flow according to the following rules: + // 1. If the parameter is of a pointer type then there is taint from the + // indirection of the parameter. Otherwise, there is taint from the + // parameter. + // 2. If the return value is of a pointer type then there is taint to the + // indirection of the return. Otherwise, there is taint to the return. exists(ParameterIndex i | - ( - input.isParameter(i) and - exists(this.getParameter(i)) - or - input.isParameterDeref(i) and - this.getParameter(i).getUnspecifiedType() instanceof PointerType - ) and + exists(this.getParameter(i)) and // Functions that end with _l also take a locale argument (always as the last argument), // and we don't want taint from those arguments. - (not this.getName().matches("%\\_l") or exists(this.getParameter(i + 1))) + not this.isLocaleParameter(i) + | + if this.getParameter(i).getUnspecifiedType() instanceof PointerType + then input.isParameterDeref(i) + else input.isParameter(i) ) and ( - output.isReturnValueDeref() and - this.getUnspecifiedType() instanceof PointerType - or + if this.getUnspecifiedType() instanceof PointerType + then output.isReturnValueDeref() + else output.isReturnValue() + ) + or + // If there is taint flow from *input to *output then there is also taint + // flow from input to output. + this.hasTaintFlow(input.getIndirectionInput(), output.getIndirectionOutput()) and + // No need to add taint flow if we already have data flow. + not this.hasDataFlow(input, output) + } + + override predicate hasDataFlow(FunctionInput input, FunctionOutput output) { + exists(int i | + input.isParameter(i) and + not this.isLocaleParameter(i) and + // These functions always return the same pointer as they are given + this.hasGlobalOrStdOrBslName([strrev(), strlwr(), strupr()]) and + this.getParameter(i).getUnspecifiedType() instanceof PointerType and output.isReturnValue() ) } 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 cdf094ab7007..ed09207adf0b 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -7741,6 +7741,32 @@ WARNING: module 'TaintTracking' has been deprecated and may be removed in future | taint.cpp:809:8:809:9 | p2 | taint.cpp:809:7:809:9 | * ... | TAINT | | taint.cpp:811:12:811:28 | call to SysAllocStringLen | taint.cpp:812:8:812:9 | p3 | | | taint.cpp:812:8:812:9 | p3 | taint.cpp:812:7:812:9 | * ... | TAINT | +| taint.cpp:817:42:817:46 | p_out | taint.cpp:817:42:817:46 | p_out | | +| taint.cpp:817:42:817:46 | p_out | taint.cpp:819:4:819:8 | p_out | | +| taint.cpp:817:62:817:65 | p_in | taint.cpp:817:62:817:65 | p_in | | +| taint.cpp:817:62:817:65 | p_in | taint.cpp:818:20:818:23 | p_in | | +| taint.cpp:818:19:818:23 | * ... | taint.cpp:819:19:819:19 | q | | +| taint.cpp:818:20:818:23 | p_in | taint.cpp:818:19:818:23 | * ... | TAINT | +| taint.cpp:819:3:819:8 | * ... [post update] | taint.cpp:817:42:817:46 | p_out | | +| taint.cpp:819:3:819:8 | * ... [post update] | taint.cpp:819:4:819:8 | p_out [inner post update] | | +| taint.cpp:819:3:819:25 | ... = ... | taint.cpp:819:3:819:8 | * ... [post update] | | +| taint.cpp:819:4:819:8 | p_out | taint.cpp:819:3:819:8 | * ... | TAINT | +| taint.cpp:819:12:819:17 | call to strchr | taint.cpp:819:3:819:25 | ... = ... | | +| taint.cpp:819:19:819:19 | q | taint.cpp:819:12:819:17 | call to strchr | TAINT | +| taint.cpp:819:22:819:24 | 47 | taint.cpp:819:12:819:17 | call to strchr | TAINT | +| taint.cpp:822:33:822:35 | out | taint.cpp:822:33:822:35 | out | | +| taint.cpp:822:33:822:35 | out | taint.cpp:826:27:826:29 | out | | +| taint.cpp:822:50:822:51 | in | taint.cpp:822:50:822:51 | in | | +| taint.cpp:822:50:822:51 | in | taint.cpp:826:33:826:34 | in | | +| taint.cpp:826:26:826:29 | ref arg & ... | taint.cpp:822:33:822:35 | out | | +| taint.cpp:826:26:826:29 | ref arg & ... | taint.cpp:826:27:826:29 | out [inner post update] | | +| taint.cpp:826:27:826:29 | out | taint.cpp:826:26:826:29 | & ... | | +| taint.cpp:826:32:826:34 | ref arg & ... | taint.cpp:822:50:822:51 | in | | +| taint.cpp:826:32:826:34 | ref arg & ... | taint.cpp:826:33:826:34 | in [inner post update] | | +| taint.cpp:826:33:826:34 | in | taint.cpp:826:32:826:34 | & ... | | +| taint.cpp:830:20:830:34 | call to indirect_source | taint.cpp:832:23:832:24 | in | | +| taint.cpp:831:15:831:17 | out | taint.cpp:832:18:832:20 | out | | +| taint.cpp:831:15:831:17 | out | taint.cpp:833:8:833:10 | out | | | vector.cpp:16:43:16:49 | source1 | vector.cpp:17:26:17:32 | source1 | | | vector.cpp:16:43:16:49 | source1 | vector.cpp:31:38:31:44 | source1 | | | vector.cpp:17:21:17:33 | call to vector | vector.cpp:19:14:19:14 | v | | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp index 1be594456612..74bcf26ea7d2 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp @@ -810,4 +810,25 @@ void test_sysalloc() { auto p3 = SysAllocStringLen((LPOLESTR)indirect_source(), 10); sink(*p3); // $ ir MISSING: ast +} + +char* strchr(const char*, int); + +void write_to_const_ptr_ptr(const char **p_out, const char **p_in) { + const char* q = *p_in; + *p_out = strchr(q, '/'); +} + +void take_const_ptr(const char *out, const char *in) { + // NOTE: We take the address of `out` in `take_const_ptr`'s stack space. + // Assigning to this pointer does not change `out` in + // `test_write_to_const_ptr_ptr`. + write_to_const_ptr_ptr(&out, &in); +} + +void test_write_to_const_ptr_ptr() { + const char* in = indirect_source(); + const char* out; + take_const_ptr(out, in); + sink(out); // $ SPURIOUS: ast } \ No newline at end of file diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected index b057c541085f..6996b3d1c66c 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected @@ -626,6 +626,11 @@ signatureMatches | taint.cpp:725:10:725:15 | strtol | (XCHAR *,const XCHAR *,int) | CSimpleStringT | CopyCharsOverlapped | 2 | | taint.cpp:727:6:727:16 | test_strtol | (char *) | CStringT | CStringT | 0 | | taint.cpp:785:6:785:15 | fopen_test | (char *) | CStringT | CStringT | 0 | +| taint.cpp:815:7:815:12 | strchr | (LPCOLESTR,int) | CComBSTR | Append | 1 | +| taint.cpp:815:7:815:12 | strchr | (char,int) | CStringT | CStringT | 1 | +| taint.cpp:815:7:815:12 | strchr | (const XCHAR *,int) | CStringT | CStringT | 1 | +| taint.cpp:815:7:815:12 | strchr | (const YCHAR *,int) | CStringT | CStringT | 1 | +| taint.cpp:815:7:815:12 | strchr | (wchar_t,int) | CStringT | CStringT | 1 | | vector.cpp:333:6:333:35 | vector_iterator_assign_wrapper | (LPCOLESTR,int) | CComBSTR | Append | 1 | | vector.cpp:333:6:333:35 | vector_iterator_assign_wrapper | (char,int) | CStringT | CStringT | 1 | | vector.cpp:333:6:333:35 | vector_iterator_assign_wrapper | (const XCHAR *,int) | CStringT | CStringT | 1 | @@ -2029,6 +2034,12 @@ getParameterTypeName | taint.cpp:802:6:802:22 | SysAllocStringLen | 0 | const OLECHAR * | | taint.cpp:802:6:802:22 | SysAllocStringLen | 0 | const wchar_t * | | taint.cpp:802:6:802:22 | SysAllocStringLen | 1 | unsigned int | +| taint.cpp:815:7:815:12 | strchr | 0 | const char * | +| taint.cpp:815:7:815:12 | strchr | 1 | int | +| taint.cpp:817:6:817:27 | write_to_const_ptr_ptr | 0 | const char ** | +| taint.cpp:817:6:817:27 | write_to_const_ptr_ptr | 1 | const char ** | +| taint.cpp:822:6:822:19 | take_const_ptr | 0 | const char * | +| taint.cpp:822:6:822:19 | take_const_ptr | 1 | const char * | | vector.cpp:13:6:13:9 | sink | 0 | int | | vector.cpp:14:27:14:30 | sink | 0 | vector> & | | vector.cpp:14:27:14:30 | sink | 0 | vector> & |