Merge pull request #1128 from taus-semmle/python-paramiko-unsafe-host-key-validation

Python: Add query for insecure SSH host key policies in Paramiko.

Author: markshannon

change-notes/1.21/analysis-python.md

@@ -0,0 +1,26 @@
+# Improvements to Python analysis
+
+
+## General improvements
+
+> Changes that affect alerts in many files or from many queries
+> For example, changes to file classification
+
+## New queries
+  | **Query** | **Tags** | **Purpose** |
+  |-----------|----------|-------------|
+  | Accepting unknown SSH host keys when using Paramiko (`py/paramiko-missing-host-key-validation`) | security, external/cwe/cwe-295 | Finds instances where Paramiko is configured to accept unknown host keys. Results are shown on LGTM by default. |
+
+
+## Changes to existing queries
+
+  | **Query** | **Expected impact** | **Change** |
+  |-----------|---------------------|------------|
+
+## Changes to code extraction
+
+* *Series of bullet points*
+
+## Changes to QL libraries
+
+* *Series of bullet points*

python/ql/src/Security/CWE-295/MissingHostKeyValidation.qhelp

@@ -0,0 +1,43 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+  <p>
+In the Secure Shell (SSH) protocol, host keys are used to verify the identity of
+remote hosts. Accepting unknown host keys may leave the connection open to
+man-in-the-middle attacks.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Do not accept unknown host keys. In particular, do not set the default missing
+host key policy for the Paramiko library to either <code>AutoAddPolicy</code> or
+<code>WarningPolicy</code>. Both of these policies continue even when the host
+key is unknown. The default setting of <code>RejectPolicy</code> is secure
+because it throws an exception when it encounters an unknown host key.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows two ways of opening an SSH connection to
+<code>example.com</code>. The first function sets the missing host key policy to
+<code>AutoAddPolicy</code>. If the host key verification fails, the client will
+continue to interact with the server, even though the connection may be
+compromised. The second function sets the host key policy to
+<code>RejectPolicy</code>, and will throw an exception if the host key
+verification fails.
+</p>
+<sample src="examples/paramiko_host_key.py" />
+</example>
+
+<references>
+<li>
+Paramiko documentation: <a href="http://docs.paramiko.org/en/2.4/api/client.html?highlight=set_missing_host_key_policy#paramiko.client.SSHClient.set_missing_host_key_policy">set_missing_host_key_policy</a>.
+</li>
+</references>
+</qhelp>
+

python/ql/src/Security/CWE-295/MissingHostKeyValidation.ql

@@ -0,0 +1,36 @@
+/**
+ * @name Accepting unknown SSH host keys when using Paramiko
+ * @description Accepting unknown host keys can allow man-in-the-middle attacks.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id py/paramiko-missing-host-key-validation
+ * @tags security
+ *       external/cwe/cwe-295
+ */
+
+import python
+
+private ModuleObject theParamikoClientModule() { result = ModuleObject::named("paramiko.client") }
+
+private ClassObject theParamikoSSHClientClass() {
+    result = theParamikoClientModule().attr("SSHClient")
+}
+
+private ClassObject unsafe_paramiko_policy(string name) {
+    (name = "AutoAddPolicy" or name = "WarningPolicy") and
+    result = theParamikoClientModule().attr(name)
+}
+
+from CallNode call, ControlFlowNode arg, string name
+where
+    call = theParamikoSSHClientClass()
+           .lookupAttribute("set_missing_host_key_policy")
+           .(FunctionObject)
+           .getACall() and
+    arg = call.getAnArg() and
+    (
+        arg.refersTo(unsafe_paramiko_policy(name)) or
+        arg.refersTo(_, unsafe_paramiko_policy(name), _)
+    )
+select call, "Setting missing host key policy to " + name + " may be unsafe."

python/ql/src/Security/CWE-295/examples/paramiko_host_key.py

@@ -0,0 +1,19 @@
+from paramiko.client import SSHClient, AutoAddPolicy, RejectPolicy
+
+def unsafe_connect():
+    client = SSHClient()
+    client.set_missing_host_key_policy(AutoAddPolicy)
+    client.connect("example.com")
+
+    # ... interaction with server
+
+    client.close()
+
+def safe_connect():
+    client = SSHClient()
+    client.set_missing_host_key_policy(RejectPolicy)
+    client.connect("example.com")
+
+    # ... interaction with server
+
+    client.close()

python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.expected

@@ -0,0 +1,4 @@
+| paramiko_host_key.py:5:1:5:49 | ControlFlowNode for Attribute() | Setting missing host key policy to AutoAddPolicy may be unsafe. |
+| paramiko_host_key.py:7:1:7:49 | ControlFlowNode for Attribute() | Setting missing host key policy to WarningPolicy may be unsafe. |
+| paramiko_host_key.py:11:1:11:51 | ControlFlowNode for Attribute() | Setting missing host key policy to AutoAddPolicy may be unsafe. |
+| paramiko_host_key.py:13:1:13:51 | ControlFlowNode for Attribute() | Setting missing host key policy to WarningPolicy may be unsafe. |

python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.qlref

@@ -0,0 +1 @@
+Security/CWE-295/MissingHostKeyValidation.ql

python/ql/test/query-tests/Security/CWE-295/paramiko_host_key.py

@@ -0,0 +1,13 @@
+from paramiko.client import AutoAddPolicy, WarningPolicy, RejectPolicy, SSHClient
+
+client = SSHClient()
+
+client.set_missing_host_key_policy(AutoAddPolicy) # bad
+client.set_missing_host_key_policy(RejectPolicy)  # good
+client.set_missing_host_key_policy(WarningPolicy) # bad
+
+# Using instances
+
+client.set_missing_host_key_policy(AutoAddPolicy()) # bad
+client.set_missing_host_key_policy(RejectPolicy())  # good
+client.set_missing_host_key_policy(WarningPolicy()) # bad

python/ql/test/query-tests/Security/lib/paramiko/__init__.py

@@ -0,0 +1,15 @@
+class SSHClient(object):
+    def __init__(self, *args, **kwargs):
+        pass
+
+    def set_missing_host_key_policy(self, *args, **kwargs):
+        pass
+
+class AutoAddPolicy(object):
+    pass
+
+class WarningPolicy(object):
+    pass
+
+class RejectPolicy(object):
+    pass