Skip to content

Commit d8645cc

Browse files
hvitvedhvitved
authored
Merge pull request github#15694 from hvitved/csharp/assignable-definition-node
C#: Use separate `newtype` branch for `AssignableDefinitionNode`
2 parents 63a5b49 + 303a2bb commit d8645cc

File tree

66 files changed

+2994
-1436
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

66 files changed

+2994
-1436
lines changed

‎csharp/ql/consistency-queries/DataFlowConsistency.ql

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,25 @@
11
import csharp
22
import cil
3+
private import semmle.code.csharp.controlflow.internal.ControlFlowGraphImpl as ControlFlowGraphImpl
34
private import semmle.code.csharp.dataflow.internal.DataFlowImplSpecific
45
private import semmle.code.csharp.dataflow.internal.TaintTrackingImplSpecific
56
private import codeql.dataflow.internal.DataFlowImplConsistency
67

78
private module Input implements InputSig<CsharpDataFlow> {
89
private import CsharpDataFlow
910

11+
private predicate isStaticAssignable(Assignable a) { a.(Modifiable).isStatic() }
12+
13+
predicate uniqueEnclosingCallableExclude(Node node) {
14+
// TODO: Remove once static initializers are folded into the
15+
// static constructors
16+
isStaticAssignable(ControlFlowGraphImpl::getNodeCfgScope(node.getControlFlowNode()))
17+
}
18+
1019
predicate uniqueCallEnclosingCallableExclude(DataFlowCall call) {
1120
// TODO: Remove once static initializers are folded into the
1221
// static constructors
13-
exists(ControlFlow::Node cfn |
14-
cfn.getAstNode() = any(FieldOrProperty f | f.isStatic()).getAChild+() and
15-
cfn = call.getControlFlowNode()
16-
)
22+
isStaticAssignable(ControlFlowGraphImpl::getNodeCfgScope(call.getControlFlowNode()))
1723
}
1824

1925
predicate uniqueNodeLocationExclude(Node n) {

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,8 @@ module CsharpDataFlow implements InputSig {
2424
predicate mayBenefitFromCallContext = Private::mayBenefitFromCallContext/1;
2525

2626
predicate viableImplInCallContext = Private::viableImplInCallContext/2;
27+
28+
predicate neverSkipInPathGraph(Node n) {
29+
exists(n.(AssignableDefinitionNode).getDefinition().getTargetAccess())
30+
}
2731
}

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

Lines changed: 64 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -208,16 +208,10 @@ predicate hasNodePath(ControlFlowReachabilityConfiguration conf, ExprNode n1, No
208208
cfn2 = n2.(ExprNode).getControlFlowNode()
209209
)
210210
or
211-
exists(
212-
ControlFlow::Node cfn, AssignableDefinition def, ControlFlow::Node cfnDef,
213-
Ssa::ExplicitDefinition ssaDef
214-
|
215-
conf.hasDefPath(_, cfn, def, cfnDef)
216-
|
211+
exists(ControlFlow::Node cfn, AssignableDefinition def, ControlFlow::Node cfnDef |
212+
conf.hasDefPath(_, cfn, def, cfnDef) and
217213
cfn = n1.getControlFlowNode() and
218-
ssaDef.getADefinition() = def and
219-
ssaDef.getControlFlowNode() = cfnDef and
220-
n2.(SsaDefinitionExtNode).getDefinitionExt() = ssaDef
214+
n2 = TAssignableDefinitionNode(def, cfnDef)
221215
)
222216
}
223217

@@ -544,6 +538,13 @@ module LocalFlow {
544538
ThisFlow::adjacentThisRefs(nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
545539
or
546540
CilFlow::localFlowStepCil(nodeFrom, nodeTo)
541+
or
542+
exists(AssignableDefinition def, ControlFlow::Node cfn, Ssa::ExplicitDefinition ssaDef |
543+
ssaDef.getADefinition() = def and
544+
ssaDef.getControlFlowNode() = cfn and
545+
nodeFrom = TAssignableDefinitionNode(def, cfn) and
546+
nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = ssaDef
547+
)
547548
}
548549

549550
/**
@@ -599,7 +600,7 @@ module LocalFlow {
599600
or
600601
hasNodePath(any(LocalExprStepConfiguration x), node1, node2) and
601602
(
602-
node2 instanceof SsaDefinitionExtNode or
603+
node2 instanceof AssignableDefinitionNode or
603604
node2.asExpr() instanceof Cast or
604605
node2.asExpr() instanceof AssignExpr
605606
)
@@ -906,6 +907,11 @@ private module Cached {
906907
not def.(Ssa::ExplicitDefinition).getADefinition() instanceof
907908
AssignableDefinitions::ImplicitParameterDefinition
908909
} or
910+
TAssignableDefinitionNode(AssignableDefinition def, ControlFlow::Node cfn) {
911+
cfn = def.getAControlFlowNode() and
912+
// Handled by `TExplicitParameterNode` below
913+
not def instanceof AssignableDefinitions::ImplicitParameterDefinition
914+
} or
909915
TExplicitParameterNode(DotNet::Parameter p) {
910916
p = any(DataFlowCallable dfc).asCallable().getAParameter()
911917
} or
@@ -1044,15 +1050,7 @@ import Cached
10441050

10451051
/** Holds if `n` should be hidden from path explanations. */
10461052
predicate nodeIsHidden(Node n) {
1047-
exists(SsaImpl::DefinitionExt def | def = n.(SsaDefinitionExtNode).getDefinitionExt() |
1048-
def instanceof Ssa::PhiNode
1049-
or
1050-
def instanceof SsaImpl::PhiReadNode
1051-
or
1052-
def instanceof Ssa::ImplicitEntryDefinition
1053-
or
1054-
def instanceof Ssa::ImplicitCallDefinition
1055-
)
1053+
n instanceof SsaDefinitionExtNode
10561054
or
10571055
exists(Parameter p | p = n.(ParameterNode).getParameter() | not p.fromSource())
10581056
or
@@ -1080,6 +1078,8 @@ predicate nodeIsHidden(Node n) {
10801078
n instanceof InstanceParameterAccessNode
10811079
or
10821080
n instanceof PrimaryConstructorThisAccessNode
1081+
or
1082+
n = any(AssignableDefinitionNode def | not exists(def.getDefinition().getTargetAccess()))
10831083
}
10841084

10851085
/** A CIL SSA definition, viewed as a node in a data flow graph. */
@@ -1128,6 +1128,43 @@ class SsaDefinitionExtNode extends NodeImpl, TSsaDefinitionExtNode {
11281128
override string toStringImpl() { result = def.toString() }
11291129
}
11301130

1131+
/** A definition, viewed as a node in a data flow graph. */
1132+
class AssignableDefinitionNodeImpl extends NodeImpl, TAssignableDefinitionNode {
1133+
private AssignableDefinition def;
1134+
private ControlFlow::Node cfn_;
1135+
1136+
AssignableDefinitionNodeImpl() { this = TAssignableDefinitionNode(def, cfn_) }
1137+
1138+
/** Gets the underlying definition. */
1139+
AssignableDefinition getDefinition() { result = def }
1140+
1141+
/** Gets the underlying definition, at control flow node `cfn`, if any. */
1142+
AssignableDefinition getDefinitionAtNode(ControlFlow::Node cfn) {
1143+
result = def and
1144+
cfn = cfn_
1145+
}
1146+
1147+
override DataFlowCallable getEnclosingCallableImpl() {
1148+
result.asCallable() = cfn_.getEnclosingCallable()
1149+
}
1150+
1151+
override Type getTypeImpl() { result = def.getTarget().getType() }
1152+
1153+
override ControlFlow::Node getControlFlowNodeImpl() { result = cfn_ }
1154+
1155+
override Location getLocationImpl() {
1156+
result = def.getTargetAccess().getLocation()
1157+
or
1158+
not exists(def.getTargetAccess()) and result = def.getLocation()
1159+
}
1160+
1161+
override string toStringImpl() {
1162+
result = def.getTargetAccess().toString()
1163+
or
1164+
not exists(def.getTargetAccess()) and result = def.toString()
1165+
}
1166+
}
1167+
11311168
abstract class ParameterNodeImpl extends NodeImpl {
11321169
abstract predicate isParameterOf(DataFlowCallable c, ParameterPosition pos);
11331170
}
@@ -2021,12 +2058,11 @@ private PropertyContent getResultContent() {
20212058
}
20222059

20232060
private predicate primaryConstructorParameterStore(
2024-
SsaDefinitionExtNode node1, PrimaryConstructorParameterContent c, Node node2
2061+
AssignableDefinitionNode node1, PrimaryConstructorParameterContent c, Node node2
20252062
) {
2026-
exists(Ssa::ExplicitDefinition def, ControlFlow::Node cfn, Parameter p |
2027-
def = node1.getDefinitionExt() and
2028-
p = def.getSourceVariable().getAssignable() and
2029-
cfn = def.getControlFlowNode() and
2063+
exists(AssignableDefinition def, ControlFlow::Node cfn, Parameter p |
2064+
node1 = TAssignableDefinitionNode(def, cfn) and
2065+
p = def.getTarget() and
20302066
node2 = TInstanceParameterAccessNode(cfn, true) and
20312067
c.getParameter() = p
20322068
)
@@ -2193,18 +2229,16 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
21932229
hasNodePath(x, node1, node2)
21942230
or
21952231
// item = variable in node1 = (..., variable, ...)
2196-
exists(AssignableDefinitions::TupleAssignmentDefinition tad, Ssa::ExplicitDefinition def |
2197-
node2.(SsaDefinitionExtNode).getDefinitionExt() = def and
2198-
def.getADefinition() = tad and
2232+
exists(AssignableDefinitions::TupleAssignmentDefinition tad |
2233+
node2.(AssignableDefinitionNode).getDefinition() = tad and
21992234
tad.getLeaf() = item and
22002235
hasNodePath(x, node1, node2)
22012236
)
22022237
or
22032238
// item = variable in node1 = (..., variable, ...) in a case/is var (..., ...)
22042239
te = any(PatternExpr pe).getAChildExpr*() and
2205-
exists(AssignableDefinitions::LocalVariableDefinition lvd, Ssa::ExplicitDefinition def |
2206-
node2.(SsaDefinitionExtNode).getDefinitionExt() = def and
2207-
def.getADefinition() = lvd and
2240+
exists(AssignableDefinitions::LocalVariableDefinition lvd |
2241+
node2.(AssignableDefinitionNode).getDefinition() = lvd and
22082242
lvd.getDeclaration() = item and
22092243
hasNodePath(x, node1, node2)
22102244
)

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

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -109,18 +109,13 @@ class ParameterNode extends Node instanceof ParameterNodeImpl {
109109
}
110110

111111
/** A definition, viewed as a node in a data flow graph. */
112-
class AssignableDefinitionNode extends Node, TSsaDefinitionExtNode {
113-
private Ssa::ExplicitDefinition edef;
114-
115-
AssignableDefinitionNode() { this = TSsaDefinitionExtNode(edef) }
116-
112+
class AssignableDefinitionNode extends Node instanceof AssignableDefinitionNodeImpl {
117113
/** Gets the underlying definition. */
118-
AssignableDefinition getDefinition() { result = this.getDefinitionAtNode(_) }
114+
AssignableDefinition getDefinition() { result = super.getDefinition() }
119115

120116
/** Gets the underlying definition, at control flow node `cfn`, if any. */
121117
AssignableDefinition getDefinitionAtNode(ControlFlow::Node cfn) {
122-
result = edef.getADefinition() and
123-
cfn = edef.getControlFlowNode()
118+
result = super.getDefinitionAtNode(cfn)
124119
}
125120
}
126121

‎csharp/ql/test/experimental/Security Features/CWE-759/HashWithoutSalt.expected

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,23 @@
11
edges
2-
| HashWithoutSalt.cs:18:28:18:105 | call to method ConvertStringToBinary : IBuffer | HashWithoutSalt.cs:20:49:20:56 | access to local variable passBuff | provenance | |
2+
| HashWithoutSalt.cs:18:17:18:24 | access to local variable passBuff : IBuffer | HashWithoutSalt.cs:20:49:20:56 | access to local variable passBuff | provenance | |
3+
| HashWithoutSalt.cs:18:28:18:105 | call to method ConvertStringToBinary : IBuffer | HashWithoutSalt.cs:18:17:18:24 | access to local variable passBuff : IBuffer | provenance | |
34
| HashWithoutSalt.cs:18:70:18:77 | access to parameter password : String | HashWithoutSalt.cs:18:28:18:105 | call to method ConvertStringToBinary : IBuffer | provenance | |
4-
| HashWithoutSalt.cs:38:28:38:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:39:51:39:59 | access to local variable passBytes | provenance | |
5+
| HashWithoutSalt.cs:38:16:38:24 | access to local variable passBytes : Byte[] | HashWithoutSalt.cs:39:51:39:59 | access to local variable passBytes | provenance | |
6+
| HashWithoutSalt.cs:38:28:38:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:38:16:38:24 | access to local variable passBytes : Byte[] | provenance | |
57
| HashWithoutSalt.cs:38:64:38:71 | access to parameter password : String | HashWithoutSalt.cs:38:28:38:72 | call to method GetBytes : Byte[] | provenance | |
6-
| HashWithoutSalt.cs:70:28:70:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:71:48:71:56 | access to local variable passBytes | provenance | |
8+
| HashWithoutSalt.cs:70:16:70:24 | access to local variable passBytes : Byte[] | HashWithoutSalt.cs:71:48:71:56 | access to local variable passBytes | provenance | |
9+
| HashWithoutSalt.cs:70:28:70:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:70:16:70:24 | access to local variable passBytes : Byte[] | provenance | |
710
| HashWithoutSalt.cs:70:64:70:71 | access to parameter password : String | HashWithoutSalt.cs:70:28:70:72 | call to method GetBytes : Byte[] | provenance | |
811
nodes
12+
| HashWithoutSalt.cs:18:17:18:24 | access to local variable passBuff : IBuffer | semmle.label | access to local variable passBuff : IBuffer |
913
| HashWithoutSalt.cs:18:28:18:105 | call to method ConvertStringToBinary : IBuffer | semmle.label | call to method ConvertStringToBinary : IBuffer |
1014
| HashWithoutSalt.cs:18:70:18:77 | access to parameter password : String | semmle.label | access to parameter password : String |
1115
| HashWithoutSalt.cs:20:49:20:56 | access to local variable passBuff | semmle.label | access to local variable passBuff |
16+
| HashWithoutSalt.cs:38:16:38:24 | access to local variable passBytes : Byte[] | semmle.label | access to local variable passBytes : Byte[] |
1217
| HashWithoutSalt.cs:38:28:38:72 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] |
1318
| HashWithoutSalt.cs:38:64:38:71 | access to parameter password : String | semmle.label | access to parameter password : String |
1419
| HashWithoutSalt.cs:39:51:39:59 | access to local variable passBytes | semmle.label | access to local variable passBytes |
20+
| HashWithoutSalt.cs:70:16:70:24 | access to local variable passBytes : Byte[] | semmle.label | access to local variable passBytes : Byte[] |
1521
| HashWithoutSalt.cs:70:28:70:72 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] |
1622
| HashWithoutSalt.cs:70:64:70:71 | access to parameter password : String | semmle.label | access to parameter password : String |
1723
| HashWithoutSalt.cs:71:48:71:56 | access to local variable passBytes | semmle.label | access to local variable passBytes |

‎csharp/ql/test/experimental/Security Features/backdoor/PotentialTimeBomb.expected

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
nodes
2+
| test.cs:69:18:69:30 | access to local variable lastWriteTime : DateTime | semmle.label | access to local variable lastWriteTime : DateTime |
23
| test.cs:69:34:69:76 | call to method GetLastWriteTime : DateTime | semmle.label | call to method GetLastWriteTime : DateTime |
34
| test.cs:71:13:71:71 | call to method CompareTo | semmle.label | call to method CompareTo |
45
| test.cs:71:13:71:71 | call to method CompareTo : Int32 | semmle.label | call to method CompareTo : Int32 |
@@ -7,7 +8,8 @@ nodes
78
| test.cs:71:36:71:70 | call to method AddHours | semmle.label | call to method AddHours |
89
subpaths
910
edges
10-
| test.cs:69:34:69:76 | call to method GetLastWriteTime : DateTime | test.cs:71:36:71:48 | access to local variable lastWriteTime | provenance | |
11+
| test.cs:69:18:69:30 | access to local variable lastWriteTime : DateTime | test.cs:71:36:71:48 | access to local variable lastWriteTime | provenance | |
12+
| test.cs:69:34:69:76 | call to method GetLastWriteTime : DateTime | test.cs:69:18:69:30 | access to local variable lastWriteTime : DateTime | provenance | |
1113
| test.cs:71:13:71:71 | call to method CompareTo : Int32 | test.cs:71:13:71:76 | ... >= ... | provenance | |
1214
| test.cs:71:36:71:48 | access to local variable lastWriteTime | test.cs:71:13:71:71 | call to method CompareTo | provenance | |
1315
| test.cs:71:36:71:48 | access to local variable lastWriteTime | test.cs:71:13:71:71 | call to method CompareTo : Int32 | provenance | |

‎csharp/ql/test/library-tests/cil/dataflow/DataFlow.expected

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,10 @@ edges
4545
| dataflow.cs:100:30:100:33 | null : null | dataflow.cs:72:39:72:52 | call to method IndirectNull : null | provenance | |
4646
| dataflow.cs:100:30:100:33 | null : null | dataflow.cs:106:20:106:33 | call to method IndirectNull | provenance | |
4747
| dataflow.cs:100:30:100:33 | null : null | dataflow.cs:106:20:106:33 | call to method IndirectNull : null | provenance | |
48-
| dataflow.cs:106:20:106:33 | call to method IndirectNull : null | dataflow.cs:108:16:108:16 | access to local variable x : null | provenance | |
49-
| dataflow.cs:107:23:107:26 | null : null | dataflow.cs:108:16:108:16 | access to local variable x : null | provenance | |
48+
| dataflow.cs:106:16:106:16 | access to local variable x : null | dataflow.cs:108:16:108:16 | access to local variable x : null | provenance | |
49+
| dataflow.cs:106:20:106:33 | call to method IndirectNull : null | dataflow.cs:106:16:106:16 | access to local variable x : null | provenance | |
50+
| dataflow.cs:107:19:107:19 | access to local variable x : null | dataflow.cs:108:16:108:16 | access to local variable x : null | provenance | |
51+
| dataflow.cs:107:23:107:26 | null : null | dataflow.cs:107:19:107:19 | access to local variable x : null | provenance | |
5052
| dataflow.cs:108:16:108:16 | access to local variable x : null | dataflow.cs:72:21:72:34 | call to method NullFunction : null | provenance | |
5153
| dataflow.cs:108:16:108:16 | access to local variable x : null | dataflow.cs:87:31:87:44 | call to method NullFunction : null | provenance | |
5254
nodes
@@ -104,8 +106,10 @@ nodes
104106
| dataflow.cs:87:24:87:51 | ... ? ... : ... | semmle.label | ... ? ... : ... |
105107
| dataflow.cs:87:31:87:44 | call to method NullFunction : null | semmle.label | call to method NullFunction : null |
106108
| dataflow.cs:100:30:100:33 | null : null | semmle.label | null : null |
109+
| dataflow.cs:106:16:106:16 | access to local variable x : null | semmle.label | access to local variable x : null |
107110
| dataflow.cs:106:20:106:33 | call to method IndirectNull | semmle.label | call to method IndirectNull |
108111
| dataflow.cs:106:20:106:33 | call to method IndirectNull : null | semmle.label | call to method IndirectNull : null |
112+
| dataflow.cs:107:19:107:19 | access to local variable x : null | semmle.label | access to local variable x : null |
109113
| dataflow.cs:107:23:107:26 | null : null | semmle.label | null : null |
110114
| dataflow.cs:108:16:108:16 | access to local variable x : null | semmle.label | access to local variable x : null |
111115
subpaths

0 commit comments

Comments
 (0)