From a89b87f64366d9fd5fc452b4e85a8d882c9b492e Mon Sep 17 00:00:00 2001 From: Remco Vermeulen Date: Tue, 30 Jun 2020 16:38:21 +0200 Subject: [PATCH] CWE-322 InsecureHostKeyCallback (#234) --- .../CWE-322/InsecureHostKeyCallback.qhelp | 49 ++++++++++++ .../CWE-322/InsecureHostKeyCallback.ql | 54 +++++++++++++ .../CWE-322/InsecureHostKeyCallbackExample.go | 27 +++++++ .../CWE-322/SecureHostKeyCallbackExample.go | 19 +++++ .../go/dataflow/internal/DataFlowUtil.qll | 11 +++ .../CWE-322/InsecureHostKeyCallback.expected | 34 ++++++++ .../CWE-322/InsecureHostKeyCallback.qlref | 1 + .../CWE-322/InsecureHostKeyCallbackExample.go | 80 +++++++++++++++++++ ql/test/experimental/CWE-322/go.mod | 8 ++ .../CWE-322/vendor/golang.org/LICENSE | 27 +++++++ .../vendor/golang.org/x/crypto/ssh/stub.go | 58 ++++++++++++++ .../experimental/CWE-322/vendor/modules.txt | 6 ++ 12 files changed, 374 insertions(+) create mode 100644 ql/src/experimental/CWE-322/InsecureHostKeyCallback.qhelp create mode 100644 ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql create mode 100644 ql/src/experimental/CWE-322/InsecureHostKeyCallbackExample.go create mode 100644 ql/src/experimental/CWE-322/SecureHostKeyCallbackExample.go create mode 100644 ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected create mode 100644 ql/test/experimental/CWE-322/InsecureHostKeyCallback.qlref create mode 100644 ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go create mode 100644 ql/test/experimental/CWE-322/go.mod create mode 100644 ql/test/experimental/CWE-322/vendor/golang.org/LICENSE create mode 100644 ql/test/experimental/CWE-322/vendor/golang.org/x/crypto/ssh/stub.go create mode 100644 ql/test/experimental/CWE-322/vendor/modules.txt diff --git a/ql/src/experimental/CWE-322/InsecureHostKeyCallback.qhelp b/ql/src/experimental/CWE-322/InsecureHostKeyCallback.qhelp new file mode 100644 index 00000000000..02fa1e6bc8d --- /dev/null +++ b/ql/src/experimental/CWE-322/InsecureHostKeyCallback.qhelp @@ -0,0 +1,49 @@ + + + +

+ The ClientConfig specifying the configuration for establishing a SSH connection has a field HostKeyCallback that must be initialized with a function that validates the host key returned by the server. +

+ +

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

+ +

+ The ssh package provides the predefined callback InsecureIgnoreHostKey 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. +

+ + + +

+The HostKeyCallback field of ClientConfig 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. +

+

+When the allow list contains only a single host key then the function FixedHostKey can be used. +

+ + + +

The following example shows the use of InsecureIgnoreHostKey and an insecure host key callback implemention commonly used in non-production code.

+ + + +

The next example shows a secure implementation using the FixedHostKey that implements an allow-list.

+ + + + + + +
  • + Go Dev: + package ssh. +
    • +     +     diff --git a/ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql b/ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql new file mode 100644 index 00000000000..fb810b29a8f --- /dev/null +++ b/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" diff --git a/ql/src/experimental/CWE-322/InsecureHostKeyCallbackExample.go b/ql/src/experimental/CWE-322/InsecureHostKeyCallbackExample.go new file mode 100644 index 00000000000..4617ffe7b39 --- /dev/null +++ b/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 + }), + } +} \ No newline at end of file diff --git a/ql/src/experimental/CWE-322/SecureHostKeyCallbackExample.go b/ql/src/experimental/CWE-322/SecureHostKeyCallbackExample.go new file mode 100644 index 00000000000..1cc1b5d1b1b --- /dev/null +++ b/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), + } +} \ No newline at end of file diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index cdb6d012d06..ed6958c73d3 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/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. */ diff --git a/ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected b/ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected new file mode 100644 index 00000000000..351496e00fa --- /dev/null +++ b/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 | diff --git a/ql/test/experimental/CWE-322/InsecureHostKeyCallback.qlref b/ql/test/experimental/CWE-322/InsecureHostKeyCallback.qlref new file mode 100644 index 00000000000..006d685c747 --- /dev/null +++ b/ql/test/experimental/CWE-322/InsecureHostKeyCallback.qlref @@ -0,0 +1 @@ +experimental/CWE-322/InsecureHostKeyCallback.ql \ No newline at end of file diff --git a/ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go b/ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go new file mode 100644 index 00000000000..5664e88650b --- /dev/null +++ b/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()) +} \ No newline at end of file diff --git a/ql/test/experimental/CWE-322/go.mod b/ql/test/experimental/CWE-322/go.mod new file mode 100644 index 00000000000..5b18384325a --- /dev/null +++ b/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 +) diff --git a/ql/test/experimental/CWE-322/vendor/golang.org/LICENSE b/ql/test/experimental/CWE-322/vendor/golang.org/LICENSE new file mode 100644 index 00000000000..ea5ea898692 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/ql/test/experimental/CWE-322/vendor/golang.org/x/crypto/ssh/stub.go b/ql/test/experimental/CWE-322/vendor/golang.org/x/crypto/ssh/stub.go new file mode 100644 index 00000000000..c7102e0eb50 --- /dev/null +++ b/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 +} diff --git a/ql/test/experimental/CWE-322/vendor/modules.txt b/ql/test/experimental/CWE-322/vendor/modules.txt new file mode 100644 index 00000000000..50b76de6785 --- /dev/null +++ b/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