python: remove protocol family

this concept was due to my confusion between TLS and SSL23, but they are aliases. We might want to bring back the concept if we model DTLS. Also, model what exactly creations allow, bring this back from the unrestrictions they used to be. We accept the changes regarding sources being reported differently.
2025-12-24 04:36:35 +01:00 · 2023-03-07 14:41:13 +01:00
parent 8160f742a5
commit 072df5dbc0
5 changed files with 85 additions and 82 deletions
--- a/python/ql/src/Security/CWE-327/FluentApiModel.qll
+++ b/python/ql/src/Security/CWE-327/FluentApiModel.qll
@@ -61,9 +61,14 @@ module InsecureContextConfiguration implements DataFlow::StateConfigSig {
  }

  predicate isSource(DataFlow::Node source, FlowState state) {
-    exists(ProtocolFamily family |
-      source = state.getLibrary().unspecific_context_creation(family) and
-      state.getBits() = family.getBits()
+    exists(ContextCreation creation | source = creation |
+      creation = state.getLibrary().unspecific_context_creation() and
+      state.getBits() =
+        sum(ProtocolVersion version |
+          version = creation.getProtocol() and version.isInsecure()
+        |
+          version.getBit()
+        )
    )
  }

--- a/python/ql/src/Security/CWE-327/PyOpenSSL.qll
+++ b/python/ql/src/Security/CWE-327/PyOpenSSL.qll
@@ -12,14 +12,18 @@ class PyOpenSslContextCreation extends ContextCreation, DataFlow::CallCfgNode {
    this = API::moduleImport("OpenSSL").getMember("SSL").getMember("Context").getACall()
  }

-  override string getProtocol() {
+  override ProtocolVersion getProtocol() {
    exists(DataFlow::Node protocolArg, PyOpenSsl pyo |
      protocolArg in [this.getArg(0), this.getArgByName("method")]
    |
-      protocolArg in [
-          pyo.specific_version(result).getAValueReachableFromSource(),
-          pyo.unspecific_version(result).getAValueReachableFromSource()
-        ]
+      protocolArg = pyo.specific_version(result).getAValueReachableFromSource()
+      or
+      protocolArg = pyo.unspecific_version().getAValueReachableFromSource() and
+      // PyOpenSSL also allows DTLS
+      // see https://www.pyopenssl.org/en/stable/api/ssl.html#OpenSSL.SSL.Context
+      // although they are not mentioned here:
+      // https://www.pyopenssl.org/en/stable/api/ssl.html#OpenSSL.SSL.TLS_METHOD
+      result = any(ProtocolVersion pv)
    )
  }
 }
@@ -51,18 +55,21 @@ class SetOptionsCall extends ProtocolRestriction, DataFlow::CallCfgNode {
  }
 }

