Merge branch 'main' into python/rewrite-InsecureContextConfiguration

python/ql/src/CHANGELOG.md

@@ -1,3 +1,13 @@
+## 0.6.5
+
+### New Queries
+
+* Added a new query, `py/shell-command-constructed-from-input`, to detect libraries that unsafely construct shell commands from their inputs.
+
+## 0.6.4
+
+No user-facing changes.
+
 ## 0.6.3
 
 No user-facing changes.

python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp

@@ -0,0 +1,73 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+	<p>
+		Dynamically constructing a shell command with inputs from library 
+		functions may inadvertently change the meaning of the shell command.
+		
+		Clients using the exported function may use inputs containing
+		characters that the shell interprets in a special way, for instance
+		quotes and spaces.
+
+		This can result in the shell command misbehaving, or even
+		allowing a malicious user to execute arbitrary commands on the system.
+	</p>
+
+
+</overview>
+<recommendation>
+
+	<p>
+		If possible, provide the dynamic arguments to the shell as an array 
+		to APIs such as <code>subprocess.run</code> to avoid interpretation by the shell.
+	</p>
+
+	<p>
+		Alternatively, if the shell command must be constructed
+		dynamically, then add code to ensure that special characters 
+		do not alter the shell command unexpectedly.
+	</p>
+
+</recommendation>
+<example>
+
+	<p>
+		The following example shows a dynamically constructed shell
+		command that downloads a file from a remote URL.
+	</p>
+
+	<sample src="examples/unsafe-shell-command-construction.py" />
+
+	<p>
+		The shell command will, however, fail to work as intended if the
+		input contains spaces or other special characters interpreted in a 
+		special way by the shell. 
+	</p>
+
+	<p>
+		Even worse, a client might pass in user-controlled
+		data, not knowing that the input is interpreted as a shell command. 
+		This could allow a malicious user to provide the input <code>http://example.org; cat /etc/passwd</code>
+		in order to execute the command <code>cat /etc/passwd</code>.
+	</p>
+
+	<p>
+		To avoid such potentially catastrophic behaviors, provide the
+		input from library functions as an argument that does not
+		get interpreted by a shell:
+	</p>
+
+	<sample src="examples/unsafe-shell-command-construction_fixed.py" />
+
+</example>
+<references>
+
+	<li>
+		OWASP:
+		<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+	</li>
+
+</references>
+</qhelp>

python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql

@@ -0,0 +1,27 @@
+/**
+ * @name Unsafe shell command constructed from library input
+ * @description Using externally controlled strings in a command line may allow a malicious
+ *              user to change the meaning of the command.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 6.3
+ * @precision medium
+ * @id py/shell-command-constructed-from-input
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ *       external/cwe/cwe-073
+ */
+
+import python
+import semmle.python.security.dataflow.UnsafeShellCommandConstructionQuery
+import DataFlow::PathGraph
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
+where
+  config.hasFlowPath(source, sink) and
+  sinkNode = sink.getNode()
+select sinkNode.getStringConstruction(), source, sink,
+  "This " + sinkNode.describe() + " which depends on $@ is later used in a $@.", source.getNode(),
+  "library input", sinkNode.getCommandExecution(), "shell command"

python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.py

@@ -0,0 +1,4 @@
+import os
+
+def download(path): 
+    os.system("wget " + path) # NOT OK

python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.py

@@ -0,0 +1,4 @@
+import subprocess
+
+def download(path): 
+    subprocess.run(["wget", path]) # OK

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

@@ -112,7 +112,7 @@ module InsecureContextConfiguration implements DataFlow::StateConfigSig {
   }
 }
 
-private module InsecureContextFlow = DataFlow::MakeWithState<InsecureContextConfiguration>;
+private module InsecureContextFlow = DataFlow::GlobalWithState<InsecureContextConfiguration>;
 
 /**
  * Holds if `conectionCreation` marks the creation of a connection based on the contex
@@ -127,7 +127,7 @@ predicate unsafe_connection_creation_with_context(
 ) {
   // Connection created from a context allowing `insecure_version`.
   exists(InsecureContextFlow::PathNode src, InsecureContextFlow::PathNode sink |
-    InsecureContextFlow::hasFlowPath(src, sink) and
+    InsecureContextFlow::flowPath(src, sink) and
     src.getNode() = contextOrigin and
     sink.getNode() = connectionCreation and
     sink.getState().allowsInsecureVersion(insecure_version) and

python/ql/src/Variables/UninitializedLocal.ql

@@ -15,7 +15,9 @@ import Undefined
 import semmle.python.pointsto.PointsTo
 
 predicate uninitialized_local(NameNode use) {
-  exists(FastLocalVariable local | use.uses(local) or use.deletes(local) | not local.escapes()) and
+  exists(FastLocalVariable local | use.uses(local) or use.deletes(local) |
+    not local.escapes() and not local = any(Nonlocal nl).getAVariable()
+  ) and
   (
     any(Uninitialized uninit).taints(use) and
     PointsToInternal::reachableBlock(use.getBasicBlock(), _)

python/ql/src/change-notes/2023-03-20-uninitialized-local.md

@@ -0,0 +1,4 @@
+---
+category: fix
+---
+* Nonlocal variables are excluded from alerts.

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

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

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

@@ -0,0 +1,5 @@
+## 0.6.5
+
+### New Queries
+
+* Added a new query, `py/shell-command-constructed-from-input`, to detect libraries that unsafely construct shell commands from their inputs.

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

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

python/ql/src/experimental/Security/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll

@@ -16,7 +16,8 @@ private import semmle.python.frameworks.Tornado
 abstract class ClientSuppliedIpUsedInSecurityCheck extends DataFlow::Node { }
 
 private class FlaskClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck,
-  DataFlow::MethodCallNode {
+  DataFlow::MethodCallNode
+{
   FlaskClientSuppliedIpUsedInSecurityCheck() {
     this = Flask::request().getMember("headers").getMember(["get", "get_all", "getlist"]).getACall() and
     this.getArg(0).asExpr().(StrConst).getText().toLowerCase() = clientIpParameterName()
@@ -24,7 +25,8 @@ private class FlaskClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpU
 }
 
 private class DjangoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck,
-  DataFlow::MethodCallNode {
+  DataFlow::MethodCallNode
+{
   DjangoClientSuppliedIpUsedInSecurityCheck() {
     exists(DataFlow::Node req, DataFlow::AttrRead headers |
       // a call to request.headers.get or request.META.get
@@ -38,7 +40,8 @@ private class DjangoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIp
 }
 
 private class TornadoClientSuppliedIpUsedInSecurityCheck extends ClientSuppliedIpUsedInSecurityCheck,
-  DataFlow::MethodCallNode {
+  DataFlow::MethodCallNode
+{
   TornadoClientSuppliedIpUsedInSecurityCheck() {
     // a call to self.request.headers.get or self.request.headers.get_list inside a tornado requesthandler
     exists(

python/ql/src/qlpack.yml

@@ -1,12 +1,11 @@
 name: codeql/python-queries
-version: 0.6.4-dev
+version: 0.6.6-dev
 groups: 
   - python
   - queries
 dependencies:
   codeql/python-all: ${workspace}
   codeql/suite-helpers: ${workspace}
-  codeql/util: ${workspace}
 suites: codeql-suites
 extractor: python
 defaultSuiteFile: codeql-suites/python-code-scanning.qls