From 52278b25d9057f705b2dcb157588e6be317c4a57 Mon Sep 17 00:00:00 2001 From: Taus Brock-Nannestad Date: Mon, 18 Mar 2019 16:45:54 +0100 Subject: [PATCH 1/4] Python: Add query for insecure SSH host key policies in Paramiko. --- .../CWE-295/MissingHostKeyValidation.qhelp | 40 +++++++++++++++++++ .../CWE-295/MissingHostKeyValidation.ql | 32 +++++++++++++++ .../CWE-295/examples/paramiko_host_key.py | 9 +++++ .../CWE-295/MissingHostKeyValidation.expected | 2 + .../CWE-295/MissingHostKeyValidation.qlref | 1 + .../Security/CWE-295/paramiko_host_key.py | 7 ++++ .../Security/lib/paramiko/__init__.py | 0 .../Security/lib/paramiko/client.py | 15 +++++++ 8 files changed, 106 insertions(+) create mode 100644 python/ql/src/Security/CWE-295/MissingHostKeyValidation.qhelp create mode 100644 python/ql/src/Security/CWE-295/MissingHostKeyValidation.ql create mode 100644 python/ql/src/Security/CWE-295/examples/paramiko_host_key.py create mode 100644 python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.expected create mode 100644 python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.qlref create mode 100644 python/ql/test/query-tests/Security/CWE-295/paramiko_host_key.py create mode 100644 python/ql/test/query-tests/Security/lib/paramiko/__init__.py create mode 100644 python/ql/test/query-tests/Security/lib/paramiko/client.py diff --git a/python/ql/src/Security/CWE-295/MissingHostKeyValidation.qhelp b/python/ql/src/Security/CWE-295/MissingHostKeyValidation.qhelp new file mode 100644 index 000000000000..c77b64314776 --- /dev/null +++ b/python/ql/src/Security/CWE-295/MissingHostKeyValidation.qhelp @@ -0,0 +1,40 @@ + + + + +

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

+ + + +

+Do not accept unknown host keys. For the Paramiko library in particular, avoid +setting the missing host key policy to either AutoAddPolicy or +WarningPolicy, as both of these will continue even when the host +key is unknown. The default RejectPolicy throws an exception when +unknown host keys are encountered. +

+ + + +

+The following example opens a connection to example.com with the +missing host key policy set to AutoAddPolicy. If the host key +verification fails, the client will continue to interact with the server, even +though the connection may be compromised. +

