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

Commit 99f0875

Browse files
smowtonsmowton
committed
Polish CWE-322: detect and exclude cases where host-checking is optional
1 parent e9ae697 commit 99f0875

File tree

3 files changed

+118
-29
lines changed

3 files changed

+118
-29
lines changed

‎ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql

Lines changed: 67 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,27 +2,48 @@
22
* @name Use of insecure HostKeyCallback implementation
33
* @description Detects insecure SSL client configurations with an implementation of the `HostKeyCallback` that accepts all host keys.
44
* @kind path-problem
5-
* @problem.severity error
5+
* @problem.severity warning
6+
* @precision high
67
* @id go/insecure-hostkeycallback
78
* @tags security
89
*/
910

1011
import go
1112
import DataFlow::PathGraph
1213

14+
/** The `ssh.InsecureIgnoreHostKey` function, which allows connecting to any host regardless of its host key. */
1315
class InsecureIgnoreHostKey extends Function {
1416
InsecureIgnoreHostKey() {
1517
this.hasQualifiedName("golang.org/x/crypto/ssh", "InsecureIgnoreHostKey")
1618
}
1719
}
1820

21+
/** An SSH host-key checking function. */
22+
class HostKeyCallbackFunc extends DataFlow::Node {
23+
HostKeyCallbackFunc() {
24+
exists(NamedType nt | nt.hasQualifiedName("golang.org/x/crypto/ssh", "HostKeyCallback") |
25+
getType().getUnderlyingType() = nt.getUnderlyingType()
26+
) and
27+
// Restrict possible sources to either function definitions or
28+
// the result of some external function call (e.g. InsecureIgnoreHostKey())
29+
// Otherwise we also consider typecasts, variables and other such intermediates
30+
// as sources.
31+
(
32+
this instanceof DataFlow::FunctionNode
33+
or
34+
this instanceof DataFlow::CallNode and
35+
not exists(this.(DataFlow::CallNode).getACallee().getBody())
36+
)
37+
}
38+
}
39+
1940
/** A callback function value that is insecure when used as a `HostKeyCallback`, because it always returns `nil`. */
20-
class InsecureHostKeyCallbackFunc extends DataFlow::Node {
41+
class InsecureHostKeyCallbackFunc extends HostKeyCallbackFunc {
2142
InsecureHostKeyCallbackFunc() {
2243
// Either a call to InsecureIgnoreHostKey(), which we know returns an insecure callback.
2344
this = any(InsecureIgnoreHostKey f).getACall()
2445
or
25-
// Or a callback function in the source code (named or anonymous) that always returns nil.
46+
// Or a callback function in the source code (named or anonymous) that always returns nil.
2647
forex(DataFlow::ResultNode returnValue |
2748
returnValue = this.(DataFlow::FunctionNode).getAResult()
2849
|
@@ -31,23 +52,60 @@ class InsecureHostKeyCallbackFunc extends DataFlow::Node {
3152
}
3253
}
3354

55+
/**
56+
* A data-flow configuration for identifying `HostKeyCallbackFunc` instances that reach `ClientConfig.HostKeyCallback` fields.
57+
*/
3458
class HostKeyCallbackAssignmentConfig extends DataFlow::Configuration {
3559
HostKeyCallbackAssignmentConfig() { this = "HostKeyCallbackAssignmentConfig" }
3660

37-
override predicate isSource(DataFlow::Node source) {
38-
source instanceof InsecureHostKeyCallbackFunc
39-
}
61+
override predicate isSource(DataFlow::Node source) { source instanceof HostKeyCallbackFunc }
4062

41-
override predicate isSink(DataFlow::Node sink) {
63+
/**
64+
* Holds if `sink` is a value written by `write` to a field `ClientConfig.HostKeyCallback`.
65+
*/
66+
predicate isSink(DataFlow::Node sink, Write write) {
4267
exists(Field f |
4368
f.hasQualifiedName("golang.org/x/crypto/ssh", "ClientConfig", "HostKeyCallback") and
44-
sink = f.getAWrite().getRhs()
69+
write.writesField(_, f, sink)
4570
)
4671
}
72+
73+
override predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
74+
}
75+
76+
/**
77+
* Holds if a secure host-check function reaches `sink` or another similar sink.
78+
*
79+
* A sink is considered similar if it writes to the same variable and field.
80+
*/
81+
predicate hostCheckReachesSink(DataFlow::PathNode sink) {
82+
exists(HostKeyCallbackAssignmentConfig config, DataFlow::PathNode source |
83+
not source.getNode() instanceof InsecureHostKeyCallbackFunc and
84+
(
85+
config.hasFlowPath(source, sink)
86+
or
87+
exists(
88+
DataFlow::PathNode otherSink, Write sinkWrite, Write otherSinkWrite,
89+
SsaWithFields sinkAccessPath, SsaWithFields otherSinkAccessPath
90+
|
91+
config.hasFlowPath(source, otherSink) and
92+
config.isSink(sink.getNode(), sinkWrite) and
93+
config.isSink(otherSink.getNode(), otherSinkWrite) and
94+
sinkWrite.writesField(sinkAccessPath.getAUse(), _, sink.getNode()) and
95+
otherSinkWrite.writesField(otherSinkAccessPath.getAUse(), _, otherSink.getNode()) and
96+
otherSinkAccessPath = sinkAccessPath.similar()
97+
)
98+
)
99+
)
47100
}
48101

49102
from HostKeyCallbackAssignmentConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
50-
where config.hasFlowPath(source, sink)
103+
where
104+
config.hasFlowPath(source, sink) and
105+
source.getNode() instanceof InsecureHostKeyCallbackFunc and
106+
// Exclude cases where a good access-path function reaches the same or a similar sink
107+
// (these probably indicate optional host-checking)
108+
not hostCheckReachesSink(sink)
51109
select sink, source, sink,
52110
"Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@.",
53111
source.getNode(), "this source"

‎ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,18 @@ edges
33
| InsecureHostKeyCallbackExample.go:27:14:30:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback |
44
| InsecureHostKeyCallbackExample.go:28:3:30:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:27:14:30:4 | type conversion : signature type |
55
| InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion |
6-
| InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : HostKeyCallback | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback |
7-
| InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback |
8-
| InsecureHostKeyCallbackExample.go:63:22:66:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:68:35:68:50 | insecureCallback : signature type |
9-
| InsecureHostKeyCallbackExample.go:64:3:66:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:63:22:66:4 | type conversion : signature type |
10-
| InsecureHostKeyCallbackExample.go:68:35:68:50 | insecureCallback : signature type | InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : signature type |
11-
| InsecureHostKeyCallbackExample.go:79:35:79:61 | call to InsecureIgnoreHostKey : HostKeyCallback | InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : HostKeyCallback |
6+
| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : HostKeyCallback | InsecureHostKeyCallbackExample.go:58:20:58:27 | callback |
7+
| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:58:20:58:27 | callback |
8+
| InsecureHostKeyCallbackExample.go:64:48:64:55 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:74:28:74:35 | callback |
9+
| InsecureHostKeyCallbackExample.go:81:22:84:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:86:35:86:50 | insecureCallback : signature type |
10+
| InsecureHostKeyCallbackExample.go:82:3:84:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:81:22:84:4 | type conversion : signature type |
11+
| InsecureHostKeyCallbackExample.go:86:35:86:50 | insecureCallback : signature type | InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type |
12+
| InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:96:35:96:59 | potentiallySecureCallback : signature type |
13+
| InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:99:44:99:68 | potentiallySecureCallback : signature type |
14+
| InsecureHostKeyCallbackExample.go:89:3:94:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type |
15+
| InsecureHostKeyCallbackExample.go:96:35:96:59 | potentiallySecureCallback : signature type | InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type |
16+
| InsecureHostKeyCallbackExample.go:97:35:97:61 | call to InsecureIgnoreHostKey : HostKeyCallback | InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : HostKeyCallback |
17+
| InsecureHostKeyCallbackExample.go:99:44:99:68 | potentiallySecureCallback : signature type | InsecureHostKeyCallbackExample.go:64:48:64:55 | definition of callback : signature type |
1218
nodes
1319
| InsecureHostKeyCallbackExample.go:11:20:14:5 | type conversion | semmle.label | type conversion |
1420
| InsecureHostKeyCallbackExample.go:12:4:14:4 | function literal : signature type | semmle.label | function literal : signature type |
@@ -18,17 +24,22 @@ nodes
1824
| InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | semmle.label | callback |
1925
| InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal : signature type | semmle.label | function literal : signature type |
2026
| InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | semmle.label | type conversion |
21-
| InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : HostKeyCallback | semmle.label | definition of callback : HostKeyCallback |
22-
| InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : signature type | semmle.label | definition of callback : signature type |
23-
| InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | semmle.label | callback |
24-
| InsecureHostKeyCallbackExample.go:63:22:66:4 | type conversion : signature type | semmle.label | type conversion : signature type |
25-
| InsecureHostKeyCallbackExample.go:64:3:66:3 | function literal : signature type | semmle.label | function literal : signature type |
26-
| InsecureHostKeyCallbackExample.go:68:35:68:50 | insecureCallback : signature type | semmle.label | insecureCallback : signature type |
27-
| InsecureHostKeyCallbackExample.go:79:35:79:61 | call to InsecureIgnoreHostKey : HostKeyCallback | semmle.label | call to InsecureIgnoreHostKey : HostKeyCallback |
27+
| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : HostKeyCallback | semmle.label | definition of callback : HostKeyCallback |
28+
| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type | semmle.label | definition of callback : signature type |
29+
| InsecureHostKeyCallbackExample.go:58:20:58:27 | callback | semmle.label | callback |
30+
| InsecureHostKeyCallbackExample.go:64:48:64:55 | definition of callback : signature type | semmle.label | definition of callback : signature type |
31+
| InsecureHostKeyCallbackExample.go:72:28:72:54 | call to InsecureIgnoreHostKey | semmle.label | call to InsecureIgnoreHostKey |
32+
| InsecureHostKeyCallbackExample.go:74:28:74:35 | callback | semmle.label | callback |
33+
| InsecureHostKeyCallbackExample.go:81:22:84:4 | type conversion : signature type | semmle.label | type conversion : signature type |
34+
| InsecureHostKeyCallbackExample.go:82:3:84:3 | function literal : signature type | semmle.label | function literal : signature type |
35+
| InsecureHostKeyCallbackExample.go:86:35:86:50 | insecureCallback : signature type | semmle.label | insecureCallback : signature type |
36+
| InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type | semmle.label | type conversion : signature type |
37+
| InsecureHostKeyCallbackExample.go:89:3:94:3 | function literal : signature type | semmle.label | function literal : signature type |
38+
| InsecureHostKeyCallbackExample.go:96:35:96:59 | potentiallySecureCallback : signature type | semmle.label | potentiallySecureCallback : signature type |
39+
| InsecureHostKeyCallbackExample.go:97:35:97:61 | call to InsecureIgnoreHostKey : HostKeyCallback | semmle.label | call to InsecureIgnoreHostKey : HostKeyCallback |
40+
| InsecureHostKeyCallbackExample.go:99:44:99:68 | potentiallySecureCallback : signature type | semmle.label | potentiallySecureCallback : signature type |
2841
#select
2942
| InsecureHostKeyCallbackExample.go:11:20:14:5 | type conversion | InsecureHostKeyCallbackExample.go:12:4:14:4 | function literal : signature type | InsecureHostKeyCallbackExample.go:11:20:14:5 | type conversion | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:12:4:14:4 | function literal | this source |
3043
| InsecureHostKeyCallbackExample.go:22:20:22:46 | call to InsecureIgnoreHostKey | InsecureHostKeyCallbackExample.go:22:20:22:46 | call to InsecureIgnoreHostKey | InsecureHostKeyCallbackExample.go:22:20:22:46 | call to InsecureIgnoreHostKey | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:22:20:22:46 | call to InsecureIgnoreHostKey | this source |
3144
| InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | InsecureHostKeyCallbackExample.go:28:3:30:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:28:3:30:3 | function literal | this source |
3245
| InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal | this source |
33-
| InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | InsecureHostKeyCallbackExample.go:64:3:66:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:64:3:66:3 | function literal | this source |
34-
| InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | InsecureHostKeyCallbackExample.go:79:35:79:61 | call to InsecureIgnoreHostKey : HostKeyCallback | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:79:35:79:61 | call to InsecureIgnoreHostKey | this source |

‎ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ func insecureSSHClientConfig() {
88
_ = &ssh.ClientConfig{
99
User: "user",
1010
Auth: []ssh.AuthMethod{nil},
11-
HostKeyCallback: ssh.HostKeyCallback(
11+
HostKeyCallback: ssh.HostKeyCallback( // BAD
1212
func(hostname string, remote net.Addr, key ssh.PublicKey) error {
1313
return nil
1414
}),
@@ -19,7 +19,7 @@ func insecureSSHClientConfigAlt() {
1919
_ = &ssh.ClientConfig{
2020
User: "user",
2121
Auth: []ssh.AuthMethod{nil},
22-
HostKeyCallback: ssh.InsecureIgnoreHostKey(),
22+
HostKeyCallback: ssh.InsecureIgnoreHostKey(), // BAD
2323
}
2424
}
2525

@@ -32,7 +32,7 @@ func insecureSSHClientConfigLocalFlow() {
3232
_ = &ssh.ClientConfig{
3333
User: "user",
3434
Auth: []ssh.AuthMethod{nil},
35-
HostKeyCallback: callback,
35+
HostKeyCallback: callback, // BAD
3636
}
3737
}
3838

@@ -45,15 +45,33 @@ func insecureSSHClientConfigLocalFlowAlt() {
4545
_ = &ssh.ClientConfig{
4646
User: "user",
4747
Auth: []ssh.AuthMethod{nil},
48-
HostKeyCallback: ssh.HostKeyCallback(callback),
48+
HostKeyCallback: ssh.HostKeyCallback(callback), // BAD
4949
}
5050
}
5151

52+
// Check that insecure and secure functions flowing together to the same
53+
// sink is not flagged (we assume this is configurable security)
5254
func potentialInsecureSSHClientConfig(callback ssh.HostKeyCallback) {
5355
_ = &ssh.ClientConfig{
5456
User: "user",
5557
Auth: []ssh.AuthMethod{nil},
56-
HostKeyCallback: callback,
58+
HostKeyCallback: callback, // OK
59+
}
60+
}
61+
62+
// Check that insecure and secure functions flowing to different writes to
63+
// the same objects are not flagged (we assume this is configurable security)
64+
func potentialInsecureSSHClientConfigTwoWrites(callback ssh.HostKeyCallback) {
65+
config := &ssh.ClientConfig{
66+
User: "user",
67+
Auth: []ssh.AuthMethod{nil},
68+
HostKeyCallback: nil,
69+
}
70+
71+
if callback == nil {
72+
config.HostKeyCallback = ssh.InsecureIgnoreHostKey() // OK
73+
} else {
74+
config.HostKeyCallback = callback
5775
}
5876
}
5977

@@ -77,4 +95,6 @@ func main() {
7795

7896
potentialInsecureSSHClientConfig(potentiallySecureCallback)
7997
potentialInsecureSSHClientConfig(ssh.InsecureIgnoreHostKey())
98+
99+
potentialInsecureSSHClientConfigTwoWrites(potentiallySecureCallback)
80100
}

0 commit comments

Comments
 (0)