Merge branch 'main' into ruby-mad-argument-self

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

@@ -216,10 +216,9 @@ private module LambdaFlow {
     or
     // jump step
     exists(Node mid, DataFlowType t0 |
-      revLambdaFlow(lambdaCall, kind, mid, t0, _, _, _) and
+      revLambdaFlow(lambdaCall, kind, mid, t0, _, _, lastCall) and
       toReturn = false and
-      toJump = true and
-      lastCall = TDataFlowCallNone()
+      toJump = true
     |
       jumpStepCached(node, mid) and
       t = t0
@@ -789,24 +788,31 @@ private module Cached {
   cached
   predicate readSet(Node node1, ContentSet c, Node node2) { readStep(node1, c, node2) }
 
+  cached
+  predicate storeSet(
+    Node node1, ContentSet c, Node node2, DataFlowType contentType, DataFlowType containerType
+  ) {
+    storeStep(node1, c, node2) and
+    contentType = getNodeDataFlowType(node1) and
+    containerType = getNodeDataFlowType(node2)
+    or
+    exists(Node n1, Node n2 |
+      n1 = node1.(PostUpdateNode).getPreUpdateNode() and
+      n2 = node2.(PostUpdateNode).getPreUpdateNode()
+    |
+      argumentValueFlowsThrough(n2, TReadStepTypesSome(containerType, c, contentType), n1)
+      or
+      readSet(n2, c, n1) and
+      contentType = getNodeDataFlowType(n1) and
+      containerType = getNodeDataFlowType(n2)
+    )
+  }
+
   private predicate store(
     Node node1, Content c, Node node2, DataFlowType contentType, DataFlowType containerType
   ) {
-    exists(ContentSet cs | c = cs.getAStoreContent() |
-      storeStep(node1, cs, node2) and
-      contentType = getNodeDataFlowType(node1) and
-      containerType = getNodeDataFlowType(node2)
-      or
-      exists(Node n1, Node n2 |
-        n1 = node1.(PostUpdateNode).getPreUpdateNode() and
-        n2 = node2.(PostUpdateNode).getPreUpdateNode()
-      |
-        argumentValueFlowsThrough(n2, TReadStepTypesSome(containerType, cs, contentType), n1)
-        or
-        readSet(n2, cs, n1) and
-        contentType = getNodeDataFlowType(n1) and
-        containerType = getNodeDataFlowType(n2)
-      )
+    exists(ContentSet cs |
+      c = cs.getAStoreContent() and storeSet(node1, cs, node2, contentType, containerType)
     )
   }
 

python/ql/src/Security/CWE-079/Jinja2WithoutEscaping.ql

@@ -12,6 +12,8 @@
  */
 
 import python
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.ApiGraphs
 
 /*
  * Jinja 2 Docs:
@@ -25,25 +27,24 @@ import python
  * safe1_tmpl = Template('Hello {{ name }}!', autoescape=True)
  */
 
-ClassValue jinja2EnvironmentOrTemplate() {
-  result = Value::named("jinja2.Environment")
+private API::Node jinja2EnvironmentOrTemplate() {
+  result = API::moduleImport("jinja2").getMember("Environment")
   or
-  result = Value::named("jinja2.Template")
+  result = API::moduleImport("jinja2").getMember("Template")
 }
 
-ControlFlowNode getAutoEscapeParameter(CallNode call) { result = call.getArgByName("autoescape") }
-
-from CallNode call
+from API::CallNode call
 where
-  call.getFunction().pointsTo(jinja2EnvironmentOrTemplate()) and
-  not exists(call.getNode().getStarargs()) and
-  not exists(call.getNode().getKwargs()) and
+  call = jinja2EnvironmentOrTemplate().getACall() and
+  not exists(call.asCfgNode().(CallNode).getNode().getStarargs()) and
+  not exists(call.asCfgNode().(CallNode).getNode().getKwargs()) and
   (
-    not exists(getAutoEscapeParameter(call))
+    not exists(call.getArgByName("autoescape"))
     or
-    exists(Value isFalse |
-      getAutoEscapeParameter(call).pointsTo(isFalse) and
-      isFalse.getDefiniteBooleanValue() = false
-    )
+    call.getKeywordParameter("autoescape")
+        .getAValueReachingRhs()
+        .asExpr()
+        .(ImmutableLiteral)
+        .booleanValue() = false
   )
 select call, "Using jinja2 templates with autoescape=False can potentially allow XSS attacks."

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

@@ -9,7 +9,9 @@
     <p>
       A
       <code>pam_authenticate</code>
-      only verifies the credentials of a user. It does not check if a user has an appropriate authorization to actually login. This means a user with a expired login or a password can still access the system.
+      only verifies the credentials of a user. It does not check if a user has an
+      appropriate authorization to actually login. This means a user with an expired
+      login or a password can still access the system.
     </p>
 
   </overview>
@@ -26,7 +28,9 @@
 
   <example>
     <p>
-      In the following example, the code only checks the credentials of a user. Hence, in this case, a user expired with expired creds can still login. This can be verified by creating a new user account, expiring it with
+      In the following example, the code only checks the credentials of a user. Hence,
+      in this case, a user with expired credentials can still login. This can be
+      verified by creating a new user account, expiring it with
       <code>chage -E0 `username` </code>
       and then trying to log in.
     </p>
@@ -46,4 +50,4 @@
       <a href="https://man7.org/linux/man-pages/man3/pam_acct_mgmt.3.html">pam_acct_mgmt</a>
     </li>
   </references>
-</qhelp>
+</qhelp>

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

@@ -1,8 +1,9 @@
 /**
- * @name Authorization bypass due to incorrect usage of PAM
- * @description Using only the `pam_authenticate` call to check the validity of a login can lead to a authorization bypass.
+ * @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
  * @problem.severity warning
+ * @security-severity 8.1
  * @precision high
  * @id py/pam-auth-bypass
  * @tags security
@@ -33,4 +34,5 @@ where
     acctMgmtCall = libPam().getMember("pam_acct_mgmt").getACall() and
     DataFlow::localFlow(handle, acctMgmtCall.getArg(0))
   )
-select authenticateCall, "This PAM authentication call may be lead to an authorization bypass."
+select authenticateCall,
+  "This PAM authentication call may lead to an authorization bypass, since 'pam_acct_mgmt' is not called afterwards."

python/ql/src/Security/CWE-285/PamAuthorizationBad.py

@@ -0,0 +1,19 @@
+libpam                    = CDLL(find_library("pam"))
+
+pam_authenticate          = libpam.pam_authenticate
+pam_authenticate.restype  = c_int
+pam_authenticate.argtypes = [PamHandle, c_int]
+
+def authenticate(username, password, service='login'):
+    def my_conv(n_messages, messages, p_response, app_data):
+        """
+        Simple conversation function that responds to any prompt where the echo is off with the supplied password
+        """
+        ...
+
+    handle = PamHandle()
+    conv   = PamConv(my_conv, 0)
+    retval = pam_start(service, username, byref(conv), byref(handle))
+
+    retval = pam_authenticate(handle, 0)
+    return retval == 0

python/ql/src/Security/CWE-285/PamAuthorizationGood.py

@@ -0,0 +1,25 @@
+libpam                    = CDLL(find_library("pam"))
+
+pam_authenticate          = libpam.pam_authenticate
+pam_authenticate.restype  = c_int
+pam_authenticate.argtypes = [PamHandle, c_int]
+
+pam_acct_mgmt          = libpam.pam_acct_mgmt
+pam_acct_mgmt.restype  = c_int
+pam_acct_mgmt.argtypes = [PamHandle, c_int]
+
+def authenticate(username, password, service='login'):
+    def my_conv(n_messages, messages, p_response, app_data):
+        """
+        Simple conversation function that responds to any prompt where the echo is off with the supplied password
+        """
+        ...
+
+    handle = PamHandle()
+    conv   = PamConv(my_conv, 0)
+    retval = pam_start(service, username, byref(conv), byref(handle))
+
+    retval = pam_authenticate(handle, 0)
+    if retval == 0:
+        retval = pam_acct_mgmt(handle, 0)
+    return retval == 0

python/ql/src/change-notes/2022-05-10-promote-pam-authentication-bypass.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* The query "PAM authorization bypass due to incorrect usage" (`py/pam-auth-bypass`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @porcupineyhairs](https://github.com/github/codeql/pull/8595).

python/ql/src/experimental/Security/CWE-285/PamAuthorizationBad.py

@@ -1,13 +0,0 @@
-def authenticate(self, username, password, service='login', encoding='utf-8', resetcreds=True):
-  libpam                    = CDLL(find_library("pam"))
-  pam_authenticate          = libpam.pam_authenticate
-  pam_authenticate.restype  = c_int
-  pam_authenticate.argtypes = [PamHandle, c_int]
-
-  
-  handle = PamHandle()
-  conv   = PamConv(my_conv, 0)
-  retval = pam_start(service, username, byref(conv), byref(handle))
-
-  retval = pam_authenticate(handle, 0)  
-  return retval == 0

python/ql/src/experimental/Security/CWE-285/PamAuthorizationGood.py

@@ -1,17 +0,0 @@
-def authenticate(self, username, password, service='login', encoding='utf-8', resetcreds=True):
-  libpam                    = CDLL(find_library("pam"))
-  pam_authenticate          = libpam.pam_authenticate
-  pam_acct_mgmt          = libpam.pam_acct_mgmt
-  pam_authenticate.restype  = c_int
-  pam_authenticate.argtypes = [PamHandle, c_int]
-  pam_acct_mgmt.restype  = c_int
-  pam_acct_mgmt.argtypes = [PamHandle, c_int]
-  
-  handle = PamHandle()
-  conv   = PamConv(my_conv, 0)
-  retval = pam_start(service, username, byref(conv), byref(handle))
-
-  retval = pam_authenticate(handle, 0)
-  if retval == 0:
-      retval = pam_acct_mgmt(handle, 0)
-  return retval == 0

python/ql/src/experimental/semmle/python/libraries/Authlib.qll

@@ -43,7 +43,7 @@ private module Authlib {
     override DataFlow::Node getAlgorithm() {
       exists(KeyValuePair headerDict |
         headerDict = this.getArg(0).asExpr().(Dict).getItem(_) and
-        headerDict.getKey().(Str_).getS().matches("alg") and
+        headerDict.getKey().(Str_).getS() = "alg" and
         result.asExpr() = headerDict.getValue()
       )
     }

python/ql/src/external/Thrift.qll

@@ -104,28 +104,28 @@ class ThriftType extends ThriftNamedElement {
 
 /** A thrift typedef */
 class ThriftTypeDef extends ThriftNamedElement {
-  ThriftTypeDef() { kind.matches("typedef") }
+  ThriftTypeDef() { kind = "typedef" }
 
   override ThriftElement getNameElement() { result = this.getChild(2).getChild(0) }
 }
 
 /** A thrift enum declaration */
 class ThriftEnum extends ThriftNamedElement {
-  ThriftEnum() { kind.matches("enum") }
+  ThriftEnum() { kind = "enum" }
 
   override ThriftElement getNameElement() { result = this.getChild(0).getChild(0) }
 }
 
 /** A thrift enum field */
 class ThriftEnumField extends ThriftNamedElement {
-  ThriftEnumField() { kind.matches("enumfield") }
+  ThriftEnumField() { kind = "enumfield" }
 
   override ThriftElement getNameElement() { result = this.getChild(0).getChild(0) }
 }
 
 /** A thrift service declaration */
 class ThriftService extends ThriftNamedElement {
-  ThriftService() { kind.matches("service") }
+  ThriftService() { kind = "service" }
 
   override ThriftElement getNameElement() { result = this.getChild(0).getChild(0) }
 
@@ -139,7 +139,7 @@ class ThriftService extends ThriftNamedElement {
 
 /** A thrift function declaration */
 class ThriftFunction extends ThriftNamedElement {
-  ThriftFunction() { kind.matches("function") }
+  ThriftFunction() { kind = "function" }
 
   override ThriftElement getNameElement() { result = this.getChild(2).getChild(0) }
 
@@ -166,7 +166,7 @@ class ThriftFunction extends ThriftNamedElement {
 }
 
 class ThriftField extends ThriftNamedElement {
-  ThriftField() { kind.matches("field") }
+  ThriftField() { kind = "field" }
 
   override ThriftElement getNameElement() { result = this.getChild(4) }
 
@@ -174,7 +174,7 @@ class ThriftField extends ThriftNamedElement {
 }
 
 class ThriftStruct extends ThriftNamedElement {
-  ThriftStruct() { kind.matches("struct") }
+  ThriftStruct() { kind = "struct" }
 
   override ThriftElement getNameElement() { result = this.getChild(0).getChild(0) }
 
@@ -184,7 +184,7 @@ class ThriftStruct extends ThriftNamedElement {
 }
 
 class ThriftException extends ThriftNamedElement {
-  ThriftException() { kind.matches("exception") }
+  ThriftException() { kind = "exception" }
 
   override ThriftElement getNameElement() { result = this.getChild(0).getChild(0) }
 
@@ -194,7 +194,7 @@ class ThriftException extends ThriftNamedElement {
 }
 
 class ThriftThrows extends ThriftElement {
-  ThriftThrows() { kind.matches("throws") }
+  ThriftThrows() { kind = "throws" }
 
   ThriftField getAThrows() { result = this.getChild(_) }
 }

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

@@ -1 +0,0 @@
-| pam_test.py:48:18:48:44 | ControlFlowNode for pam_authenticate() | This PAM authentication call may be lead to an authorization bypass. |

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

@@ -1 +0,0 @@
-experimental/Security/CWE-285/PamAuthorization.ql

python/ql/test/query-tests/Security/CWE-079-Jinja2WithoutEscaping/Jinja2WithoutEscaping.expected

@@ -2,4 +2,5 @@
 | jinja2_escaping.py:41:5:41:29 | ControlFlowNode for Environment() | Using jinja2 templates with autoescape=False can potentially allow XSS attacks. |
 | jinja2_escaping.py:43:1:43:3 | ControlFlowNode for E() | Using jinja2 templates with autoescape=False can potentially allow XSS attacks. |
 | jinja2_escaping.py:44:1:44:15 | ControlFlowNode for E() | Using jinja2 templates with autoescape=False can potentially allow XSS attacks. |
+| jinja2_escaping.py:50:13:50:40 | ControlFlowNode for Environment() | Using jinja2 templates with autoescape=False can potentially allow XSS attacks. |
 | jinja2_escaping.py:53:15:53:43 | ControlFlowNode for Template() | Using jinja2 templates with autoescape=False can potentially allow XSS attacks. |

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

@@ -0,0 +1 @@
+| 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. |

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

@@ -0,0 +1 @@
+Security/CWE-285/PamAuthorization.ql