Merge pull request github#5246 from yoff/python-port-insecure-default-protocol

Python: Port insecure default protocol

Author: RasmusWL

python/change-notes/2021-02-23-port-insecure-default-protocol.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Updated the query that detects insecure SSL/TLS protocol creation from default values (`py/insecure-default-protocol`) to use the new API graphs. The query also no longer reports use of the default value for constructing `ssl.SSLContext`, since that _can_ still be secure, either through manipulation of the `options` field or the `minimum_version` field. If the usage is not secure, this should be reported by the `py/insecure-protocol` query.

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.qhelp

@@ -3,21 +3,22 @@
 "qhelp.dtd">
 <qhelp>
      <overview>
-          <p> The <code>ssl</code> library defaults to an insecure version of
-          SSL/TLS when no specific protocol version is specified. This may leave
-          the connection vulnerable to attack.
+          <p>
+          The <code>ssl.wrap_socket</code> function defaults
+          to an insecure version of SSL/TLS when no specific protocol version is
+          specified. This may leave the connection vulnerable to attack.
           </p>
 
      </overview>
      <recommendation>
 
           <p>
             Ensure that a modern, strong protocol is used. All versions of SSL,
-            and TLS 1.0 are known to be vulnerable to attacks. Using TLS 1.1 or
+            and TLS 1.0 and 1.1 are known to be vulnerable to attacks. Using TLS 1.2 or
             above is strongly recommended. If no explicit
             <code>ssl_version</code> is specified, the default
-            <code>PROTOCOL_TLS</code> is chosen. This protocol is insecure and
-            should not be used.
+            <code>PROTOCOL_TLS</code> is chosen. This protocol is insecure because it
+            allows TLS 1.0 and TLS 1.1 and so should not be used.
           </p>
 
      </recommendation>
@@ -34,20 +35,43 @@
           <p>
             Both of the cases above should be updated to use a secure protocol
             instead, for instance by specifying
-            <code>ssl_version=PROTOCOL_TLSv1_1</code> as a keyword argument.
+            <code>ssl_version=PROTOCOL_TLSv1_2</code> as a keyword argument.
+          </p>
+          <p>
+            The latter example can also be made secure by modifying the created
+            context before it is used to create a connection. Therefore it will not be
+            flagged by this query. However, if a connection is created before
+            the context has been secured (for example, by setting the value of <code>minimum_version</code>),
+            then the code should be flagged by the query <code>py/insecure-protocol</code>.
           </p>
           <p>
             Note that <code>ssl.wrap_socket</code> has been deprecated in
-            Python 3.7. A preferred alternative is to use
-            <code>ssl.SSLContext</code>, which is supported in Python 2.7.9 and
-            3.2 and later versions.
+            Python 3.7. The recommended alternatives are:
           </p>
+          <ul>
+            <li><code>ssl.SSLContext</code> - supported in Python 2.7.9,
+                  3.2, and later versions</li>
+            <li><code>ssl.create_default_context</code> - a convenience function,
+                  supported in Python 3.4 and later versions.</li>
+          </ul>
+
+          <p>
+            Even when you use these alternatives, you should
+            ensure that a safe protocol is used. The following code illustrates
+            how to use flags (available since Python 3.2) or the `minimum_version`
+            field (favored since Python 3.7) to restrict the protocols accepted when
+            creating a connection.
+          </p>
+
+          <sample src="examples/secure_default_protocol.py" />
      </example>
 
      <references>
        <li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Transport_Layer_Security"> Transport Layer Security</a>.</li>
        <li>Python 3 documentation: <a href="https://docs.python.org/3/library/ssl.html#ssl.SSLContext"> class ssl.SSLContext</a>.</li>
        <li>Python 3 documentation: <a href="https://docs.python.org/3/library/ssl.html#ssl.wrap_socket"> ssl.wrap_socket</a>.</li>
+       <li>Python 3 documentation: <a href="https://docs.python.org/3/library/ssl.html#functions-constants-and-exceptions"> notes on context creation</a>.</li>
+       <li>Python 3 documentation: <a href="https://docs.python.org/3/library/ssl.html#ssl-security"> notes on security considerations</a>.</li>
      </references>
 
 </qhelp>

python/ql/src/Security/CWE-327/InsecureDefaultProtocol.ql

@@ -10,25 +10,19 @@
  *       external/cwe/cwe-327
  */
 
+// Connections are generally created based on a context which controls the range of acceptable
+// protocols. This query reports the deprecated way of creating connections without referring
+// to a context (via `ssl.wrap_socket`). Doing this and not specifying which protocols are
+// acceptable means that connections will be created with the insecure default settings.
+//
+// Detecting that a connection is created with a context that has not been suitably configured
+// is handled by the data-flow query py/insecure-protocol.
 import python
+import semmle.python.ApiGraphs
 
-FunctionValue ssl_wrap_socket() { result = Value::named("ssl.wrap_socket") }
-
-ClassValue ssl_Context_class() { result = Value::named("ssl.SSLContext") }
-
-CallNode unsafe_call(string method_name) {
-  result = ssl_wrap_socket().getACall() and
-  not exists(result.getArgByName("ssl_version")) and
-  method_name = "deprecated method ssl.wrap_socket"
-  or
-  result = ssl_Context_class().getACall() and
-  not exists(result.getArgByName("protocol")) and
-  not exists(result.getArg(0)) and
-  method_name = "ssl.SSLContext"
-}
-
-from CallNode call, string method_name
-where call = unsafe_call(method_name)
+from DataFlow::CallCfgNode call
+where
+  call = API::moduleImport("ssl").getMember("wrap_socket").getACall() and
+  not exists(call.getArgByName("ssl_version"))
 select call,
-  "Call to " + method_name +
-    " does not specify a protocol, which may result in an insecure default being used."
+  "Call to deprecated method ssl.wrap_socket does not specify a protocol, which may result in an insecure default being used."

python/ql/src/Security/CWE-327/examples/secure_default_protocol.py

@@ -0,0 +1,9 @@
+import ssl
+
+# Using flags to restrict the protocol
+context = ssl.SSLContext()
+context.options |= ssl.OP_NO_TLSv1 | ssl.OP_NO_TLSv1_1
+
+# Declaring a minimum version to restrict the protocol
+context = ssl.create_default_context()
+context.minimum_version = ssl.TLSVersion.TLSv1_2

python/ql/test/query-tests/Security/CWE-327-py2/InsecureDefaultProtocol.expected

@@ -0,0 +1 @@
+| InsecureProtocol.py:7:1:7:17 | ControlFlowNode for Attribute() | Call to deprecated method ssl.wrap_socket does not specify a protocol, which may result in an insecure default being used. |

python/ql/test/query-tests/Security/CWE-327/InsecureDefaultProtocol.qlref renamed to python/ql/test/query-tests/Security/CWE-327-py2/InsecureDefaultProtocol.qlref

@@ -0,0 +1,7 @@
+import ssl
+
+# secure versions
+ssl.wrap_socket(ssl_version=ssl.PROTOCOL_TLSv1_2)
+
+# possibly insecure default
+ssl.wrap_socket()

python/ql/test/query-tests/Security/CWE-327-py2/InsecureProtocol.py

@@ -0,0 +1 @@
+semmle-extractor-options: --lang=2 --max-import-depth=1