Skip to content

Commit a8c6444

Browse files
owen-mcowen-mc
authored
Merge pull request #13645 from porcupineyhairs/goTiming
Go : Improvements to Timing Attacks query
2 parents df1e8e2 + 216911d commit a8c6444

File tree

3 files changed

+101
-20
lines changed

3 files changed

+101
-20
lines changed

‎go/ql/src/experimental/CWE-203/Timing.ql‎

Lines changed: 54 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,38 @@ private predicate isBadResult(DataFlow::Node e) {
2424
*/
2525
abstract class Sink extends DataFlow::Node { }
2626

27-
/** A taint-tracking sink which models comparisons of sensitive variables. */
28-
private class SensitiveCompareSink extends Sink {
29-
ComparisonExpr c;
27+
/** A taint-tracking sink which models comparisons of sensitive expressions using `strings.Compare` function. */
28+
private class SensitiveStringCompareSink extends Sink {
29+
SensitiveStringCompareSink() {
30+
// We select a comparison where a secret or password is tested.
31+
exists(DataFlow::CallNode c, Expr op1, Expr nonSensitiveOperand |
32+
c.getTarget().hasQualifiedName("strings", "Compare") and
33+
c.getArgument(_).asExpr() = op1 and
34+
op1.(SensitiveVariableAccess).getClassification() =
35+
[SensitiveExpr::secret(), SensitiveExpr::password()] and
36+
c.getArgument(_).asExpr() = nonSensitiveOperand and
37+
not op1 = nonSensitiveOperand and
38+
not (
39+
// Comparisons with `nil` should be excluded.
40+
nonSensitiveOperand = Builtin::nil().getAReference()
41+
or
42+
// Comparisons with empty string should also be excluded.
43+
nonSensitiveOperand.getStringValue().length() = 0
44+
)
45+
|
46+
// It is important to note that the name of both the operands need not be
47+
// `sensitive`. Even if one of the operands appears to be sensitive, we consider it a potential sink.
48+
nonSensitiveOperand = this.asExpr()
49+
)
50+
}
51+
}
3052

53+
/** A taint-tracking sink which models comparisons of sensitive expressions. */
54+
private class SensitiveCompareSink extends Sink {
3155
SensitiveCompareSink() {
3256
// We select a comparison where a secret or password is tested.
33-
exists(SensitiveVariableAccess op1, Expr op2 |
57+
exists(SensitiveExpr op1, Expr op2, EqualityTestExpr c |
3458
op1.getClassification() = [SensitiveExpr::secret(), SensitiveExpr::password()] and
35-
// exclude grant to avoid FP from OAuth
36-
not op1.getClassification().matches("%grant%") and
3759
op1 = c.getAnOperand() and
3860
op2 = c.getAnOperand() and
3961
not op1 = op2 and
@@ -45,13 +67,34 @@ private class SensitiveCompareSink extends Sink {
4567
op2.getStringValue().length() = 0
4668
)
4769
|
48-
// It is important to note that the name of both the operands need not be
49-
// `sensitive`. Even if one of the operands appears to be sensitive, we consider it a potential sink.
50-
c.getAnOperand() = this.asExpr()
70+
op2 = this.asExpr()
5171
)
5272
}
73+
}
5374

54-
DataFlow::Node getOtherOperand() { result.asExpr() = c.getAnOperand() and not result = this }
75+
/** A taint-tracking sink which models comparisons of sensitive strings. */
76+
private class SensitiveStringSink extends Sink {
77+
SensitiveStringSink() {
78+
// We select a comparison where a secret or password is tested.
79+
exists(StringLit op1, Expr op2, EqualityTestExpr c |
80+
op1.getStringValue()
81+
.regexpMatch(HeuristicNames::maybeSensitive([
82+
SensitiveExpr::secret(), SensitiveExpr::password()
83+
])) and
84+
op1 = c.getAnOperand() and
85+
op2 = c.getAnOperand() and
86+
not op1 = op2 and
87+
not (
88+
// Comparisons with `nil` should be excluded.
89+
op2 = Builtin::nil().getAReference()
90+
or
91+
// Comparisons with empty string should also be excluded.
92+
op2.getStringValue().length() = 0
93+
)
94+
|
95+
op2 = this.asExpr()
96+
)
97+
}
5598
}
5699

57100
class SecretTracking extends TaintTracking::Configuration {
@@ -65,8 +108,6 @@ class SecretTracking extends TaintTracking::Configuration {
65108
}
66109

67110
from SecretTracking cfg, DataFlow::PathNode source, DataFlow::PathNode sink
68-
where
69-
cfg.hasFlowPath(source, sink) and
70-
not cfg.hasFlowTo(sink.getNode().(SensitiveCompareSink).getOtherOperand())
111+
where cfg.hasFlowPath(source, sink)
71112
select sink.getNode(), source, sink, "$@ may be vulnerable to timing attacks.", source.getNode(),
72113
"Hardcoded String"

‎go/ql/test/experimental/CWE-203/Timing.expected‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,22 @@
11
edges
2-
| timing.go:14:18:14:27 | selection of Header | timing.go:14:18:14:45 | call to Get |
3-
| timing.go:14:18:14:45 | call to Get | timing.go:16:25:16:36 | headerSecret |
2+
| timing.go:15:18:15:27 | selection of Header | timing.go:15:18:15:45 | call to Get |
3+
| timing.go:15:18:15:45 | call to Get | timing.go:17:31:17:42 | headerSecret |
4+
| timing.go:28:18:28:27 | selection of Header | timing.go:28:18:28:45 | call to Get |
5+
| timing.go:28:18:28:45 | call to Get | timing.go:30:47:30:58 | headerSecret |
6+
| timing.go:41:18:41:27 | selection of Header | timing.go:41:18:41:45 | call to Get |
7+
| timing.go:41:18:41:45 | call to Get | timing.go:42:25:42:36 | headerSecret |
48
nodes
5-
| timing.go:14:18:14:27 | selection of Header | semmle.label | selection of Header |
6-
| timing.go:14:18:14:45 | call to Get | semmle.label | call to Get |
7-
| timing.go:16:25:16:36 | headerSecret | semmle.label | headerSecret |
9+
| timing.go:15:18:15:27 | selection of Header | semmle.label | selection of Header |
10+
| timing.go:15:18:15:45 | call to Get | semmle.label | call to Get |
11+
| timing.go:17:31:17:42 | headerSecret | semmle.label | headerSecret |
12+
| timing.go:28:18:28:27 | selection of Header | semmle.label | selection of Header |
13+
| timing.go:28:18:28:45 | call to Get | semmle.label | call to Get |
14+
| timing.go:30:47:30:58 | headerSecret | semmle.label | headerSecret |
15+
| timing.go:41:18:41:27 | selection of Header | semmle.label | selection of Header |
16+
| timing.go:41:18:41:45 | call to Get | semmle.label | call to Get |
17+
| timing.go:42:25:42:36 | headerSecret | semmle.label | headerSecret |
818
subpaths
919
#select
10-
| timing.go:16:25:16:36 | headerSecret | timing.go:14:18:14:27 | selection of Header | timing.go:16:25:16:36 | headerSecret | $@ may be vulnerable to timing attacks. | timing.go:14:18:14:27 | selection of Header | Hardcoded String |
20+
| timing.go:17:31:17:42 | headerSecret | timing.go:15:18:15:27 | selection of Header | timing.go:17:31:17:42 | headerSecret | $@ may be vulnerable to timing attacks. | timing.go:15:18:15:27 | selection of Header | Hardcoded String |
21+
| timing.go:30:47:30:58 | headerSecret | timing.go:28:18:28:27 | selection of Header | timing.go:30:47:30:58 | headerSecret | $@ may be vulnerable to timing attacks. | timing.go:28:18:28:27 | selection of Header | Hardcoded String |
22+
| timing.go:42:25:42:36 | headerSecret | timing.go:41:18:41:27 | selection of Header | timing.go:42:25:42:36 | headerSecret | $@ may be vulnerable to timing attacks. | timing.go:41:18:41:27 | selection of Header | Hardcoded String |

‎go/ql/test/experimental/CWE-203/timing.go‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"crypto/subtle"
55
"fmt"
66
"net/http"
7+
"strings"
78
)
89

910
func bad(w http.ResponseWriter, req *http.Request) (interface{}, error) {
@@ -13,7 +14,32 @@ func bad(w http.ResponseWriter, req *http.Request) (interface{}, error) {
1314

1415
headerSecret := req.Header.Get(secretHeader)
1516
secretStr := string(secret)
16-
if len(secret) != 0 && headerSecret != secretStr {
17+
if len(headerSecret) != 0 && headerSecret != secretStr {
18+
return nil, fmt.Errorf("header %s=%s did not match expected secret", secretHeader, headerSecret)
19+
}
20+
return nil, nil
21+
}
22+
23+
func bad2(w http.ResponseWriter, req *http.Request) (interface{}, error) {
24+
25+
secret := "MySuperSecretPasscode"
26+
secretHeader := "X-Secret"
27+
28+
headerSecret := req.Header.Get(secretHeader)
29+
secretStr := string(secret)
30+
if len(headerSecret) != 0 && strings.Compare(headerSecret, secretStr) != 0 {
31+
return nil, fmt.Errorf("header %s=%s did not match expected secret", secretHeader, headerSecret)
32+
}
33+
return nil, nil
34+
}
35+
36+
func bad4(w http.ResponseWriter, req *http.Request) (interface{}, error) {
37+
38+
secret := "MySuperSecretPasscode"
39+
secretHeader := "X-Secret"
40+
41+
headerSecret := req.Header.Get(secretHeader)
42+
if len(secret) != 0 && headerSecret != "SecretStringLiteral" {
1743
return nil, fmt.Errorf("header %s=%s did not match expected secret", secretHeader, headerSecret)
1844
}
1945
return nil, nil
@@ -34,4 +60,6 @@ func good(w http.ResponseWriter, req *http.Request) (interface{}, error) {
3460
func main() {
3561
bad(nil, nil)
3662
good(nil, nil)
63+
bad2(nil, nil)
64+
bad4(nil, nil)
3765
}

0 commit comments

Comments
 (0)