Address review comments.

Author: taus-semmle

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

@@ -13,20 +13,23 @@ man-in-the-middle attacks.
 
 <recommendation>
 <p>
-Do not accept unknown host keys. For the Paramiko library in particular, avoid
-setting the missing host key policy to either <code>AutoAddPolicy</code> or
-<code>WarningPolicy</code>, as both of these will continue even when the host
-key is unknown. The default <code>RejectPolicy</code> throws an exception when
-unknown host keys are encountered.
+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 opens a connection to <code>example.com</code> with the
-missing host key policy set 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 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>

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

@@ -1,10 +1,10 @@
 /**
- * @name Accepting unknown host keys.
+ * @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/missing-host-key-validation
+ * @id py/paramiko-missing-host-key-validation
  * @tags security
  *       external/cwe/cwe-295
  */
@@ -22,11 +22,15 @@ private ClassObject unsafe_paramiko_policy(string name) {
     result = theParamikoClientModule().attr(name)
 }
 
-from CallNode call, string name
+from CallNode call, ControlFlowNode arg, string name
 where
     call = theParamikoSSHClientClass()
-        .declaredAttribute("set_missing_host_key_policy")
-        .(FunctionObject)
-        .getACall() and
-    call.getAnArg().refersTo(unsafe_paramiko_policy(name))
+           .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

@@ -1,9 +1,19 @@
-from paramiko.client import SSHClient, AutoAddPolicy
+from paramiko.client import SSHClient, AutoAddPolicy, RejectPolicy
 
-client = SSHClient()
-client.set_missing_host_key_policy(AutoAddPolicy)
-client.connect("example.com")
+def unsafe_connect():
+    client = SSHClient()
+    client.set_missing_host_key_policy(AutoAddPolicy)
+    client.connect("example.com")
 
-# ... interaction with server
+    # ... interaction with server
 
-client.close()
+    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

@@ -1,2 +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/paramiko_host_key.py

@@ -5,3 +5,9 @@
 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