From 99f08750f3cae90bf98df0f4f273d95210ea6d0e Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 17 Jul 2020 16:26:44 +0100 Subject: [PATCH] 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) }