-class UnspecificPyOpenSslContextCreation extends PyOpenSslContextCreation, UnspecificContextCreation {
-  // UnspecificPyOpenSslContextCreation() { library instanceof PyOpenSsl }
-}
-
+// class UnspecificPyOpenSslContextCreation extends PyOpenSslContextCreation, UnspecificContextCreation {
+//   override ProtocolVersion getProtocol() { result = PyOpenSslContextCreation.super.getProtocol() }
+// }
 class PyOpenSsl extends TlsLibrary {
  PyOpenSsl() { this = "pyOpenSSL" }

  override string specific_version_name(ProtocolVersion version) { result = version + "_METHOD" }

-  override string unspecific_version_name(ProtocolFamily family) {
-    // `"TLS_METHOD"` is not actually available in pyOpenSSL yet, but should be coming soon..
-    result = family + "_METHOD"
+  override string unspecific_version_name() {
+    // See
+    // - https://www.pyopenssl.org/en/23.0.0/api/ssl.html#module-OpenSSL.SSL
+    // - https://www.openssl.org/docs/manmaster/man3/DTLS_server_method.html#NOTES
+    result = ["TLS", "SSLv23"] + "_METHOD"
+    or
+    result = "TLS_" + ["CLIENT", "SERVER"] + "_METHOD"
  }

  override API::Node version_constants() { result = API::moduleImport("OpenSSL").getMember("SSL") }
--- a/python/ql/src/Security/CWE-327/Ssl.qll
+++ b/python/ql/src/Security/CWE-327/Ssl.qll
@@ -10,20 +10,21 @@ import TlsLibraryModel
 class SslContextCreation extends ContextCreation, DataFlow::CallCfgNode {
  SslContextCreation() { this = API::moduleImport("ssl").getMember("SSLContext").getACall() }

-  override string getProtocol() {
+  override ProtocolVersion getProtocol() {
    exists(DataFlow::Node protocolArg, Ssl ssl |
      protocolArg in [this.getArg(0), this.getArgByName("protocol")]
    |
-      protocolArg =
-        [
-          ssl.specific_version(result).getAValueReachableFromSource(),
-          ssl.unspecific_version(result).getAValueReachableFromSource()
-        ]
+      protocolArg = ssl.specific_version(result).getAValueReachableFromSource()
+      or
+      protocolArg = ssl.unspecific_version().getAValueReachableFromSource() and
+      // see https://docs.python.org/3/library/ssl.html#id7
+      result in ["TLSv1", "TLSv1_1", "TLSv1_2", "TLSv1_3"]
    )
    or
    not exists(this.getArg(_)) and
    not exists(this.getArgByName(_)) and
-    result = "TLS"
+    // see https://docs.python.org/3/library/ssl.html#id7
+    result in ["TLSv1", "TLSv1_1", "TLSv1_2", "TLSv1_3"]
  }
 }

@@ -34,7 +35,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 string getProtocol() { result = "TLS" }
+  override ProtocolVersion getProtocol() { result in ["TLSv1", "TLSv1_1", "TLSv1_2", "TLSv1_3"] }
 }

 /** Gets a reference to an `ssl.Context` instance. */
@@ -161,33 +162,29 @@ class ContextSetVersion extends ProtocolRestriction, ProtocolUnrestriction, Data
  }
 }

