CWE-322 InsecureHostKeyCallback

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

@@ -0,0 +1,49 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+    The <code>ClientConfig</code> specifying the configuration for establishing a SSH connection has a field <code>HostKeyCallback</code> that must be initialized with a function that validates the host key returned by the server.
+</p>
+
+<p>
+    Not properly verifying the host key returned by a server provides attackers with an opportunity to perform a Machine-in-the-Middle (MitM) attack.
+    A successful attack can compromise the confidentiality and integrity of the information communicated with the server.
+</p>
+
+<p>
+    The <code>ssh</code> package provides the predefined callback <code>InsecureIgnoreHostKey</code> that can be used during development and testing.
+    It accepts any provided host key.
+    This callback, or a semantically similar callback, should not be used in production code. 
+</p>
+</overview>
+
+<recommendation>
+<p>
+The <code>HostKeyCallback</code> field of <code>ClientConfig</code> should be initialized with a function that validates a host key against an allow list.
+If a key is not on a predefined allow list, the connection must be terminated and the failed security operation should be logged.
+</p>
+<p>
+When the allow list contains only a single host key then the function <code>FixedHostKey</code> can be used.
+</p>
+</recommendation>
+
+<example>
+<p>The following example shows the use of <code>InsecureIgnoreHostKey</code> and an insecure host key callback implemention commonly used in non-production code.</p>
+
+<sample src="InsecureHostKeyCallbackExample.go" />
+
+<p>The next example shows a secure implementation using the <code>FixedHostKey</code> that implements an allow-list.</p>
+
+<sample src="SecureHostKeyCallbackExample.go" />
+
+</example>
+
+<references>
+<li>
+  Go Dev:
+  <a href="https://pkg.go.dev/golang.org/x/crypto/ssh?tab=doc">package ssh</a>.
+</li>
+</references>
+</qhelp>

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

@@ -0,0 +1,54 @@
+/**
+ * @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
+ * @precision very-high
+ * @id go/insecure-hostkeycallback
+ * @tags security
+ */
+
+import go
+import DataFlow::PathGraph
+
+class InsecureIgnoreHostKey extends Function {
+  InsecureIgnoreHostKey() {
+    this.hasQualifiedName("golang.org/x/crypto/ssh", "InsecureIgnoreHostKey")
+  }
+}
+
+/** A callback function value that is insecure when used as a `HostKeyCallback`, because it always returns `nil`. */
+class InsecureHostKeyCallbackFunc extends DataFlow::Node {
+  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.
+    forex(DataFlow::ResultNode returnValue |
+      returnValue = this.(DataFlow::FunctionNode).getAResult()
+    |
+      returnValue = Builtin::nil().getARead()
+    )
+  }
+}
+
+class HostKeyCallbackAssignmentConfig extends DataFlow::Configuration {
+  HostKeyCallbackAssignmentConfig() { this = "HostKeyCallbackAssignmentConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof InsecureHostKeyCallbackFunc
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(Field f |
+      f.hasQualifiedName("golang.org/x/crypto/ssh", "ClientConfig", "HostKeyCallback") and
+      sink = f.getAWrite().getRhs()
+    )
+  }
+}
+
+from HostKeyCallbackAssignmentConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink, source, sink,
+  "Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@.",
+  source.getNode(), "this source"

ql/src/experimental/CWE-322/InsecureHostKeyCallbackExample.go

@@ -0,0 +1,27 @@
+package main
+
+import (
+	"golang.org/x/crypto/ssh"
+	"net"
+)
+
+func main() {}
+
+func insecureIgnoreHostKey() {
+	_ = &ssh.ClientConfig{
+		User: "username",
+		Auth: []ssh.AuthMethod{nil},
+		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
+	}
+}
+
+func insecureHostKeyCallback() {
+	_ = &ssh.ClientConfig{
+		User: "username",
+		Auth: []ssh.AuthMethod{nil},
+		HostKeyCallback: ssh.HostKeyCallback(
+			func(hostname string, remote net.Addr, key ssh.PublicKey) error {
+				return nil
+		}),
+	}
+}

ql/src/experimental/CWE-322/SecureHostKeyCallbackExample.go

@@ -0,0 +1,19 @@
+package main
+
+import (
+	"golang.org/x/crypto/ssh"
+	"io/ioutil"
+)
+
+func main() {}
+
+func secureHostKeyCallback() {
+	publicKeyBytes, _ := ioutil.ReadFile("allowed_hostkey.pub")
+	publicKey, _ := ssh.ParsePublicKey(publicKeyBytes)
+
+	_ = &ssh.ClientConfig{
+		User: "username",
+		Auth: []ssh.AuthMethod{nil},
+		HostKeyCallback: ssh.FixedHostKey(publicKey),
+	}
+}

ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll

@@ -234,6 +234,11 @@ abstract class FunctionNode extends Node {
    * Gets the dataflow node holding the value of the receiver, if any.
    */
   abstract ReceiverNode getReceiver();
+
+  /**
+   * Gets a value returned by the given function via a return statement or an assignment to a result variable.
+   */
+  abstract ResultNode getAResult();
 }
 
 /** A representation of a function that is declared in the module scope. */
@@ -260,6 +265,10 @@ class GlobalFunctionNode extends FunctionNode, MkGlobalFunctionNode {
   ) {
     func.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
   }
