Skip to content

Commit 8cf25ba

Browse files
MathiasVPMathiasVP
authored
Merge pull request #13191 from MathiasVP/fix-pointer-pointee-conflation
C++: Fix pointer/pointee conflation
2 parents 2e73475 + 402212b commit 8cf25ba

File tree

7 files changed

+33
-45
lines changed

7 files changed

+33
-45
lines changed

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll‎

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -657,24 +657,16 @@ private predicate indirectConversionFlowStep(Node nFrom, Node nTo) {
657657
* So this predicate recurses back along conversions and `PointerArithmeticInstruction`s to find the
658658
* first use that has provides use-use flow, and uses that target as the target of the `nodeFrom`.
659659
*/
660-
private predicate adjustForPointerArith(
661-
DefOrUse defOrUse, Node nodeFrom, UseOrPhi use, boolean uncertain
662-
) {
663-
nodeFrom = any(PostUpdateNode pun).getPreUpdateNode() and
664-
exists(Node adjusted |
665-
indirectConversionFlowStep*(adjusted, nodeFrom) and
666-
nodeToDefOrUse(adjusted, defOrUse, uncertain) and
660+
private predicate adjustForPointerArith(PostUpdateNode pun, UseOrPhi use) {
661+
exists(DefOrUse defOrUse, Node adjusted |
662+
indirectConversionFlowStep*(adjusted, pun.getPreUpdateNode()) and
663+
nodeToDefOrUse(adjusted, defOrUse, _) and
667664
adjacentDefRead(defOrUse, use)
668665
)
669666
}
670667

671668
private predicate ssaFlowImpl(SsaDefOrUse defOrUse, Node nodeFrom, Node nodeTo, boolean uncertain) {
672-
// `nodeFrom = any(PostUpdateNode pun).getPreUpdateNode()` is implied by adjustedForPointerArith.
673669
exists(UseOrPhi use |
674-
adjustForPointerArith(defOrUse, nodeFrom, use, uncertain) and
675-
useToNode(use, nodeTo)
676-
or
677-
not nodeFrom = any(PostUpdateNode pun).getPreUpdateNode() and
678670
nodeToDefOrUse(nodeFrom, defOrUse, uncertain) and
679671
adjacentDefRead(defOrUse, use) and
680672
useToNode(use, nodeTo) and
@@ -719,14 +711,19 @@ predicate ssaFlow(Node nodeFrom, Node nodeTo) {
719711
)
720712
}
721713

714+
private predicate isArgumentOfCallable(DataFlowCall call, ArgumentNode arg) {
715+
arg.argumentOf(call, _)
716+
}
717+
718+
/** Holds if there is def-use or use-use flow from `pun` to `nodeTo`. */
722719
predicate postUpdateFlow(PostUpdateNode pun, Node nodeTo) {
723-
exists(Node preUpdate, Node nFrom, boolean uncertain, SsaDefOrUse defOrUse |
720+
exists(UseOrPhi use, Node preUpdate |
721+
adjustForPointerArith(pun, use) and
722+
useToNode(use, nodeTo) and
724723
preUpdate = pun.getPreUpdateNode() and
725-
ssaFlowImpl(defOrUse, nFrom, nodeTo, uncertain)
726-
|
727-
if uncertain = true
728-
then preUpdate = [nFrom, getAPriorDefinition(defOrUse)]
729-
else preUpdate = nFrom
724+
not exists(DataFlowCall call |
725+
isArgumentOfCallable(call, preUpdate) and isArgumentOfCallable(call, nodeTo)
726+
)
730727
)
731728
}
732729

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// semmle-extractor-options: --edg --clang
22

33
int source();
4-
void sink(int); void sink(const int *); void sink(int **);
4+
void sink(int); void sink(const int *); void sink(int **); void indirect_sink(...);
55

66
struct twoIntFields {
77
int m1, m2;
@@ -19,7 +19,8 @@ void following_pointers( // $ ast-def=sourceStruct1_ptr
1919

2020
sink(sourceArray1[0]); // no flow
2121
sink(*sourceArray1); // no flow
22-
sink(&sourceArray1); // $ ast,ir // [should probably be taint only]
22+
sink(&sourceArray1); // $ ast // [should probably be taint only]
23+
indirect_sink(&sourceArray1); // $ ast,ir
2324

2425
sink(sourceStruct1.m1); // no flow
2526
sink(sourceStruct1_ptr->m1); // no flow
@@ -48,5 +49,6 @@ void following_pointers( // $ ast-def=sourceStruct1_ptr
4849

4950
int stackArray[2] = { source(), source() };
5051
stackArray[0] = source();
51-
sink(stackArray); // $ ast ir ir=49:25 ir=49:35 ir=50:19
52+
sink(stackArray); // $ ast,ir
53+
indirect_sink(stackArray); // $ ast ir=50:25 ir=50:35 ir=51:19
5254
}

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,10 @@ postWithInFlow
2828
| BarrierGuard.cpp:49:6:49:6 | x [post update] | PostUpdateNode should not be the target of local flow. |
2929
| BarrierGuard.cpp:60:7:60:7 | x [post update] | PostUpdateNode should not be the target of local flow. |
3030
| clang.cpp:22:9:22:20 | sourceArray1 [inner post update] | PostUpdateNode should not be the target of local flow. |
31-
| clang.cpp:28:22:28:23 | m1 [post update] | PostUpdateNode should not be the target of local flow. |
32-
| clang.cpp:50:3:50:12 | stackArray [inner post update] | PostUpdateNode should not be the target of local flow. |
33-
| clang.cpp:50:3:50:15 | access to array [post update] | PostUpdateNode should not be the target of local flow. |
31+
| clang.cpp:23:18:23:29 | sourceArray1 [inner post update] | PostUpdateNode should not be the target of local flow. |
32+
| clang.cpp:29:22:29:23 | m1 [post update] | PostUpdateNode should not be the target of local flow. |
33+
| clang.cpp:51:3:51:12 | stackArray [inner post update] | PostUpdateNode should not be the target of local flow. |
34+
| clang.cpp:51:3:51:15 | access to array [post update] | PostUpdateNode should not be the target of local flow. |
3435
| dispatch.cpp:60:3:60:14 | globalBottom [post update] | PostUpdateNode should not be the target of local flow. |
3536
| dispatch.cpp:61:3:61:14 | globalMiddle [post update] | PostUpdateNode should not be the target of local flow. |
3637
| dispatch.cpp:78:24:78:37 | call to allocateBottom [inner post update] | PostUpdateNode should not be the target of local flow. |

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
int source();
2-
void sink(int); void sink(const int *); void sink(int **);
2+
void sink(int); void sink(const int *); void sink(int **); void indirect_sink(...);
33

44
void intraprocedural_with_local_flow() {
55
int t2;
@@ -626,7 +626,7 @@ void test_def_via_phi_read(bool b)
626626
use(buffer);
627627
}
628628
intPointerSource(buffer);
629-
sink(buffer); // $ ast,ir
629+
indirect_sink(buffer); // $ ast,ir
630630
}
631631

632632
void test_static_local_1() {
@@ -692,7 +692,7 @@ void test_static_local_9() {
692692

693693
void increment_buf(int** buf) { // $ ast-def=buf ir-def=*buf ir-def=**buf
694694
*buf += 10;
695-
sink(buf); // $ SPURIOUS: ast,ir // should only be flow to the indirect argument, but there's also flow to the non-indirect argument
695+
sink(buf); // $ SPURIOUS: ast
696696
}
697697

698698
void call_increment_buf(int** buf) { // $ ast-def=buf

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ module AstTest {
3434

3535
override predicate isSink(DataFlow::Node sink) {
3636
exists(FunctionCall call |
37-
call.getTarget().getName() = "sink" and
37+
call.getTarget().getName() = ["sink", "indirect_sink"] and
3838
sink.asExpr() = call.getAnArgument()
3939
)
4040
}
@@ -83,9 +83,12 @@ module IRTest {
8383
}
8484

8585
override predicate isSink(DataFlow::Node sink) {
86-
exists(FunctionCall call |
86+
exists(FunctionCall call, Expr e | e = call.getAnArgument() |
8787
call.getTarget().getName() = "sink" and
88-
call.getAnArgument() in [sink.asExpr(), sink.asIndirectExpr()]
88+
sink.asExpr() = e
89+
or
90+
call.getTarget().getName() = "indirect_sink" and
91+
sink.asIndirectExpr() = e
8992
)
9093
}
9194

‎cpp/ql/test/query-tests/Security/CWE/CWE-078/SAMATE/ExecTainted/ExecTainted.expected‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,16 @@
11
edges
22
| tests.cpp:26:15:26:23 | badSource indirection | tests.cpp:51:12:51:20 | call to badSource indirection |
3-
| tests.cpp:26:32:26:35 | data indirection | tests.cpp:26:15:26:23 | badSource indirection |
4-
| tests.cpp:26:32:26:35 | data indirection | tests.cpp:38:25:38:36 | strncat output argument |
53
| tests.cpp:33:34:33:39 | call to getenv indirection | tests.cpp:38:39:38:49 | environment indirection |
64
| tests.cpp:38:25:38:36 | strncat output argument | tests.cpp:26:15:26:23 | badSource indirection |
7-
| tests.cpp:38:25:38:36 | strncat output argument | tests.cpp:26:15:26:23 | badSource indirection |
8-
| tests.cpp:38:25:38:36 | strncat output argument | tests.cpp:51:22:51:25 | badSource output argument |
95
| tests.cpp:38:39:38:49 | environment indirection | tests.cpp:38:25:38:36 | strncat output argument |
106
| tests.cpp:51:12:51:20 | call to badSource indirection | tests.cpp:53:16:53:19 | data indirection |
11-
| tests.cpp:51:22:51:25 | badSource output argument | tests.cpp:51:22:51:25 | data indirection |
12-
| tests.cpp:51:22:51:25 | data indirection | tests.cpp:26:32:26:35 | data indirection |
13-
| tests.cpp:51:22:51:25 | data indirection | tests.cpp:51:12:51:20 | call to badSource indirection |
147
nodes
158
| tests.cpp:26:15:26:23 | badSource indirection | semmle.label | badSource indirection |
16-
| tests.cpp:26:15:26:23 | badSource indirection | semmle.label | badSource indirection |
17-
| tests.cpp:26:32:26:35 | data indirection | semmle.label | data indirection |
189
| tests.cpp:33:34:33:39 | call to getenv indirection | semmle.label | call to getenv indirection |
1910
| tests.cpp:38:25:38:36 | strncat output argument | semmle.label | strncat output argument |
20-
| tests.cpp:38:25:38:36 | strncat output argument | semmle.label | strncat output argument |
2111
| tests.cpp:38:39:38:49 | environment indirection | semmle.label | environment indirection |
2212
| tests.cpp:51:12:51:20 | call to badSource indirection | semmle.label | call to badSource indirection |
23-
| tests.cpp:51:22:51:25 | badSource output argument | semmle.label | badSource output argument |
24-
| tests.cpp:51:22:51:25 | data indirection | semmle.label | data indirection |
2513
| tests.cpp:53:16:53:19 | data indirection | semmle.label | data indirection |
2614
subpaths
27-
| tests.cpp:51:22:51:25 | data indirection | tests.cpp:26:32:26:35 | data indirection | tests.cpp:26:15:26:23 | badSource indirection | tests.cpp:51:12:51:20 | call to badSource indirection |
2815
#select
2916
| tests.cpp:53:16:53:19 | data | tests.cpp:33:34:33:39 | call to getenv indirection | tests.cpp:53:16:53:19 | data indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | tests.cpp:33:34:33:39 | call to getenv indirection | user input (an environment variable) | tests.cpp:38:25:38:36 | strncat output argument | strncat output argument |

‎cpp/ql/test/query-tests/Security/CWE/CWE-078/semmle/ExecTainted/ExecTainted.expected‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,6 @@ edges
4545
| test.cpp:186:47:186:54 | filename indirection | test.cpp:188:20:188:24 | flags indirection |
4646
| test.cpp:187:11:187:15 | strncat output argument | test.cpp:188:11:188:17 | strncat output argument |
4747
| test.cpp:187:18:187:25 | filename indirection | test.cpp:187:11:187:15 | strncat output argument |
48-
| test.cpp:188:11:188:17 | strncat output argument | test.cpp:188:11:188:17 | strncat output argument |
49-
| test.cpp:188:11:188:17 | strncat output argument | test.cpp:188:11:188:17 | strncat output argument |
5048
| test.cpp:188:20:188:24 | flags indirection | test.cpp:188:11:188:17 | strncat output argument |
5149
| test.cpp:194:9:194:16 | fread output argument | test.cpp:196:26:196:33 | filename indirection |
5250
| test.cpp:196:10:196:16 | concat output argument | test.cpp:198:32:198:38 | command indirection |

0 commit comments

Comments
 (0)