-class UnspecificSslContextCreation extends SslContextCreation, UnspecificContextCreation {
-  // UnspecificSslContextCreation() { library instanceof Ssl }
-  // override ProtocolVersion getUnrestriction() {
-  //   result = UnspecificContextCreation.super.getUnrestriction() and
-  //   // 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"]
-  // }
-}
-
-class UnspecificSslDefaultContextCreation extends SslDefaultContextCreation {
-  // override DataFlow::Node getContext() { result = this }
-  // // see https://docs.python.org/3/library/ssl.html#ssl.create_default_context
-  // override ProtocolVersion getUnrestriction() {
-  //   result in ["TLSv1", "TLSv1_1", "TLSv1_2", "TLSv1_3"]
-  // }
-}
-
+// class UnspecificSslContextCreation extends SslContextCreation, UnspecificContextCreation {
+//   // UnspecificSslContextCreation() { library instanceof Ssl }
+//   override ProtocolVersion getProtocol() {
+//     result = UnspecificContextCreation.super.getProtocol() and
+//     // 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"]
+//   }
+// }
+// class UnspecificSslDefaultContextCreation extends SslDefaultContextCreation {
+//   // override DataFlow::Node getContext() { result = this }
+//   // see https://docs.python.org/3/library/ssl.html#ssl.create_default_context
+//   override ProtocolVersion getProtocol() { result in ["TLSv1", "TLSv1_1", "TLSv1_2", "TLSv1_3"] }
+// }
 class Ssl extends TlsLibrary {
  Ssl() { this = "ssl" }

  override string specific_version_name(ProtocolVersion version) { result = "PROTOCOL_" + version }

-  override string unspecific_version_name(ProtocolFamily family) {
-    family = "SSLv23" and result = "PROTOCOL_" + family
+  override string unspecific_version_name() {
+    result = "PROTOCOL_SSLv23"
    or
-    family = "TLS" and result = "PROTOCOL_" + family + ["", "_CLIENT", "_SERVER"]
+    result = "PROTOCOL_TLS" + ["", "_CLIENT", "_SERVER"]
  }

  override API::Node version_constants() { result = API::moduleImport("ssl") }
--- a/python/ql/src/Security/CWE-327/TlsLibraryModel.qll
+++ b/python/ql/src/Security/CWE-327/TlsLibraryModel.qll
@@ -37,29 +37,23 @@ class ProtocolVersion extends string {
    or
    this = "TLSv1_3" and result = 32
  }
-
-  /** Gets the protocol family for this protocol version. */
-  ProtocolFamily getFamily() {
-    result = "SSLv23" and this in ["SSLv2", "SSLv3"]
-    or
-    result = "TLS" and this in ["TLSv1", "TLSv1_1", "TLSv1_2", "TLSv1_3"]
-  }
-}
-
-/** An unspecific protocol version */
-class ProtocolFamily extends string {
-  ProtocolFamily() { this in ["SSLv23", "TLS"] }
-
-  /** Gets the bit mask for this protocol family. */
-  int getBits() {
-    result = sum(ProtocolVersion version | version.getFamily() = this | version.getBit())
-  }
 }

 /** The creation of a context. */
 abstract class ContextCreation extends DataFlow::Node {
-  /** Gets the protocol version or family for this context. */
-  abstract string getProtocol();
+  /**
+   * Gets the protocol version for this context.
+   * There can be multiple values if the context was created
+   * using a non-specific version such as `TLS`.
+   */
+  abstract ProtocolVersion getProtocol();
+
+  /**
+   * Holds if the context was created with a specific version
+   * rather than with a version flexible method, see:
+   * https://www.openssl.org/docs/manmaster/man3/DTLS_server_method.html#NOTES
+   */
+  predicate specificVersion() { count(this.getProtocol()) = 1 }
 }

 /** The creation of a connection from a context. */
@@ -91,13 +85,12 @@ abstract class ProtocolUnrestriction extends DataFlow::Node {
 * This also serves as unrestricting these protocols.
 */
 abstract class UnspecificContextCreation extends ContextCreation {
-  // override ProtocolVersion getUnrestriction() {
-  //   // There is only one family, the two names are aliases in OpenSSL.
-  //   // see https://github.com/openssl/openssl/blob/13888e797c5a3193e91d71e5f5a196a2d68d266f/include/openssl/ssl.h.in#L1953-L1955
-  //   family in ["SSLv23", "TLS"] and
-  //   // see https://docs.python.org/3/library/ssl.html#ssl-contexts
-  //   result in ["SSLv2", "SSLv3", "TLSv1", "TLSv1_1", "TLSv1_2", "TLSv1_3"]
-  // }
+  override ProtocolVersion getProtocol() {
+    // There is only one family, the two names are aliases in OpenSSL.
+    // see https://github.com/openssl/openssl/blob/13888e797c5a3193e91d71e5f5a196a2d68d266f/include/openssl/ssl.h.in#L1953-L1955
+    // see https://docs.python.org/3/library/ssl.html#ssl-contexts
+    result in ["SSLv2", "SSLv3", "TLSv1", "TLSv1_1", "TLSv1_2", "TLSv1_3"]
+  }
 }

 /** A model of a SSL/TLS library. */
@@ -108,8 +101,8 @@ abstract class TlsLibrary extends string {
  /** Gets the name of a specific protocol version. */
  abstract string specific_version_name(ProtocolVersion version);

-  /** 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);
+  /** Gets a name, which is a member of `version_constants`, that can be used to specify the entire protocol family. */
+  abstract string unspecific_version_name();

  /** Gets an API node representing the module or class holding the version constants. */
  abstract API::Node version_constants();
@@ -119,9 +112,9 @@ abstract class TlsLibrary extends string {
    result = this.version_constants().getMember(this.specific_version_name(version))
  }

-  /** Gets an API node representing the protocol family `family`. */
-  API::Node unspecific_version(ProtocolFamily family) {
-    result = this.version_constants().getMember(this.unspecific_version_name(family))
+  /** Gets an API node representing the protocol entire family. */
+  API::Node unspecific_version() {
+    result = this.version_constants().getMember(this.unspecific_version_name())
  }

  /** Gets a creation of a context with a default protocol. */
@@ -133,14 +126,15 @@ abstract class TlsLibrary extends string {
  /** Gets a creation of a context with a specific protocol version, known to be insecure. */
  ContextCreation insecure_context_creation(ProtocolVersion version) {
    result in [this.specific_context_creation(), this.default_context_creation()] and
+    result.specificVersion() and
    result.getProtocol() = version and
    version.isInsecure()
  }

  /** Gets a context that was created using `family`, known to have insecure instances. */
-  ContextCreation unspecific_context_creation(ProtocolFamily family) {
+  ContextCreation unspecific_context_creation() {
    result in [this.specific_context_creation(), this.default_context_creation()] and
-    result.getProtocol() = family
+    not result.specificVersion()
  }

  /** Gets a dataflow node representing a connection being created in an insecure manner, not from a context. */
--- a/python/ql/test/query-tests/Security/CWE-327-InsecureProtocol/InsecureProtocol.expected
+++ b/python/ql/test/query-tests/Security/CWE-327-InsecureProtocol/InsecureProtocol.expected
@@ -27,13 +27,13 @@
 | ssl_fluent.py:71:14:71:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1 allowed by $@. | ssl_fluent.py:62:12:62:43 | ControlFlowNode for Attribute() | call to ssl.SSLContext |
 | ssl_fluent.py:71:14:71:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1 allowed by $@. | ssl_fluent.py:101:15:101:46 | ControlFlowNode for Attribute() | call to ssl.SSLContext |
 | ssl_fluent.py:71:14:71:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:62:12:62:43 | ControlFlowNode for Attribute() | call to ssl.SSLContext |
+| ssl_fluent.py:71:14:71:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:65:15:65:46 | ControlFlowNode for Attribute() | call to ssl.SSLContext |
 | ssl_fluent.py:71:14:71:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:101:15:101:46 | ControlFlowNode for Attribute() | call to ssl.SSLContext |
-| ssl_fluent.py:71:14:71:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:117:5:117:11 | ControlFlowNode for context | context modification |
-| ssl_fluent.py:71:14:71:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:135:5:135:11 | ControlFlowNode for context | context modification |
+| ssl_fluent.py:71:14:71:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:115:15:115:46 | ControlFlowNode for Attribute() | call to ssl.SSLContext |
 | ssl_fluent.py:77:14:77:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1 allowed by $@. | ssl_fluent.py:62:12:62:43 | ControlFlowNode for Attribute() | call to ssl.SSLContext |
 | ssl_fluent.py:77:14:77:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:62:12:62:43 | ControlFlowNode for Attribute() | call to ssl.SSLContext |
-| ssl_fluent.py:97:14:97:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:95:5:95:11 | ControlFlowNode for context | context modification |
-| ssl_fluent.py:146:14:146:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:143:5:143:11 | ControlFlowNode for context | context modification |
-| ssl_fluent.py:165:14:165:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version SSLv3 allowed by $@. | ssl_fluent.py:162:5:162:11 | ControlFlowNode for context | context modification |
+| ssl_fluent.py:97:14:97:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:65:15:65:46 | ControlFlowNode for Attribute() | call to ssl.SSLContext |
+| ssl_fluent.py:146:14:146:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:142:15:142:46 | ControlFlowNode for Attribute() | call to ssl.SSLContext |
+| ssl_fluent.py:165:14:165:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version SSLv3 allowed by $@. | ssl_fluent.py:161:15:161:65 | ControlFlowNode for Attribute() | call to ssl.create_default_context |
 | ssl_fluent.py:165:14:165:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1 allowed by $@. | ssl_fluent.py:161:15:161:65 | ControlFlowNode for Attribute() | call to ssl.create_default_context |
 | ssl_fluent.py:165:14:165:20 | ControlFlowNode for context | Insecure SSL/TLS protocol version TLSv1_1 allowed by $@. | ssl_fluent.py:161:15:161:65 | ControlFlowNode for Attribute() | call to ssl.create_default_context |