Merge branch 'main' into jcogs33/mad-metrics-query

python/ql/lib/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.6.5
+
+No user-facing changes.
+
 ## 0.6.4
 
 ### Minor Analysis Improvements

python/ql/lib/change-notes/2022-11-17-py-pam-improve.md

@@ -0,0 +1,4 @@
+---
+ category: majorAnalysis
+---
+* The _PAM authorization bypass due to incorrect usage_ (`py/pam-auth-bypass`) query has been converted to a taint-tracking query, resulting in significantly fewer false positives.

python/ql/lib/change-notes/released/0.6.5.md

@@ -0,0 +1,3 @@
+## 0.6.5
+
+No user-facing changes.

python/ql/lib/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.6.4
+lastReleaseVersion: 0.6.5

python/ql/lib/qlpack.yml

@@ -1,9 +1,11 @@
 name: codeql/python-all
-version: 0.6.5-dev
+version: 0.6.6-dev
 groups: python
 dbscheme: semmlecode.python.dbscheme
 extractor: python
 library: true
 upgrades: upgrades
 dependencies:
-  codeql/regex: ${workspace}
+  codeql/regex: ${workspace}
+dataExtensions:
+  - semmle/python/frameworks/**/model.yml

python/ql/lib/semmle/python/Concepts.qll

@@ -9,6 +9,7 @@ private import semmle.python.dataflow.new.DataFlow
 private import semmle.python.dataflow.new.RemoteFlowSources
 private import semmle.python.dataflow.new.TaintTracking
 private import semmle.python.Frameworks
+private import semmle.python.security.internal.EncryptionKeySizes
 
 /**
  * A data-flow node that executes an operating system command,
@@ -1141,21 +1142,21 @@ module Cryptography {
       abstract class RsaRange extends Range {
         final override string getName() { result = "RSA" }
 
-        final override int minimumSecureKeySize() { result = 2048 }
+        final override int minimumSecureKeySize() { result = minSecureKeySizeRsa() }
       }
 
       /** A data-flow node that generates a new DSA key-pair. */
       abstract class DsaRange extends Range {
         final override string getName() { result = "DSA" }
 
-        final override int minimumSecureKeySize() { result = 2048 }
+        final override int minimumSecureKeySize() { result = minSecureKeySizeDsa() }
       }
 
       /** A data-flow node that generates a new ECC key-pair. */
       abstract class EccRange extends Range {
         final override string getName() { result = "ECC" }
 
-        final override int minimumSecureKeySize() { result = 224 }
+        final override int minimumSecureKeySize() { result = minSecureKeySizeEcc() }
       }
     }
   }

python/ql/lib/semmle/python/dataflow/new/internal/FlowSummaryImpl.qll

@@ -252,14 +252,14 @@ module Public {
     predicate hasProvenance(boolean generated) { summaryElement(this, _, _, _, generated) }
   }
 
-  /** A callable with a flow summary stating there is no flow via the callable. */
-  class NegativeSummarizedCallable extends SummarizedCallableBase {
-    NegativeSummarizedCallable() { negativeSummaryElement(this, _) }
+  /** A callable where there is no flow via the callable. */
+  class NeutralCallable extends SummarizedCallableBase {
+    NeutralCallable() { neutralElement(this, _) }
 
     /**
-     *  Holds if the negative summary is auto generated.
+     *  Holds if the neutral is auto generated.
      */
-    predicate isAutoGenerated() { negativeSummaryElement(this, true) }
+    predicate isAutoGenerated() { neutralElement(this, true) }
   }
 }
 
@@ -1167,9 +1167,9 @@ module Private {
       string toString() { result = super.toString() }
     }
 
