Skip to content

Commit eed02a2

Browse files
michaelnebelmichaelnebel
committed
C#: Fix issue with isAutoGenerated predicate and make sure that data flow only use relevant summaries.
1 parent 8248f60 commit eed02a2

File tree

6 files changed

+66
-37
lines changed

6 files changed

+66
-37
lines changed

‎csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowDispatch.qll‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,12 +84,16 @@ newtype TReturnKind =
8484
}
8585

8686
/**
87-
* Holds if the summary for `c` should be used for dataflow analysis.
87+
* A summarized callable where the summary should be used for dataflow analysis.
8888
*/
89-
predicate useFlowSummary(FlowSummary::SummarizedCallable c) {
90-
not c.fromSource()
91-
or
92-
c.fromSource() and not c.isAutoGenerated()
89+
class DataFlowSummarizedCallable instanceof FlowSummary::SummarizedCallable {
90+
DataFlowSummarizedCallable() {
91+
not this.fromSource()
92+
or
93+
this.fromSource() and not this.isAutoGenerated()
94+
}
95+
96+
string toString() { result = super.toString() }
9397
}
9498

9599
private module Cached {
@@ -101,8 +105,10 @@ private module Cached {
101105
*/
102106
cached
103107
newtype TDataFlowCallable =
104-
TDotNetCallable(DotNet::Callable c) { c.isUnboundDeclaration() and not useFlowSummary(c) } or
105-
TSummarizedCallable(FlowSummary::SummarizedCallable c) { useFlowSummary(c) }
108+
TDotNetCallable(DotNet::Callable c) {
109+
c.isUnboundDeclaration() and not c instanceof DataFlowSummarizedCallable
110+
} or
111+
TSummarizedCallable(DataFlowSummarizedCallable sc)
106112

107113
cached
108114
newtype TDataFlowCall =

‎csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,8 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
421421
or
422422
exists(Ssa::Definition def |
423423
LocalFlow::localSsaFlowStepUseUse(def, nodeFrom, nodeTo) and
424-
not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom) and
424+
not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom,
425+
any(DataFlowSummarizedCallable sc)) and
425426
not LocalFlow::usesInstanceField(def)
426427
)
427428
or
@@ -743,11 +744,11 @@ private module Cached {
743744
FlowSummaryImpl::Public::SummarizedCallable c,
744745
FlowSummaryImpl::Private::SummaryNodeState state
745746
) {
746-
useFlowSummary(c) and
747+
c instanceof DataFlowSummarizedCallable and
747748
FlowSummaryImpl::Private::summaryNodeRange(c, state)
748749
} or
749750
TSummaryParameterNode(FlowSummaryImpl::Public::SummarizedCallable c, ParameterPosition pos) {
750-
useFlowSummary(c) and
751+
c instanceof DataFlowSummarizedCallable and
751752
FlowSummaryImpl::Private::summaryParameterNodeRange(c, pos)
752753
} or
753754
TParamsArgumentNode(ControlFlow::Node callCfn) {
@@ -771,7 +772,8 @@ private module Cached {
771772
or
772773
// Simple flow through library code is included in the exposed local
773774
// step relation, even though flow is technically inter-procedural
774-
FlowSummaryImpl::Private::Steps::summaryThroughStepValue(nodeFrom, nodeTo)
775+
FlowSummaryImpl::Private::Steps::summaryThroughStepValue(nodeFrom, nodeTo,
776+
any(DataFlowSummarizedCallable sc))
775777
}
776778

777779
cached

‎csharp/ql/lib/semmle/code/csharp/dataflow/internal/FlowSummaryImpl.qll‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -759,8 +759,8 @@ module Private {
759759
}
760760

761761
pragma[nomagic]
762-
private ParamNode summaryArgParam0(DataFlowCall call, ArgNode arg) {
763-
exists(ParameterPosition ppos, SummarizedCallable sc |
762+
private ParamNode summaryArgParam0(DataFlowCall call, ArgNode arg, SummarizedCallable sc) {
763+
exists(ParameterPosition ppos |
764764
argumentPositionMatch(call, arg, ppos) and
765765
viableParam(call, sc, ppos, result)
766766
)
@@ -774,9 +774,9 @@ module Private {
774774
* or expects contents.
775775
*/
776776
pragma[nomagic]
777-
predicate prohibitsUseUseFlow(ArgNode arg) {
777+
predicate prohibitsUseUseFlow(ArgNode arg, SummarizedCallable sc) {
778778
exists(ParamNode p, Node mid, ParameterPosition ppos, Node ret |
779-
p = summaryArgParam0(_, arg) and
779+
p = summaryArgParam0(_, arg, sc) and
780780
p.isParameterOf(_, ppos) and
781781
summaryLocalStep(p, mid, true) and
782782
summaryLocalStep(mid, ret, true) and
@@ -788,9 +788,11 @@ module Private {
788788
}
789789

790790
bindingset[ret]
791-
private ParamNode summaryArgParam(ArgNode arg, ReturnNodeExt ret, OutNodeExt out) {
791+
private ParamNode summaryArgParam(
792+
ArgNode arg, ReturnNodeExt ret, OutNodeExt out, SummarizedCallable sc
793+
) {
792794
exists(DataFlowCall call, ReturnKindExt rk |
793-
result = summaryArgParam0(call, arg) and
795+
result = summaryArgParam0(call, arg, sc) and
794796
ret.getKind() = pragma[only_bind_into](rk) and
795797
out = pragma[only_bind_into](rk).getAnOutNode(call)
796798
)
@@ -803,9 +805,9 @@ module Private {
803805
* NOTE: This step should not be used in global data-flow/taint-tracking, but may
804806
* be useful to include in the exposed local data-flow/taint-tracking relations.
805807
*/
806-
predicate summaryThroughStepValue(ArgNode arg, Node out) {
808+
predicate summaryThroughStepValue(ArgNode arg, Node out, SummarizedCallable sc) {
807809
exists(ReturnKind rk, ReturnNode ret, DataFlowCall call |
808-
summaryLocalStep(summaryArgParam0(call, arg), ret, true) and
810+
summaryLocalStep(summaryArgParam0(call, arg, sc), ret, true) and
809811
ret.getKind() = pragma[only_bind_into](rk) and
810812
out = getAnOutNode(call, pragma[only_bind_into](rk))
811813
)
@@ -818,8 +820,8 @@ module Private {
818820
* NOTE: This step should not be used in global data-flow/taint-tracking, but may
819821
* be useful to include in the exposed local data-flow/taint-tracking relations.
820822
*/
821-
predicate summaryThroughStepTaint(ArgNode arg, Node out) {
822-
exists(ReturnNodeExt ret | summaryLocalStep(summaryArgParam(arg, ret, out), ret, false))
823+
predicate summaryThroughStepTaint(ArgNode arg, Node out, SummarizedCallable sc) {
824+
exists(ReturnNodeExt ret | summaryLocalStep(summaryArgParam(arg, ret, out, sc), ret, false))
823825
}
824826

825827
/**
@@ -829,9 +831,9 @@ module Private {
829831
* NOTE: This step should not be used in global data-flow/taint-tracking, but may
830832
* be useful to include in the exposed local data-flow/taint-tracking relations.
831833
*/
832-
predicate summaryGetterStep(ArgNode arg, ContentSet c, Node out) {
834+
predicate summaryGetterStep(ArgNode arg, ContentSet c, Node out, SummarizedCallable sc) {
833835
exists(Node mid, ReturnNodeExt ret |
834-
summaryReadStep(summaryArgParam(arg, ret, out), c, mid) and
836+
summaryReadStep(summaryArgParam(arg, ret, out, sc), c, mid) and
835837
summaryLocalStep(mid, ret, _)
836838
)
837839
}
@@ -843,9 +845,9 @@ module Private {
843845
* NOTE: This step should not be used in global data-flow/taint-tracking, but may
844846
* be useful to include in the exposed local data-flow/taint-tracking relations.
845847
*/
846-
predicate summarySetterStep(ArgNode arg, ContentSet c, Node out) {
848+
predicate summarySetterStep(ArgNode arg, ContentSet c, Node out, SummarizedCallable sc) {
847849
exists(Node mid, ReturnNodeExt ret |
848-
summaryLocalStep(summaryArgParam(arg, ret, out), mid, _) and
850+
summaryLocalStep(summaryArgParam(arg, ret, out, sc), mid, _) and
849851
summaryStoreStep(mid, c, ret)
850852
)
851853
}
@@ -929,14 +931,20 @@ module Private {
929931
private class SummarizedCallableExternal extends SummarizedCallable {
930932
SummarizedCallableExternal() { summaryElement(this, _, _, _, _) }
931933

932-
private predicate relevantSummaryElement(AccessPath inSpec, AccessPath outSpec, string kind) {
933-
summaryElement(this, inSpec, outSpec, kind, false)
934-
or
934+
private predicate relevantSummaryElementGenerated(
935+
AccessPath inSpec, AccessPath outSpec, string kind
936+
) {
935937
summaryElement(this, inSpec, outSpec, kind, true) and
936938
not summaryElement(this, _, _, _, false) and
937939
not this.clearsContent(_, _)
938940
}
939941

942+
private predicate relevantSummaryElement(AccessPath inSpec, AccessPath outSpec, string kind) {
943+
summaryElement(this, inSpec, outSpec, kind, false)
944+
or
945+
this.relevantSummaryElementGenerated(inSpec, outSpec, kind)
946+
}
947+
940948
override predicate propagatesFlow(
941949
SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
942950
) {
@@ -951,7 +959,7 @@ module Private {
951959
)
952960
}
953961

954-
override predicate isAutoGenerated() { summaryElement(this, _, _, _, true) }
962+
override predicate isAutoGenerated() { this.relevantSummaryElementGenerated(_, _, _) }
955963
}
956964

957965
/** Holds if component `c` of specification `spec` cannot be parsed. */

‎csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ private import csharp
22
private import TaintTrackingPublic
33
private import FlowSummaryImpl as FlowSummaryImpl
44
private import semmle.code.csharp.Caching
5+
private import semmle.code.csharp.dataflow.internal.DataFlowDispatch
56
private import semmle.code.csharp.dataflow.internal.DataFlowPrivate
67
private import semmle.code.csharp.dataflow.internal.ControlFlowReachability
78
private import semmle.code.csharp.dispatch.Dispatch
@@ -117,19 +118,22 @@ private module Cached {
117118
(
118119
// Simple flow through library code is included in the exposed local
119120
// step relation, even though flow is technically inter-procedural
120-
FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(nodeFrom, nodeTo)
121+
FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(nodeFrom, nodeTo,
122+
any(DataFlowSummarizedCallable sc))
121123
or
122124
// Taint collection by adding a tainted element
123125
exists(DataFlow::ElementContent c |
124126
storeStep(nodeFrom, c, nodeTo)
125127
or
126-
FlowSummaryImpl::Private::Steps::summarySetterStep(nodeFrom, c, nodeTo)
128+
FlowSummaryImpl::Private::Steps::summarySetterStep(nodeFrom, c, nodeTo,
129+
any(DataFlowSummarizedCallable sc))
127130
)
128131
or
129132
exists(DataFlow::Content c |
130133
readStep(nodeFrom, c, nodeTo)
131134
or
132-
FlowSummaryImpl::Private::Steps::summaryGetterStep(nodeFrom, c, nodeTo)
135+
FlowSummaryImpl::Private::Steps::summaryGetterStep(nodeFrom, c, nodeTo,
136+
any(DataFlowSummarizedCallable sc))
133137
|
134138
// Taint members
135139
c = any(TaintedMember m).(FieldOrProperty).getContent()

‎csharp/ql/src/Language Abuse/ForeachCapture.ql‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
import csharp
1515
import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
16+
import semmle.code.csharp.dataflow.internal.DataFlowDispatch as DataFlowDispatch
1617
import semmle.code.csharp.dataflow.internal.DataFlowPrivate as DataFlowPrivate
1718
import semmle.code.csharp.frameworks.system.Collections
1819
import semmle.code.csharp.frameworks.system.collections.Generic
@@ -76,7 +77,8 @@ Element getAssignmentTarget(Expr e) {
7677
Element getCollectionAssignmentTarget(Expr e) {
7778
// Store into collection via method
7879
exists(DataFlowPrivate::PostUpdateNode postNode |
79-
FlowSummaryImpl::Private::Steps::summarySetterStep(DataFlow::exprNode(e), _, postNode) and
80+
FlowSummaryImpl::Private::Steps::summarySetterStep(DataFlow::exprNode(e), _, postNode,
81+
any(DataFlowDispatch::DataFlowSummarizedCallable sc)) and
8082
result.(Variable).getAnAccess() = postNode.getPreUpdateNode().asExpr()
8183
)
8284
or

‎csharp/ql/test/library-tests/dataflow/external-models/steps.ql‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import csharp
22
import DataFlow
33
import semmle.code.csharp.dataflow.ExternalFlow
44
import semmle.code.csharp.dataflow.FlowSummary
5+
import semmle.code.csharp.dataflow.internal.DataFlowDispatch as DataFlowDispatch
56
import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
67
import CsvValidation
78

@@ -43,17 +44,23 @@ private class SummarizedCallableClear extends SummarizedCallable {
4344
query predicate summaryThroughStep(
4445
DataFlow::Node node1, DataFlow::Node node2, boolean preservesValue
4546
) {
46-
FlowSummaryImpl::Private::Steps::summaryThroughStepValue(node1, node2) and preservesValue = true
47+
FlowSummaryImpl::Private::Steps::summaryThroughStepValue(node1, node2,
48+
any(DataFlowDispatch::DataFlowSummarizedCallable sc)) and
49+
preservesValue = true
4750
or
48-
FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(node1, node2) and preservesValue = false
51+
FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(node1, node2,
52+
any(DataFlowDispatch::DataFlowSummarizedCallable sc)) and
53+
preservesValue = false
4954
}
5055

5156
query predicate summaryGetterStep(DataFlow::Node arg, DataFlow::Node out, Content c) {
52-
FlowSummaryImpl::Private::Steps::summaryGetterStep(arg, c, out)
57+
FlowSummaryImpl::Private::Steps::summaryGetterStep(arg, c, out,
58+
any(DataFlowDispatch::DataFlowSummarizedCallable sc))
5359
}
5460

5561
query predicate summarySetterStep(DataFlow::Node arg, DataFlow::Node out, Content c) {
56-
FlowSummaryImpl::Private::Steps::summarySetterStep(arg, c, out)
62+
FlowSummaryImpl::Private::Steps::summarySetterStep(arg, c, out,
63+
any(DataFlowDispatch::DataFlowSummarizedCallable sc))
5764
}
5865

5966
query predicate clearsContent(SummarizedCallable c, DataFlow::Content k, ParameterPosition pos) {

0 commit comments

Comments
 (0)