+ + + + +
  • +Paramiko documentation: set_missing_host_key_policy. +
    • +     +     + diff --git a/python/ql/src/Security/CWE-295/MissingHostKeyValidation.ql b/python/ql/src/Security/CWE-295/MissingHostKeyValidation.ql new file mode 100644 index 000000000000..5f4295211255 --- /dev/null +++ b/python/ql/src/Security/CWE-295/MissingHostKeyValidation.ql @@ -0,0 +1,32 @@ +/** + * @name Accepting unknown host keys. + * @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 + * @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, string name +where + call = theParamikoSSHClientClass() + .declaredAttribute("set_missing_host_key_policy") + .(FunctionObject) + .getACall() and + call.getAnArg().refersTo(unsafe_paramiko_policy(name)) +select call, "Setting missing host key policy to " + name + " may be unsafe." diff --git a/python/ql/src/Security/CWE-295/examples/paramiko_host_key.py b/python/ql/src/Security/CWE-295/examples/paramiko_host_key.py new file mode 100644 index 000000000000..70d42612f56a --- /dev/null +++ b/python/ql/src/Security/CWE-295/examples/paramiko_host_key.py @@ -0,0 +1,9 @@ +from paramiko.client import SSHClient, AutoAddPolicy + +client = SSHClient() +client.set_missing_host_key_policy(AutoAddPolicy) +client.connect("example.com") + +# ... interaction with server + +client.close() diff --git a/python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.expected b/python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.expected new file mode 100644 index 000000000000..dc85718d804c --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.expected @@ -0,0 +1,2 @@ +| 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. | diff --git a/python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.qlref b/python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.qlref new file mode 100644 index 000000000000..c366095516af --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.qlref @@ -0,0 +1 @@ +Security/CWE-295/MissingHostKeyValidation.ql diff --git a/python/ql/test/query-tests/Security/CWE-295/paramiko_host_key.py b/python/ql/test/query-tests/Security/CWE-295/paramiko_host_key.py new file mode 100644 index 000000000000..e59db86c8b17 --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-295/paramiko_host_key.py @@ -0,0 +1,7 @@ +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 diff --git a/python/ql/test/query-tests/Security/lib/paramiko/__init__.py b/python/ql/test/query-tests/Security/lib/paramiko/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/python/ql/test/query-tests/Security/lib/paramiko/client.py b/python/ql/test/query-tests/Security/lib/paramiko/client.py new file mode 100644 index 000000000000..6343fbe78dd3 --- /dev/null +++ b/python/ql/test/query-tests/Security/lib/paramiko/client.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 From 129baea8358bad99fc2c8022f35549893a08302f Mon Sep 17 00:00:00 2001 From: Taus Brock-Nannestad Date: Tue, 19 Mar 2019 15:07:50 +0100 Subject: [PATCH 2/4] Add change note for 1.21. --- change-notes/1.21/analysis-python.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 change-notes/1.21/analysis-python.md diff --git a/change-notes/1.21/analysis-python.md b/change-notes/1.21/analysis-python.md new file mode 100644 index 000000000000..645bbb33cf9a --- /dev/null +++ b/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* From c7c6c83627f11026eea535b39d3d37ae82d69fe0 Mon Sep 17 00:00:00 2001 From: Taus Brock-Nannestad Date: Tue, 19 Mar 2019 15:44:11 +0100 Subject: [PATCH 3/4] Address review comments. --- .../CWE-295/MissingHostKeyValidation.qhelp | 21 ++++++++++-------- .../CWE-295/MissingHostKeyValidation.ql | 18 +++++++++------ .../CWE-295/examples/paramiko_host_key.py | 22 ++++++++++++++----- .../CWE-295/MissingHostKeyValidation.expected | 2 ++ .../Security/CWE-295/paramiko_host_key.py | 6 +++++ 5 files changed, 47 insertions(+), 22 deletions(-) diff --git a/python/ql/src/Security/CWE-295/MissingHostKeyValidation.qhelp b/python/ql/src/Security/CWE-295/MissingHostKeyValidation.qhelp index c77b64314776..a32debbac427 100644 --- a/python/ql/src/Security/CWE-295/MissingHostKeyValidation.qhelp +++ b/python/ql/src/Security/CWE-295/MissingHostKeyValidation.qhelp @@ -13,20 +13,23 @@ man-in-the-middle attacks.

    -Do not accept unknown host keys. For the Paramiko library in particular, avoid -setting the missing host key policy to either AutoAddPolicy or -WarningPolicy, as both of these will continue even when the host -key is unknown. The default RejectPolicy 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 AutoAddPolicy or +WarningPolicy. Both of these policies continue even when the host +key is unknown. The default setting of RejectPolicy is secure +because it throws an exception when it encounters an unknown host key.

    -The following example opens a connection to example.com with the -missing host key policy set to AutoAddPolicy. 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 +example.com. The first function sets the missing host key policy to +AutoAddPolicy. 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 +RejectPolicy, and will throw an exception if the host key +verification fails.

    diff --git a/python/ql/src/Security/CWE-295/MissingHostKeyValidation.ql b/python/ql/src/Security/CWE-295/MissingHostKeyValidation.ql index 5f4295211255..5ebca56986f0 100644 --- a/python/ql/src/Security/CWE-295/MissingHostKeyValidation.ql +++ b/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." diff --git a/python/ql/src/Security/CWE-295/examples/paramiko_host_key.py b/python/ql/src/Security/CWE-295/examples/paramiko_host_key.py index 70d42612f56a..d197cb9741dd 100644 --- a/python/ql/src/Security/CWE-295/examples/paramiko_host_key.py +++ b/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() diff --git a/python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.expected b/python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.expected index dc85718d804c..bcbb79ad6ff4 100644 --- a/python/ql/test/query-tests/Security/CWE-295/MissingHostKeyValidation.expected +++ b/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. | diff --git a/python/ql/test/query-tests/Security/CWE-295/paramiko_host_key.py b/python/ql/test/query-tests/Security/CWE-295/paramiko_host_key.py index e59db86c8b17..2dd13aafe18a 100644 --- a/python/ql/test/query-tests/Security/CWE-295/paramiko_host_key.py +++ b/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 From 20e2f9ee4ec86ba77db23b04cb66c5b49ea4f5e9 Mon Sep 17 00:00:00 2001 From: Taus <45175834+taus-semmle@users.noreply.github.com> Date: Wed, 20 Mar 2019 11:48:02 +0100 Subject: [PATCH 4/4] Remove extraneous period. --- change-notes/1.21/analysis-python.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change-notes/1.21/analysis-python.md b/change-notes/1.21/analysis-python.md index 645bbb33cf9a..49f0127c6043 100644 --- a/change-notes/1.21/analysis-python.md +++ b/change-notes/1.21/analysis-python.md @@ -9,7 +9,7 @@ ## 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. | + | 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