Skip to content

Commit 17bf057

Browse files
owen-mcowen-mc
committed
Fix incorrect guard boolean value
1 parent 8e6ad5a commit 17bf057

12 files changed

Lines changed: 162 additions & 52 deletions

File tree

‎go/ql/lib/semmle/go/controlflow/Guards.qll‎

Lines changed: 81 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -96,12 +96,19 @@ private module GuardsInput implements
9696
}
9797

9898
/**
99-
* A case expression in an expression `switch` statement.
99+
* A case clause in a tagged expression `switch` statement, or a case expression in an
100+
* expressionless `switch` statement.
100101
*/
101-
class Case extends Expr {
102+
class Case extends AstNode {
102103
G::ExpressionSwitchStmt switch;
103104

104-
Case() { this = switch.getACase().getAnExpr() }
105+
Case() {
106+
exists(switch.getExpr()) and
107+
this = switch.getANonDefaultCase()
108+
or
109+
not exists(switch.getExpr()) and
110+
this = switch.getANonDefaultCase().getAnExpr()
111+
}
105112

106113
Expr getSwitchExpr() {
107114
result = switch.getExpr()
@@ -111,19 +118,71 @@ private module GuardsInput implements
111118

112119
predicate isDefaultCase() { none() }
113120

114-
ConstantExpr asConstantCase() { exists(switch.getExpr()) and result = this }
121+
ConstantExpr asConstantCase() {
122+
exists(G::CaseClause cc |
123+
this = cc and
124+
cc.getNumExpr() = 1 and
125+
result = cc.getExpr(0)
126+
)
127+
}
115128

116129
predicate matchEdge(CfgImpl::Cfg::BasicBlock bb1, CfgImpl::Cfg::BasicBlock bb2) {
117-
bb1.getLastNode() = this.getControlFlowNode() and
118-
bb1.getASuccessor(any(MatchingSuccessor successor | successor.getValue() = true)) = bb2
130+
exists(Expr caseExpr |
131+
caseExpr = this.(G::CaseClause).getAnExpr()
132+
or
133+
caseExpr = this
134+
|
135+
caseExpressionBranch(caseExpr, bb1,
136+
any(MatchingSuccessor successor |
137+
bb1.getASuccessor(successor) = bb2 and successor.getValue() = true
138+
))
139+
)
119140
}
120141

121142
predicate nonMatchEdge(CfgImpl::Cfg::BasicBlock bb1, CfgImpl::Cfg::BasicBlock bb2) {
122-
bb1.getLastNode() = this.getControlFlowNode() and
123-
bb1.getASuccessor(any(MatchingSuccessor successor | successor.getValue() = false)) = bb2
143+
exists(G::CaseClause cc, int last, Expr caseExpr |
144+
cc = this and
145+
last = max(int i | exists(cc.getExpr(i))) and
146+
caseExpr = cc.getExpr(last)
147+
|
148+
caseExpressionBranch(caseExpr, bb1,
149+
any(MatchingSuccessor successor |
150+
bb1.getASuccessor(successor) = bb2 and successor.getValue() = false
151+
))
152+
)
153+
or
154+
caseExpressionBranch(this.(Expr), bb1,
155+
any(MatchingSuccessor successor |
156+
bb1.getASuccessor(successor) = bb2 and successor.getValue() = false
157+
))
124158
}
125159
}
126160

161+
additional predicate caseExpressionBranch(
162+
Expr caseExpr, CfgImpl::Cfg::BasicBlock bb, MatchingSuccessor successor
163+
) {
164+
exists(G::CaseClause cc, G::ExpressionSwitchStmt switch |
165+
cc = switch.getACase() and
166+
caseExpr = cc.getAnExpr() and
167+
bb.getLastNode() = caseExpr.getControlFlowNode() and
168+
exists(bb.getASuccessor(successor))
169+
)
170+
}
171+
172+
predicate equalityBranchEdge(
173+
Expr left, Expr right, CfgImpl::Cfg::BasicBlock bb1, CfgImpl::Cfg::BasicBlock bb2, boolean equal
174+
) {
175+
exists(G::CaseClause cc, G::ExpressionSwitchStmt switch |
176+
cc = switch.getACase() and
177+
left = switch.getExpr() and
178+
right = cc.getAnExpr() and
179+
caseExpressionBranch(right, bb1,
180+
any(MatchingSuccessor successor |
181+
bb1.getASuccessor(successor) = bb2 and successor.getValue() = equal
182+
))
183+
)
184+
}
185+
127186
class AndExpr extends Expr instanceof G::LandExpr {
128187
/** Gets an operand of this expression. */
129188
Expr getAnOperand() { result = super.getAnOperand() }
@@ -349,14 +408,26 @@ class GuardValue = GuardsImpl::GuardValue;
349408
private module GuardsLogic = GuardsImpl::Logic<LogicInput>;
350409

351410
/**
352-
* A guard. This is an expression whose value determines subsequent control
353-
* flow.
411+
* A guard. This is an expression whose value, or a switch case whose match outcome, determines
412+
* subsequent control flow.
354413
*/
355414
final class Guard extends GuardsLogic::Guard {
356415
/** Gets the innermost function or file to which this guard belongs. */
357416
ControlFlow::Root getRoot() { result.isRootOf(this) }
358417
}
359418

419+
/**
420+
* Holds if `caseExpr` is a case expression in a tagged switch and its matching edge controls
421+
* `block`.
422+
*/
423+
predicate caseExpressionMatchControls(Expr caseExpr, BasicBlock block) {
424+
exists(BasicBlock guard, MatchingSuccessor successor |
425+
GuardsInput::caseExpressionBranch(caseExpr, guard, successor) and
426+
successor.getValue() = true and
427+
guard.edgeDominates(block, successor)
428+
)
429+
}
430+
360431
/**
361432
* Provides a set of barrier nodes for a guard that validates an expression.
362433
*/

‎go/ql/lib/semmle/go/security/InsecureFeatureFlag.qll‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -121,16 +121,23 @@ module InsecureFeatureFlag {
121121
* branches, including the default case, are reached when the flag does not match.
122122
*/
123123
predicate flagControls(FlagKind flagKind, BasicBlock block) {
124-
exists(GVN flag, Guard guard, boolean branch |
124+
exists(GVN flag, Guard guard |
125125
flag = flagKind.getAFlag() and
126126
guard = flag.getANode().asExpr() and
127-
guard.controls(block, branch) and
128127
(
129-
branch = true
130-
or
131-
not exists(Expr caseExpr |
132-
caseExpr = flag.getANode().asExpr() and caseExpr.getParent() instanceof CaseClause
128+
exists(CaseClause cc, ExpressionSwitchStmt switch |
129+
guard.getParent() = cc and
130+
cc = switch.getACase() and
131+
exists(switch.getExpr()) and
132+
caseExpressionMatchControls(guard, block)
133133
)
134+
or
135+
not exists(CaseClause cc, ExpressionSwitchStmt switch |
136+
guard.getParent() = cc and
137+
cc = switch.getACase() and
138+
exists(switch.getExpr())
139+
) and
140+
guard.controls(block, _)
134141
)
135142
)
136143
}

‎go/ql/src/Security/CWE-209/StackTraceExposure.ql‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,7 @@ module StackTraceExposureConfig implements DataFlow::ConfigSig {
5757
// Sanitize everything controlled by an is-debug-mode check.
5858
// Imprecision: I don't try to guess which arm of a branch is intended
5959
// to mean debug mode, and which is production mode.
60-
exists(Guard g | g = any(DebugModeFlag f).getAFlag().getANode().asExpr() |
61-
g.controls(node.getBasicBlock(), _)
62-
)
60+
flagControls(any(DebugModeFlag f), node.getBasicBlock())
6361
}
6462

6563
predicate observeDiffInformedIncrementalMode() { any() }

‎go/ql/src/experimental/CWE-942/CorsMisconfiguration.ql‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,7 @@ module UntrustedToAllowOriginHeaderConfig implements DataFlow::ConfigSig {
6060
}
6161

6262
predicate isBarrier(DataFlow::Node node) {
63-
exists(Guard g | g = any(AllowedFlag f).getAFlag().getANode().asExpr() |
64-
g.controls(node.getBasicBlock(), _)
65-
)
63+
flagControls(any(AllowedFlag f), node.getBasicBlock())
6664
}
6765

6866
predicate isSink(DataFlow::Node sink) { isSinkHW(sink, _) }
@@ -232,7 +230,5 @@ where
232230
allowOriginIsNull(allowOriginHW, message)
233231
) and
234232
not flowsToGuardedByCheckOnUntrusted(allowOriginHW) and
235-
not exists(Guard g | g = any(AllowedFlag f).getAFlag().getANode().asExpr() |
236-
g.controls(allowOriginHW.getBasicBlock(), _)
237-
)
233+
not flagControls(any(AllowedFlag f), allowOriginHW.getBasicBlock())
238234
select allowOriginHW, message

‎go/ql/test/experimental/CWE-942/CorsMisconfiguration.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
| CorsMisconfiguration.go:53:4:53:44 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
88
| CorsMisconfiguration.go:60:4:60:56 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
99
| CorsMisconfiguration.go:67:5:67:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
10+
| CorsMisconfiguration.go:224:5:224:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
1011
| RsCors.go:11:21:11:59 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` |
1112
| RsCors.go:31:23:31:61 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` |
1213
| RsCors.go:59:20:59:58 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` |

‎go/ql/test/experimental/CWE-942/CorsMisconfiguration.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ func main() {
221221
case "allowed":
222222
w.Header().Set("Access-Control-Allow-Origin", origin)
223223
default:
224-
w.Header().Set("Access-Control-Allow-Origin", origin) // $ MISSING: Alert
224+
w.Header().Set("Access-Control-Allow-Origin", origin) // $ Alert
225225
}
226226
w.Header().Set("Access-Control-Allow-Credentials", "true")
227227
})

‎go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.expected‎

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,16 @@ controlsResult
44
| guards.go:7:7:7:15 | ...<... | zero | false |
55
| guards.go:9:7:9:16 | ...==... | positive | false |
66
| guards.go:9:7:9:16 | ...==... | zero | true |
7-
| guards.go:18:7:18:7 | 0 | tagged one or two | false |
8-
| guards.go:18:7:18:7 | 0 | tagged other | false |
9-
| guards.go:18:7:18:7 | 0 | tagged zero | true |
10-
| guards.go:20:7:20:7 | 1 | tagged other | false |
11-
| guards.go:20:10:20:10 | 2 | tagged other | false |
12-
| guards.go:29:7:29:11 | value | tagged false boolean | true |
7+
| guards.go:18:2:19:21 | case clause | tagged one or two | false |
8+
| guards.go:18:2:19:21 | case clause | tagged other | false |
9+
| guards.go:18:2:19:21 | case clause | tagged zero | true |
10+
| guards.go:20:2:21:27 | case clause | tagged other | false |
11+
| guards.go:29:2:30:30 | case clause | tagged false boolean | true |
12+
| guards.go:29:7:29:11 | value | tagged false boolean | false |
13+
| guards.go:33:2:34:29 | case clause | tagged true boolean | true |
1314
| guards.go:33:7:33:11 | value | tagged true boolean | true |
14-
| guards.go:37:7:37:17 | ...<... | tagged false comparison | true |
15+
| guards.go:37:2:38:33 | case clause | tagged false comparison | true |
16+
| guards.go:37:7:37:17 | ...<... | tagged false comparison | false |
1517
| guards.go:43:5:43:5 | a | compound true | true |
1618
| guards.go:43:5:43:17 | ...&&... | compound false | false |
1719
| guards.go:43:5:43:17 | ...&&... | compound true | true |
@@ -39,15 +41,17 @@ valueControlsResult
3941
| guards.go:17:9:17:13 | value | tagged other | not 1 |
4042
| guards.go:17:9:17:13 | value | tagged other | not 2 |
4143
| guards.go:17:9:17:13 | value | tagged zero | 0 |
42-
| guards.go:18:7:18:7 | 0 | tagged one or two | false |
43-
| guards.go:18:7:18:7 | 0 | tagged other | false |
44-
| guards.go:18:7:18:7 | 0 | tagged zero | true |
45-
| guards.go:20:7:20:7 | 1 | tagged other | false |
46-
| guards.go:20:10:20:10 | 2 | tagged other | false |
47-
| guards.go:29:7:29:11 | value | tagged false boolean | true |
44+
| guards.go:18:2:19:21 | case clause | tagged one or two | false |
45+
| guards.go:18:2:19:21 | case clause | tagged other | false |
46+
| guards.go:18:2:19:21 | case clause | tagged zero | true |
47+
| guards.go:20:2:21:27 | case clause | tagged other | false |
48+
| guards.go:29:2:30:30 | case clause | tagged false boolean | true |
49+
| guards.go:29:7:29:11 | value | tagged false boolean | false |
50+
| guards.go:33:2:34:29 | case clause | tagged true boolean | true |
4851
| guards.go:33:7:33:11 | value | tagged true boolean | true |
49-
| guards.go:37:7:37:12 | number | tagged false comparison | Upper bound 9 |
50-
| guards.go:37:7:37:17 | ...<... | tagged false comparison | true |
52+
| guards.go:37:2:38:33 | case clause | tagged false comparison | true |
53+
| guards.go:37:7:37:12 | number | tagged false comparison | Lower bound 10 |
54+
| guards.go:37:7:37:17 | ...<... | tagged false comparison | false |
5155
| guards.go:43:5:43:5 | a | compound true | true |
5256
| guards.go:43:5:43:17 | ...&&... | compound false | false |
5357
| guards.go:43:5:43:17 | ...&&... | compound true | true |
@@ -71,12 +75,8 @@ valueControlsResult
7175
ensuresEqResult
7276
| guards.go:9:7:9:16 | ...==... | 0 = value | true |
7377
| guards.go:9:7:9:16 | ...==... | value = 0 | true |
74-
| guards.go:18:7:18:7 | 0 | 0 = value | true |
75-
| guards.go:18:7:18:7 | 0 | value = 0 | true |
76-
| guards.go:20:7:20:7 | 1 | 1 = value | true |
77-
| guards.go:20:7:20:7 | 1 | value = 1 | true |
78-
| guards.go:20:10:20:10 | 2 | 2 = value | true |
79-
| guards.go:20:10:20:10 | 2 | value = 2 | true |
78+
| guards.go:18:2:19:21 | case clause | 0 = value | true |
79+
| guards.go:18:2:19:21 | case clause | value = 0 | true |
8080
| guards.go:51:5:51:20 | ...==... | 0 = type conversion | true |
8181
| guards.go:51:5:51:20 | ...==... | type conversion = 0 | true |
8282
| guards.go:54:5:54:19 | ...==... | 0 = type conversion | true |

‎go/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
1+
#select
2+
| test.go:18:10:18:12 | buf | test.go:15:28:15:30 | buf [postupdate] | test.go:18:10:18:12 | buf | HTTP response depends on $@ and may be exposed to an external user. | test.go:15:28:15:30 | buf [postupdate] | stack trace information |
3+
| test.go:46:11:46:13 | buf | test.go:15:28:15:30 | buf [postupdate] | test.go:46:11:46:13 | buf | HTTP response depends on $@ and may be exposed to an external user. | test.go:15:28:15:30 | buf [postupdate] | stack trace information |
14
edges
25
| test.go:15:28:15:30 | buf [postupdate] | test.go:18:10:18:12 | buf | provenance | |
6+
| test.go:15:28:15:30 | buf [postupdate] | test.go:21:29:21:31 | buf | provenance | |
7+
| test.go:21:29:21:31 | buf | test.go:46:11:46:13 | buf | provenance | |
38
nodes
49
| test.go:15:28:15:30 | buf [postupdate] | semmle.label | buf [postupdate] |
510
| test.go:18:10:18:12 | buf | semmle.label | buf |
11+
| test.go:21:29:21:31 | buf | semmle.label | buf |
12+
| test.go:46:11:46:13 | buf | semmle.label | buf |
613
subpaths
7-
#select
8-
| test.go:18:10:18:12 | buf | test.go:15:28:15:30 | buf [postupdate] | test.go:18:10:18:12 | buf | HTTP response depends on $@ and may be exposed to an external user. | test.go:15:28:15:30 | buf [postupdate] | stack trace information |

‎go/ql/test/query-tests/Security/CWE-209/test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,6 @@ func handlePanic(w http.ResponseWriter, r *http.Request) {
4343
case "debug":
4444
w.Write(buf)
4545
default:
46-
w.Write(buf) // $ MISSING: Alert
46+
w.Write(buf) // $ Alert
4747
}
4848
}

‎go/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/DisabledCertificateCheck.expected‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,3 @@
22
| main.go:9:2:9:30 | assign:0 ... = ... | InsecureSkipVerify should not be used in production code. |
33
| main.go:57:21:57:44 | lit-init key-value pair | InsecureSkipVerify should not be used in production code. |
44
| main.go:62:32:62:55 | lit-init key-value pair | InsecureSkipVerify should not be used in production code. |
5-
| main.go:96:3:96:31 | assign:0 ... = ... | InsecureSkipVerify should not be used in production code. |

0 commit comments

Comments
 (0)