Polish CWE-322: detect and exclude cases where host-checking is optional

2026-01-29 14:23:03 +01:00 · 2020-07-17 16:26:44 +01:00
parent e9ae697d0d
commit 99f08750f3
3 changed files with 118 additions and 29 deletions
--- 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"
--- 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 |
--- 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)
 }