+
+  override ResultNode getAResult() {
+    result.getRoot() = getFunction().(DeclaredFunction).getFuncDecl()
+  }
 }
 
 /** A representation of the function that is defined by a function literal. */
@@ -273,6 +282,8 @@ class FuncLitNode extends FunctionNode, ExprNode {
   override ReceiverNode getReceiver() { none() }
 
   override string toString() { result = "function literal" }
+
+  override ResultNode getAResult() { result.getRoot() = getExpr() }
 }
 
 /** A data flow node that represents a call. */

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

@@ -0,0 +1,34 @@
+edges
+| InsecureHostKeyCallbackExample.go:12:4:14:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:11:20:14:4 | type conversion |
+| InsecureHostKeyCallbackExample.go:27:14:30:3 | type conversion : signature type | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback |
+| InsecureHostKeyCallbackExample.go:28:3:30:2 | function literal : signature type | InsecureHostKeyCallbackExample.go:27:14:30:3 | type conversion : signature type |
+| InsecureHostKeyCallbackExample.go:41:3:43:2 | 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:3 | type conversion : signature type | InsecureHostKeyCallbackExample.go:68:35:68:50 | insecureCallback : signature type |
+| InsecureHostKeyCallbackExample.go:64:3:66:2 | function literal : signature type | InsecureHostKeyCallbackExample.go:63:22:66:3 | 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 |
+nodes
+| InsecureHostKeyCallbackExample.go:11:20:14:4 | type conversion | semmle.label | type conversion |
+| InsecureHostKeyCallbackExample.go:12:4:14:3 | 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:3 | type conversion : signature type | semmle.label | type conversion : signature type |
+| InsecureHostKeyCallbackExample.go:28:3:30:2 | 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:2 | 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:3 | type conversion : signature type | semmle.label | type conversion : signature type |
+| InsecureHostKeyCallbackExample.go:64:3:66:2 | 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 |
+#select
+| InsecureHostKeyCallbackExample.go:11:20:14:4 | type conversion | InsecureHostKeyCallbackExample.go:12:4:14:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:11:20:14:4 | type conversion | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:12:4:14:3 | 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:2 | function literal : signature type | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:28:3:30:2 | function literal | this source |
+| InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | InsecureHostKeyCallbackExample.go:41:3:43:2 | 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:2 | function literal | this source |
+| InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | InsecureHostKeyCallbackExample.go:64:3:66:2 | function literal : signature type | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:64:3:66:2 | 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 |

ql/test/experimental/CWE-322/InsecureHostKeyCallback.qlref

@@ -0,0 +1 @@
+experimental/CWE-322/InsecureHostKeyCallback.ql

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

@@ -0,0 +1,80 @@
+package main
+
+import "net"
+import "fmt"
+import "golang.org/x/crypto/ssh"
+
+func insecureSSHClientConfig() {
+	_ = &ssh.ClientConfig{
+		User: "user",
+		Auth: []ssh.AuthMethod{nil},
+		HostKeyCallback: ssh.HostKeyCallback(
+			func(hostname string, remote net.Addr, key ssh.PublicKey) error {
+				 return nil
+		}),
+	 }
+}
+
+func insecureSSHClientConfigAlt() {
+	_ = &ssh.ClientConfig{
+		User: "user",
+		Auth: []ssh.AuthMethod{nil},
+		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
+	 }
+}
+
+func insecureSSHClientConfigLocalFlow() {
+	callback := ssh.HostKeyCallback(
+		func(hostname string, remote net.Addr, key ssh.PublicKey) error {
+			 return nil
+	})
+
+	_ = &ssh.ClientConfig{
+		User: "user",
+		Auth: []ssh.AuthMethod{nil},
+		HostKeyCallback: callback,
+	 }
+}
+
+func insecureSSHClientConfigLocalFlowAlt() {
+	callback := 
+		func(hostname string, remote net.Addr, key ssh.PublicKey) error {
+			 return nil
+	};
+
+	_ = &ssh.ClientConfig{
+		User: "user",
+		Auth: []ssh.AuthMethod{nil},
+		HostKeyCallback: ssh.HostKeyCallback(callback),
+	 }
+}
+
+func potentialInsecureSSHClientConfig(callback ssh.HostKeyCallback) {
+	_ = &ssh.ClientConfig{
+		User: "user",
+		Auth: []ssh.AuthMethod{nil},
+		HostKeyCallback: callback,
+	 }
+}
+
+func main() {
+	fmt.Printf("Hello insecure SSH client config!\n")
+
+	insecureCallback := ssh.HostKeyCallback(
+		func(hostname string, remote net.Addr, key ssh.PublicKey) error {
+			 return nil
+	})
+
+	potentialInsecureSSHClientConfig(insecureCallback)
+
+	potentiallySecureCallback := ssh.HostKeyCallback(
+		func(hostname string, remote net.Addr, key ssh.PublicKey) error {
+			if hostname == "localhost" {
+				return nil
+			}
+			 return fmt.Errorf("ssh: Unexpected host for key")
+	})
+
+	potentialInsecureSSHClientConfig(potentiallySecureCallback)
+	potentialInsecureSSHClientConfig(ssh.InsecureIgnoreHostKey())
+}

ql/test/experimental/CWE-322/go.mod

@@ -0,0 +1,8 @@
+module custom-queries-go-tests/insecure-ssh-client-config
+
+go 1.14
+
+require (
+	github.com/github/depstubber v0.0.0-20200618063247-cc37722ad494 // indirect
+	golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9
+)