Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Commit 471d843

Browse files
Sauyon LeeGitHub Enterprise
Sauyon Lee
authored and
GitHub Enterprise
committed
Merge pull request #222 from max/switch-guard-nodes
Switch guard nodes
2 parents 2d97b39 + 24f9fce commit 471d843

16 files changed

Lines changed: 209 additions & 49 deletions

File tree

‎ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@ package main
33
import "net/url"
44

55
func sanitizeUrl(urlstr string) string {
6-
u, err := url.Parse(urlstr)
7-
if err != nil || u.Scheme == "javascript" {
8-
return "about:blank"
9-
}
10-
return urlstr
6+
u, err := url.Parse(urlstr)
7+
if err != nil || u.Scheme == "javascript" {
8+
return "about:blank"
9+
}
10+
return urlstr
1111
}

‎ql/src/Security/CWE-020/IncompleteUrlSchemeCheckGood.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@ package main
33
import "net/url"
44

55
func sanitizeUrlGod(urlstr string) string {
6-
u, err := url.Parse(urlstr)
7-
if err != nil || u.Scheme == "javascript" || u.Scheme == "data" || u.Scheme == "vbscript" {
8-
return "about:blank"
9-
}
10-
return urlstr
6+
u, err := url.Parse(urlstr)
7+
if err != nil || u.Scheme == "javascript" || u.Scheme == "data" || u.Scheme == "vbscript" {
8+
return "about:blank"
9+
}
10+
return urlstr
1111
}

‎ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll‎

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ private predicate isCondRoot(Expr e) {
2222
e = any(ForStmt fs).getCond()
2323
or
2424
e = any(IfStmt is).getCond()
25+
or
26+
e = any(ExpressionSwitchStmt ess | not exists(ess.getExpr())).getACase().getAnExpr()
2527
}
2628

