Skip to content

Commit ccb28ed

Browse files
MathiasVPMathiasVP
authored
Merge pull request #18556 from MathiasVP/remove-conflation-from-pure-functions
C++: Remove pointer/pointee conflation from models of "pure" functions
2 parents e096bdb + 7792839 commit ccb28ed

File tree

4 files changed

+96
-12
lines changed

4 files changed

+96
-12
lines changed

‎cpp/ql/lib/semmle/code/cpp/models/implementations/Pure.qll

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import semmle.code.cpp.models.interfaces.ArrayFunction
22
import semmle.code.cpp.models.interfaces.Taint
3+
import semmle.code.cpp.models.interfaces.DataFlow
34
import semmle.code.cpp.models.interfaces.Alias
45
import semmle.code.cpp.models.interfaces.SideEffect
56

@@ -8,7 +9,7 @@ import semmle.code.cpp.models.interfaces.SideEffect
89
* guaranteed to be side-effect free.
910
*/
1011
private class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction,
11-
SideEffectFunction
12+
SideEffectFunction, DataFlowFunction
1213
{
1314
PureStrFunction() {
1415
this.hasGlobalOrStdOrBslName([
@@ -25,23 +26,48 @@ private class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunctio
2526
this.getParameter(bufParam).getUnspecifiedType() instanceof PointerType
2627
}
2728

29+
/** Holds if `i` is a locale parameter that does not carry taint. */
30+
private predicate isLocaleParameter(ParameterIndex i) {
31+
this.getName().matches("%\\_l") and i + 1 = this.getNumberOfParameters()
32+
}
33+
2834
override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
35+
// For these functions we add taint flow according to the following rules:
36+
// 1. If the parameter is of a pointer type then there is taint from the
37+
// indirection of the parameter. Otherwise, there is taint from the
38+
// parameter.
39+
// 2. If the return value is of a pointer type then there is taint to the
40+
// indirection of the return. Otherwise, there is taint to the return.
2941
exists(ParameterIndex i |
30-
(
31-
input.isParameter(i) and
32-
exists(this.getParameter(i))
33-
or
34-
input.isParameterDeref(i) and
35-
this.getParameter(i).getUnspecifiedType() instanceof PointerType
36-
) and
42+
exists(this.getParameter(i)) and
3743
// Functions that end with _l also take a locale argument (always as the last argument),
3844
// and we don't want taint from those arguments.
39-
(not this.getName().matches("%\\_l") or exists(this.getParameter(i + 1)))
45+
not this.isLocaleParameter(i)
46+
|
47+
if this.getParameter(i).getUnspecifiedType() instanceof PointerType
48+
then input.isParameterDeref(i)
49+
else input.isParameter(i)
4050
) and
4151
(
42-
output.isReturnValueDeref() and
43-
this.getUnspecifiedType() instanceof PointerType
44-
or
52+
if this.getUnspecifiedType() instanceof PointerType
53+
then output.isReturnValueDeref()
54+
else output.isReturnValue()
55+
)
56+
or
57+
// If there is taint flow from *input to *output then there is also taint
58+
// flow from input to output.
59+
this.hasTaintFlow(input.getIndirectionInput(), output.getIndirectionOutput()) and
60+
// No need to add taint flow if we already have data flow.
61+
not this.hasDataFlow(input, output)
62+
}
63+
64+
override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
65+
exists(int i |
66+
input.isParameter(i) and
67+
not this.isLocaleParameter(i) and
68+
// These functions always return the same pointer as they are given
69+
this.hasGlobalOrStdOrBslName([strrev(), strlwr(), strupr()]) and
70+
this.getParameter(i).getUnspecifiedType() instanceof PointerType and
4571
output.isReturnValue()
4672
)
4773
}

‎cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7741,6 +7741,32 @@ WARNING: module 'TaintTracking' has been deprecated and may be removed in future
77417741
| taint.cpp:809:8:809:9 | p2 | taint.cpp:809:7:809:9 | * ... | TAINT |
77427742
| taint.cpp:811:12:811:28 | call to SysAllocStringLen | taint.cpp:812:8:812:9 | p3 | |
77437743
| taint.cpp:812:8:812:9 | p3 | taint.cpp:812:7:812:9 | * ... | TAINT |
7744+
| taint.cpp:817:42:817:46 | p_out | taint.cpp:817:42:817:46 | p_out | |
7745+
| taint.cpp:817:42:817:46 | p_out | taint.cpp:819:4:819:8 | p_out | |
7746+
| taint.cpp:817:62:817:65 | p_in | taint.cpp:817:62:817:65 | p_in | |
7747+
| taint.cpp:817:62:817:65 | p_in | taint.cpp:818:20:818:23 | p_in | |
7748+
| taint.cpp:818:19:818:23 | * ... | taint.cpp:819:19:819:19 | q | |
7749+
| taint.cpp:818:20:818:23 | p_in | taint.cpp:818:19:818:23 | * ... | TAINT |
7750+
| taint.cpp:819:3:819:8 | * ... [post update] | taint.cpp:817:42:817:46 | p_out | |
7751+
| taint.cpp:819:3:819:8 | * ... [post update] | taint.cpp:819:4:819:8 | p_out [inner post update] | |
7752+
| taint.cpp:819:3:819:25 | ... = ... | taint.cpp:819:3:819:8 | * ... [post update] | |
7753+
| taint.cpp:819:4:819:8 | p_out | taint.cpp:819:3:819:8 | * ... | TAINT |
7754+
| taint.cpp:819:12:819:17 | call to strchr | taint.cpp:819:3:819:25 | ... = ... | |
7755+
| taint.cpp:819:19:819:19 | q | taint.cpp:819:12:819:17 | call to strchr | TAINT |
7756+
| taint.cpp:819:22:819:24 | 47 | taint.cpp:819:12:819:17 | call to strchr | TAINT |
7757+
| taint.cpp:822:33:822:35 | out | taint.cpp:822:33:822:35 | out | |
7758+
| taint.cpp:822:33:822:35 | out | taint.cpp:826:27:826:29 | out | |
7759+
| taint.cpp:822:50:822:51 | in | taint.cpp:822:50:822:51 | in | |
7760+
| taint.cpp:822:50:822:51 | in | taint.cpp:826:33:826:34 | in | |
7761+
| taint.cpp:826:26:826:29 | ref arg & ... | taint.cpp:822:33:822:35 | out | |
7762+
| taint.cpp:826:26:826:29 | ref arg & ... | taint.cpp:826:27:826:29 | out [inner post update] | |
7763+
| taint.cpp:826:27:826:29 | out | taint.cpp:826:26:826:29 | & ... | |
7764+
| taint.cpp:826:32:826:34 | ref arg & ... | taint.cpp:822:50:822:51 | in | |
7765+
| taint.cpp:826:32:826:34 | ref arg & ... | taint.cpp:826:33:826:34 | in [inner post update] | |
7766+
| taint.cpp:826:33:826:34 | in | taint.cpp:826:32:826:34 | & ... | |
7767+
| taint.cpp:830:20:830:34 | call to indirect_source | taint.cpp:832:23:832:24 | in | |
7768+
| taint.cpp:831:15:831:17 | out | taint.cpp:832:18:832:20 | out | |
7769+
| taint.cpp:831:15:831:17 | out | taint.cpp:833:8:833:10 | out | |
77447770
| vector.cpp:16:43:16:49 | source1 | vector.cpp:17:26:17:32 | source1 | |
77457771
| vector.cpp:16:43:16:49 | source1 | vector.cpp:31:38:31:44 | source1 | |
77467772
| vector.cpp:17:21:17:33 | call to vector | vector.cpp:19:14:19:14 | v | |

‎cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -810,4 +810,25 @@ void test_sysalloc() {
810810

811811
auto p3 = SysAllocStringLen((LPOLESTR)indirect_source(), 10);
812812
sink(*p3); // $ ir MISSING: ast
813+
}
814+
815+
char* strchr(const char*, int);
816+
817+
void write_to_const_ptr_ptr(const char **p_out, const char **p_in) {
818+
const char* q = *p_in;
819+
*p_out = strchr(q, '/');
820+
}
821+
822+
void take_const_ptr(const char *out, const char *in) {
823+
// NOTE: We take the address of `out` in `take_const_ptr`'s stack space.
824+
// Assigning to this pointer does not change `out` in
825+
// `test_write_to_const_ptr_ptr`.
826+
write_to_const_ptr_ptr(&out, &in);
827+
}
828+
829+
void test_write_to_const_ptr_ptr() {
830+
const char* in = indirect_source();
831+
const char* out;
832+
take_const_ptr(out, in);
833+
sink(out); // $ SPURIOUS: ast
813834
}

‎cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -626,6 +626,11 @@ signatureMatches
626626
| taint.cpp:725:10:725:15 | strtol | (XCHAR *,const XCHAR *,int) | CSimpleStringT | CopyCharsOverlapped | 2 |
627627
| taint.cpp:727:6:727:16 | test_strtol | (char *) | CStringT | CStringT | 0 |
628628
| taint.cpp:785:6:785:15 | fopen_test | (char *) | CStringT | CStringT | 0 |
629+
| taint.cpp:815:7:815:12 | strchr | (LPCOLESTR,int) | CComBSTR | Append | 1 |
630+
| taint.cpp:815:7:815:12 | strchr | (char,int) | CStringT | CStringT | 1 |
631+
| taint.cpp:815:7:815:12 | strchr | (const XCHAR *,int) | CStringT | CStringT | 1 |
632+
| taint.cpp:815:7:815:12 | strchr | (const YCHAR *,int) | CStringT | CStringT | 1 |
633+
| taint.cpp:815:7:815:12 | strchr | (wchar_t,int) | CStringT | CStringT | 1 |
629634
| vector.cpp:333:6:333:35 | vector_iterator_assign_wrapper | (LPCOLESTR,int) | CComBSTR | Append | 1 |
630635
| vector.cpp:333:6:333:35 | vector_iterator_assign_wrapper | (char,int) | CStringT | CStringT | 1 |
631636
| vector.cpp:333:6:333:35 | vector_iterator_assign_wrapper | (const XCHAR *,int) | CStringT | CStringT | 1 |
@@ -2029,6 +2034,12 @@ getParameterTypeName
20292034
| taint.cpp:802:6:802:22 | SysAllocStringLen | 0 | const OLECHAR * |
20302035
| taint.cpp:802:6:802:22 | SysAllocStringLen | 0 | const wchar_t * |
20312036
| taint.cpp:802:6:802:22 | SysAllocStringLen | 1 | unsigned int |
2037+
| taint.cpp:815:7:815:12 | strchr | 0 | const char * |
2038+
| taint.cpp:815:7:815:12 | strchr | 1 | int |
2039+
| taint.cpp:817:6:817:27 | write_to_const_ptr_ptr | 0 | const char ** |
2040+
| taint.cpp:817:6:817:27 | write_to_const_ptr_ptr | 1 | const char ** |
2041+
| taint.cpp:822:6:822:19 | take_const_ptr | 0 | const char * |
2042+
| taint.cpp:822:6:822:19 | take_const_ptr | 1 | const char * |
20322043
| vector.cpp:13:6:13:9 | sink | 0 | int |
20332044
| vector.cpp:14:27:14:30 | sink | 0 | vector> & |
20342045
| vector.cpp:14:27:14:30 | sink | 0 | vector> & |

0 commit comments

Comments
 (0)