CWE-322 InsecureHostKeyCallback (#234)

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
+)

ql/test/experimental/CWE-322/vendor/golang.org/LICENSE

@@ -0,0 +1,27 @@
+Copyright (c) 2009 The Go Authors. All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are
+met:
+
+   * Redistributions of source code must retain the above copyright
+notice, this list of conditions and the following disclaimer.
+   * Redistributions in binary form must reproduce the above
+copyright notice, this list of conditions and the following disclaimer
+in the documentation and/or other materials provided with the
+distribution.
+   * Neither the name of Google Inc. nor the names of its
+contributors may be used to endorse or promote products derived from
+this software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

ql/test/experimental/CWE-322/vendor/golang.org/x/crypto/ssh/stub.go

@@ -0,0 +1,58 @@
+// Code generated by depstubber. DO NOT EDIT.
+// This is a simple stub for golang.org/x/crypto/ssh, strictly for use in testing.
+
+// See the LICENSE file for information about the licensing of the original library.
+// Source: golang.org/x/crypto/ssh (exports: ClientConfig,AuthMethod,HostKeyCallback,PublicKey; functions: InsecureIgnoreHostKey)
+
+// Package ssh is a stub of golang.org/x/crypto/ssh, generated by depstubber.
+package ssh
+
+import (
+	io "io"
+	time "time"
+)
+
+type AuthMethod interface{}
+
+type BannerCallback func(string) error
+
+type ClientConfig struct {
+	Config            Config
+	User              string
+	Auth              []AuthMethod
+	HostKeyCallback   HostKeyCallback
+	BannerCallback    BannerCallback
+	ClientVersion     string
+	HostKeyAlgorithms []string
+	Timeout           time.Duration
+}
+
+func (_ *ClientConfig) SetDefaults() {}
+
+type Config struct {
+	Rand           io.Reader
+	RekeyThreshold uint64
+	KeyExchanges   []string
+	Ciphers        []string
+	MACs           []string
+}
+
+func (_ *Config) SetDefaults() {}
+
+type HostKeyCallback func(string, Addr, PublicKey) error
+
+func InsecureIgnoreHostKey() HostKeyCallback {
+	return nil
+}
+
+type PublicKey interface {
+	Marshal() []byte
+	Type() string
+	Verify(_ []byte, _ *Signature) error
+}
+
+type Signature struct {
+	Format string
+	Blob   []byte
+	Rest   []byte
+}

ql/test/experimental/CWE-322/vendor/modules.txt

@@ -0,0 +1,6 @@
+# github.com/github/depstubber v0.0.0-20200618063247-cc37722ad494
+## explicit
+github.com/github/depstubber
+# golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9
+## explicit
+golang.org/x/crypto