Apply suggestions from code review

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

Author: yoff

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

@@ -1,4 +1,4 @@
-import python
+private import python
 import TlsLibraryModel
 
 /**
@@ -89,17 +89,14 @@ predicate unsafe_connection_creation_with_context(
 /**
  * Holds if `conectionCreation` marks the creation of a connetion witout reference to a context
  * and allowing `insecure_version`.
- *
- * `specific` is true iff the context is configured for a specific protocol version rather
- * than for a family of protocols.
  */
 predicate unsafe_connection_creation_without_context(
   DataFlow::CallCfgNode connectionCreation, string insecure_version
 ) {
   exists(TlsLibrary l | connectionCreation = l.insecure_connection_creation(insecure_version))
 }
 
-/** Holds if `contextCreation` is creating a context ties to a specific insecure version. */
+/** Holds if `contextCreation` is creating a context tied to a specific insecure version. */
 predicate unsafe_context_creation(DataFlow::CallCfgNode contextCreation, string insecure_version) {
   exists(TlsLibrary l | contextCreation = l.insecure_context_creation(insecure_version))
 }

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

@@ -1,5 +1,9 @@
-import python
-import semmle.python.ApiGraphs
+/**
+ * Provides modeling of SSL/TLS functionality of the `OpenSSL` module from the `pyOpenSSL` PyPI package.
+ * See https://www.pyopenssl.org/en/stable/
+ */
+private import python
+private import semmle.python.ApiGraphs
 import TlsLibraryModel
 
 class PyOpenSSLContextCreation extends ContextCreation {
@@ -49,7 +53,7 @@ class SetOptionsCall extends ProtocolRestriction {
 }
 
 class UnspecificPyOpenSSLContextCreation extends PyOpenSSLContextCreation, UnspecificContextCreation {
-  UnspecificPyOpenSSLContextCreation() { library = "pyOpenSSL" }
+  UnspecificPyOpenSSLContextCreation() { library instanceof PyOpenSSL }
 }
 
 class PyOpenSSL extends TlsLibrary {

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

@@ -1,5 +1,9 @@
-import python
-import semmle.python.ApiGraphs
+/**
+ * Provides modeling of SSL/TLS functionality of the `ssl` module from the standard library.
+ * See https://docs.python.org/3.9/library/ssl.html
+ */
+private import python
+private import semmle.python.ApiGraphs
 import TlsLibraryModel
 
 class SSLContextCreation extends ContextCreation {
@@ -145,12 +149,12 @@ class ContextSetVersion extends ProtocolRestriction, ProtocolUnrestriction {
 }
 
 class UnspecificSSLContextCreation extends SSLContextCreation, UnspecificContextCreation {
-  UnspecificSSLContextCreation() { library = "ssl" }
+  UnspecificSSLContextCreation() { library instanceof Ssl }
 
   override ProtocolVersion getUnrestriction() {
     result = UnspecificContextCreation.super.getUnrestriction() and
-    // These are turned off by default
-    // see https://docs.python.org/3/library/ssl.html#ssl-contexts
+    // These are turned off by default since Python 3.6
+    // see https://docs.python.org/3.6/library/ssl.html#ssl.SSLContext
     not result in ["SSLv2", "SSLv3"]
   }
 }
@@ -185,8 +189,8 @@ class Ssl extends TlsLibrary {
 
   override DataFlow::CfgNode insecure_connection_creation(ProtocolVersion version) {
     result = API::moduleImport("ssl").getMember("wrap_socket").getACall() and
-    specific_version(version).asCfgNode() =
-      result.asCfgNode().(CallNode).getArgByName("ssl_version") and
+    this.specific_version(version) =
+      result.(DataFlow::CallCfgNode).getArgByName("ssl_version") and
     version.isInsecure()
   }
 

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

@@ -1,15 +1,15 @@
-import python
-import semmle.python.ApiGraphs
+private import python
+private import semmle.python.ApiGraphs
 import Ssl
 import PyOpenSSL
 
 /**
- * A specific protocol version.
- * We use this to identify a protocol.
+ * A specific protocol version of SSL or TLS.
  */
 class ProtocolVersion extends string {
   ProtocolVersion() { this in ["SSLv2", "SSLv3", "TLSv1", "TLSv1_1", "TLSv1_2", "TLSv1_3"] }
 
+  /** Gets a `ProtocolVersion` that is less than this `ProtocolVersion`, if any. */
   predicate lessThan(ProtocolVersion version) {
     this = "SSLv2" and version = "SSLv3"
     or
@@ -20,6 +20,7 @@ class ProtocolVersion extends string {
     this = ["TLSv1", "TLSv1_1", "TLSv1_2"] and version = "TLSv1_3"
   }
 
+  /** Holds if this protocol version is known to be insecure. */
   predicate isInsecure() { this in ["SSLv2", "SSLv3", "TLSv1", "TLSv1_1"] }
 }
 
@@ -81,12 +82,13 @@ abstract class UnspecificContextCreation extends ContextCreation, ProtocolUnrest
 
 /** A model of a SSL/TLS library. */
 abstract class TlsLibrary extends string {
-  TlsLibrary() { this in ["ssl", "pyOpenSSL"] }
+  bindingset[this]
+  TlsLibrary() { any() }
 
   /** The name of a specific protocol version. */
   abstract string specific_version_name(ProtocolVersion version);
 
-  /** The name of an unspecific protocol version, say TLS, known to have insecure instances. */
+  /** Gets a name, which is a member of `version_constants`,  that can be used to specify the protocol family `family`. */
   abstract string unspecific_version_name(ProtocolFamily family);
 
   /** The module or class holding the version constants. */
@@ -97,12 +99,12 @@ abstract class TlsLibrary extends string {
     result = version_constants().getMember(specific_version_name(version)).getAUse()
   }
 
-  /** A dataflow node representing an unspecific protocol version, say TLS, known to have insecure instances. */
+  /** Gets a dataflow node representing the protocol family `family`. */
   DataFlow::Node unspecific_version(ProtocolFamily family) {
     result = version_constants().getMember(unspecific_version_name(family)).getAUse()
   }
 
-  /** The creation of a context with a deafult protocol. */
+  /** The creation of a context with a default protocol. */
   abstract ContextCreation default_context_creation();
 
   /** The creation of a context with a specific protocol. */
@@ -115,7 +117,7 @@ abstract class TlsLibrary extends string {
     version.isInsecure()
   }
 
-  /** The creation of a context with an unspecific protocol version, say TLS, known to have insecure instances. */
+  /** Gets a context that was created using `family`, known to have insecure instances. */
   ContextCreation unspecific_context_creation(ProtocolFamily family) {
     result in [specific_context_creation(), default_context_creation()] and
     result.getProtocol() = family