-    /** A flow summary to include in the `negativeSummary/1` query predicate. */
-    abstract class RelevantNegativeSummarizedCallable instanceof NegativeSummarizedCallable {
-      /** Gets the string representation of this callable used by `summary/1`. */
+    /** A model to include in the `neutral/1` query predicate. */
+    abstract class RelevantNeutralCallable instanceof NeutralCallable {
+      /** Gets the string representation of this callable used by `neutral/1`. */
       abstract string getCallableCsv();
 
       string toString() { result = super.toString() }
@@ -1186,13 +1186,13 @@ module Private {
       if c.isAutoGenerated() then result = "generated" else result = "manual"
     }
 
-    private string renderProvenanceNegative(NegativeSummarizedCallable c) {
+    private string renderProvenanceNeutral(NeutralCallable c) {
       if c.isAutoGenerated() then result = "generated" else result = "manual"
     }
 
     /**
      * A query predicate for outputting flow summaries in semi-colon separated format in QL tests.
-     * The syntax is: "namespace;type;overrides;name;signature;ext;inputspec;outputspec;kind;provenance"",
+     * The syntax is: "namespace;type;overrides;name;signature;ext;inputspec;outputspec;kind;provenance",
      * ext is hardcoded to empty.
      */
     query predicate summary(string csv) {
@@ -1211,14 +1211,14 @@ module Private {
     }
 
     /**
-     * Holds if a negative flow summary `csv` exists (semi-colon separated format). Used for testing purposes.
+     * Holds if a neutral model `csv` exists (semi-colon separated format). Used for testing purposes.
      * The syntax is: "namespace;type;name;signature;provenance"",
      */
-    query predicate negativeSummary(string csv) {
-      exists(RelevantNegativeSummarizedCallable c |
+    query predicate neutral(string csv) {
+      exists(RelevantNeutralCallable c |
         csv =
           c.getCallableCsv() // Callable information
-            + renderProvenanceNegative(c) // provenance
+            + renderProvenanceNeutral(c) // provenance
       )
     }
   }

python/ql/lib/semmle/python/dataflow/new/internal/FlowSummaryImplSpecific.qll

@@ -92,11 +92,11 @@ predicate summaryElement(
 }
 
 /**
- * Holds if a negative flow summary exists for `c`, which means that there is no
- * flow through `c`. The flag `generated` states whether the summary is autogenerated.
- * Note. Negative flow summaries has not been implemented for Python.
+ * Holds if a neutral model exists for `c`, which means that there is no
+ * flow through `c`. The flag `generated` states whether the neutral model is autogenerated.
+ * Note. Neutral models have not been implemented for Python.
  */
-predicate negativeSummaryElement(FlowSummary::SummarizedCallable c, boolean generated) { none() }
+predicate neutralElement(FlowSummary::SummarizedCallable c, boolean generated) { none() }
 
 /**
  * Gets the summary component for specification component `c`, if any.

python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll

@@ -72,6 +72,7 @@ private module API = Specific::API;
 private module DataFlow = Specific::DataFlow;
 
 private import Specific::AccessPathSyntax
+private import ApiGraphModelsExtensions as Extensions
 
 /** Module containing hooks for providing input data to be interpreted as a model. */
 module ModelInput {
@@ -236,6 +237,8 @@ predicate sourceModel(string type, string path, string kind) {
     row.splitAt(";", 1) = path and
     row.splitAt(";", 2) = kind
   )
+  or
+  Extensions::sourceModel(type, path, kind)
 }
 
 /** Holds if a sink model exists for the given parameters. */
@@ -246,6 +249,8 @@ private predicate sinkModel(string type, string path, string kind) {
     row.splitAt(";", 1) = path and
     row.splitAt(";", 2) = kind
   )
+  or
+  Extensions::sinkModel(type, path, kind)
 }
 
 /** Holds if a summary model `row` exists for the given parameters. */
@@ -258,6 +263,8 @@ private predicate summaryModel(string type, string path, string input, string ou
     row.splitAt(";", 3) = output and
     row.splitAt(";", 4) = kind
   )
+  or
+  Extensions::summaryModel(type, path, input, output, kind)
 }
 
 /** Holds if a type model exists for the given parameters. */
@@ -268,6 +275,8 @@ private predicate typeModel(string type1, string type2, string path) {
     row.splitAt(";", 1) = type2 and
     row.splitAt(";", 2) = path
   )
+  or
+  Extensions::typeModel(type1, type2, path)
 }
 
 /** Holds if a type variable model exists for the given parameters. */
@@ -277,6 +286,8 @@ private predicate typeVariableModel(string name, string path) {
     row.splitAt(";", 0) = name and
     row.splitAt(";", 1) = path
   )
+  or
+  Extensions::typeVariableModel(name, path)
 }
 
 /**

python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModelsExtensions.qll

@@ -0,0 +1,36 @@
+/**
+ * Defines extensible predicates for contributing library models from data extensions.
+ */
+
+/**
+ * Holds if the value at `(type, path)` should be seen as a flow
+ * source of the given `kind`.
+ *
+ * The kind `remote` represents a general remote flow source.
+ */
+extensible predicate sourceModel(string type, string path, string kind);
+
+/**
+ * Holds if the value at `(type, path)` should be seen as a sink
+ * of the given `kind`.
+ */
+extensible predicate sinkModel(string type, string path, string kind);
+
+/**
+ * Holds if calls to `(type, path)`, the value referred to by `input`
+ * can flow to the value referred to by `output`.
+ *
+ * `kind` should be either `value` or `taint`, for value-preserving or taint-preserving steps,
+ * respectively.
+ */
+extensible predicate summaryModel(string type, string path, string input, string output, string kind);
+
+/**
+ * Holds if `(type2, path)` should be seen as an instance of `type1`.
+ */
+extensible predicate typeModel(string type1, string type2, string path);
+
+/**
+ * Holds if `path` can be substituted for a token `TypeVar[name]`.
+ */
+extensible predicate typeVariableModel(string name, string path);

python/ql/lib/semmle/python/frameworks/data/internal/model.yml

@@ -0,0 +1,26 @@
+extensions:
+  # Contribute empty data sets to avoid errors about an undefined extensionals
+  - addsTo:
+      pack: codeql/python-all
+      extensible: sourceModel
+    data: []
+
+  - addsTo:
+      pack: codeql/python-all
+      extensible: sinkModel
+    data: []
+
+  - addsTo:
+      pack: codeql/python-all
+      extensible: summaryModel
+    data: []
+
+  - addsTo:
+      pack: codeql/python-all
+      extensible: typeModel
+    data: []
+
+  - addsTo:
+      pack: codeql/python-all
+      extensible: typeVariableModel
+    data: []

python/ql/lib/semmle/python/security/dataflow/PamAuthorizationCustomizations.qll

@@ -0,0 +1,61 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "PAM Authorization" vulnerabilities.
+ */
+
+import python
+import semmle.python.ApiGraphs
+import semmle.python.dataflow.new.TaintTracking
+import semmle.python.dataflow.new.RemoteFlowSources
+
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "PAM Authorization" vulnerabilities.
+ */
+module PamAuthorizationCustomizations {
+  /**
+   * Models a node corresponding to the `pam` library
+   */
+  API::Node libPam() {
+    exists(API::CallNode findLibCall, API::CallNode cdllCall |
+      findLibCall =
+        API::moduleImport("ctypes").getMember("util").getMember("find_library").getACall() and
+      findLibCall.getParameter(0).getAValueReachingSink().asExpr().(StrConst).getText() = "pam" and
+      cdllCall = API::moduleImport("ctypes").getMember("CDLL").getACall() and
+      cdllCall.getParameter(0).getAValueReachingSink() = findLibCall
+    |
+      result = cdllCall.getReturn()
+    )
+  }
+
+  /**
+   * A data flow source for "PAM Authorization" vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for "PAM Authorization" vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A source of remote user input, considered as a flow source.
+   */
+  class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+
+  /**
+   * A vulnerable `pam_authenticate` call considered as a flow sink.
+   */
+  class VulnPamAuthCall extends API::CallNode, Sink {
+    VulnPamAuthCall() {
+      exists(DataFlow::Node h |
+        this = libPam().getMember("pam_authenticate").getACall() and
+        h = this.getArg(0) and
+        not exists(API::CallNode acctMgmtCall |
+          acctMgmtCall = libPam().getMember("pam_acct_mgmt").getACall() and
+          DataFlow::localFlow(h, acctMgmtCall.getArg(0))
+        )
+      )
+    }
+  }
+}

python/ql/lib/semmle/python/security/dataflow/PamAuthorizationQuery.qll

@@ -0,0 +1,39 @@
+/**
+ * Provides a taint-tracking configuration for detecting "PAM Authorization" vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `PamAuthorization::Configuration` is needed, otherwise
+ * `PamAuthorizationCustomizations` should be imported instead.
+ */
+
+import python
+import semmle.python.ApiGraphs
+import semmle.python.dataflow.new.TaintTracking
+import PamAuthorizationCustomizations::PamAuthorizationCustomizations
+
+/**
+ * A taint-tracking configuration for detecting "PAM Authorization" vulnerabilities.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "PamAuthorization" }
+
+  override predicate isSource(DataFlow::Node node) { node instanceof Source }
+
+  override predicate isSink(DataFlow::Node node) { node instanceof Sink }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    // Models flow from a remotely supplied username field to a PAM `handle`.
+    // `retval = pam_start(service, username, byref(conv), byref(handle))`
+    exists(API::CallNode pamStart, DataFlow::Node handle, API::CallNode pointer |
+      pointer = API::moduleImport("ctypes").getMember(["pointer", "byref"]).getACall() and
+      pamStart = libPam().getMember("pam_start").getACall() and
+      pointer = pamStart.getArg(3) and
+      handle = pointer.getArg(0) and
+      pamStart.getArg(1) = node1 and
+      handle = node2
+    )
+    or
+    // Flow from handle to the authenticate call in the final step
+    exists(VulnPamAuthCall c | c.getArg(0) = node1 | node2 = c)
+  }
+}

python/ql/lib/semmle/python/security/internal/EncryptionKeySizes.qll

@@ -0,0 +1,21 @@
+/**
+ * INTERNAL: Do not use.
+ *
+ * Provides predicates for recommended encryption key sizes.
+ * Such that we can share this logic across our CodeQL analysis of different languages.
+ */
+
+/** Returns the minimum recommended key size for RSA. */
+int minSecureKeySizeRsa() { result = 2048 }
+
+/** Returns the minimum recommended key size for DSA. */
+int minSecureKeySizeDsa() { result = 2048 }
+
+/** Returns the minimum recommended key size for DH. */
+int minSecureKeySizeDh() { result = 2048 }
+
+/** Returns the minimum recommended key size for elliptic curve cryptography. */
+int minSecureKeySizeEcc() { result = 256 }
+
+/** Returns the minimum recommended key size for AES. */
+int minSecureKeySizeAes() { result = 128 }

python/ql/src/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.5.5
+
+No user-facing changes.
+
 ## 0.5.4
 
 No user-facing changes.

python/ql/src/Security/CWE-285/PamAuthorization.ql

@@ -1,7 +1,7 @@
 /**
  * @name PAM authorization bypass due to incorrect usage
  * @description Not using `pam_acct_mgmt` after `pam_authenticate` to check the validity of a login can lead to authorization bypass.
- * @kind problem
+ * @kind path-problem
  * @problem.severity warning
  * @security-severity 8.1
  * @precision high
@@ -11,28 +11,12 @@
  */
 
 import python
+import DataFlow::PathGraph
 import semmle.python.ApiGraphs
-import experimental.semmle.python.Concepts
-import semmle.python.dataflow.new.TaintTracking
+import semmle.python.security.dataflow.PamAuthorizationQuery
 
-API::Node libPam() {
-  exists(API::CallNode findLibCall, API::CallNode cdllCall |
-    findLibCall = API::moduleImport("ctypes").getMember("util").getMember("find_library").getACall() and
-    findLibCall.getParameter(0).getAValueReachingSink().asExpr().(StrConst).getText() = "pam" and
-    cdllCall = API::moduleImport("ctypes").getMember("CDLL").getACall() and
-    cdllCall.getParameter(0).getAValueReachingSink() = findLibCall
-  |
-    result = cdllCall.getReturn()
-  )
-}
-
-from API::CallNode authenticateCall, DataFlow::Node handle
-where
-  authenticateCall = libPam().getMember("pam_authenticate").getACall() and
-  handle = authenticateCall.getArg(0) and
-  not exists(API::CallNode acctMgmtCall |
-    acctMgmtCall = libPam().getMember("pam_acct_mgmt").getACall() and
-    DataFlow::localFlow(handle, acctMgmtCall.getArg(0))
-  )
-select authenticateCall,
-  "This PAM authentication call may lead to an authorization bypass, since 'pam_acct_mgmt' is not called afterwards."
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "This PAM authentication depends on a $@, and 'pam_acct_mgmt' is not called afterwards.",
+  source.getNode(), "user-provided value"

python/ql/src/Security/CWE-326/WeakCryptoKey.qhelp

@@ -11,13 +11,13 @@ As computational power increases, the ability to break ciphers grows and keys ne
 <p>
 The three main asymmetric key algorithms currently in use are Rivest–Shamir–Adleman (RSA) cryptography, Digital Signature Algorithm (DSA), and Elliptic-curve cryptography (ECC).
 With current technology, key sizes of 2048 bits for RSA and DSA,
-or 224 bits for ECC, are regarded as unbreakable.
+or 256 bits for ECC, are regarded as unbreakable.
 </p>
 </overview>
 
 <recommendation>
 <p>
-Increase the key size to the recommended amount or larger. For RSA or DSA this is at least 2048 bits, for ECC this is at least 224 bits.
+Increase the key size to the recommended amount or larger. For RSA or DSA this is at least 2048 bits, for ECC this is at least 256 bits.
 </p>
 </recommendation>
 
@@ -45,4 +45,3 @@ Recommendation for Transitioning the Use of Cryptographic Algorithms and Key Len
 </li>
 </references>
 </qhelp>
-

python/ql/src/change-notes/2022-11-29-ecc-min-secure-keysize.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Bumped the minimum keysize we consider secure for elliptic curve cryptography from 224 to 256 bits, following current best practices. This might effect results from the _Use of weak cryptographic key_ (`py/weak-crypto-key`) query.

python/ql/src/change-notes/released/0.5.5.md

@@ -0,0 +1,3 @@
+## 0.5.5
+
+No user-facing changes.

python/ql/src/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.5.4
+lastReleaseVersion: 0.5.5

python/ql/src/experimental/Security/CWE-022bis/TarSlipImprov.qhelp.disabled

@@ -1,6 +1,7 @@
 <!DOCTYPE qhelp PUBLIC
   "-//Semmle//qhelp//EN"
   "qhelp.dtd">
+<!-- Disabled since it refers to examples which do not exist. -->
 <qhelp>
 
 <overview>

python/ql/src/experimental/Security/CWE-022bis/TarSlipImprov.ql

@@ -4,7 +4,7 @@
  *              destination file path is within the destination directory can cause files outside
  *              the destination directory to be overwritten.
  * @kind path-problem
- * @id py/tarslip
+ * @id py/tarslip-extended
  * @problem.severity error
  * @security-severity 7.5
  * @precision high

python/ql/src/experimental/Security/CWE-079/ReflectedXSS.ql

@@ -6,7 +6,7 @@
  * @problem.severity error
  * @security-severity 2.9
  * @sub-severity high
- * @id py/reflective-xss
+ * @id py/reflective-xss-email
  * @tags security
  *       external/cwe/cwe-079
  *       external/cwe/cwe-116

python/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/python-queries
-version: 0.5.5-dev
+version: 0.5.6-dev
 groups: 
   - python
   - queries

python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.expected

@@ -1 +1,16 @@
-| pam_test.py:48:18:48:44 | ControlFlowNode for pam_authenticate() | This PAM authentication call may lead to an authorization bypass, since 'pam_acct_mgmt' is not called afterwards. |
+edges
+| pam_test.py:0:0:0:0 | ModuleVariableNode for pam_test.request | pam_test.py:71:16:71:22 | ControlFlowNode for request |
+| pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | pam_test.py:4:26:4:32 | GSSA Variable request |
+| pam_test.py:4:26:4:32 | GSSA Variable request | pam_test.py:0:0:0:0 | ModuleVariableNode for pam_test.request |
+| pam_test.py:71:16:71:22 | ControlFlowNode for request | pam_test.py:71:16:71:27 | ControlFlowNode for Attribute |
+| pam_test.py:71:16:71:27 | ControlFlowNode for Attribute | pam_test.py:76:14:76:40 | ControlFlowNode for pam_authenticate() |
+nodes
+| pam_test.py:0:0:0:0 | ModuleVariableNode for pam_test.request | semmle.label | ModuleVariableNode for pam_test.request |
+| pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember |
+| pam_test.py:4:26:4:32 | GSSA Variable request | semmle.label | GSSA Variable request |
+| pam_test.py:71:16:71:22 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
+| pam_test.py:71:16:71:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
+| pam_test.py:76:14:76:40 | ControlFlowNode for pam_authenticate() | semmle.label | ControlFlowNode for pam_authenticate() |
+subpaths
+#select
+| pam_test.py:76:14:76:40 | ControlFlowNode for pam_authenticate() | pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | pam_test.py:76:14:76:40 | ControlFlowNode for pam_authenticate() | This PAM authentication depends on a $@, and 'pam_acct_mgmt' is not called afterwards. | pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | user-provided value |

python/ql/test/query-tests/Security/CWE-285-PamAuthorization/pam_test.py

@@ -1,6 +1,7 @@
 from ctypes import CDLL, POINTER, Structure, byref
 from ctypes import c_char_p, c_int
 from ctypes.util import find_library
+from flask import Flask, request, redirect
 
 
 class PamHandle(Structure):
@@ -38,26 +39,57 @@ pam_acct_mgmt.restype = c_int
 pam_acct_mgmt.argtypes = [PamHandle, c_int]
 
 
-class pam():
+def authenticate_bad_but_no_alert(self, username, service='login'):
+    # This is not OK, but since we don't have flow from a remote-flow-source, we
+    # don't give an alert.
+    handle = PamHandle()
+    conv = PamConv(None, 0)
+    retval = pam_start(service, username, byref(conv), byref(handle))
+    retval = pam_authenticate(handle, 0)
+    # NOT OK: no call to `pam_acct_mgmt`
+    auth_success = retval == 0
 
-    def authenticate_bad(self, username, service='login'):
-        handle = PamHandle()
-        conv = PamConv(None, 0)
-        retval = pam_start(service, username, byref(conv), byref(handle))
+    return auth_success
 
-        retval = pam_authenticate(handle, 0)
-        auth_success = retval == 0
 
-        return auth_success
+def authenticate_good(self, username, service='login'):
+    handle = PamHandle()
+    conv = PamConv(None, 0)
+    retval = pam_start(service, username, byref(conv), byref(handle))
 
-    def authenticate_good(self, username, service='login'):
-        handle = PamHandle()
-        conv = PamConv(None, 0)
-        retval = pam_start(service, username, byref(conv), byref(handle))
+    retval = pam_authenticate(handle, 0)
+    if retval == 0:
+        retval = pam_acct_mgmt(handle, 0)
+    auth_success = retval == 0
 
-        retval = pam_authenticate(handle, 0)
-        if retval == 0:
-            retval = pam_acct_mgmt(handle, 0)
-        auth_success = retval == 0
+    return auth_success
 
-        return auth_success
+
+app = Flask(__name__)
+@app.route('/bad')
+def bad():
+    username = request.args.get('username', '')
+    handle = PamHandle()
+    conv = PamConv(None, 0)
+    retval = pam_start(service, username, byref(conv), byref(handle))
+
+    retval = pam_authenticate(handle, 0)
+    # NOT OK: no call to `pam_acct_mgmt`
+    auth_success = retval == 0
+
+    return auth_success
+
+
+@app.route('/good')
+def good():
+    username = request.args.get('username', '')
+    handle = PamHandle()
+    conv = PamConv(None, 0)
+    retval = pam_start(service, username, byref(conv), byref(handle))
+
+    retval = pam_authenticate(handle, 0)
+    if retval == 0:
+        retval = pam_acct_mgmt(handle, 0)
+    auth_success = retval == 0
+
+    return auth_success

python/ql/test/query-tests/Security/CWE-326-WeakCryptoKey/WeakCryptoKey.expected

@@ -1,8 +1,8 @@
 | weak_crypto.py:68:1:68:21 | ControlFlowNode for dsa_gen_key() | Creation of an DSA key uses $@ bits, which is below 2048 and considered breakable. | weak_crypto.py:16:12:16:15 | ControlFlowNode for IntegerLiteral | 1024 |
-| weak_crypto.py:69:1:69:19 | ControlFlowNode for ec_gen_key() | Creation of an ECC key uses $@ bits, which is below 224 and considered breakable. | weak_crypto.py:22:11:22:22 | ControlFlowNode for Attribute | 163 |
+| weak_crypto.py:69:1:69:19 | ControlFlowNode for ec_gen_key() | Creation of an ECC key uses $@ bits, which is below 256 and considered breakable. | weak_crypto.py:22:11:22:22 | ControlFlowNode for Attribute | 224 |
 | weak_crypto.py:70:1:70:28 | ControlFlowNode for rsa_gen_key() | Creation of an RSA key uses $@ bits, which is below 2048 and considered breakable. | weak_crypto.py:12:12:12:15 | ControlFlowNode for IntegerLiteral | 1024 |
 | weak_crypto.py:72:1:72:30 | ControlFlowNode for dsa_gen_key() | Creation of an DSA key uses $@ bits, which is below 2048 and considered breakable. | weak_crypto.py:16:12:16:15 | ControlFlowNode for IntegerLiteral | 1024 |
-| weak_crypto.py:73:1:73:25 | ControlFlowNode for ec_gen_key() | Creation of an ECC key uses $@ bits, which is below 224 and considered breakable. | weak_crypto.py:22:11:22:22 | ControlFlowNode for Attribute | 163 |
+| weak_crypto.py:73:1:73:25 | ControlFlowNode for ec_gen_key() | Creation of an ECC key uses $@ bits, which is below 256 and considered breakable. | weak_crypto.py:22:11:22:22 | ControlFlowNode for Attribute | 224 |
 | weak_crypto.py:74:1:74:37 | ControlFlowNode for rsa_gen_key() | Creation of an RSA key uses $@ bits, which is below 2048 and considered breakable. | weak_crypto.py:12:12:12:15 | ControlFlowNode for IntegerLiteral | 1024 |
 | weak_crypto.py:76:1:76:22 | ControlFlowNode for Attribute() | Creation of an DSA key uses $@ bits, which is below 2048 and considered breakable. | weak_crypto.py:16:12:16:15 | ControlFlowNode for IntegerLiteral | 1024 |
 | weak_crypto.py:77:1:77:22 | ControlFlowNode for Attribute() | Creation of an RSA key uses $@ bits, which is below 2048 and considered breakable. | weak_crypto.py:12:12:12:15 | ControlFlowNode for IntegerLiteral | 1024 |

python/ql/test/query-tests/Security/CWE-326-WeakCryptoKey/weak_crypto.py

@@ -19,8 +19,8 @@ DSA_STRONG = 3076
 
 BIG = 10000
 
-EC_WEAK = ec.SECT163K1()  # has key size of 163
-EC_OK = ec.SECP224R1()
+EC_WEAK = ec.SECP224R1()
+EC_OK = ec.SECP256R1()
 EC_STRONG = ec.SECP384R1()
 EC_BIG = ec.SECT571R1()