From 99f08750f3cae90bf98df0f4f273d95210ea6d0e Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 17 Jul 2020 16:26:44 +0100 Subject: [PATCH 1/4] Polish CWE-322: detect and exclude cases where host-checking is optional --- .../CWE-322/InsecureHostKeyCallback.ql | 76 ++++++++++++++++--- .../CWE-322/InsecureHostKeyCallback.expected | 41 ++++++---- .../CWE-322/InsecureHostKeyCallbackExample.go | 30 ++++++-- 3 files changed, 118 insertions(+), 29 deletions(-) diff --git a/ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql b/ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql index 35df89e1a77..8f3ebe03973 100644 --- a/ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql +++ b/ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql @@ -2,7 +2,8 @@ * @name Use of insecure HostKeyCallback implementation * @description Detects insecure SSL client configurations with an implementation of the `HostKeyCallback` that accepts all host keys. * @kind path-problem - * @problem.severity error + * @problem.severity warning + * @precision high * @id go/insecure-hostkeycallback * @tags security */ @@ -10,19 +11,39 @@ import go import DataFlow::PathGraph +/** The `ssh.InsecureIgnoreHostKey` function, which allows connecting to any host regardless of its host key. */ class InsecureIgnoreHostKey extends Function { InsecureIgnoreHostKey() { this.hasQualifiedName("golang.org/x/crypto/ssh", "InsecureIgnoreHostKey") } } +/** An SSH host-key checking function. */ +class HostKeyCallbackFunc extends DataFlow::Node { + HostKeyCallbackFunc() { + exists(NamedType nt | nt.hasQualifiedName("golang.org/x/crypto/ssh", "HostKeyCallback") | + getType().getUnderlyingType() = nt.getUnderlyingType() + ) and + // Restrict possible sources to either function definitions or + // the result of some external function call (e.g. InsecureIgnoreHostKey()) + // Otherwise we also consider typecasts, variables and other such intermediates + // as sources. + ( + this instanceof DataFlow::FunctionNode + or + this instanceof DataFlow::CallNode and + not exists(this.(DataFlow::CallNode).getACallee().getBody()) + ) + } +} + /** A callback function value that is insecure when used as a `HostKeyCallback`, because it always returns `nil`. */ -class InsecureHostKeyCallbackFunc extends DataFlow::Node { +class InsecureHostKeyCallbackFunc extends HostKeyCallbackFunc { InsecureHostKeyCallbackFunc() { // Either a call to InsecureIgnoreHostKey(), which we know returns an insecure callback. this = any(InsecureIgnoreHostKey f).getACall() or - // Or a callback function in the source code (named or anonymous) that always returns nil. + // Or a callback function in the source code (named or anonymous) that always returns nil. forex(DataFlow::ResultNode returnValue | returnValue = this.(DataFlow::FunctionNode).getAResult() | @@ -31,23 +52,60 @@ class InsecureHostKeyCallbackFunc extends DataFlow::Node { } } +/** + * A data-flow configuration for identifying `HostKeyCallbackFunc` instances that reach `ClientConfig.HostKeyCallback` fields. + */ class HostKeyCallbackAssignmentConfig extends DataFlow::Configuration { HostKeyCallbackAssignmentConfig() { this = "HostKeyCallbackAssignmentConfig" } - override predicate isSource(DataFlow::Node source) { - source instanceof InsecureHostKeyCallbackFunc - } + override predicate isSource(DataFlow::Node source) { source instanceof HostKeyCallbackFunc } - override predicate isSink(DataFlow::Node sink) { + /** + * Holds if `sink` is a value written by `write` to a field `ClientConfig.HostKeyCallback`. + */ + predicate isSink(DataFlow::Node sink, Write write) { exists(Field f | f.hasQualifiedName("golang.org/x/crypto/ssh", "ClientConfig", "HostKeyCallback") and - sink = f.getAWrite().getRhs() + write.writesField(_, f, sink) ) } + + override predicate isSink(DataFlow::Node sink) { isSink(sink, _) } +} + +/** + * Holds if a secure host-check function reaches `sink` or another similar sink. + * + * A sink is considered similar if it writes to the same variable and field. + */ +predicate hostCheckReachesSink(DataFlow::PathNode sink) { + exists(HostKeyCallbackAssignmentConfig config, DataFlow::PathNode source | + not source.getNode() instanceof InsecureHostKeyCallbackFunc and + ( + config.hasFlowPath(source, sink) + or + exists( + DataFlow::PathNode otherSink, Write sinkWrite, Write otherSinkWrite, + SsaWithFields sinkAccessPath, SsaWithFields otherSinkAccessPath + | + config.hasFlowPath(source, otherSink) and + config.isSink(sink.getNode(), sinkWrite) and + config.isSink(otherSink.getNode(), otherSinkWrite) and + sinkWrite.writesField(sinkAccessPath.getAUse(), _, sink.getNode()) and + otherSinkWrite.writesField(otherSinkAccessPath.getAUse(), _, otherSink.getNode()) and + otherSinkAccessPath = sinkAccessPath.similar() + ) + ) + ) } from HostKeyCallbackAssignmentConfig config, DataFlow::PathNode source, DataFlow::PathNode sink -where config.hasFlowPath(source, sink) +where + config.hasFlowPath(source, sink) and + source.getNode() instanceof InsecureHostKeyCallbackFunc and + // Exclude cases where a good access-path function reaches the same or a similar sink + // (these probably indicate optional host-checking) + not hostCheckReachesSink(sink) select sink, source, sink, "Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@.", source.getNode(), "this source" diff --git a/ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected b/ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected index 07ed8c2522b..6ab43145857 100644 --- a/ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected +++ b/ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected @@ -3,12 +3,18 @@ edges | InsecureHostKeyCallbackExample.go:27:14:30:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | | InsecureHostKeyCallbackExample.go:28:3:30:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:27:14:30:4 | type conversion : signature type | | InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | -| InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : HostKeyCallback | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | -| InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | -| InsecureHostKeyCallbackExample.go:63:22:66:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:68:35:68:50 | insecureCallback : signature type | -| InsecureHostKeyCallbackExample.go:64:3:66:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:63:22:66:4 | type conversion : signature type | -| InsecureHostKeyCallbackExample.go:68:35:68:50 | insecureCallback : signature type | InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : signature type | -| InsecureHostKeyCallbackExample.go:79:35:79:61 | call to InsecureIgnoreHostKey : HostKeyCallback | InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : HostKeyCallback | +| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : HostKeyCallback | InsecureHostKeyCallbackExample.go:58:20:58:27 | callback | +| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:58:20:58:27 | callback | +| InsecureHostKeyCallbackExample.go:64:48:64:55 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:74:28:74:35 | callback | +| InsecureHostKeyCallbackExample.go:81:22:84:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:86:35:86:50 | insecureCallback : signature type | +| InsecureHostKeyCallbackExample.go:82:3:84:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:81:22:84:4 | type conversion : signature type | +| InsecureHostKeyCallbackExample.go:86:35:86:50 | insecureCallback : signature type | InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type | +| InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:96:35:96:59 | potentiallySecureCallback : signature type | +| InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:99:44:99:68 | potentiallySecureCallback : signature type | +| InsecureHostKeyCallbackExample.go:89:3:94:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type | +| InsecureHostKeyCallbackExample.go:96:35:96:59 | potentiallySecureCallback : signature type | InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type | +| InsecureHostKeyCallbackExample.go:97:35:97:61 | call to InsecureIgnoreHostKey : HostKeyCallback | InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : HostKeyCallback | +| InsecureHostKeyCallbackExample.go:99:44:99:68 | potentiallySecureCallback : signature type | InsecureHostKeyCallbackExample.go:64:48:64:55 | definition of callback : signature type | nodes | InsecureHostKeyCallbackExample.go:11:20:14:5 | type conversion | semmle.label | type conversion | | InsecureHostKeyCallbackExample.go:12:4:14:4 | function literal : signature type | semmle.label | function literal : signature type | @@ -18,17 +24,22 @@ nodes | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | semmle.label | callback | | InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal : signature type | semmle.label | function literal : signature type | | InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | semmle.label | type conversion | -| InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : HostKeyCallback | semmle.label | definition of callback : HostKeyCallback | -| InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : signature type | semmle.label | definition of callback : signature type | -| InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | semmle.label | callback | -| InsecureHostKeyCallbackExample.go:63:22:66:4 | type conversion : signature type | semmle.label | type conversion : signature type | -| InsecureHostKeyCallbackExample.go:64:3:66:3 | function literal : signature type | semmle.label | function literal : signature type | -| InsecureHostKeyCallbackExample.go:68:35:68:50 | insecureCallback : signature type | semmle.label | insecureCallback : signature type | -| InsecureHostKeyCallbackExample.go:79:35:79:61 | call to InsecureIgnoreHostKey : HostKeyCallback | semmle.label | call to InsecureIgnoreHostKey : HostKeyCallback | +| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : HostKeyCallback | semmle.label | definition of callback : HostKeyCallback | +| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type | semmle.label | definition of callback : signature type | +| InsecureHostKeyCallbackExample.go:58:20:58:27 | callback | semmle.label | callback | +| InsecureHostKeyCallbackExample.go:64:48:64:55 | definition of callback : signature type | semmle.label | definition of callback : signature type | +| InsecureHostKeyCallbackExample.go:72:28:72:54 | call to InsecureIgnoreHostKey | semmle.label | call to InsecureIgnoreHostKey | +| InsecureHostKeyCallbackExample.go:74:28:74:35 | callback | semmle.label | callback | +| InsecureHostKeyCallbackExample.go:81:22:84:4 | type conversion : signature type | semmle.label | type conversion : signature type | +| InsecureHostKeyCallbackExample.go:82:3:84:3 | function literal : signature type | semmle.label | function literal : signature type | +| InsecureHostKeyCallbackExample.go:86:35:86:50 | insecureCallback : signature type | semmle.label | insecureCallback : signature type | +| InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type | semmle.label | type conversion : signature type | +| InsecureHostKeyCallbackExample.go:89:3:94:3 | function literal : signature type | semmle.label | function literal : signature type | +| InsecureHostKeyCallbackExample.go:96:35:96:59 | potentiallySecureCallback : signature type | semmle.label | potentiallySecureCallback : signature type | +| InsecureHostKeyCallbackExample.go:97:35:97:61 | call to InsecureIgnoreHostKey : HostKeyCallback | semmle.label | call to InsecureIgnoreHostKey : HostKeyCallback | +| InsecureHostKeyCallbackExample.go:99:44:99:68 | potentiallySecureCallback : signature type | semmle.label | potentiallySecureCallback : signature type | #select | 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 | | 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 | | 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 | | 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 | -| 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 | -| 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 | diff --git a/ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go b/ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go index f9870e4937e..7fc9de0b9d4 100644 --- a/ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go +++ b/ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go @@ -8,7 +8,7 @@ func insecureSSHClientConfig() { _ = &ssh.ClientConfig{ User: "user", Auth: []ssh.AuthMethod{nil}, - HostKeyCallback: ssh.HostKeyCallback( + HostKeyCallback: ssh.HostKeyCallback( // BAD func(hostname string, remote net.Addr, key ssh.PublicKey) error { return nil }), @@ -19,7 +19,7 @@ func insecureSSHClientConfigAlt() { _ = &ssh.ClientConfig{ User: "user", Auth: []ssh.AuthMethod{nil}, - HostKeyCallback: ssh.InsecureIgnoreHostKey(), + HostKeyCallback: ssh.InsecureIgnoreHostKey(), // BAD } } @@ -32,7 +32,7 @@ func insecureSSHClientConfigLocalFlow() { _ = &ssh.ClientConfig{ User: "user", Auth: []ssh.AuthMethod{nil}, - HostKeyCallback: callback, + HostKeyCallback: callback, // BAD } } @@ -45,15 +45,33 @@ func insecureSSHClientConfigLocalFlowAlt() { _ = &ssh.ClientConfig{ User: "user", Auth: []ssh.AuthMethod{nil}, - HostKeyCallback: ssh.HostKeyCallback(callback), + HostKeyCallback: ssh.HostKeyCallback(callback), // BAD } } +// Check that insecure and secure functions flowing together to the same +// sink is not flagged (we assume this is configurable security) func potentialInsecureSSHClientConfig(callback ssh.HostKeyCallback) { _ = &ssh.ClientConfig{ User: "user", Auth: []ssh.AuthMethod{nil}, - HostKeyCallback: callback, + HostKeyCallback: callback, // OK + } +} + +// Check that insecure and secure functions flowing to different writes to +// the same objects are not flagged (we assume this is configurable security) +func potentialInsecureSSHClientConfigTwoWrites(callback ssh.HostKeyCallback) { + config := &ssh.ClientConfig{ + User: "user", + Auth: []ssh.AuthMethod{nil}, + HostKeyCallback: nil, + } + + if callback == nil { + config.HostKeyCallback = ssh.InsecureIgnoreHostKey() // OK + } else { + config.HostKeyCallback = callback } } @@ -77,4 +95,6 @@ func main() { potentialInsecureSSHClientConfig(potentiallySecureCallback) potentialInsecureSSHClientConfig(ssh.InsecureIgnoreHostKey()) + + potentialInsecureSSHClientConfigTwoWrites(potentiallySecureCallback) } From f162a5be94028a0b8f6010c8fb76066b972734e6 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 28 Jul 2020 15:36:38 +0100 Subject: [PATCH 2/4] Promote CWE-322 out of experimental status --- change-notes/2020-07-22-ssh-host-checking.md | 2 ++ .../CWE-322/InsecureHostKeyCallback.qhelp | 0 .../CWE-322/InsecureHostKeyCallback.ql | 0 .../CWE-322/InsecureHostKeyCallbackExample.go | 0 .../CWE-322/SecureHostKeyCallbackExample.go | 0 ql/test/experimental/CWE-322/InsecureHostKeyCallback.qlref | 1 - .../Security}/CWE-322/InsecureHostKeyCallback.expected | 0 .../query-tests/Security/CWE-322/InsecureHostKeyCallback.qlref | 1 + .../Security}/CWE-322/InsecureHostKeyCallbackExample.go | 0 ql/test/{experimental => query-tests/Security}/CWE-322/go.mod | 0 .../Security}/CWE-322/vendor/golang.org/LICENSE | 0 .../Security}/CWE-322/vendor/golang.org/x/crypto/ssh/stub.go | 0 .../Security}/CWE-322/vendor/modules.txt | 0 13 files changed, 3 insertions(+), 1 deletion(-) create mode 100644 change-notes/2020-07-22-ssh-host-checking.md rename ql/src/{experimental => Security}/CWE-322/InsecureHostKeyCallback.qhelp (100%) rename ql/src/{experimental => Security}/CWE-322/InsecureHostKeyCallback.ql (100%) rename ql/src/{experimental => Security}/CWE-322/InsecureHostKeyCallbackExample.go (100%) rename ql/src/{experimental => Security}/CWE-322/SecureHostKeyCallbackExample.go (100%) delete mode 100644 ql/test/experimental/CWE-322/InsecureHostKeyCallback.qlref rename ql/test/{experimental => query-tests/Security}/CWE-322/InsecureHostKeyCallback.expected (100%) create mode 100644 ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.qlref rename ql/test/{experimental => query-tests/Security}/CWE-322/InsecureHostKeyCallbackExample.go (100%) rename ql/test/{experimental => query-tests/Security}/CWE-322/go.mod (100%) rename ql/test/{experimental => query-tests/Security}/CWE-322/vendor/golang.org/LICENSE (100%) rename ql/test/{experimental => query-tests/Security}/CWE-322/vendor/golang.org/x/crypto/ssh/stub.go (100%) rename ql/test/{experimental => query-tests/Security}/CWE-322/vendor/modules.txt (100%) diff --git a/change-notes/2020-07-22-ssh-host-checking.md b/change-notes/2020-07-22-ssh-host-checking.md new file mode 100644 index 00000000000..7f83626bf0e --- /dev/null +++ b/change-notes/2020-07-22-ssh-host-checking.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* Query "Use of insecure HostKeyCallback implementation" (`go/insecure-hostkeycallback`) is promoted from experimental status. This checks for insecurely omitting SSH host-key verification. diff --git a/ql/src/experimental/CWE-322/InsecureHostKeyCallback.qhelp b/ql/src/Security/CWE-322/InsecureHostKeyCallback.qhelp similarity index 100% rename from ql/src/experimental/CWE-322/InsecureHostKeyCallback.qhelp rename to ql/src/Security/CWE-322/InsecureHostKeyCallback.qhelp diff --git a/ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql b/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql similarity index 100% rename from ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql rename to ql/src/Security/CWE-322/InsecureHostKeyCallback.ql diff --git a/ql/src/experimental/CWE-322/InsecureHostKeyCallbackExample.go b/ql/src/Security/CWE-322/InsecureHostKeyCallbackExample.go similarity index 100% rename from ql/src/experimental/CWE-322/InsecureHostKeyCallbackExample.go rename to ql/src/Security/CWE-322/InsecureHostKeyCallbackExample.go diff --git a/ql/src/experimental/CWE-322/SecureHostKeyCallbackExample.go b/ql/src/Security/CWE-322/SecureHostKeyCallbackExample.go similarity index 100% rename from ql/src/experimental/CWE-322/SecureHostKeyCallbackExample.go rename to ql/src/Security/CWE-322/SecureHostKeyCallbackExample.go diff --git a/ql/test/experimental/CWE-322/InsecureHostKeyCallback.qlref b/ql/test/experimental/CWE-322/InsecureHostKeyCallback.qlref deleted file mode 100644 index 006d685c747..00000000000 --- a/ql/test/experimental/CWE-322/InsecureHostKeyCallback.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/CWE-322/InsecureHostKeyCallback.ql \ No newline at end of file diff --git a/ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.expected similarity index 100% rename from ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected rename to ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.expected diff --git a/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.qlref b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.qlref new file mode 100644 index 00000000000..b5f8712594d --- /dev/null +++ b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.qlref @@ -0,0 +1 @@ +Security/CWE-322/InsecureHostKeyCallback.ql diff --git a/ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallbackExample.go similarity index 100% rename from ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go rename to ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallbackExample.go diff --git a/ql/test/experimental/CWE-322/go.mod b/ql/test/query-tests/Security/CWE-322/go.mod similarity index 100% rename from ql/test/experimental/CWE-322/go.mod rename to ql/test/query-tests/Security/CWE-322/go.mod diff --git a/ql/test/experimental/CWE-322/vendor/golang.org/LICENSE b/ql/test/query-tests/Security/CWE-322/vendor/golang.org/LICENSE similarity index 100% rename from ql/test/experimental/CWE-322/vendor/golang.org/LICENSE rename to ql/test/query-tests/Security/CWE-322/vendor/golang.org/LICENSE diff --git a/ql/test/experimental/CWE-322/vendor/golang.org/x/crypto/ssh/stub.go b/ql/test/query-tests/Security/CWE-322/vendor/golang.org/x/crypto/ssh/stub.go similarity index 100% rename from ql/test/experimental/CWE-322/vendor/golang.org/x/crypto/ssh/stub.go rename to ql/test/query-tests/Security/CWE-322/vendor/golang.org/x/crypto/ssh/stub.go diff --git a/ql/test/experimental/CWE-322/vendor/modules.txt b/ql/test/query-tests/Security/CWE-322/vendor/modules.txt similarity index 100% rename from ql/test/experimental/CWE-322/vendor/modules.txt rename to ql/test/query-tests/Security/CWE-322/vendor/modules.txt From d0e86f787d8b9aff34dc43cd8fa1a8ed3e283b0b Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 29 Jul 2020 16:06:38 +0100 Subject: [PATCH 3/4] SSH host checking: Expand definition of a host-key checking function to include calls with multiple return types For example, https://godoc.org/golang.org/x/crypto/ssh/knownhosts#New returns a host-key checker and an error value, and we previously didn't consider the first return value a candidate checker function. --- ql/src/Security/CWE-322/InsecureHostKeyCallback.ql | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql b/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql index 8f3ebe03973..fbaf1f7e5f4 100644 --- a/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql +++ b/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql @@ -31,8 +31,9 @@ class HostKeyCallbackFunc extends DataFlow::Node { ( this instanceof DataFlow::FunctionNode or - this instanceof DataFlow::CallNode and - not exists(this.(DataFlow::CallNode).getACallee().getBody()) + exists(DataFlow::CallNode call | not exists(call.getACallee().getBody()) | + this = call.getAResult() + ) ) } } @@ -41,7 +42,7 @@ class HostKeyCallbackFunc extends DataFlow::Node { class InsecureHostKeyCallbackFunc extends HostKeyCallbackFunc { InsecureHostKeyCallbackFunc() { // Either a call to InsecureIgnoreHostKey(), which we know returns an insecure callback. - this = any(InsecureIgnoreHostKey f).getACall() + this = any(InsecureIgnoreHostKey f).getACall().getAResult() or // Or a callback function in the source code (named or anonymous) that always returns nil. forex(DataFlow::ResultNode returnValue | From d7c0671ea148b06bdaf3795ccc13a0c0c99fe250 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 29 Jul 2020 16:30:51 +0100 Subject: [PATCH 4/4] Add test using SSH host-key checker factory knownhosts.New This produces a secure host-key checker; we assume by default that an opaque function not otherwise specified returns an acceptable checker, but we need to particularly cope with its multiple return values to handle this factory function. --- .../CWE-322/InsecureHostKeyCallback.expected | 88 ++++++++++--------- .../CWE-322/InsecureHostKeyCallbackExample.go | 27 +++++- .../x/crypto/ssh/knownhosts/stub.go | 12 +++ 3 files changed, 82 insertions(+), 45 deletions(-) create mode 100644 ql/test/query-tests/Security/CWE-322/vendor/golang.org/x/crypto/ssh/knownhosts/stub.go diff --git a/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.expected b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.expected index 6ab43145857..0e851d743b2 100644 --- a/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.expected +++ b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallback.expected @@ -1,45 +1,49 @@ edges -| InsecureHostKeyCallbackExample.go:12:4:14:4 | function literal : signature type | InsecureHostKeyCallbackExample.go:11:20:14:5 | type conversion | -| InsecureHostKeyCallbackExample.go:27:14:30:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | -| InsecureHostKeyCallbackExample.go:28:3:30:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:27:14:30:4 | type conversion : signature type | -| InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | -| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : HostKeyCallback | InsecureHostKeyCallbackExample.go:58:20:58:27 | callback | -| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:58:20:58:27 | callback | -| InsecureHostKeyCallbackExample.go:64:48:64:55 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:74:28:74:35 | callback | -| InsecureHostKeyCallbackExample.go:81:22:84:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:86:35:86:50 | insecureCallback : signature type | -| InsecureHostKeyCallbackExample.go:82:3:84:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:81:22:84:4 | type conversion : signature type | -| InsecureHostKeyCallbackExample.go:86:35:86:50 | insecureCallback : signature type | InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type | -| InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:96:35:96:59 | potentiallySecureCallback : signature type | -| InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:99:44:99:68 | potentiallySecureCallback : signature type | -| InsecureHostKeyCallbackExample.go:89:3:94:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type | -| InsecureHostKeyCallbackExample.go:96:35:96:59 | potentiallySecureCallback : signature type | InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type | -| InsecureHostKeyCallbackExample.go:97:35:97:61 | call to InsecureIgnoreHostKey : HostKeyCallback | InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : HostKeyCallback | -| InsecureHostKeyCallbackExample.go:99:44:99:68 | potentiallySecureCallback : signature type | InsecureHostKeyCallbackExample.go:64:48:64:55 | definition of callback : signature type | +| InsecureHostKeyCallbackExample.go:16:4:18:4 | function literal : signature type | InsecureHostKeyCallbackExample.go:15:20:18:5 | type conversion | +| InsecureHostKeyCallbackExample.go:31:14:34:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:39:20:39:27 | callback | +| InsecureHostKeyCallbackExample.go:32:3:34:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:31:14:34:4 | type conversion : signature type | +| InsecureHostKeyCallbackExample.go:45:3:47:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:52:20:52:48 | type conversion | +| InsecureHostKeyCallbackExample.go:58:39:58:46 | definition of callback : HostKeyCallback | InsecureHostKeyCallbackExample.go:62:20:62:27 | callback | +| InsecureHostKeyCallbackExample.go:58:39:58:46 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:62:20:62:27 | callback | +| InsecureHostKeyCallbackExample.go:68:48:68:55 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:78:28:78:35 | callback | +| InsecureHostKeyCallbackExample.go:94:3:94:45 | ... := ...[0] : HostKeyCallback | InsecureHostKeyCallbackExample.go:95:28:95:35 | callback | +| InsecureHostKeyCallbackExample.go:102:22:105:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:107:35:107:50 | insecureCallback : signature type | +| InsecureHostKeyCallbackExample.go:103:3:105:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:102:22:105:4 | type conversion : signature type | +| InsecureHostKeyCallbackExample.go:107:35:107:50 | insecureCallback : signature type | InsecureHostKeyCallbackExample.go:58:39:58:46 | definition of callback : signature type | +| InsecureHostKeyCallbackExample.go:109:31:115:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:117:35:117:59 | potentiallySecureCallback : signature type | +| InsecureHostKeyCallbackExample.go:109:31:115:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:120:44:120:68 | potentiallySecureCallback : signature type | +| InsecureHostKeyCallbackExample.go:110:3:115:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:109:31:115:4 | type conversion : signature type | +| InsecureHostKeyCallbackExample.go:117:35:117:59 | potentiallySecureCallback : signature type | InsecureHostKeyCallbackExample.go:58:39:58:46 | definition of callback : signature type | +| InsecureHostKeyCallbackExample.go:118:35:118:61 | call to InsecureIgnoreHostKey : HostKeyCallback | InsecureHostKeyCallbackExample.go:58:39:58:46 | definition of callback : HostKeyCallback | +| InsecureHostKeyCallbackExample.go:120:44:120:68 | potentiallySecureCallback : signature type | InsecureHostKeyCallbackExample.go:68:48:68:55 | definition of callback : signature type | nodes -| InsecureHostKeyCallbackExample.go:11:20:14:5 | type conversion | semmle.label | type conversion | -| InsecureHostKeyCallbackExample.go:12:4:14:4 | function literal : signature type | semmle.label | function literal : signature type | -| InsecureHostKeyCallbackExample.go:22:20:22:46 | call to InsecureIgnoreHostKey | semmle.label | call to InsecureIgnoreHostKey | -| InsecureHostKeyCallbackExample.go:27:14:30:4 | type conversion : signature type | semmle.label | type conversion : signature type | -| InsecureHostKeyCallbackExample.go:28:3:30:3 | function literal : signature type | semmle.label | function literal : signature type | -| InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | semmle.label | callback | -| InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal : signature type | semmle.label | function literal : signature type | -| InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | semmle.label | type conversion | -| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : HostKeyCallback | semmle.label | definition of callback : HostKeyCallback | -| InsecureHostKeyCallbackExample.go:54:39:54:46 | definition of callback : signature type | semmle.label | definition of callback : signature type | -| InsecureHostKeyCallbackExample.go:58:20:58:27 | callback | semmle.label | callback | -| InsecureHostKeyCallbackExample.go:64:48:64:55 | definition of callback : signature type | semmle.label | definition of callback : signature type | -| InsecureHostKeyCallbackExample.go:72:28:72:54 | call to InsecureIgnoreHostKey | semmle.label | call to InsecureIgnoreHostKey | -| InsecureHostKeyCallbackExample.go:74:28:74:35 | callback | semmle.label | callback | -| InsecureHostKeyCallbackExample.go:81:22:84:4 | type conversion : signature type | semmle.label | type conversion : signature type | -| InsecureHostKeyCallbackExample.go:82:3:84:3 | function literal : signature type | semmle.label | function literal : signature type | -| InsecureHostKeyCallbackExample.go:86:35:86:50 | insecureCallback : signature type | semmle.label | insecureCallback : signature type | -| InsecureHostKeyCallbackExample.go:88:31:94:4 | type conversion : signature type | semmle.label | type conversion : signature type | -| InsecureHostKeyCallbackExample.go:89:3:94:3 | function literal : signature type | semmle.label | function literal : signature type | -| InsecureHostKeyCallbackExample.go:96:35:96:59 | potentiallySecureCallback : signature type | semmle.label | potentiallySecureCallback : signature type | -| InsecureHostKeyCallbackExample.go:97:35:97:61 | call to InsecureIgnoreHostKey : HostKeyCallback | semmle.label | call to InsecureIgnoreHostKey : HostKeyCallback | -| InsecureHostKeyCallbackExample.go:99:44:99:68 | potentiallySecureCallback : signature type | semmle.label | potentiallySecureCallback : signature type | +| InsecureHostKeyCallbackExample.go:15:20:18:5 | type conversion | semmle.label | type conversion | +| InsecureHostKeyCallbackExample.go:16:4:18:4 | function literal : signature type | semmle.label | function literal : signature type | +| InsecureHostKeyCallbackExample.go:26:20:26:46 | call to InsecureIgnoreHostKey | semmle.label | call to InsecureIgnoreHostKey | +| InsecureHostKeyCallbackExample.go:31:14:34:4 | type conversion : signature type | semmle.label | type conversion : signature type | +| InsecureHostKeyCallbackExample.go:32:3:34:3 | function literal : signature type | semmle.label | function literal : signature type | +| InsecureHostKeyCallbackExample.go:39:20:39:27 | callback | semmle.label | callback | +| InsecureHostKeyCallbackExample.go:45:3:47:3 | function literal : signature type | semmle.label | function literal : signature type | +| InsecureHostKeyCallbackExample.go:52:20:52:48 | type conversion | semmle.label | type conversion | +| InsecureHostKeyCallbackExample.go:58:39:58:46 | definition of callback : HostKeyCallback | semmle.label | definition of callback : HostKeyCallback | +| InsecureHostKeyCallbackExample.go:58:39:58:46 | definition of callback : signature type | semmle.label | definition of callback : signature type | +| InsecureHostKeyCallbackExample.go:62:20:62:27 | callback | semmle.label | callback | +| InsecureHostKeyCallbackExample.go:68:48:68:55 | definition of callback : signature type | semmle.label | definition of callback : signature type | +| InsecureHostKeyCallbackExample.go:76:28:76:54 | call to InsecureIgnoreHostKey | semmle.label | call to InsecureIgnoreHostKey | +| InsecureHostKeyCallbackExample.go:78:28:78:35 | callback | semmle.label | callback | +| InsecureHostKeyCallbackExample.go:92:28:92:54 | call to InsecureIgnoreHostKey | semmle.label | call to InsecureIgnoreHostKey | +| InsecureHostKeyCallbackExample.go:94:3:94:45 | ... := ...[0] : HostKeyCallback | semmle.label | ... := ...[0] : HostKeyCallback | +| InsecureHostKeyCallbackExample.go:95:28:95:35 | callback | semmle.label | callback | +| InsecureHostKeyCallbackExample.go:102:22:105:4 | type conversion : signature type | semmle.label | type conversion : signature type | +| InsecureHostKeyCallbackExample.go:103:3:105:3 | function literal : signature type | semmle.label | function literal : signature type | +| InsecureHostKeyCallbackExample.go:107:35:107:50 | insecureCallback : signature type | semmle.label | insecureCallback : signature type | +| InsecureHostKeyCallbackExample.go:109:31:115:4 | type conversion : signature type | semmle.label | type conversion : signature type | +| InsecureHostKeyCallbackExample.go:110:3:115:3 | function literal : signature type | semmle.label | function literal : signature type | +| InsecureHostKeyCallbackExample.go:117:35:117:59 | potentiallySecureCallback : signature type | semmle.label | potentiallySecureCallback : signature type | +| InsecureHostKeyCallbackExample.go:118:35:118:61 | call to InsecureIgnoreHostKey : HostKeyCallback | semmle.label | call to InsecureIgnoreHostKey : HostKeyCallback | +| InsecureHostKeyCallbackExample.go:120:44:120:68 | potentiallySecureCallback : signature type | semmle.label | potentiallySecureCallback : signature type | #select -| 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 | -| 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 | -| 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 | -| 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 | +| InsecureHostKeyCallbackExample.go:15:20:18:5 | type conversion | InsecureHostKeyCallbackExample.go:16:4:18:4 | function literal : signature type | InsecureHostKeyCallbackExample.go:15:20:18:5 | type conversion | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:16:4:18:4 | function literal | this source | +| InsecureHostKeyCallbackExample.go:26:20:26:46 | call to InsecureIgnoreHostKey | InsecureHostKeyCallbackExample.go:26:20:26:46 | call to InsecureIgnoreHostKey | InsecureHostKeyCallbackExample.go:26:20:26:46 | call to InsecureIgnoreHostKey | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:26:20:26:46 | call to InsecureIgnoreHostKey | this source | +| InsecureHostKeyCallbackExample.go:39:20:39:27 | callback | InsecureHostKeyCallbackExample.go:32:3:34:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:39:20:39:27 | callback | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:32:3:34:3 | function literal | this source | +| InsecureHostKeyCallbackExample.go:52:20:52:48 | type conversion | InsecureHostKeyCallbackExample.go:45:3:47:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:52:20:52:48 | type conversion | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:45:3:47:3 | function literal | this source | diff --git a/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallbackExample.go b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallbackExample.go index 7fc9de0b9d4..0c7f9466488 100644 --- a/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallbackExample.go +++ b/ql/test/query-tests/Security/CWE-322/InsecureHostKeyCallbackExample.go @@ -1,8 +1,12 @@ package main -import "net" -import "fmt" -import "golang.org/x/crypto/ssh" +import ( + "fmt" + "net" + + "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/knownhosts" +) func insecureSSHClientConfig() { _ = &ssh.ClientConfig{ @@ -75,6 +79,23 @@ func potentialInsecureSSHClientConfigTwoWrites(callback ssh.HostKeyCallback) { } } +// Check that insecure and secure functions flowing to different writes to +// the same objects are not flagged (we assume this is configurable security) +func potentialInsecureSSHClientConfigUsingKnownHosts(x bool) { + config := &ssh.ClientConfig{ + User: "user", + Auth: []ssh.AuthMethod{nil}, + HostKeyCallback: nil, + } + + if x { + config.HostKeyCallback = ssh.InsecureIgnoreHostKey() // OK + } else { + callback, err := knownhosts.New("somefile") + config.HostKeyCallback = callback + } +} + func main() { fmt.Printf("Hello insecure SSH client config!\n") diff --git a/ql/test/query-tests/Security/CWE-322/vendor/golang.org/x/crypto/ssh/knownhosts/stub.go b/ql/test/query-tests/Security/CWE-322/vendor/golang.org/x/crypto/ssh/knownhosts/stub.go new file mode 100644 index 00000000000..374e3e83356 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-322/vendor/golang.org/x/crypto/ssh/knownhosts/stub.go @@ -0,0 +1,12 @@ +// A simple manual stub of golang.org/x/crypto/ssh/knownhosts.New +// See the LICENSE file for information about the licensing of the original library. + +package knownhosts + +import ( + "golang.org/x/crypto/ssh" +) + +func New(files ...string) (ssh.HostKeyCallback, error) { + return nil, nil +}