2729
private predicate isCond(Expr e) {
@@ -245,9 +247,9 @@ newtype TControlFlowNode =
245247
MkImplicitTrue(ExpressionSwitchStmt stmt) { not exists(stmt.getExpr()) } or
246248
/**
247249
* A control-flow node that represents the implicit comparison or type check performed by
248-
* the the `i`th expression of a case clause `cc`.
250+
* the `i`th expression of a case clause `cc`.
249251
*/
250-
MkCaseNode(CaseClause cc, int i) { exists(cc.getExpr(i)) } or
252+
MkCaseCheckNode(CaseClause cc, int i) { exists(cc.getExpr(i)) } or
251253
/**
252254
* A control-flow node that represents the implicit lower bound of a slice expression.
253255
*/
@@ -943,7 +945,16 @@ module CFG {
943945
firstNode(getExpr(i), result)
944946
or
945947
getExpr(i) instanceof TypeExpr and
946-
result = MkCaseNode(this, i)
948+
result = MkCaseCheckNode(this, i)
949+
}
950+
951+
ControlFlow::Node getExprEnd(int i, Boolean outcome) {
952+
exists(Expr e | e = getExpr(i) |
953+
result = MkConditionGuardNode(e, outcome)
954+
or
955+
not exists(MkConditionGuardNode(e, _)) and
956+
result = MkCaseCheckNode(this, i)
957+
)
947958
}
948959

949960
private ControlFlow::Node getBodyStart() {
@@ -961,7 +972,7 @@ module CFG {
961972
ControlFlowTree.super.lastNode(last, cmpl)
962973
or
963974
// TODO: shouldn't be here
964-
last = MkCaseNode(this, getNumExpr() - 1) and
975+
last = getExprEnd(getNumExpr() - 1, false) and
965976
cmpl = Bool(false)
966977
or
967978
last = MkSkipNode(this) and
@@ -975,14 +986,18 @@ module CFG {
975986
or
976987
exists(int i |
977988
lastNode(getExpr(i), pred, normalCompletion()) and
978-
succ = MkCaseNode(this, i)
989+
succ = MkCaseCheckNode(this, i)
979990
or
980-
pred = MkCaseNode(this, i) and
981-
(
982-
succ = getExprStart(i + 1)
983-
or
984-
succ = getBodyStart()
985-
)
991+
// visit guard node if there is one
992+
pred = MkCaseCheckNode(this, i) and
993+
succ = getExprEnd(i, _) and
994+
succ != pred // this avoids self-loops if there isn't a guard node
995+
or
996+
pred = getExprEnd(i, false) and
997+
succ = getExprStart(i + 1)
998+
or
999+
pred = getExprEnd(i, true) and
1000+
succ = getBodyStart()
9861001
)
9871002
}
9881003

@@ -1713,11 +1728,11 @@ module CFG {
17131728
|
17141729
not exists(this.getDefault()) and
17151730
i = this.getNumCase() - 1 and
1716-
last = MkCaseNode(cc, cc.getNumExpr() - 1) and
1731+
last = cc.(CaseClauseTree).getExprEnd(cc.getNumExpr() - 1, false) and
17171732
inner.isNormal() and
17181733
cmpl = inner
17191734
or
1720-
not last instanceof MkCaseNode and
1735+
not last = cc.(CaseClauseTree).getExprEnd(_, _) and
17211736
inner.isNormal() and
17221737
cmpl = inner
17231738
or
@@ -1754,7 +1769,7 @@ module CFG {
17541769
exists(CaseClause cc, int i |
17551770
cc = getNonDefaultCase(i) and
17561771
lastNode(cc, pred, normalCompletion()) and
1757-
pred instanceof MkCaseNode
1772+
pred = cc.(CaseClauseTree).getExprEnd(_, false)
17581773
|
17591774
firstNode(getNonDefaultCase(i + 1), succ)
17601775
or

‎ql/src/semmle/go/controlflow/IR.qll‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ module IR {
4343
this instanceof MkResultInit or
4444
this instanceof MkNextNode or
4545
this instanceof MkImplicitTrue or
46-
this instanceof MkCaseNode or
46+
this instanceof MkCaseCheckNode or
4747
this instanceof MkImplicitLowerSliceBound or
4848
this instanceof MkImplicitUpperSliceBound or
4949
this instanceof MkImplicitMaxSliceBound
@@ -138,7 +138,7 @@ module IR {
138138
this instanceof MkResultInit and result = "result initialization" or
139139
this instanceof MkNextNode and result = "next key-value pair" or
140140
this instanceof MkImplicitTrue and result = "implicit true" or
141-
this instanceof MkCaseNode and result = "case" or
141+
this instanceof MkCaseCheckNode and result = "case" or
142142
this instanceof MkImplicitLowerSliceBound and result = "implicit lower bound" or
143143
this instanceof MkImplicitUpperSliceBound and result = "implicit upper bound" or
144144
this instanceof MkImplicitMaxSliceBound and result = "implicit maximum"
@@ -1050,14 +1050,27 @@ module IR {
10501050
}
10511051

10521052
/**
1053-
* An instruction corresponding to a `case` clause.
1053+
* An instruction corresponding to the implicit comparison or type check performed by an
1054+
* expression in a `case` clause.
1055+
*
1056+
* For example, consider this `switch` statement:
1057+
*
1058+
* ```go
1059+
* switch x {
1060+
* case 2, y+1:
1061+
* ...
1062+
* }
1063+
* ```
1064+
*
1065+
* The expressions `2` and `y+1` are implicitly compared to `x`. These comparisons are
1066+
* represented by case instructions.
10541067
*/
1055-
class CaseInstruction extends Instruction, MkCaseNode {
1068+
class CaseInstruction extends Instruction, MkCaseCheckNode {
10561069
CaseClause cc;
10571070

10581071
int i;
10591072

1060-
CaseInstruction() { this = MkCaseNode(cc, i) }
1073+
CaseInstruction() { this = MkCaseCheckNode(cc, i) }
10611074

10621075
override ControlFlow::Root getRoot() { result.isRootOf(cc) }
10631076

‎ql/test/extractor-tests/go1.13/tst.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,4 +10,4 @@ const (
1010
s1 = 1_000_000
1111
s2 = 0b_0101_0110
1212
s3 = 3.1415_9265
13-
)
13+
)

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

Lines changed: 97 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,41 @@
1+
| DuplicateSwitchCase.go:0:0:0:0 | entry | DuplicateSwitchCase.go:3:6:3:15 | skip |
2+
| DuplicateSwitchCase.go:3:1:3:1 | entry | DuplicateSwitchCase.go:3:17:3:19 | argument corresponding to msg |
3+
| DuplicateSwitchCase.go:3:1:12:1 | function declaration | DuplicateSwitchCase.go:14:6:14:10 | skip |
4+
| DuplicateSwitchCase.go:3:6:3:15 | skip | DuplicateSwitchCase.go:3:1:12:1 | function declaration |
5+
| DuplicateSwitchCase.go:3:17:3:19 | argument corresponding to msg | DuplicateSwitchCase.go:3:17:3:19 | initialization of msg |
6+
| DuplicateSwitchCase.go:3:17:3:19 | initialization of msg | DuplicateSwitchCase.go:4:2:4:2 | true |
7+
| DuplicateSwitchCase.go:4:2:4:2 | true | DuplicateSwitchCase.go:5:7:5:9 | msg |
8+
| DuplicateSwitchCase.go:5:7:5:9 | msg | DuplicateSwitchCase.go:5:14:5:20 | "start" |
9+
| DuplicateSwitchCase.go:5:7:5:20 | ...==... | DuplicateSwitchCase.go:5:7:5:20 | case ...==... |
10+
| DuplicateSwitchCase.go:5:7:5:20 | ...==... | DuplicateSwitchCase.go:12:1:12:1 | exit |
11+
| DuplicateSwitchCase.go:5:7:5:20 | case ...==... | DuplicateSwitchCase.go:5:20:5:20 | ...==... is false |
12+
| DuplicateSwitchCase.go:5:7:5:20 | case ...==... | DuplicateSwitchCase.go:5:20:5:20 | ...==... is true |
13+
| DuplicateSwitchCase.go:5:14:5:20 | "start" | DuplicateSwitchCase.go:5:7:5:20 | ...==... |
14+
| DuplicateSwitchCase.go:5:20:5:20 | ...==... is false | DuplicateSwitchCase.go:7:7:7:9 | msg |
15+
| DuplicateSwitchCase.go:5:20:5:20 | ...==... is true | DuplicateSwitchCase.go:6:3:6:7 | start |
16+
| DuplicateSwitchCase.go:6:3:6:7 | start | DuplicateSwitchCase.go:6:3:6:9 | call to start |
17+
| DuplicateSwitchCase.go:6:3:6:9 | call to start | DuplicateSwitchCase.go:12:1:12:1 | exit |
18+
| DuplicateSwitchCase.go:7:7:7:9 | msg | DuplicateSwitchCase.go:7:14:7:20 | "start" |
19+
| DuplicateSwitchCase.go:7:7:7:20 | ...==... | DuplicateSwitchCase.go:7:7:7:20 | case ...==... |
20+
| DuplicateSwitchCase.go:7:7:7:20 | ...==... | DuplicateSwitchCase.go:12:1:12:1 | exit |
21+
| DuplicateSwitchCase.go:7:7:7:20 | case ...==... | DuplicateSwitchCase.go:7:20:7:20 | ...==... is false |
22+
| DuplicateSwitchCase.go:7:7:7:20 | case ...==... | DuplicateSwitchCase.go:7:20:7:20 | ...==... is true |
23+
| DuplicateSwitchCase.go:7:14:7:20 | "start" | DuplicateSwitchCase.go:7:7:7:20 | ...==... |
24+
| DuplicateSwitchCase.go:7:20:7:20 | ...==... is false | DuplicateSwitchCase.go:10:3:10:7 | panic |
25+
| DuplicateSwitchCase.go:7:20:7:20 | ...==... is true | DuplicateSwitchCase.go:8:3:8:6 | stop |
26+
| DuplicateSwitchCase.go:8:3:8:6 | stop | DuplicateSwitchCase.go:8:3:8:8 | call to stop |
27+
| DuplicateSwitchCase.go:8:3:8:8 | call to stop | DuplicateSwitchCase.go:12:1:12:1 | exit |
28+
| DuplicateSwitchCase.go:10:3:10:7 | panic | DuplicateSwitchCase.go:10:9:10:33 | "Message not understood." |
29+
| DuplicateSwitchCase.go:10:3:10:34 | call to panic | DuplicateSwitchCase.go:12:1:12:1 | exit |
30+
| DuplicateSwitchCase.go:10:9:10:33 | "Message not understood." | DuplicateSwitchCase.go:10:3:10:34 | call to panic |
31+
| DuplicateSwitchCase.go:14:1:14:1 | entry | DuplicateSwitchCase.go:14:14:14:15 | skip |
32+
| DuplicateSwitchCase.go:14:1:14:15 | function declaration | DuplicateSwitchCase.go:16:6:16:9 | skip |
33+
| DuplicateSwitchCase.go:14:6:14:10 | skip | DuplicateSwitchCase.go:14:1:14:15 | function declaration |
34+
| DuplicateSwitchCase.go:14:14:14:15 | skip | DuplicateSwitchCase.go:14:15:14:15 | exit |
35+
| DuplicateSwitchCase.go:16:1:16:1 | entry | DuplicateSwitchCase.go:16:13:16:14 | skip |
36+
| DuplicateSwitchCase.go:16:1:16:14 | function declaration | DuplicateSwitchCase.go:0:0:0:0 | exit |
37+
| DuplicateSwitchCase.go:16:6:16:9 | skip | DuplicateSwitchCase.go:16:1:16:14 | function declaration |
38+
| DuplicateSwitchCase.go:16:13:16:14 | skip | DuplicateSwitchCase.go:16:14:16:14 | exit |
139
| exprs.go:0:0:0:0 | entry | exprs.go:3:1:3:29 | skip |
240
| exprs.go:3:1:3:29 | skip | exprs.go:5:6:5:9 | skip |
341
| exprs.go:5:1:5:1 | entry | exprs.go:6:6:6:6 | skip |
@@ -1079,9 +1117,11 @@
10791117
| stmts.go:102:2:102:2 | true | stmts.go:105:7:105:10 | true |
10801118
| stmts.go:104:3:104:7 | skip | stmts.go:107:1:107:1 | exit |
10811119
| stmts.go:105:2:105:11 | skip | stmts.go:107:1:107:1 | exit |
1082-
| stmts.go:105:7:105:10 | case true | stmts.go:104:3:104:7 | skip |
1083-
| stmts.go:105:7:105:10 | case true | stmts.go:105:2:105:11 | skip |
1120+
| stmts.go:105:7:105:10 | case true | stmts.go:105:10:105:10 | true is false |
1121+
| stmts.go:105:7:105:10 | case true | stmts.go:105:10:105:10 | true is true |
10841122
| stmts.go:105:7:105:10 | true | stmts.go:105:7:105:10 | case true |
1123+
| stmts.go:105:10:105:10 | true is false | stmts.go:104:3:104:7 | skip |
1124+
| stmts.go:105:10:105:10 | true is true | stmts.go:105:2:105:11 | skip |
10851125
| stmts.go:110:1:110:1 | entry | stmts.go:110:12:110:12 | argument corresponding to x |
10861126
| stmts.go:110:1:123:1 | function declaration | stmts.go:126:6:126:11 | skip |
10871127
| stmts.go:110:6:110:10 | skip | stmts.go:110:1:123:1 | function declaration |
@@ -1165,3 +1205,58 @@
11651205
| stmts.go:143:12:143:13 | next key-value pair in range | stmts.go:145:1:145:1 | exit |
11661206
| stmts.go:143:12:143:13 | xs | stmts.go:143:12:143:13 | next key-value pair in range |
11671207
| stmts.go:143:15:144:2 | skip | stmts.go:143:12:143:13 | next key-value pair in range |
1208+
| tst.go:0:0:0:0 | entry | tst.go:3:6:3:10 | skip |
1209+
| tst.go:3:1:3:1 | entry | tst.go:3:12:3:12 | argument corresponding to x |
1210+
| tst.go:3:1:12:1 | function declaration | tst.go:14:6:14:11 | skip |
1211+
| tst.go:3:6:3:10 | skip | tst.go:3:1:12:1 | function declaration |
1212+
| tst.go:3:12:3:12 | argument corresponding to x | tst.go:3:12:3:12 | initialization of x |
1213+
| tst.go:3:12:3:12 | initialization of x | tst.go:4:2:4:2 | true |
1214+
| tst.go:4:2:4:2 | true | tst.go:5:7:5:7 | x |
1215+
| tst.go:4:2:4:2 | true | tst.go:12:1:12:1 | exit |
1216+
| tst.go:5:2:5:13 | skip | tst.go:12:1:12:1 | exit |
1217+
| tst.go:5:7:5:7 | x | tst.go:5:11:5:12 | 23 |
1218+
| tst.go:5:7:5:12 | ...<... | tst.go:5:7:5:12 | case ...<... |
1219+
| tst.go:5:7:5:12 | case ...<... | tst.go:5:12:5:12 | ...<... is false |
1220+
| tst.go:5:7:5:12 | case ...<... | tst.go:5:12:5:12 | ...<... is true |
1221+
| tst.go:5:11:5:12 | 23 | tst.go:5:7:5:12 | ...<... |
1222+
| tst.go:5:12:5:12 | ...<... is false | tst.go:7:7:7:7 | x |
1223+
| tst.go:5:12:5:12 | ...<... is true | tst.go:5:2:5:13 | skip |
1224+
| tst.go:7:2:7:13 | skip | tst.go:12:1:12:1 | exit |
1225+
| tst.go:7:7:7:7 | x | tst.go:7:11:7:12 | 42 |
1226+
| tst.go:7:7:7:12 | ...<... | tst.go:7:7:7:12 | case ...<... |
1227+
| tst.go:7:7:7:12 | case ...<... | tst.go:7:12:7:12 | ...<... is false |
1228+
| tst.go:7:7:7:12 | case ...<... | tst.go:7:12:7:12 | ...<... is true |
1229+
| tst.go:7:11:7:12 | 42 | tst.go:7:7:7:12 | ...<... |
1230+
| tst.go:7:12:7:12 | ...<... is false | tst.go:9:7:9:7 | x |
1231+
| tst.go:7:12:7:12 | ...<... is true | tst.go:7:2:7:13 | skip |
1232+
| tst.go:9:2:9:13 | skip | tst.go:12:1:12:1 | exit |
1233+
| tst.go:9:7:9:7 | x | tst.go:9:11:9:12 | 23 |
1234+
| tst.go:9:7:9:12 | ...<... | tst.go:9:7:9:12 | case ...<... |
1235+
| tst.go:9:7:9:12 | case ...<... | tst.go:9:12:9:12 | ...<... is false |
1236+
| tst.go:9:7:9:12 | case ...<... | tst.go:9:12:9:12 | ...<... is true |
1237+
| tst.go:9:11:9:12 | 23 | tst.go:9:7:9:12 | ...<... |
1238+
| tst.go:9:12:9:12 | ...<... is false | tst.go:12:1:12:1 | exit |
1239+
| tst.go:9:12:9:12 | ...<... is true | tst.go:9:2:9:13 | skip |
1240+
| tst.go:14:1:14:1 | entry | tst.go:14:13:14:17 | argument corresponding to value |
1241+
| tst.go:14:1:21:1 | function declaration | tst.go:0:0:0:0 | exit |
1242+
| tst.go:14:6:14:11 | skip | tst.go:14:1:21:1 | function declaration |
1243+
| tst.go:14:13:14:17 | argument corresponding to value | tst.go:14:13:14:17 | initialization of value |
1244+
| tst.go:14:13:14:17 | initialization of value | tst.go:15:2:15:2 | true |
1245+
| tst.go:15:2:15:2 | true | tst.go:16:7:16:11 | value |
1246+
| tst.go:15:2:15:2 | true | tst.go:21:1:21:1 | exit |
1247+
| tst.go:16:2:16:34 | skip | tst.go:21:1:21:1 | exit |
1248+
| tst.go:16:7:16:11 | value | tst.go:16:15:16:33 | ...*... |
1249+
| tst.go:16:7:16:33 | ...<... | tst.go:16:7:16:33 | case ...<... |
1250+
| tst.go:16:7:16:33 | case ...<... | tst.go:16:33:16:33 | ...<... is false |
1251+
| tst.go:16:7:16:33 | case ...<... | tst.go:16:33:16:33 | ...<... is true |
1252+
| tst.go:16:15:16:33 | ...*... | tst.go:16:7:16:33 | ...<... |
1253+
| tst.go:16:33:16:33 | ...<... is false | tst.go:18:7:18:11 | value |
1254+
| tst.go:16:33:16:33 | ...<... is true | tst.go:16:2:16:34 | skip |
1255+
| tst.go:18:2:18:39 | skip | tst.go:21:1:21:1 | exit |
1256+
| tst.go:18:7:18:11 | value | tst.go:18:15:18:38 | ...*... |
1257+
| tst.go:18:7:18:38 | ...<... | tst.go:18:7:18:38 | case ...<... |
1258+
| tst.go:18:7:18:38 | case ...<... | tst.go:18:38:18:38 | ...<... is false |
1259+
| tst.go:18:7:18:38 | case ...<... | tst.go:18:38:18:38 | ...<... is true |
1260+
| tst.go:18:15:18:38 | ...*... | tst.go:18:7:18:38 | ...<... |
1261+
| tst.go:18:38:18:38 | ...<... is false | tst.go:21:1:21:1 | exit |
1262+
| tst.go:18:38:18:38 | ...<... is true | tst.go:18:2:18:39 | skip |

‎ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/DuplicateSwitchCase.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
package main
2+
3+
func controller(msg string) {
4+
switch {
5+
case msg == "start":
6+
start()
7+
case msg == "start":
8+
stop()
9+
default:
10+
panic("Message not understood.")
11+
}
12+
}
13+
14+
func start() {}
15+
16+
func stop() {}

‎ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/tst.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
package main
2+
3+
func check(x int) {
4+
switch {
5+
case x < 23:
6+
7+
case x < 42:
8+
9+
case x < 23: // NOT OK
10+
11+
}
12+
}
13+
14+
func check2(value int64) {
15+
switch {
16+
case value < 1024*1024*1024*1024:
17+
18+
case value < 1024*1024*1024*1024*1024:
19+
20+
}
21+
}

‎ql/test/library-tests/semmle/go/frameworks/TaintSteps/crypto.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,4 +10,4 @@ func cryptoTest(key []byte, nonce []byte, ciphertext []byte) []byte {
1010
aesgcm, _ := cipher.NewGCM(block)
1111
plaintext, _ := aesgcm.Open(nil, nonce, ciphertext, nil)
1212
return plaintext
13-
}
13+
}

‎ql/test/query-tests/InconsistentCode/ConstantLengthComparison/tst.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
package main
22

33
func isPrefixOfGood2(xs, ys []int) bool {
4-
if len(ys) == 0 { // OK: not inside the loop
4+
if len(ys) == 0 { // OK: not inside the loop
55
return len(xs) == 0
66
}
77

@@ -11,4 +11,4 @@ func isPrefixOfGood2(xs, ys []int) bool {
1111
}
1212
}
1313
return true
14-
}
14+
}

0 commit comments

Comments
 (0)