Python: Refactor based on review

- more natural handling of default arguments
- do not assume default construction gives a family
- simplifies `UnspecificSSLContextCreation`

Author: yoff

python/ql/src/Security/CWE-327/PyOpenSSL.qll

@@ -9,8 +9,12 @@ class PyOpenSSLContextCreation extends ContextCreation {
     this = API::moduleImport("OpenSSL").getMember("SSL").getMember("Context").getACall()
   }
 
-  override DataFlow::CfgNode getProtocol() {
-    result.getNode() in [node.getArg(0), node.getArgByName("method")]
+  override string getProtocol() {
+    exists(ControlFlowNode protocolArg, PyOpenSSL pyo |
+      protocolArg in [node.getArg(0), node.getArgByName("method")]
+    |
+      protocolArg = [pyo.specific_version(result), pyo.unspecific_version(result)].asCfgNode()
+    )
   }
 }
 

python/ql/src/Security/CWE-327/Ssl.qll

@@ -7,8 +7,15 @@ class SSLContextCreation extends ContextCreation {
 
   SSLContextCreation() { this = API::moduleImport("ssl").getMember("SSLContext").getACall() }
 
-  override DataFlow::CfgNode getProtocol() {
-    result.getNode() in [node.getArg(0), node.getArgByName("protocol")]
+  override string getProtocol() {
+    exists(ControlFlowNode protocolArg, Ssl ssl |
+      protocolArg in [node.getArg(0), node.getArgByName("protocol")]
+    |
+      protocolArg = [ssl.specific_version(result), ssl.unspecific_version(result)].asCfgNode()
+    )
+    or
+    not exists(node.getAnArg()) and
+    result = "TLS"
   }
 }
 
@@ -19,7 +26,7 @@ class SSLDefaultContextCreation extends ContextCreation {
 
   // Allowed insecure versions are "TLSv1" and "TLSv1_1"
   // see https://docs.python.org/3/library/ssl.html#context-creation
-  override DataFlow::CfgNode getProtocol() { none() }
+  override string getProtocol() { result = "TLS" }
 }
 
 /** Gets a reference to an `ssl.Context` instance. */
@@ -141,17 +148,10 @@ class UnspecificSSLContextCreation extends SSLContextCreation, UnspecificContext
   UnspecificSSLContextCreation() { library = "ssl" }
 
   override ProtocolVersion getUnrestriction() {
-    // Case: A protocol argument is present.
     result = UnspecificContextCreation.super.getUnrestriction() and
     // These are turned off by default
     // see https://docs.python.org/3/library/ssl.html#ssl-contexts
     not result in ["SSLv2", "SSLv3"]
-    or
-    // Case: No protocol arguemnt is present.
-    not exists(this.getProtocol()) and
-    // The default argument is TLS and the SSL versions are turned off by default since Python 3.6
-    // see https://docs.python.org/3.6/library/ssl.html#ssl.SSLContext
-    result in ["TLSv1", "TLSv1_1", "TLSv1_2", "TLSv1_3"]
   }
 }
 
@@ -185,8 +185,9 @@ class Ssl extends TlsLibrary {
 
   override DataFlow::CfgNode insecure_connection_creation(ProtocolVersion version) {
     result = API::moduleImport("ssl").getMember("wrap_socket").getACall() and
-    insecure_version(version).asCfgNode() =
-      result.asCfgNode().(CallNode).getArgByName("ssl_version")
+    specific_version(version).asCfgNode() =
+      result.asCfgNode().(CallNode).getArgByName("ssl_version") and
+    version.isInsecure()
   }
 
   override ConnectionCreation connection_creation() { result instanceof WrapSocketCall }

python/ql/src/Security/CWE-327/TlsLibraryModel.qll

@@ -30,8 +30,8 @@ class ProtocolFamily extends string {
 
 /** The creation of a context. */
 abstract class ContextCreation extends DataFlow::CfgNode {
-  /** Gets the requested protocol if any. */
-  abstract DataFlow::CfgNode getProtocol();
+  /** Gets the protocol version or family for this context. */
+  abstract string getProtocol();
 }
 
 /** The creation of a connection from a context. */
@@ -66,7 +66,7 @@ abstract class UnspecificContextCreation extends ContextCreation, ProtocolUnrest
   TlsLibrary library;
   ProtocolFamily family;
 
-  UnspecificContextCreation() { this.getProtocol() = library.unspecific_version(family) }
+  UnspecificContextCreation() { this.getProtocol() = family }
 
   override DataFlow::CfgNode getContext() { result = this }
 
@@ -92,9 +92,8 @@ abstract class TlsLibrary extends string {
   /** The module or class holding the version constants. */
   abstract API::Node version_constants();
 
-  /** A dataflow node representing a specific protocol version, known to be insecure. */
-  DataFlow::Node insecure_version(ProtocolVersion version) {
-    version.isInsecure() and
+  /** A dataflow node representing a specific protocol version. */
+  DataFlow::Node specific_version(ProtocolVersion version) {
     result = version_constants().getMember(specific_version_name(version)).getAUse()
   }
 
@@ -111,16 +110,15 @@ abstract class TlsLibrary extends string {
 
   /** The creation of a context with a specific protocol version, known to be insecure. */
   ContextCreation insecure_context_creation(ProtocolVersion version) {
-    result = specific_context_creation() and
-    result.getProtocol() = insecure_version(version)
+    result in [specific_context_creation(), default_context_creation()] and
+    result.getProtocol() = version and
+    version.isInsecure()
   }
 
   /** The creation of a context with an unspecific protocol version, say TLS, known to have insecure instances. */
   ContextCreation unspecific_context_creation(ProtocolFamily family) {
-    result = default_context_creation()
-    or
-    result = specific_context_creation() and
-    result.(ContextCreation).getProtocol() = unspecific_version(family)
+    result in [specific_context_creation(), default_context_creation()] and
+    result.getProtocol() = family
   }
 
   /** A connection is created in an insecure manner, not from a context. */