From 54582031d835da8c52ea2c70e7b1fb632fbc30ef Mon Sep 17 00:00:00 2001
From: amammad
Date: Thu, 16 Feb 2023 17:14:32 +0100
Subject: [PATCH 001/704] v1
---
.../Security/CWE-074/paramiko/paramiko.qhelp | 17 +++++
.../Security/CWE-074/paramiko/paramiko.ql | 72 +++++++++++++++++++
.../Security/CWE-074/paramiko/paramikoBad.py | 36 ++++++++++
.../CWE-074/paramiko/paramiko.expected | 16 +++++
.../Security/CWE-074/paramiko/paramiko.py | 27 +++++++
.../Security/CWE-074/paramiko/paramiko.qlref | 1 +
6 files changed, 169 insertions(+)
create mode 100644 python/ql/src/experimental/Security/CWE-074/paramiko/paramiko.qhelp
create mode 100644 python/ql/src/experimental/Security/CWE-074/paramiko/paramiko.ql
create mode 100644 python/ql/src/experimental/Security/CWE-074/paramiko/paramikoBad.py
create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.expected
create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.py
create mode 100644 python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.qlref
diff --git a/python/ql/src/experimental/Security/CWE-074/paramiko/paramiko.qhelp b/python/ql/src/experimental/Security/CWE-074/paramiko/paramiko.qhelp
new file mode 100644
index 00000000000..98cc0e1e4de
--- /dev/null
+++ b/python/ql/src/experimental/Security/CWE-074/paramiko/paramiko.qhelp
@@ -0,0 +1,17 @@
+
+
+
+
+ Processing an unvalidated user input can allow an attacker to inject arbitrary command in your local and remote servers when creating a ssh connection.
+
+
+
+
+ This vulnerability can be prevented by not allowing untrusted user input to be passed as ProxyCommand or exec_command.
+
+
+
+ In the example below, the ProxyCommand and exec_command are controlled by the user and hence leads to a vulnerability.
+
+
+
diff --git a/python/ql/src/experimental/Security/CWE-074/paramiko/paramiko.ql b/python/ql/src/experimental/Security/CWE-074/paramiko/paramiko.ql
new file mode 100644
index 00000000000..28db4e129b4
--- /dev/null
+++ b/python/ql/src/experimental/Security/CWE-074/paramiko/paramiko.ql
@@ -0,0 +1,72 @@
+/**
+ * @name RCE with user provided command with paramiko ssh client
+ * @description user provided command can lead to execute code on a external server that can be belong to other users or admins
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 9.3
+ * @precision high
+ * @id py/command-injection
+ * @tags security
+ * experimental
+ * external/cwe/cwe-074
+ */
+
+import python
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.dataflow.new.TaintTracking
+import semmle.python.dataflow.new.RemoteFlowSources
+import semmle.python.ApiGraphs
+import DataFlow::PathGraph
+
+class ParamikoCMDInjectionConfiguration extends TaintTracking::Configuration {
+ ParamikoCMDInjectionConfiguration() { this = "ParamikoCMDInjectionConfiguration" }
+
+ override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+ override predicate isSink(DataFlow::Node sink) {
+ sink =
+ [
+ API::moduleImport("paramiko")
+ .getMember("SSHClient")
+ .getReturn()
+ .getMember("exec_command")
+ .getACall()
+ .getArgByName("command"),
+ API::moduleImport("paramiko")
+ .getMember("SSHClient")
+ .getReturn()
+ .getMember("exec_command")
+ .getACall()
+ .getArg(0)
+ ]
+ or
+ sink =
+ [
+ API::moduleImport("paramiko")
+ .getMember("SSHClient")
+ .getReturn()
+ .getMember("connect")
+ .getACall()
+ .getArgByName("sock"),
+ API::moduleImport("paramiko")
+ .getMember("SSHClient")
+ .getReturn()
+ .getMember("connect")
+ .getACall()
+ .getArg(11)
+ ]
+ }
+
+ override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+ exists(API::CallNode call |
+ call = API::moduleImport("paramiko").getMember("ProxyCommand").getACall() and
+ nodeFrom = [call.getArg(0), call.getArgByName("command_line")] and
+ nodeTo = call
+ )
+ }
+}
+
+from ParamikoCMDInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This code execution depends on a $@.", source.getNode(),
+ "a user-provided value"
diff --git a/python/ql/src/experimental/Security/CWE-074/paramiko/paramikoBad.py b/python/ql/src/experimental/Security/CWE-074/paramiko/paramikoBad.py
new file mode 100644
index 00000000000..b54a88f2e4a
--- /dev/null
+++ b/python/ql/src/experimental/Security/CWE-074/paramiko/paramikoBad.py
@@ -0,0 +1,36 @@
+#!/usr/bin/env python
+
+from flask import request, Flask
+import paramiko
+from paramiko import SSHClient
+
+app = Flask(__name__)
+paramiko_ssh_client = SSHClient()
+paramiko_ssh_client.load_system_host_keys()
+paramiko_ssh_client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
+paramiko_ssh_client.connect(hostname="127.0.0.1", port="22", username="ssh_user_name", pkey="k", timeout=11, banner_timeout=200)
+
+
+@app.route('/external_exec_command_1')
+def bad1():
+ user_cmd = request.args.get('command')
+ stdin, stdout, stderr = paramiko_ssh_client.exec_command(user_cmd)
+ return stdout
+
+@app.route('/external_exec_command_2')
+def bad2():
+ user_cmd = request.args.get('command')
+ stdin, stdout, stderr = paramiko_ssh_client.exec_command(command=user_cmd)
+ return stdout
+
+
+@app.route('/proxycommand')
+def bad2():
+ user_cmd = request.args.get('command')
+ stdin, stdout, stderr = paramiko_ssh_client.connect('hostname', username='user',password='yourpassword',sock=paramiko.ProxyCommand(user_cmd))
+ return stdout
+
+if __name__ == '__main__':
+ app.debug = False
+ app.run()
+
diff --git a/python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.expected b/python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.expected
new file mode 100644
index 00000000000..85e1e7b326d
--- /dev/null
+++ b/python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.expected
@@ -0,0 +1,16 @@
+edges
+| paramiko.py:15:21:15:23 | ControlFlowNode for cmd | paramiko.py:16:62:16:64 | ControlFlowNode for cmd |
+| paramiko.py:20:21:20:23 | ControlFlowNode for cmd | paramiko.py:21:70:21:72 | ControlFlowNode for cmd |
+| paramiko.py:25:21:25:23 | ControlFlowNode for cmd | paramiko.py:26:114:26:139 | ControlFlowNode for Attribute() |
+nodes
+| paramiko.py:15:21:15:23 | ControlFlowNode for cmd | semmle.label | ControlFlowNode for cmd |
+| paramiko.py:16:62:16:64 | ControlFlowNode for cmd | semmle.label | ControlFlowNode for cmd |
+| paramiko.py:20:21:20:23 | ControlFlowNode for cmd | semmle.label | ControlFlowNode for cmd |
+| paramiko.py:21:70:21:72 | ControlFlowNode for cmd | semmle.label | ControlFlowNode for cmd |
+| paramiko.py:25:21:25:23 | ControlFlowNode for cmd | semmle.label | ControlFlowNode for cmd |
+| paramiko.py:26:114:26:139 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
+subpaths
+#select
+| paramiko.py:16:62:16:64 | ControlFlowNode for cmd | paramiko.py:15:21:15:23 | ControlFlowNode for cmd | paramiko.py:16:62:16:64 | ControlFlowNode for cmd | This code execution depends on a $@. | paramiko.py:15:21:15:23 | ControlFlowNode for cmd | a user-provided value |
+| paramiko.py:21:70:21:72 | ControlFlowNode for cmd | paramiko.py:20:21:20:23 | ControlFlowNode for cmd | paramiko.py:21:70:21:72 | ControlFlowNode for cmd | This code execution depends on a $@. | paramiko.py:20:21:20:23 | ControlFlowNode for cmd | a user-provided value |
+| paramiko.py:26:114:26:139 | ControlFlowNode for Attribute() | paramiko.py:25:21:25:23 | ControlFlowNode for cmd | paramiko.py:26:114:26:139 | ControlFlowNode for Attribute() | This code execution depends on a $@. | paramiko.py:25:21:25:23 | ControlFlowNode for cmd | a user-provided value |
diff --git a/python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.py b/python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.py
new file mode 100644
index 00000000000..1e625d18345
--- /dev/null
+++ b/python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.py
@@ -0,0 +1,27 @@
+#!/usr/bin/env python
+
+from fastapi import FastAPI
+import paramiko
+from paramiko import SSHClient
+paramiko_ssh_client = SSHClient()
+paramiko_ssh_client.load_system_host_keys()
+paramiko_ssh_client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
+paramiko_ssh_client.connect(hostname="127.0.0.1", port="22", username="ssh_user_name", pkey="k", timeout=11, banner_timeout=200)
+
+app = FastAPI()
+
+
+@app.get("/bad1")
+async def read_item(cmd: str):
+ stdin, stdout, stderr = paramiko_ssh_client.exec_command(cmd)
+ return {"success": stdout}
+
+@app.get("/bad2")
+async def read_item(cmd: str):
+ stdin, stdout, stderr = paramiko_ssh_client.exec_command(command=cmd)
+ return {"success": "OK"}
+
+@app.get("/bad3")
+async def read_item(cmd: str):
+ stdin, stdout, stderr = paramiko_ssh_client.connect('hostname', username='user',password='yourpassword',sock=paramiko.ProxyCommand(cmd))
+ return {"success": "OK"}
diff --git a/python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.qlref b/python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.qlref
new file mode 100644
index 00000000000..8a164fcc8cc
--- /dev/null
+++ b/python/ql/test/experimental/query-tests/Security/CWE-074/paramiko/paramiko.qlref
@@ -0,0 +1 @@
+experimental/Security/CWE-074/paramiko/paramiko.ql
\ No newline at end of file
From 7e003f63b9502d08d6a5348e6ec4efc43af85713 Mon Sep 17 00:00:00 2001
From: Rasmus Lerchedahl Petersen
Date: Wed, 15 Mar 2023 14:21:26 +0100
Subject: [PATCH 002/704] python: add test for flask example This is a
condensed versio of the user reported example found
[here](https://github.com/dsp-testing/apictf/blob/eb377d5918bb4ac316a32361e5e0c082e61036d6/app.py#L278)
The `MISSING` annotation indicates where our API graph falls short.
---
.../ApiGraphs/py3/test_captured_flask.py | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
create mode 100644 python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py
diff --git a/python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py b/python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py
new file mode 100644
index 00000000000..fbab3164eb5
--- /dev/null
+++ b/python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py
@@ -0,0 +1,26 @@
+from flask import Flask
+from flask_sqlalchemy import SQLAlchemy
+from flask_user import UserMixin
+
+def create_app():
+ app = Flask(__name__)
+ db = SQLAlchemy(app) #$ use=moduleImport("flask_sqlalchemy").getMember("SQLAlchemy").getReturn()
+
+ class Users(db.Model, UserMixin): #$ use=moduleImport("flask_sqlalchemy").getMember("SQLAlchemy").getReturn().getMember("Model").getASubclass()
+ __tablename__ = 'users'
+
+ @app.route('/v2/user/', methods=['GET','PUT'])
+ def users(id):
+ if 'Authorization-Token' not in request.headers:
+ return make_response(jsonify({'Error':'Authorization-Token header is not set'}),403)
+
+ token = request.headers.get('Authorization-Token')
+ sid = check_token(token)
+
+ #if we don't have a valid session send 403
+ if not sid:
+ return make_response(jsonify({'Error':'Token check failed: {0}'.format(sid)}))
+ try:
+ user = Users.query.filter_by(id=id).first() #$ MISSING: use=moduleImport("flask_sqlalchemy").getMember("SQLAlchemy").getReturn().getMember("Model").getASubclass().getMember("query").getMember("filter_by")
+ except Exception as e:
+ return make_response(jsonify({'error':str(e)}),500)
From 2318752c1403502135e076e69b326713372a8ade Mon Sep 17 00:00:00 2001
From: Rasmus Lerchedahl Petersen
Date: Wed, 15 Mar 2023 15:00:31 +0100
Subject: [PATCH 003/704] python: add reads of captured variables to type
tracking and the API graph.
- In `TypeTrackerSpecific.qll` we add a jump step
- to every scope entry definition
- from the value of any defining `DefinitionNode`
(In our example, the definition is the class name, `Users`,
while the assigned value is the class definition, and it is
the latter which receives flow in this case.)
- In `LocalSources.qll` we allow scope entry definitions as local sources.
- This feels natural enough, as they are a local source for the value, they represent.
It is perhaps a bit funne to see an Ssa variable here,
rather than a control flow node.
- This is necessary in order for type tracking to see the local flow
from the scope entry definition.
- In `ApiGraphs.qll` we no longer restrict the result of `trackUseNode`
to be an `ExprNode`. To keep the positive formulation, we do not
prohibit module variable nodes. Instead we restrict to the new
`LocalSourceNodeNotModule` which avoids those cases.
---
python/ql/lib/semmle/python/ApiGraphs.qll | 2 +-
.../dataflow/new/internal/LocalSources.qll | 17 +++++++++++++++++
.../new/internal/TypeTrackerSpecific.qll | 14 +++++++++++++-
.../ApiGraphs/py3/dataflow-consistency.expected | 1 +
.../ql/test/library-tests/ApiGraphs/py3/test.py | 2 +-
.../ApiGraphs/py3/test_captured.py | 2 +-
.../ApiGraphs/py3/test_captured_flask.py | 2 +-
.../ApiGraphs/py3/test_import_star.py | 2 +-
8 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/python/ql/lib/semmle/python/ApiGraphs.qll b/python/ql/lib/semmle/python/ApiGraphs.qll
index c294e062f6d..7da45eb7fc2 100644
--- a/python/ql/lib/semmle/python/ApiGraphs.qll
+++ b/python/ql/lib/semmle/python/ApiGraphs.qll
@@ -987,7 +987,7 @@ module API {
DataFlow::LocalSourceNode trackUseNode(DataFlow::LocalSourceNode src) {
Stages::TypeTracking::ref() and
result = trackUseNode(src, DataFlow::TypeTracker::end()) and
- result instanceof DataFlow::ExprNode
+ result instanceof DataFlow::LocalSourceNodeNotModule
}
/**
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/LocalSources.qll b/python/ql/lib/semmle/python/dataflow/new/internal/LocalSources.qll
index 72ea5d95310..585d5c39d6b 100644
--- a/python/ql/lib/semmle/python/dataflow/new/internal/LocalSources.qll
+++ b/python/ql/lib/semmle/python/dataflow/new/internal/LocalSources.qll
@@ -51,6 +51,10 @@ class LocalSourceNode extends Node {
// We explicitly include any read of a global variable, as some of these may have local flow going
// into them.
this = any(ModuleVariableNode mvn).getARead()
+ or
+ // We include all scope entry definitions, as these act as the local source within the scope they
+ // enter.
+ this.asVar() instanceof ScopeEntryDefinition
}
/** Holds if this `LocalSourceNode` can flow to `nodeTo` in one or more local flow steps. */
@@ -133,6 +137,19 @@ class LocalSourceNode extends Node {
LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t2 = t.step(result, this) }
}
+/**
+ * A LocalSourceNode that is not a ModuleVariableNode
+ * This class provides a positive formulation of that in its charpred.
+ */
+class LocalSourceNodeNotModule extends LocalSourceNode {
+ cached
+ LocalSourceNodeNotModule() {
+ this instanceof ExprNode
+ or
+ this.asVar() instanceof ScopeEntryDefinition
+ }
+}
+
/**
* A node that can be used for type tracking or type back-tracking.
*
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackerSpecific.qll b/python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackerSpecific.qll
index 67e3db984e8..9e05b7869c5 100644
--- a/python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackerSpecific.qll
+++ b/python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackerSpecific.qll
@@ -43,7 +43,19 @@ predicate compatibleContents(TypeTrackerContent storeContent, TypeTrackerContent
predicate simpleLocalFlowStep = DataFlowPrivate::simpleLocalFlowStepForTypetracking/2;
-predicate jumpStep = DataFlowPrivate::jumpStepSharedWithTypeTracker/2;
+predicate jumpStep(Node nodeFrom, Node nodeTo) {
+ DataFlowPrivate::jumpStepSharedWithTypeTracker(nodeFrom, nodeTo)
+ or
+ capturedJumpStep(nodeFrom, nodeTo)
+}
+
+predicate capturedJumpStep(Node nodeFrom, Node nodeTo) {
+ exists(SsaSourceVariable var, DefinitionNode def | var.hasDefiningNode(def) |
+ nodeTo.asVar().(ScopeEntryDefinition).getSourceVariable() = var and
+ nodeFrom.asCfgNode() = def.getValue() and
+ var.getScope().getScope*() = nodeFrom.getScope()
+ )
+}
/** Holds if there is a level step from `nodeFrom` to `nodeTo`, which may depend on the call graph. */
predicate levelStepCall(Node nodeFrom, Node nodeTo) { none() }
diff --git a/python/ql/test/library-tests/ApiGraphs/py3/dataflow-consistency.expected b/python/ql/test/library-tests/ApiGraphs/py3/dataflow-consistency.expected
index 54acd44d74f..8de0286359a 100644
--- a/python/ql/test/library-tests/ApiGraphs/py3/dataflow-consistency.expected
+++ b/python/ql/test/library-tests/ApiGraphs/py3/dataflow-consistency.expected
@@ -3,6 +3,7 @@ uniqueCallEnclosingCallable
| test_captured.py:7:22:7:25 | p() | Call should have one enclosing callable but has 0. |
| test_captured.py:7:22:7:25 | p() | Call should have one enclosing callable but has 0. |
| test_captured.py:14:26:14:30 | pp() | Call should have one enclosing callable but has 0. |
+| test_captured.py:14:26:14:30 | pp() | Call should have one enclosing callable but has 0. |
uniqueType
uniqueNodeLocation
missingLocation
diff --git a/python/ql/test/library-tests/ApiGraphs/py3/test.py b/python/ql/test/library-tests/ApiGraphs/py3/test.py
index 29a28bc6252..424c6248907 100644
--- a/python/ql/test/library-tests/ApiGraphs/py3/test.py
+++ b/python/ql/test/library-tests/ApiGraphs/py3/test.py
@@ -89,7 +89,7 @@ def use_of_builtins():
def imported_builtins():
import builtins #$ use=moduleImport("builtins")
def open(f):
- return builtins.open(f) #$ MISSING: use=moduleImport("builtins").getMember("open").getReturn()
+ return builtins.open(f) #$ use=moduleImport("builtins").getMember("open").getReturn()
def redefine_print():
def my_print(x):
diff --git a/python/ql/test/library-tests/ApiGraphs/py3/test_captured.py b/python/ql/test/library-tests/ApiGraphs/py3/test_captured.py
index 965132c3181..9057b1d1fd7 100644
--- a/python/ql/test/library-tests/ApiGraphs/py3/test_captured.py
+++ b/python/ql/test/library-tests/ApiGraphs/py3/test_captured.py
@@ -11,4 +11,4 @@ def pp_list(l):
return escape(x) #$ use=moduleImport("html").getMember("escape").getReturn()
def pp_list_inner(l):
- return ", ".join(pp(x) for x in l) #$ MISSING: use=moduleImport("html").getMember("escape").getReturn()
+ return ", ".join(pp(x) for x in l) #$ use=moduleImport("html").getMember("escape").getReturn()
diff --git a/python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py b/python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py
index fbab3164eb5..48e0202e5de 100644
--- a/python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py
+++ b/python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py
@@ -21,6 +21,6 @@ def create_app():
if not sid:
return make_response(jsonify({'Error':'Token check failed: {0}'.format(sid)}))
try:
- user = Users.query.filter_by(id=id).first() #$ MISSING: use=moduleImport("flask_sqlalchemy").getMember("SQLAlchemy").getReturn().getMember("Model").getASubclass().getMember("query").getMember("filter_by")
+ user = Users.query.filter_by(id=id).first() #$ use=moduleImport("flask_sqlalchemy").getMember("SQLAlchemy").getReturn().getMember("Model").getASubclass().getMember("query").getMember("filter_by")
except Exception as e:
return make_response(jsonify({'error':str(e)}),500)
diff --git a/python/ql/test/library-tests/ApiGraphs/py3/test_import_star.py b/python/ql/test/library-tests/ApiGraphs/py3/test_import_star.py
index 7b2934357be..ff4d58509ec 100644
--- a/python/ql/test/library-tests/ApiGraphs/py3/test_import_star.py
+++ b/python/ql/test/library-tests/ApiGraphs/py3/test_import_star.py
@@ -32,5 +32,5 @@ def func1():
def func3():
var2 = print #$ use=moduleImport("builtins").getMember("print")
def func4():
- var2() #$ MISSING: use=moduleImport("builtins").getMember("print").getReturn()
+ var2() #$ use=moduleImport("builtins").getMember("print").getReturn()
func4()
From 4713ba1e128d3a16841506030a590f595ea22a03 Mon Sep 17 00:00:00 2001
From: Rasmus Lerchedahl Petersen
Date: Wed, 15 Mar 2023 22:05:21 +0100
Subject: [PATCH 004/704] python: more results no longer missing Adjusted
`tracked.ql` - no need to annotate results on line 0 this could happen for
global SSA variables - no need to annotate scope entry definitons they look
a bit weird, as the annotation goes on the line of the function definition.
---
python/ql/test/experimental/dataflow/coverage/test.py | 10 +++++-----
.../dataflow/typetracking/moduleattr.expected | 1 +
.../ql/test/experimental/dataflow/typetracking/test.py | 8 ++++----
.../test/experimental/dataflow/typetracking/tracked.ql | 5 +++++
4 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/python/ql/test/experimental/dataflow/coverage/test.py b/python/ql/test/experimental/dataflow/coverage/test.py
index 65f915cfd9b..f35339e4dca 100644
--- a/python/ql/test/experimental/dataflow/coverage/test.py
+++ b/python/ql/test/experimental/dataflow/coverage/test.py
@@ -726,15 +726,15 @@ def test_deep_callgraph():
return f5(arg)
x = f6(SOURCE)
- SINK(x) #$ MISSING:flow="SOURCE, l:-1 -> x"
+ SINK(x) #$ flow="SOURCE, l:-1 -> x"
x = f5(SOURCE)
- SINK(x) #$ MISSING:flow="SOURCE, l:-1 -> x"
+ SINK(x) #$ flow="SOURCE, l:-1 -> x"
x = f4(SOURCE)
- SINK(x) #$ MISSING:flow="SOURCE, l:-1 -> x"
+ SINK(x) #$ flow="SOURCE, l:-1 -> x"
x = f3(SOURCE)
- SINK(x) #$ MISSING:flow="SOURCE, l:-1 -> x"
+ SINK(x) #$ flow="SOURCE, l:-1 -> x"
x = f2(SOURCE)
- SINK(x) #$ MISSING:flow="SOURCE, l:-1 -> x"
+ SINK(x) #$ flow="SOURCE, l:-1 -> x"
x = f1(SOURCE)
SINK(x) #$ flow="SOURCE, l:-1 -> x"
diff --git a/python/ql/test/experimental/dataflow/typetracking/moduleattr.expected b/python/ql/test/experimental/dataflow/typetracking/moduleattr.expected
index 9720d759aa0..14f26b7803c 100644
--- a/python/ql/test/experimental/dataflow/typetracking/moduleattr.expected
+++ b/python/ql/test/experimental/dataflow/typetracking/moduleattr.expected
@@ -6,5 +6,6 @@ module_attr_tracker
| import_as_attr.py:1:28:1:35 | GSSA Variable attr_ref |
| import_as_attr.py:3:1:3:1 | GSSA Variable x |
| import_as_attr.py:3:5:3:12 | ControlFlowNode for attr_ref |
+| import_as_attr.py:5:1:5:10 | GSSA Variable attr_ref |
| import_as_attr.py:6:5:6:5 | SSA variable y |
| import_as_attr.py:6:9:6:16 | ControlFlowNode for attr_ref |
diff --git a/python/ql/test/experimental/dataflow/typetracking/test.py b/python/ql/test/experimental/dataflow/typetracking/test.py
index 8de0a3ded92..f0e93d79af2 100644
--- a/python/ql/test/experimental/dataflow/typetracking/test.py
+++ b/python/ql/test/experimental/dataflow/typetracking/test.py
@@ -60,10 +60,10 @@ def test_import():
def to_inner_scope():
x = tracked # $tracked
def foo():
- y = x # $ MISSING: tracked
- return y # $ MISSING: tracked
- also_x = foo() # $ MISSING: tracked
- print(also_x) # $ MISSING: tracked
+ y = x # $ tracked
+ return y # $ tracked
+ also_x = foo() # $ tracked
+ print(also_x) # $ tracked
# ------------------------------------------------------------------------------
# Function decorator
diff --git a/python/ql/test/experimental/dataflow/typetracking/tracked.ql b/python/ql/test/experimental/dataflow/typetracking/tracked.ql
index c35775d0046..c0ed62e258f 100644
--- a/python/ql/test/experimental/dataflow/typetracking/tracked.ql
+++ b/python/ql/test/experimental/dataflow/typetracking/tracked.ql
@@ -24,6 +24,11 @@ class TrackedTest extends InlineExpectationsTest {
tracked(t).flowsTo(e) and
// Module variables have no sensible location, and hence can't be annotated.
not e instanceof DataFlow::ModuleVariableNode and
+ // Global variables on line 0 also cannot be annotated
+ not e.getLocation().getStartLine() = 0 and
+ // We do not wish to annotate scope entry definitions,
+ // as they do not appear in the source code.
+ not e.asVar() instanceof ScopeEntryDefinition and
tag = "tracked" and
location = e.getLocation() and
value = t.getAttr() and
From f9bffb5454a71d7f970eb6587a83a3431b93c03c Mon Sep 17 00:00:00 2001
From: Rasmus Lerchedahl Petersen
Date: Thu, 16 Mar 2023 12:52:12 +0100
Subject: [PATCH 005/704] python: add change note
---
.../2023-03-16-typetracking-read-captured-variables.md | 4 ++++
1 file changed, 4 insertions(+)
create mode 100644 python/ql/lib/change-notes/2023-03-16-typetracking-read-captured-variables.md
diff --git a/python/ql/lib/change-notes/2023-03-16-typetracking-read-captured-variables.md b/python/ql/lib/change-notes/2023-03-16-typetracking-read-captured-variables.md
new file mode 100644
index 00000000000..6905a03c8e8
--- /dev/null
+++ b/python/ql/lib/change-notes/2023-03-16-typetracking-read-captured-variables.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Type tracking is now aware of reads of captured variables (variables defined in an outer scope). This leads to a richer API graph, and may lead to more results in some queries.
From 99d634c8a4cd62d6265bd106d8bd93e40c4247af Mon Sep 17 00:00:00 2001
From: jarlob
Date: Mon, 3 Apr 2023 15:02:02 +0200
Subject: [PATCH 006/704] Add more sources, more unit tests, fixes to the
GitHub Actions injection query
---
.../ql/lib/semmle/javascript/Actions.qll | 4 +-
.../Security/CWE-094/ExpressionInjection.ql | 61 ++++++++++++++-----
.../.github/workflows/comment_issue.yml | 5 +-
.../.github/workflows/discussion.yml | 8 +++
.../.github/workflows/discussion_comment.yml | 9 +++
.../.github/workflows/gollum.yml | 11 ++++
.../.github/workflows/issues.yml | 8 +++
.../.github/workflows/pull_request_review.yml | 14 +++++
.../workflows/pull_request_review_comment.yml | 14 +++++
.../.github/workflows/pull_request_target.yml | 16 +++++
.../.github/workflows/push.yml | 16 +++++
.../.github/workflows/workflow_run.yml | 16 +++++
.../ExpressionInjection.expected | 57 ++++++++++++++++-
13 files changed, 220 insertions(+), 19 deletions(-)
create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/discussion.yml
create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/discussion_comment.yml
create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/gollum.yml
create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yml
create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_review.yml
create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_review_comment.yml
create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_target.yml
create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/push.yml
create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/workflow_run.yml
diff --git a/javascript/ql/lib/semmle/javascript/Actions.qll b/javascript/ql/lib/semmle/javascript/Actions.qll
index 7fd3952ac85..b1ab674924d 100644
--- a/javascript/ql/lib/semmle/javascript/Actions.qll
+++ b/javascript/ql/lib/semmle/javascript/Actions.qll
@@ -267,8 +267,8 @@ module Actions {
// not just the last (greedy match) or first (reluctant match).
result =
this.getValue()
- .regexpFind("\\$\\{\\{\\s*[A-Za-z0-9_\\.\\-]+\\s*\\}\\}", _, _)
- .regexpCapture("\\$\\{\\{\\s*([A-Za-z0-9_\\.\\-]+)\\s*\\}\\}", 1)
+ .regexpFind("\\$\\{\\{\\s*[A-Za-z0-9_\\[\\]\\*\\(\\)\\.\\-]+\\s*\\}\\}", _, _)
+ .regexpCapture("\\$\\{\\{\\s*([A-Za-z0-9_\\[\\]\\*\\((\\)\\.\\-]+)\\s*\\}\\}", 1)
}
}
}
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
index 03c129711ad..c8c42b4122e 100644
--- a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
+++ b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
@@ -30,7 +30,10 @@ private predicate isExternalUserControlledPullRequest(string context) {
"\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*body\\b",
"\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*label\\b",
"\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*repo\\s*\\.\\s*default_branch\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*repo\\s*\\.\\s*description\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*repo\\s*\\.\\s*homepage\\b",
"\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pull_request\\s*\\.\\s*head\\s*\\.\\s*ref\\b",
+ "\\bgithub\\s*\\.\\s*head_ref\\b"
]
|
context.regexpMatch(reg)
@@ -39,8 +42,7 @@ private predicate isExternalUserControlledPullRequest(string context) {
bindingset[context]
private predicate isExternalUserControlledReview(string context) {
- context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*review\\s*\\.\\s*body\\b") or
- context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*review_comment\\s*\\.\\s*body\\b")
+ context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*review\\s*\\.\\s*body\\b")
}
bindingset[context]
@@ -50,8 +52,8 @@ private predicate isExternalUserControlledComment(string context) {
bindingset[context]
private predicate isExternalUserControlledGollum(string context) {
- context
- .regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pages(?:\\[[0-9]\\]|\\s*\\.\\s*\\*)+\\s*\\.\\s*page_name\\b")
+ context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pages\\[[0-9]+\\]\\s*\\.\\s*page_name\\b") or
+ context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pages\\[[0-9]+\\]\\s*\\.\\s*title\\b")
}
bindingset[context]
@@ -59,13 +61,16 @@ private predicate isExternalUserControlledCommit(string context) {
exists(string reg |
reg =
[
- "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits(?:\\[[0-9]\\]|\\s*\\.\\s*\\*)+\\s*\\.\\s*message\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*message\\b",
"\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*message\\b",
"\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*author\\s*\\.\\s*email\\b",
"\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*author\\s*\\.\\s*name\\b",
- "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits(?:\\[[0-9]\\]|\\s*\\.\\s*\\*)+\\s*\\.\\s*author\\s*\\.\\s*email\\b",
- "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits(?:\\[[0-9]\\]|\\s*\\.\\s*\\*)+\\s*\\.\\s*author\\s*\\.\\s*name\\b",
- "\\bgithub\\s*\\.\\s*head_ref\\b"
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*committer\\s*\\.\\s*email\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*head_commit\\s*\\.\\s*committer\\s*\\.\\s*name\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*author\\s*\\.\\s*email\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*author\\s*\\.\\s*name\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*committer\\s*\\.\\s*email\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*commits\\[[0-9]+\\]\\s*\\.\\s*committer\\s*\\.\\s*name\\b",
]
|
context.regexpMatch(reg)
@@ -78,6 +83,25 @@ private predicate isExternalUserControlledDiscussion(string context) {
context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*discussion\\s*\\.\\s*body\\b")
}
+bindingset[context]
+private predicate isExternalUserControlledWorkflowRun(string context) {
+ exists(string reg |
+ reg =
+ [
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_branch\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*display_title\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_repository\\b\\s*\\.\\s*description\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*message\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*author\\b\\s*\\.\\s*email\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*author\\b\\s*\\.\\s*name\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*committer\\b\\s*\\.\\s*email\\b",
+ "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_commit\\b\\s*\\.\\s*committer\\b\\s*\\.\\s*name\\b",
+ ]
+ |
+ context.regexpMatch(reg)
+ )
+}
+
from Actions::Run run, string context, Actions::On on
where
run.getASimpleReferenceExpression() = context and
@@ -89,20 +113,29 @@ where
exists(on.getNode("pull_request_target")) and
isExternalUserControlledPullRequest(context)
or
- (exists(on.getNode("pull_request_review_comment")) or exists(on.getNode("pull_request_review"))) and
- isExternalUserControlledReview(context)
+ exists(on.getNode("pull_request_review")) and
+ (isExternalUserControlledReview(context) or isExternalUserControlledPullRequest(context))
or
- (exists(on.getNode("issue_comment")) or exists(on.getNode("pull_request_target"))) and
- isExternalUserControlledComment(context)
+ exists(on.getNode("pull_request_review_comment")) and
+ (isExternalUserControlledComment(context) or isExternalUserControlledPullRequest(context))
+ or
+ exists(on.getNode("issue_comment")) and
+ (isExternalUserControlledComment(context) or isExternalUserControlledIssue(context))
or
exists(on.getNode("gollum")) and
isExternalUserControlledGollum(context)
or
- exists(on.getNode("pull_request_target")) and
+ exists(on.getNode("push")) and
isExternalUserControlledCommit(context)
or
- (exists(on.getNode("discussion")) or exists(on.getNode("discussion_comment"))) and
+ exists(on.getNode("discussion")) and
isExternalUserControlledDiscussion(context)
+ or
+ exists(on.getNode("discussion_comment")) and
+ (isExternalUserControlledDiscussion(context) or isExternalUserControlledComment(context))
+ or
+ exists(on.getNode("workflow_run")) and
+ isExternalUserControlledWorkflowRun(context)
)
select run,
"Potential injection from the " + context +
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml
index c19524f1191..fec20d7272f 100644
--- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml
@@ -10,5 +10,6 @@ jobs:
echo-chamber2:
runs-on: ubuntu-latest
steps:
- - run: |
- echo '${{ github.event.comment.body }}'
\ No newline at end of file
+ - run: echo '${{ github.event.comment.body }}'
+ - run: echo '${{ github.event.issue.body }}'
+ - run: echo '${{ github.event.issue.title }}'
\ No newline at end of file
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/discussion.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/discussion.yml
new file mode 100644
index 00000000000..fdb140ec380
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/discussion.yml
@@ -0,0 +1,8 @@
+on: discussion
+
+jobs:
+ echo-chamber:
+ runs-on: ubuntu-latest
+ steps:
+ - run: echo '${{ github.event.discussion.title }}'
+ - run: echo '${{ github.event.discussion.body }}'
\ No newline at end of file
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/discussion_comment.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/discussion_comment.yml
new file mode 100644
index 00000000000..649d3a6e131
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/discussion_comment.yml
@@ -0,0 +1,9 @@
+on: discussion_comment
+
+jobs:
+ echo-chamber:
+ runs-on: ubuntu-latest
+ steps:
+ - run: echo '${{ github.event.discussion.title }}'
+ - run: echo '${{ github.event.discussion.body }}'
+ - run: echo '${{ github.event.comment.body }}'
\ No newline at end of file
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/gollum.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/gollum.yml
new file mode 100644
index 00000000000..a952c8c1ab8
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/gollum.yml
@@ -0,0 +1,11 @@
+on: gollum
+
+jobs:
+ echo-chamber:
+ runs-on: ubuntu-latest
+ steps:
+ - run: echo '${{ github.event.pages[1].title }}'
+ - run: echo '${{ github.event.pages[11].title }}'
+ - run: echo '${{ github.event.pages[0].page_name }}'
+ - run: echo '${{ github.event.pages[2222].page_name }}'
+ - run: echo '${{ toJSON(github.event.pages.*.title) }}' # safe
\ No newline at end of file
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yml
new file mode 100644
index 00000000000..2eae85278dd
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yml
@@ -0,0 +1,8 @@
+on: issues
+
+jobs:
+ echo-chamber:
+ runs-on: ubuntu-latest
+ steps:
+ - run: echo '${{ github.event.issue.title }}'
+ - run: echo '${{ github.event.issue.body }}'
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_review.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_review.yml
new file mode 100644
index 00000000000..d4ce7885669
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_review.yml
@@ -0,0 +1,14 @@
+on: pull_request_review
+
+jobs:
+ echo-chamber:
+ runs-on: ubuntu-latest
+ steps:
+ - run: echo '${{ github.event.pull_request.title }}'
+ - run: echo '${{ github.event.pull_request.body }}'
+ - run: echo '${{ github.event.pull_request.head.label }}'
+ - run: echo '${{ github.event.pull_request.head.repo.default_branch }}'
+ - run: echo '${{ github.event.pull_request.head.repo.description }}'
+ - run: echo '${{ github.event.pull_request.head.repo.homepage }}'
+ - run: echo '${{ github.event.pull_request.head.ref }}'
+ - run: echo '${{ github.event.review.body }}'
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_review_comment.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_review_comment.yml
new file mode 100644
index 00000000000..5d288caad85
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_review_comment.yml
@@ -0,0 +1,14 @@
+on: pull_request_review_comment
+
+jobs:
+ echo-chamber:
+ runs-on: ubuntu-latest
+ steps:
+ - run: echo '${{ github.event.pull_request.title }}'
+ - run: echo '${{ github.event.pull_request.body }}'
+ - run: echo '${{ github.event.pull_request.head.label }}'
+ - run: echo '${{ github.event.pull_request.head.repo.default_branch }}'
+ - run: echo '${{ github.event.pull_request.head.repo.description }}'
+ - run: echo '${{ github.event.pull_request.head.repo.homepage }}'
+ - run: echo '${{ github.event.pull_request.head.ref }}'
+ - run: echo '${{ github.event.comment.body }}'
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_target.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_target.yml
new file mode 100644
index 00000000000..215b3252885
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/pull_request_target.yml
@@ -0,0 +1,16 @@
+on: pull_request_target
+
+jobs:
+ echo-chamber:
+ runs-on: ubuntu-latest
+ steps:
+ - run: echo '${{ github.event.issue.title }}' # not defined
+ - run: echo '${{ github.event.issue.body }}' # not defined
+ - run: echo '${{ github.event.pull_request.title }}'
+ - run: echo '${{ github.event.pull_request.body }}'
+ - run: echo '${{ github.event.pull_request.head.label }}'
+ - run: echo '${{ github.event.pull_request.head.repo.default_branch }}'
+ - run: echo '${{ github.event.pull_request.head.repo.description }}'
+ - run: echo '${{ github.event.pull_request.head.repo.homepage }}'
+ - run: echo '${{ github.event.pull_request.head.ref }}'
+ - run: echo '${{ github.head_ref }}'
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/push.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/push.yml
new file mode 100644
index 00000000000..2006a7999da
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/push.yml
@@ -0,0 +1,16 @@
+on: push
+
+jobs:
+ echo-chamber:
+ runs-on: ubuntu-latest
+ steps:
+ - run: echo '${{ github.event.commits[11].message }}'
+ - run: echo '${{ github.event.commits[11].author.email }}'
+ - run: echo '${{ github.event.commits[11].author.name }}'
+ - run: echo '${{ github.event.head_commit.message }}'
+ - run: echo '${{ github.event.head_commit.author.email }}'
+ - run: echo '${{ github.event.head_commit.author.name }}'
+ - run: echo '${{ github.event.head_commit.committer.email }}'
+ - run: echo '${{ github.event.head_commit.committer.name }}'
+ - run: echo '${{ github.event.commits[11].committer.email }}'
+ - run: echo '${{ github.event.commits[11].committer.name }}'
\ No newline at end of file
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/workflow_run.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/workflow_run.yml
new file mode 100644
index 00000000000..60e7645f60f
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/workflow_run.yml
@@ -0,0 +1,16 @@
+on:
+ workflow_run:
+ workflows: [test]
+
+jobs:
+ echo-chamber:
+ runs-on: ubuntu-latest
+ steps:
+ - run: echo '${{ github.event.workflow_run.display_title }}'
+ - run: echo '${{ github.event.workflow_run.head_commit.message }}'
+ - run: echo '${{ github.event.workflow_run.head_commit.author.email }}'
+ - run: echo '${{ github.event.workflow_run.head_commit.author.name }}'
+ - run: echo '${{ github.event.workflow_run.head_commit.committer.email }}'
+ - run: echo '${{ github.event.workflow_run.head_commit.committer.name }}'
+ - run: echo '${{ github.event.workflow_run.head_branch }}'
+ - run: echo '${{ github.event.workflow_run.head_repository.description }}'
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
index 64451c37691..b89948010b8 100644
--- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
@@ -1,3 +1,58 @@
| .github/workflows/comment_issue.yml:7:12:8:48 | \| | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
-| .github/workflows/comment_issue.yml:13:12:14:47 | \| | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:13:12:13:50 | echo '$ ... ody }}' | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:14:12:14:48 | echo '$ ... ody }}' | Potential injection from the github.event.issue.body context, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:15:12:15:49 | echo '$ ... tle }}' | Potential injection from the github.event.issue.title context, which may be controlled by an external user. |
| .github/workflows/comment_issue_newline.yml:9:14:10:50 | \| | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
+| .github/workflows/discussion.yml:7:12:7:54 | echo '$ ... tle }}' | Potential injection from the github.event.discussion.title context, which may be controlled by an external user. |
+| .github/workflows/discussion.yml:8:12:8:53 | echo '$ ... ody }}' | Potential injection from the github.event.discussion.body context, which may be controlled by an external user. |
+| .github/workflows/discussion_comment.yml:7:12:7:54 | echo '$ ... tle }}' | Potential injection from the github.event.discussion.title context, which may be controlled by an external user. |
+| .github/workflows/discussion_comment.yml:8:12:8:53 | echo '$ ... ody }}' | Potential injection from the github.event.discussion.body context, which may be controlled by an external user. |
+| .github/workflows/discussion_comment.yml:9:12:9:50 | echo '$ ... ody }}' | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
+| .github/workflows/gollum.yml:7:12:7:52 | echo '$ ... tle }}' | Potential injection from the github.event.pages[1].title context, which may be controlled by an external user. |
+| .github/workflows/gollum.yml:8:12:8:53 | echo '$ ... tle }}' | Potential injection from the github.event.pages[11].title context, which may be controlled by an external user. |
+| .github/workflows/gollum.yml:9:12:9:56 | echo '$ ... ame }}' | Potential injection from the github.event.pages[0].page_name context, which may be controlled by an external user. |
+| .github/workflows/gollum.yml:10:12:10:59 | echo '$ ... ame }}' | Potential injection from the github.event.pages[2222].page_name context, which may be controlled by an external user. |
+| .github/workflows/issues.yml:7:12:7:49 | echo '$ ... tle }}' | Potential injection from the github.event.issue.title context, which may be controlled by an external user. |
+| .github/workflows/issues.yml:8:12:8:48 | echo '$ ... ody }}' | Potential injection from the github.event.issue.body context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:7:12:7:56 | echo '$ ... tle }}' | Potential injection from the github.event.pull_request.title context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:8:12:8:55 | echo '$ ... ody }}' | Potential injection from the github.event.pull_request.body context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:9:12:9:61 | echo '$ ... bel }}' | Potential injection from the github.event.pull_request.head.label context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:10:12:10:75 | echo '$ ... nch }}' | Potential injection from the github.event.pull_request.head.repo.default_branch context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:11:12:11:72 | echo '$ ... ion }}' | Potential injection from the github.event.pull_request.head.repo.description context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:12:12:12:69 | echo '$ ... age }}' | Potential injection from the github.event.pull_request.head.repo.homepage context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:13:12:13:59 | echo '$ ... ref }}' | Potential injection from the github.event.pull_request.head.ref context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:14:12:14:49 | echo '$ ... ody }}' | Potential injection from the github.event.review.body context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:7:12:7:56 | echo '$ ... tle }}' | Potential injection from the github.event.pull_request.title context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:8:12:8:55 | echo '$ ... ody }}' | Potential injection from the github.event.pull_request.body context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:9:12:9:61 | echo '$ ... bel }}' | Potential injection from the github.event.pull_request.head.label context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:10:12:10:75 | echo '$ ... nch }}' | Potential injection from the github.event.pull_request.head.repo.default_branch context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:11:12:11:72 | echo '$ ... ion }}' | Potential injection from the github.event.pull_request.head.repo.description context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:12:12:12:69 | echo '$ ... age }}' | Potential injection from the github.event.pull_request.head.repo.homepage context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:13:12:13:59 | echo '$ ... ref }}' | Potential injection from the github.event.pull_request.head.ref context, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:14:12:14:50 | echo '$ ... ody }}' | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:9:12:9:56 | echo '$ ... tle }}' | Potential injection from the github.event.pull_request.title context, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:10:12:10:55 | echo '$ ... ody }}' | Potential injection from the github.event.pull_request.body context, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:11:12:11:61 | echo '$ ... bel }}' | Potential injection from the github.event.pull_request.head.label context, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:12:12:12:75 | echo '$ ... nch }}' | Potential injection from the github.event.pull_request.head.repo.default_branch context, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:13:12:13:72 | echo '$ ... ion }}' | Potential injection from the github.event.pull_request.head.repo.description context, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:14:12:14:69 | echo '$ ... age }}' | Potential injection from the github.event.pull_request.head.repo.homepage context, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:15:12:15:59 | echo '$ ... ref }}' | Potential injection from the github.event.pull_request.head.ref context, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:16:12:16:40 | echo '$ ... ref }}' | Potential injection from the github.head_ref context, which may be controlled by an external user. |
+| .github/workflows/push.yml:7:12:7:57 | echo '$ ... age }}' | Potential injection from the github.event.commits[11].message context, which may be controlled by an external user. |
+| .github/workflows/push.yml:8:12:8:62 | echo '$ ... ail }}' | Potential injection from the github.event.commits[11].author.email context, which may be controlled by an external user. |
+| .github/workflows/push.yml:9:12:9:61 | echo '$ ... ame }}' | Potential injection from the github.event.commits[11].author.name context, which may be controlled by an external user. |
+| .github/workflows/push.yml:10:12:10:57 | echo '$ ... age }}' | Potential injection from the github.event.head_commit.message context, which may be controlled by an external user. |
+| .github/workflows/push.yml:11:12:11:62 | echo '$ ... ail }}' | Potential injection from the github.event.head_commit.author.email context, which may be controlled by an external user. |
+| .github/workflows/push.yml:12:12:12:61 | echo '$ ... ame }}' | Potential injection from the github.event.head_commit.author.name context, which may be controlled by an external user. |
+| .github/workflows/push.yml:13:12:13:65 | echo '$ ... ail }}' | Potential injection from the github.event.head_commit.committer.email context, which may be controlled by an external user. |
+| .github/workflows/push.yml:14:12:14:64 | echo '$ ... ame }}' | Potential injection from the github.event.head_commit.committer.name context, which may be controlled by an external user. |
+| .github/workflows/push.yml:15:12:15:65 | echo '$ ... ail }}' | Potential injection from the github.event.commits[11].committer.email context, which may be controlled by an external user. |
+| .github/workflows/push.yml:16:12:16:64 | echo '$ ... ame }}' | Potential injection from the github.event.commits[11].committer.name context, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:9:12:9:64 | echo '$ ... tle }}' | Potential injection from the github.event.workflow_run.display_title context, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:10:12:10:70 | echo '$ ... age }}' | Potential injection from the github.event.workflow_run.head_commit.message context, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:11:12:11:75 | echo '$ ... ail }}' | Potential injection from the github.event.workflow_run.head_commit.author.email context, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:12:12:12:74 | echo '$ ... ame }}' | Potential injection from the github.event.workflow_run.head_commit.author.name context, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:13:12:13:78 | echo '$ ... ail }}' | Potential injection from the github.event.workflow_run.head_commit.committer.email context, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:14:12:14:77 | echo '$ ... ame }}' | Potential injection from the github.event.workflow_run.head_commit.committer.name context, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:15:12:15:62 | echo '$ ... nch }}' | Potential injection from the github.event.workflow_run.head_branch context, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:16:12:16:78 | echo '$ ... ion }}' | Potential injection from the github.event.workflow_run.head_repository.description context, which may be controlled by an external user. |
From c6eaf194a515de729ee03f74b5a4ba0f02ad11f8 Mon Sep 17 00:00:00 2001
From: jarlob
Date: Mon, 3 Apr 2023 15:09:40 +0200
Subject: [PATCH 007/704] Remove empty.js as it is not needed anymore
---
javascript/ql/test/experimental/Security/CWE-094/empty.js | 1 -
.../query-tests/Security/CWE-094/ExpressionInjection/empty.js | 1 -
2 files changed, 2 deletions(-)
delete mode 100644 javascript/ql/test/experimental/Security/CWE-094/empty.js
delete mode 100644 javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/empty.js
diff --git a/javascript/ql/test/experimental/Security/CWE-094/empty.js b/javascript/ql/test/experimental/Security/CWE-094/empty.js
deleted file mode 100644
index a243684db7f..00000000000
--- a/javascript/ql/test/experimental/Security/CWE-094/empty.js
+++ /dev/null
@@ -1 +0,0 @@
-console.log('test')
\ No newline at end of file
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/empty.js b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/empty.js
deleted file mode 100644
index a243684db7f..00000000000
--- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/empty.js
+++ /dev/null
@@ -1 +0,0 @@
-console.log('test')
\ No newline at end of file
From ba5747dff382fb2c5f86e190746be92ec9f7c97d Mon Sep 17 00:00:00 2001
From: jarlob
Date: Mon, 3 Apr 2023 15:10:27 +0200
Subject: [PATCH 008/704] fix formatting
---
javascript/ql/src/Security/CWE-094/ExpressionInjection.ql | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
index c8c42b4122e..ce815c8e11d 100644
--- a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
+++ b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
@@ -52,7 +52,8 @@ private predicate isExternalUserControlledComment(string context) {
bindingset[context]
private predicate isExternalUserControlledGollum(string context) {
- context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pages\\[[0-9]+\\]\\s*\\.\\s*page_name\\b") or
+ context
+ .regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pages\\[[0-9]+\\]\\s*\\.\\s*page_name\\b") or
context.regexpMatch("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*pages\\[[0-9]+\\]\\s*\\.\\s*title\\b")
}
From e941218e307417476a22e2ef1396e0f333bd1ae9 Mon Sep 17 00:00:00 2001
From: jarlob
Date: Mon, 3 Apr 2023 15:15:00 +0200
Subject: [PATCH 009/704] change notes added
---
javascript/ql/lib/change-notes/2023-04-03-gh-injection.md | 4 ++++
1 file changed, 4 insertions(+)
create mode 100644 javascript/ql/lib/change-notes/2023-04-03-gh-injection.md
diff --git a/javascript/ql/lib/change-notes/2023-04-03-gh-injection.md b/javascript/ql/lib/change-notes/2023-04-03-gh-injection.md
new file mode 100644
index 00000000000..04a5a2f4b6f
--- /dev/null
+++ b/javascript/ql/lib/change-notes/2023-04-03-gh-injection.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Fixes and improvements in GitHub Actions Injection query.
\ No newline at end of file
From 8ea418216c4cc997d6f73289fb37c41203310cf0 Mon Sep 17 00:00:00 2001
From: jarlob
Date: Mon, 3 Apr 2023 23:13:28 +0200
Subject: [PATCH 010/704] Look for script injections in actions/github-script
---
.../ql/lib/semmle/javascript/Actions.qll | 49 +++++++++++++------
.../Security/CWE-094/ExpressionInjection.ql | 22 +++++++--
.../.github/workflows/comment_issue.yml | 15 +++++-
.../ExpressionInjection.expected | 3 ++
4 files changed, 69 insertions(+), 20 deletions(-)
diff --git a/javascript/ql/lib/semmle/javascript/Actions.qll b/javascript/ql/lib/semmle/javascript/Actions.qll
index b1ab674924d..3d1d4c589d1 100644
--- a/javascript/ql/lib/semmle/javascript/Actions.qll
+++ b/javascript/ql/lib/semmle/javascript/Actions.qll
@@ -244,6 +244,40 @@ module Actions {
With getWith() { result = with }
}
+ /**
+ * Holds if `${{ e }}` is a GitHub Actions expression evaluated within this YAML string.
+ * See https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions.
+ * Only finds simple expressions like `${{ github.event.comment.body }}`, where the expression contains only alphanumeric characters, underscores, dots, or dashes.
+ * Does not identify more complicated expressions like `${{ fromJSON(env.time) }}`, or ${{ format('{{Hello {0}!}}', github.event.head_commit.author.name) }}
+ */
+ string getASimpleReferenceExpression(YamlString node) {
+ // We use `regexpFind` to obtain *all* matches of `${{...}}`,
+ // not just the last (greedy match) or first (reluctant match).
+ result =
+ node.getValue()
+ .regexpFind("\\$\\{\\{\\s*[A-Za-z0-9_\\[\\]\\*\\(\\)\\.\\-]+\\s*\\}\\}", _, _)
+ .regexpCapture("\\$\\{\\{\\s*([A-Za-z0-9_\\[\\]\\*\\((\\)\\.\\-]+)\\s*\\}\\}", 1)
+ }
+
+ /**
+ * A `script:` field within an Actions `with:` specific to `actions/github-script` action.
+ *
+ * For example:
+ * ```
+ * uses: actions/github-script@v3
+ * with:
+ * script: console.log('${{ github.event.pull_request.head.sha }}')
+ * ```
+ */
+ class Script extends YamlNode, YamlString {
+ With with;
+
+ Script() { with.lookup("script") = this }
+
+ /** Gets the `with` field this field belongs to. */
+ With getWith() { result = with }
+ }
+
/**
* A `run` field within an Actions job step, which runs command-line programs using an operating system shell.
* See https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstepsrun.
@@ -255,20 +289,5 @@ module Actions {
/** Gets the step that executes this `run` command. */
Step getStep() { result = step }
-
- /**
- * Holds if `${{ e }}` is a GitHub Actions expression evaluated within this `run` command.
- * See https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions.
- * Only finds simple expressions like `${{ github.event.comment.body }}`, where the expression contains only alphanumeric characters, underscores, dots, or dashes.
- * Does not identify more complicated expressions like `${{ fromJSON(env.time) }}`, or ${{ format('{{Hello {0}!}}', github.event.head_commit.author.name) }}
- */
- string getASimpleReferenceExpression() {
- // We use `regexpFind` to obtain *all* matches of `${{...}}`,
- // not just the last (greedy match) or first (reluctant match).
- result =
- this.getValue()
- .regexpFind("\\$\\{\\{\\s*[A-Za-z0-9_\\[\\]\\*\\(\\)\\.\\-]+\\s*\\}\\}", _, _)
- .regexpCapture("\\$\\{\\{\\s*([A-Za-z0-9_\\[\\]\\*\\((\\)\\.\\-]+)\\s*\\}\\}", 1)
- }
}
}
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
index ce815c8e11d..84e7215837e 100644
--- a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
+++ b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
@@ -103,10 +103,24 @@ private predicate isExternalUserControlledWorkflowRun(string context) {
)
}
-from Actions::Run run, string context, Actions::On on
+from YamlNode node, string context, Actions::On on
where
- run.getASimpleReferenceExpression() = context and
- run.getStep().getJob().getWorkflow().getOn() = on and
+ (
+ exists(Actions::Run run |
+ node = run and
+ Actions::getASimpleReferenceExpression(run) = context and
+ run.getStep().getJob().getWorkflow().getOn() = on
+ )
+ or
+ exists(Actions::Script script, Actions::Step step, Actions::Uses uses |
+ node = script and
+ script.getWith().getStep() = step and
+ uses.getStep() = step and
+ uses.getGitHubRepository() = "actions/github-script" and
+ Actions::getASimpleReferenceExpression(script) = context and
+ script.getWith().getStep().getJob().getWorkflow().getOn() = on
+ )
+ ) and
(
exists(on.getNode("issues")) and
isExternalUserControlledIssue(context)
@@ -138,6 +152,6 @@ where
exists(on.getNode("workflow_run")) and
isExternalUserControlledWorkflowRun(context)
)
-select run,
+select node,
"Potential injection from the " + context +
" context, which may be controlled by an external user."
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml
index fec20d7272f..17ead9fdd20 100644
--- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/comment_issue.yml
@@ -12,4 +12,17 @@ jobs:
steps:
- run: echo '${{ github.event.comment.body }}'
- run: echo '${{ github.event.issue.body }}'
- - run: echo '${{ github.event.issue.title }}'
\ No newline at end of file
+ - run: echo '${{ github.event.issue.title }}'
+
+ echo-chamber3:
+ runs-on: ubuntu-latest
+ steps:
+ - uses: actions/github-script@v3
+ with:
+ script: console.log('${{ github.event.comment.body }}')
+ - uses: actions/github-script@v3
+ with:
+ script: console.log('${{ github.event.issue.body }}')
+ - uses: actions/github-script@v3
+ with:
+ script: console.log('${{ github.event.issue.title }}')
\ No newline at end of file
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
index b89948010b8..775ec3ba640 100644
--- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
@@ -2,6 +2,9 @@
| .github/workflows/comment_issue.yml:13:12:13:50 | echo '$ ... ody }}' | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
| .github/workflows/comment_issue.yml:14:12:14:48 | echo '$ ... ody }}' | Potential injection from the github.event.issue.body context, which may be controlled by an external user. |
| .github/workflows/comment_issue.yml:15:12:15:49 | echo '$ ... tle }}' | Potential injection from the github.event.issue.title context, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:22:17:22:63 | console ... dy }}') | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:25:17:25:61 | console ... dy }}') | Potential injection from the github.event.issue.body context, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:28:17:28:62 | console ... le }}') | Potential injection from the github.event.issue.title context, which may be controlled by an external user. |
| .github/workflows/comment_issue_newline.yml:9:14:10:50 | \| | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
| .github/workflows/discussion.yml:7:12:7:54 | echo '$ ... tle }}' | Potential injection from the github.event.discussion.title context, which may be controlled by an external user. |
| .github/workflows/discussion.yml:8:12:8:53 | echo '$ ... ody }}' | Potential injection from the github.event.discussion.body context, which may be controlled by an external user. |
From 39ff3c72a29696058a994709b2f60d488ad96626 Mon Sep 17 00:00:00 2001
From: jarlob
Date: Mon, 3 Apr 2023 23:28:31 +0200
Subject: [PATCH 011/704] Remove label sanitizer because it is prone to race
conditions
---
.../Security/CWE-094/UntrustedCheckout.ql | 46 ++-----------------
.../CWE-094/UntrustedCheckout.expected | 6 +++
2 files changed, 10 insertions(+), 42 deletions(-)
diff --git a/javascript/ql/src/experimental/Security/CWE-094/UntrustedCheckout.ql b/javascript/ql/src/experimental/Security/CWE-094/UntrustedCheckout.ql
index 8f9622fe6e7..a81c8c65d25 100644
--- a/javascript/ql/src/experimental/Security/CWE-094/UntrustedCheckout.ql
+++ b/javascript/ql/src/experimental/Security/CWE-094/UntrustedCheckout.ql
@@ -17,7 +17,7 @@ import javascript
import semmle.javascript.Actions
/**
- * An action step that doesn't contain `actor` or `label` check in `if:` or
+ * An action step that doesn't contain `actor` check in `if:` or
* the check requires manual analysis.
*/
class ProbableStep extends Actions::Step {
@@ -29,25 +29,13 @@ class ProbableStep extends Actions::Step {
// needs manual analysis if there is OR
this.getIf().getValue().matches("%||%")
or
- // labels can be assigned by owners only
- not exists(
- this.getIf()
- .getValue()
- .regexpFind("\\bcontains\\s*\\(\\s*github\\s*\\.\\s*event\\s*\\.\\s*(?:issue|pull_request)\\s*\\.\\s*labels\\b",
- _, _)
- ) and
- not exists(
- this.getIf()
- .getValue()
- .regexpFind("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*label\\s*\\.\\s*name\\s*==", _, _)
- ) and
// actor check means only the user is able to run it
not exists(this.getIf().getValue().regexpFind("\\bgithub\\s*\\.\\s*actor\\s*==", _, _))
}
}
/**
- * An action job that doesn't contain `actor` or `label` check in `if:` or
+ * An action job that doesn't contain `actor` check in `if:` or
* the check requires manual analysis.
*/
class ProbableJob extends Actions::Job {
@@ -59,45 +47,19 @@ class ProbableJob extends Actions::Job {
// needs manual analysis if there is OR
this.getIf().getValue().matches("%||%")
or
- // labels can be assigned by owners only
- not exists(
- this.getIf()
- .getValue()
- .regexpFind("\\bcontains\\s*\\(\\s*github\\s*\\.\\s*event\\s*\\.\\s*(?:issue|pull_request)\\s*\\.\\s*labels\\b",
- _, _)
- ) and
- not exists(
- this.getIf()
- .getValue()
- .regexpFind("\\bgithub\\s*\\.\\s*event\\s*\\.\\s*label\\s*\\.\\s*name\\s*==", _, _)
- ) and
// actor check means only the user is able to run it
not exists(this.getIf().getValue().regexpFind("\\bgithub\\s*\\.\\s*actor\\s*==", _, _))
}
}
/**
- * An action step that doesn't contain `actor` or `label` check in `if:` or
+ * on: pull_request_target
*/
class ProbablePullRequestTarget extends Actions::On, YamlMappingLikeNode {
ProbablePullRequestTarget() {
exists(YamlNode prtNode |
// The `on:` is triggered on `pull_request_target`
- this.getNode("pull_request_target") = prtNode and
- (
- // and either doesn't contain `types` filter
- not exists(prtNode.getAChild())
- or
- // or has the filter, that is something else than just [labeled]
- exists(YamlMappingLikeNode prt, YamlMappingLikeNode types |
- types = prt.getNode("types") and
- prtNode = prt and
- (
- not types.getElementCount() = 1 or
- not exists(types.getNode("labeled"))
- )
- )
- )
+ this.getNode("pull_request_target") = prtNode
)
}
}
diff --git a/javascript/ql/test/experimental/Security/CWE-094/UntrustedCheckout.expected b/javascript/ql/test/experimental/Security/CWE-094/UntrustedCheckout.expected
index fc1f704c025..127ced2bb97 100644
--- a/javascript/ql/test/experimental/Security/CWE-094/UntrustedCheckout.expected
+++ b/javascript/ql/test/experimental/Security/CWE-094/UntrustedCheckout.expected
@@ -1,7 +1,13 @@
+| .github/workflows/pull_request_target_if_job.yml:9:7:12:2 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
+| .github/workflows/pull_request_target_if_job.yml:16:7:19:2 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
| .github/workflows/pull_request_target_if_job.yml:30:7:33:2 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
| .github/workflows/pull_request_target_if_job.yml:36:7:38:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
+| .github/workflows/pull_request_target_if_step.yml:9:7:14:4 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
+| .github/workflows/pull_request_target_if_step.yml:14:7:19:4 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
| .github/workflows/pull_request_target_if_step.yml:24:7:29:4 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
| .github/workflows/pull_request_target_if_step.yml:29:7:31:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
+| .github/workflows/pull_request_target_label_only.yml:10:7:12:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
+| .github/workflows/pull_request_target_label_only_mapping.yml:11:7:13:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
| .github/workflows/pull_request_target_labels_mapping.yml:13:7:15:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
| .github/workflows/pull_request_target_labels_sequence.yml:10:7:12:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
| .github/workflows/pull_request_target_mapping.yml:8:7:10:54 | uses: a ... kout@v2 | Potential unsafe checkout of untrusted pull request on 'pull_request_target'. |
From 5c5b9f99a83dfd328780e77d15892652556a5484 Mon Sep 17 00:00:00 2001
From: jarlob
Date: Wed, 5 Apr 2023 10:03:46 +0200
Subject: [PATCH 012/704] Add simple taint tracking for env variables
---
.../ql/lib/semmle/javascript/Actions.qll | 61 +++++++++++++++++++
.../CWE-094/ExpressionInjection.qhelp | 10 +--
.../Security/CWE-094/ExpressionInjection.ql | 28 +++++++--
.../.github/workflows/issues.yml | 12 ++++
.../ExpressionInjection.expected | 7 ++-
5 files changed, 106 insertions(+), 12 deletions(-)
diff --git a/javascript/ql/lib/semmle/javascript/Actions.qll b/javascript/ql/lib/semmle/javascript/Actions.qll
index 3d1d4c589d1..3567414650b 100644
--- a/javascript/ql/lib/semmle/javascript/Actions.qll
+++ b/javascript/ql/lib/semmle/javascript/Actions.qll
@@ -28,6 +28,9 @@ module Actions {
/** Gets the `jobs` mapping from job IDs to job definitions in this workflow. */
YamlMapping getJobs() { result = this.lookup("jobs") }
+ /** Gets the 'global' `env` mapping in this workflow. */
+ YamlMapping getEnv() { result = this.lookup("env") }
+
/** Gets the name of the workflow. */
string getName() { result = this.lookup("name").(YamlString).getValue() }
@@ -54,6 +57,54 @@ module Actions {
Workflow getWorkflow() { result = workflow }
}
+ /** An environment variable in 'env:' */
+ abstract class Env extends YamlNode, YamlString {
+ /** Gets the name of this environment variable. */
+ abstract string getName();
+ }
+
+ /** Workflow level 'global' environment variable. */
+ class GlobalEnv extends Env {
+ string envName;
+ Workflow workflow;
+
+ GlobalEnv() { this = workflow.getEnv().lookup(envName) }
+
+ /** Gets the workflow this field belongs to. */
+ Workflow getWorkflow() { result = workflow }
+
+ /** Gets the name of this environment variable. */
+ override string getName() { result = envName }
+ }
+
+ /** Job level environment variable. */
+ class JobEnv extends Env {
+ string envName;
+ Job job;
+
+ JobEnv() { this = job.getEnv().lookup(envName) }
+
+ /** Gets the job this field belongs to. */
+ Job getJob() { result = job }
+
+ /** Gets the name of this environment variable. */
+ override string getName() { result = envName }
+ }
+
+ /** Step level environment variable. */
+ class StepEnv extends Env {
+ string envName;
+ Step step;
+
+ StepEnv() { this = step.getEnv().lookup(envName) }
+
+ /** Gets the step this field belongs to. */
+ Step getStep() { result = step }
+
+ /** Gets the name of this environment variable. */
+ override string getName() { result = envName }
+ }
+
/**
* An Actions job within a workflow.
* See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobs.
@@ -88,6 +139,9 @@ module Actions {
/** Gets the sequence of `steps` within this job. */
YamlSequence getSteps() { result = this.lookup("steps") }
+ /** Gets the `env` mapping in this job. */
+ YamlMapping getEnv() { result = this.lookup("env") }
+
/** Gets the workflow this job belongs to. */
Workflow getWorkflow() { result = workflow }
@@ -149,6 +203,9 @@ module Actions {
/** Gets the value of the `if` field in this step, if any. */
StepIf getIf() { result.getStep() = this }
+ /** Gets the value of the `env` field in this step, if any. */
+ YamlMapping getEnv() { result = this.lookup("env") }
+
/** Gets the ID of this step, if any. */
string getId() { result = this.lookup("id").(YamlString).getValue() }
}
@@ -259,6 +316,10 @@ module Actions {
.regexpCapture("\\$\\{\\{\\s*([A-Za-z0-9_\\[\\]\\*\\((\\)\\.\\-]+)\\s*\\}\\}", 1)
}
+ /** Extracts the 'name' part from env.name */
+ bindingset[name]
+ string getEnvName(string name) { result = name.regexpCapture("env\\.([A-Za-z0-9_]+)", 1) }
+
/**
* A `script:` field within an Actions `with:` specific to `actions/github-script` action.
*
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp b/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp
index 4424fe363a2..6e248eb380e 100644
--- a/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp
+++ b/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp
@@ -8,10 +8,11 @@
code injection in contexts like run: or script:.
- Code injection in GitHub Actions may allow an attacker to
- exfiltrate the temporary GitHub repository authorization token.
+ Code injection in GitHub Actions may allow an attacker to
+ exfiltrate any secrets used in the workflow and
+ the temporary GitHub repository authorization token.
The token might have write access to the repository, allowing an attacker
- to use the token to make changes to the repository.
+ to use the token to make changes to the repository.
@@ -19,7 +20,8 @@
The best practice to avoid code injection vulnerabilities
in GitHub workflows is to set the untrusted input value of the expression
- to an intermediate environment variable.
+ to an intermediate environment variable and then use the environment variable
+ using the native syntax of the shell/script interpreter (i.e. NOT the ${{ env.VAR }}).
It is also recommended to limit the permissions of any tokens used
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
index 84e7215837e..cbeecf1405a 100644
--- a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
+++ b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
@@ -103,22 +103,38 @@ private predicate isExternalUserControlledWorkflowRun(string context) {
)
}
-from YamlNode node, string context, Actions::On on
+from YamlNode node, string injection, string context, Actions::On on
where
(
exists(Actions::Run run |
node = run and
- Actions::getASimpleReferenceExpression(run) = context and
- run.getStep().getJob().getWorkflow().getOn() = on
+ Actions::getASimpleReferenceExpression(run) = injection and
+ run.getStep().getJob().getWorkflow().getOn() = on and
+ (
+ injection = context
+ or
+ exists(Actions::Env env |
+ Actions::getEnvName(injection) = env.getName() and
+ Actions::getASimpleReferenceExpression(env) = context
+ )
+ )
)
or
exists(Actions::Script script, Actions::Step step, Actions::Uses uses |
node = script and
+ script.getWith().getStep().getJob().getWorkflow().getOn() = on and
script.getWith().getStep() = step and
uses.getStep() = step and
uses.getGitHubRepository() = "actions/github-script" and
- Actions::getASimpleReferenceExpression(script) = context and
- script.getWith().getStep().getJob().getWorkflow().getOn() = on
+ Actions::getASimpleReferenceExpression(script) = injection and
+ (
+ injection = context
+ or
+ exists(Actions::Env env |
+ Actions::getEnvName(injection) = env.getName() and
+ Actions::getASimpleReferenceExpression(env) = context
+ )
+ )
)
) and
(
@@ -153,5 +169,5 @@ where
isExternalUserControlledWorkflowRun(context)
)
select node,
- "Potential injection from the " + context +
+ "Potential injection from the " + injection +
" context, which may be controlled by an external user."
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yml
index 2eae85278dd..5e767ce0239 100644
--- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yml
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yml
@@ -1,8 +1,20 @@
on: issues
+env:
+ global_env: ${{ github.event.issue.title }}
+ test: test
+
jobs:
echo-chamber:
+ env:
+ job_env: ${{ github.event.issue.title }}
runs-on: ubuntu-latest
steps:
- run: echo '${{ github.event.issue.title }}'
- run: echo '${{ github.event.issue.body }}'
+ - run: echo '${{ env.global_env }}'
+ - run: echo '${{ env.test }}'
+ - run: echo '${{ env.job_env }}'
+ - run: echo '${{ env.step_env }}'
+ env:
+ step_env: ${{ github.event.issue.title }}
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
index 775ec3ba640..0665fe46a6b 100644
--- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
@@ -15,8 +15,11 @@
| .github/workflows/gollum.yml:8:12:8:53 | echo '$ ... tle }}' | Potential injection from the github.event.pages[11].title context, which may be controlled by an external user. |
| .github/workflows/gollum.yml:9:12:9:56 | echo '$ ... ame }}' | Potential injection from the github.event.pages[0].page_name context, which may be controlled by an external user. |
| .github/workflows/gollum.yml:10:12:10:59 | echo '$ ... ame }}' | Potential injection from the github.event.pages[2222].page_name context, which may be controlled by an external user. |
-| .github/workflows/issues.yml:7:12:7:49 | echo '$ ... tle }}' | Potential injection from the github.event.issue.title context, which may be controlled by an external user. |
-| .github/workflows/issues.yml:8:12:8:48 | echo '$ ... ody }}' | Potential injection from the github.event.issue.body context, which may be controlled by an external user. |
+| .github/workflows/issues.yml:13:12:13:49 | echo '$ ... tle }}' | Potential injection from the github.event.issue.title context, which may be controlled by an external user. |
+| .github/workflows/issues.yml:14:12:14:48 | echo '$ ... ody }}' | Potential injection from the github.event.issue.body context, which may be controlled by an external user. |
+| .github/workflows/issues.yml:15:12:15:39 | echo '$ ... env }}' | Potential injection from the env.global_env context, which may be controlled by an external user. |
+| .github/workflows/issues.yml:17:12:17:36 | echo '$ ... env }}' | Potential injection from the env.job_env context, which may be controlled by an external user. |
+| .github/workflows/issues.yml:18:12:18:37 | echo '$ ... env }}' | Potential injection from the env.step_env context, which may be controlled by an external user. |
| .github/workflows/pull_request_review.yml:7:12:7:56 | echo '$ ... tle }}' | Potential injection from the github.event.pull_request.title context, which may be controlled by an external user. |
| .github/workflows/pull_request_review.yml:8:12:8:55 | echo '$ ... ody }}' | Potential injection from the github.event.pull_request.body context, which may be controlled by an external user. |
| .github/workflows/pull_request_review.yml:9:12:9:61 | echo '$ ... bel }}' | Potential injection from the github.event.pull_request.head.label context, which may be controlled by an external user. |
From eef1973b93f8f727e9c791cca565547b892465f8 Mon Sep 17 00:00:00 2001
From: jarlob
Date: Wed, 5 Apr 2023 10:05:24 +0200
Subject: [PATCH 013/704] Change UI message
---
.../Security/CWE-094/ExpressionInjection.ql | 4 +-
.../ExpressionInjection.expected | 128 +++++++++---------
2 files changed, 66 insertions(+), 66 deletions(-)
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
index cbeecf1405a..056d7204551 100644
--- a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
+++ b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
@@ -169,5 +169,5 @@ where
isExternalUserControlledWorkflowRun(context)
)
select node,
- "Potential injection from the " + injection +
- " context, which may be controlled by an external user."
+ "Potential injection from the ${ " + injection +
+ " }, which may be controlled by an external user."
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
index 0665fe46a6b..457cb815e6a 100644
--- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
@@ -1,64 +1,64 @@
-| .github/workflows/comment_issue.yml:7:12:8:48 | \| | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
-| .github/workflows/comment_issue.yml:13:12:13:50 | echo '$ ... ody }}' | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
-| .github/workflows/comment_issue.yml:14:12:14:48 | echo '$ ... ody }}' | Potential injection from the github.event.issue.body context, which may be controlled by an external user. |
-| .github/workflows/comment_issue.yml:15:12:15:49 | echo '$ ... tle }}' | Potential injection from the github.event.issue.title context, which may be controlled by an external user. |
-| .github/workflows/comment_issue.yml:22:17:22:63 | console ... dy }}') | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
-| .github/workflows/comment_issue.yml:25:17:25:61 | console ... dy }}') | Potential injection from the github.event.issue.body context, which may be controlled by an external user. |
-| .github/workflows/comment_issue.yml:28:17:28:62 | console ... le }}') | Potential injection from the github.event.issue.title context, which may be controlled by an external user. |
-| .github/workflows/comment_issue_newline.yml:9:14:10:50 | \| | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
-| .github/workflows/discussion.yml:7:12:7:54 | echo '$ ... tle }}' | Potential injection from the github.event.discussion.title context, which may be controlled by an external user. |
-| .github/workflows/discussion.yml:8:12:8:53 | echo '$ ... ody }}' | Potential injection from the github.event.discussion.body context, which may be controlled by an external user. |
-| .github/workflows/discussion_comment.yml:7:12:7:54 | echo '$ ... tle }}' | Potential injection from the github.event.discussion.title context, which may be controlled by an external user. |
-| .github/workflows/discussion_comment.yml:8:12:8:53 | echo '$ ... ody }}' | Potential injection from the github.event.discussion.body context, which may be controlled by an external user. |
-| .github/workflows/discussion_comment.yml:9:12:9:50 | echo '$ ... ody }}' | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
-| .github/workflows/gollum.yml:7:12:7:52 | echo '$ ... tle }}' | Potential injection from the github.event.pages[1].title context, which may be controlled by an external user. |
-| .github/workflows/gollum.yml:8:12:8:53 | echo '$ ... tle }}' | Potential injection from the github.event.pages[11].title context, which may be controlled by an external user. |
-| .github/workflows/gollum.yml:9:12:9:56 | echo '$ ... ame }}' | Potential injection from the github.event.pages[0].page_name context, which may be controlled by an external user. |
-| .github/workflows/gollum.yml:10:12:10:59 | echo '$ ... ame }}' | Potential injection from the github.event.pages[2222].page_name context, which may be controlled by an external user. |
-| .github/workflows/issues.yml:13:12:13:49 | echo '$ ... tle }}' | Potential injection from the github.event.issue.title context, which may be controlled by an external user. |
-| .github/workflows/issues.yml:14:12:14:48 | echo '$ ... ody }}' | Potential injection from the github.event.issue.body context, which may be controlled by an external user. |
-| .github/workflows/issues.yml:15:12:15:39 | echo '$ ... env }}' | Potential injection from the env.global_env context, which may be controlled by an external user. |
-| .github/workflows/issues.yml:17:12:17:36 | echo '$ ... env }}' | Potential injection from the env.job_env context, which may be controlled by an external user. |
-| .github/workflows/issues.yml:18:12:18:37 | echo '$ ... env }}' | Potential injection from the env.step_env context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review.yml:7:12:7:56 | echo '$ ... tle }}' | Potential injection from the github.event.pull_request.title context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review.yml:8:12:8:55 | echo '$ ... ody }}' | Potential injection from the github.event.pull_request.body context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review.yml:9:12:9:61 | echo '$ ... bel }}' | Potential injection from the github.event.pull_request.head.label context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review.yml:10:12:10:75 | echo '$ ... nch }}' | Potential injection from the github.event.pull_request.head.repo.default_branch context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review.yml:11:12:11:72 | echo '$ ... ion }}' | Potential injection from the github.event.pull_request.head.repo.description context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review.yml:12:12:12:69 | echo '$ ... age }}' | Potential injection from the github.event.pull_request.head.repo.homepage context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review.yml:13:12:13:59 | echo '$ ... ref }}' | Potential injection from the github.event.pull_request.head.ref context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review.yml:14:12:14:49 | echo '$ ... ody }}' | Potential injection from the github.event.review.body context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review_comment.yml:7:12:7:56 | echo '$ ... tle }}' | Potential injection from the github.event.pull_request.title context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review_comment.yml:8:12:8:55 | echo '$ ... ody }}' | Potential injection from the github.event.pull_request.body context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review_comment.yml:9:12:9:61 | echo '$ ... bel }}' | Potential injection from the github.event.pull_request.head.label context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review_comment.yml:10:12:10:75 | echo '$ ... nch }}' | Potential injection from the github.event.pull_request.head.repo.default_branch context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review_comment.yml:11:12:11:72 | echo '$ ... ion }}' | Potential injection from the github.event.pull_request.head.repo.description context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review_comment.yml:12:12:12:69 | echo '$ ... age }}' | Potential injection from the github.event.pull_request.head.repo.homepage context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review_comment.yml:13:12:13:59 | echo '$ ... ref }}' | Potential injection from the github.event.pull_request.head.ref context, which may be controlled by an external user. |
-| .github/workflows/pull_request_review_comment.yml:14:12:14:50 | echo '$ ... ody }}' | Potential injection from the github.event.comment.body context, which may be controlled by an external user. |
-| .github/workflows/pull_request_target.yml:9:12:9:56 | echo '$ ... tle }}' | Potential injection from the github.event.pull_request.title context, which may be controlled by an external user. |
-| .github/workflows/pull_request_target.yml:10:12:10:55 | echo '$ ... ody }}' | Potential injection from the github.event.pull_request.body context, which may be controlled by an external user. |
-| .github/workflows/pull_request_target.yml:11:12:11:61 | echo '$ ... bel }}' | Potential injection from the github.event.pull_request.head.label context, which may be controlled by an external user. |
-| .github/workflows/pull_request_target.yml:12:12:12:75 | echo '$ ... nch }}' | Potential injection from the github.event.pull_request.head.repo.default_branch context, which may be controlled by an external user. |
-| .github/workflows/pull_request_target.yml:13:12:13:72 | echo '$ ... ion }}' | Potential injection from the github.event.pull_request.head.repo.description context, which may be controlled by an external user. |
-| .github/workflows/pull_request_target.yml:14:12:14:69 | echo '$ ... age }}' | Potential injection from the github.event.pull_request.head.repo.homepage context, which may be controlled by an external user. |
-| .github/workflows/pull_request_target.yml:15:12:15:59 | echo '$ ... ref }}' | Potential injection from the github.event.pull_request.head.ref context, which may be controlled by an external user. |
-| .github/workflows/pull_request_target.yml:16:12:16:40 | echo '$ ... ref }}' | Potential injection from the github.head_ref context, which may be controlled by an external user. |
-| .github/workflows/push.yml:7:12:7:57 | echo '$ ... age }}' | Potential injection from the github.event.commits[11].message context, which may be controlled by an external user. |
-| .github/workflows/push.yml:8:12:8:62 | echo '$ ... ail }}' | Potential injection from the github.event.commits[11].author.email context, which may be controlled by an external user. |
-| .github/workflows/push.yml:9:12:9:61 | echo '$ ... ame }}' | Potential injection from the github.event.commits[11].author.name context, which may be controlled by an external user. |
-| .github/workflows/push.yml:10:12:10:57 | echo '$ ... age }}' | Potential injection from the github.event.head_commit.message context, which may be controlled by an external user. |
-| .github/workflows/push.yml:11:12:11:62 | echo '$ ... ail }}' | Potential injection from the github.event.head_commit.author.email context, which may be controlled by an external user. |
-| .github/workflows/push.yml:12:12:12:61 | echo '$ ... ame }}' | Potential injection from the github.event.head_commit.author.name context, which may be controlled by an external user. |
-| .github/workflows/push.yml:13:12:13:65 | echo '$ ... ail }}' | Potential injection from the github.event.head_commit.committer.email context, which may be controlled by an external user. |
-| .github/workflows/push.yml:14:12:14:64 | echo '$ ... ame }}' | Potential injection from the github.event.head_commit.committer.name context, which may be controlled by an external user. |
-| .github/workflows/push.yml:15:12:15:65 | echo '$ ... ail }}' | Potential injection from the github.event.commits[11].committer.email context, which may be controlled by an external user. |
-| .github/workflows/push.yml:16:12:16:64 | echo '$ ... ame }}' | Potential injection from the github.event.commits[11].committer.name context, which may be controlled by an external user. |
-| .github/workflows/workflow_run.yml:9:12:9:64 | echo '$ ... tle }}' | Potential injection from the github.event.workflow_run.display_title context, which may be controlled by an external user. |
-| .github/workflows/workflow_run.yml:10:12:10:70 | echo '$ ... age }}' | Potential injection from the github.event.workflow_run.head_commit.message context, which may be controlled by an external user. |
-| .github/workflows/workflow_run.yml:11:12:11:75 | echo '$ ... ail }}' | Potential injection from the github.event.workflow_run.head_commit.author.email context, which may be controlled by an external user. |
-| .github/workflows/workflow_run.yml:12:12:12:74 | echo '$ ... ame }}' | Potential injection from the github.event.workflow_run.head_commit.author.name context, which may be controlled by an external user. |
-| .github/workflows/workflow_run.yml:13:12:13:78 | echo '$ ... ail }}' | Potential injection from the github.event.workflow_run.head_commit.committer.email context, which may be controlled by an external user. |
-| .github/workflows/workflow_run.yml:14:12:14:77 | echo '$ ... ame }}' | Potential injection from the github.event.workflow_run.head_commit.committer.name context, which may be controlled by an external user. |
-| .github/workflows/workflow_run.yml:15:12:15:62 | echo '$ ... nch }}' | Potential injection from the github.event.workflow_run.head_branch context, which may be controlled by an external user. |
-| .github/workflows/workflow_run.yml:16:12:16:78 | echo '$ ... ion }}' | Potential injection from the github.event.workflow_run.head_repository.description context, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:7:12:8:48 | \| | Potential injection from the ${ github.event.comment.body }, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:13:12:13:50 | echo '$ ... ody }}' | Potential injection from the ${ github.event.comment.body }, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:14:12:14:48 | echo '$ ... ody }}' | Potential injection from the ${ github.event.issue.body }, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:15:12:15:49 | echo '$ ... tle }}' | Potential injection from the ${ github.event.issue.title }, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:22:17:22:63 | console ... dy }}') | Potential injection from the ${ github.event.comment.body }, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:25:17:25:61 | console ... dy }}') | Potential injection from the ${ github.event.issue.body }, which may be controlled by an external user. |
+| .github/workflows/comment_issue.yml:28:17:28:62 | console ... le }}') | Potential injection from the ${ github.event.issue.title }, which may be controlled by an external user. |
+| .github/workflows/comment_issue_newline.yml:9:14:10:50 | \| | Potential injection from the ${ github.event.comment.body }, which may be controlled by an external user. |
+| .github/workflows/discussion.yml:7:12:7:54 | echo '$ ... tle }}' | Potential injection from the ${ github.event.discussion.title }, which may be controlled by an external user. |
+| .github/workflows/discussion.yml:8:12:8:53 | echo '$ ... ody }}' | Potential injection from the ${ github.event.discussion.body }, which may be controlled by an external user. |
+| .github/workflows/discussion_comment.yml:7:12:7:54 | echo '$ ... tle }}' | Potential injection from the ${ github.event.discussion.title }, which may be controlled by an external user. |
+| .github/workflows/discussion_comment.yml:8:12:8:53 | echo '$ ... ody }}' | Potential injection from the ${ github.event.discussion.body }, which may be controlled by an external user. |
+| .github/workflows/discussion_comment.yml:9:12:9:50 | echo '$ ... ody }}' | Potential injection from the ${ github.event.comment.body }, which may be controlled by an external user. |
+| .github/workflows/gollum.yml:7:12:7:52 | echo '$ ... tle }}' | Potential injection from the ${ github.event.pages[1].title }, which may be controlled by an external user. |
+| .github/workflows/gollum.yml:8:12:8:53 | echo '$ ... tle }}' | Potential injection from the ${ github.event.pages[11].title }, which may be controlled by an external user. |
+| .github/workflows/gollum.yml:9:12:9:56 | echo '$ ... ame }}' | Potential injection from the ${ github.event.pages[0].page_name }, which may be controlled by an external user. |
+| .github/workflows/gollum.yml:10:12:10:59 | echo '$ ... ame }}' | Potential injection from the ${ github.event.pages[2222].page_name }, which may be controlled by an external user. |
+| .github/workflows/issues.yml:13:12:13:49 | echo '$ ... tle }}' | Potential injection from the ${ github.event.issue.title }, which may be controlled by an external user. |
+| .github/workflows/issues.yml:14:12:14:48 | echo '$ ... ody }}' | Potential injection from the ${ github.event.issue.body }, which may be controlled by an external user. |
+| .github/workflows/issues.yml:15:12:15:39 | echo '$ ... env }}' | Potential injection from the ${ env.global_env }, which may be controlled by an external user. |
+| .github/workflows/issues.yml:17:12:17:36 | echo '$ ... env }}' | Potential injection from the ${ env.job_env }, which may be controlled by an external user. |
+| .github/workflows/issues.yml:18:12:18:37 | echo '$ ... env }}' | Potential injection from the ${ env.step_env }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:7:12:7:56 | echo '$ ... tle }}' | Potential injection from the ${ github.event.pull_request.title }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:8:12:8:55 | echo '$ ... ody }}' | Potential injection from the ${ github.event.pull_request.body }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:9:12:9:61 | echo '$ ... bel }}' | Potential injection from the ${ github.event.pull_request.head.label }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:10:12:10:75 | echo '$ ... nch }}' | Potential injection from the ${ github.event.pull_request.head.repo.default_branch }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:11:12:11:72 | echo '$ ... ion }}' | Potential injection from the ${ github.event.pull_request.head.repo.description }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:12:12:12:69 | echo '$ ... age }}' | Potential injection from the ${ github.event.pull_request.head.repo.homepage }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:13:12:13:59 | echo '$ ... ref }}' | Potential injection from the ${ github.event.pull_request.head.ref }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review.yml:14:12:14:49 | echo '$ ... ody }}' | Potential injection from the ${ github.event.review.body }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:7:12:7:56 | echo '$ ... tle }}' | Potential injection from the ${ github.event.pull_request.title }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:8:12:8:55 | echo '$ ... ody }}' | Potential injection from the ${ github.event.pull_request.body }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:9:12:9:61 | echo '$ ... bel }}' | Potential injection from the ${ github.event.pull_request.head.label }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:10:12:10:75 | echo '$ ... nch }}' | Potential injection from the ${ github.event.pull_request.head.repo.default_branch }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:11:12:11:72 | echo '$ ... ion }}' | Potential injection from the ${ github.event.pull_request.head.repo.description }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:12:12:12:69 | echo '$ ... age }}' | Potential injection from the ${ github.event.pull_request.head.repo.homepage }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:13:12:13:59 | echo '$ ... ref }}' | Potential injection from the ${ github.event.pull_request.head.ref }, which may be controlled by an external user. |
+| .github/workflows/pull_request_review_comment.yml:14:12:14:50 | echo '$ ... ody }}' | Potential injection from the ${ github.event.comment.body }, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:9:12:9:56 | echo '$ ... tle }}' | Potential injection from the ${ github.event.pull_request.title }, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:10:12:10:55 | echo '$ ... ody }}' | Potential injection from the ${ github.event.pull_request.body }, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:11:12:11:61 | echo '$ ... bel }}' | Potential injection from the ${ github.event.pull_request.head.label }, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:12:12:12:75 | echo '$ ... nch }}' | Potential injection from the ${ github.event.pull_request.head.repo.default_branch }, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:13:12:13:72 | echo '$ ... ion }}' | Potential injection from the ${ github.event.pull_request.head.repo.description }, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:14:12:14:69 | echo '$ ... age }}' | Potential injection from the ${ github.event.pull_request.head.repo.homepage }, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:15:12:15:59 | echo '$ ... ref }}' | Potential injection from the ${ github.event.pull_request.head.ref }, which may be controlled by an external user. |
+| .github/workflows/pull_request_target.yml:16:12:16:40 | echo '$ ... ref }}' | Potential injection from the ${ github.head_ref }, which may be controlled by an external user. |
+| .github/workflows/push.yml:7:12:7:57 | echo '$ ... age }}' | Potential injection from the ${ github.event.commits[11].message }, which may be controlled by an external user. |
+| .github/workflows/push.yml:8:12:8:62 | echo '$ ... ail }}' | Potential injection from the ${ github.event.commits[11].author.email }, which may be controlled by an external user. |
+| .github/workflows/push.yml:9:12:9:61 | echo '$ ... ame }}' | Potential injection from the ${ github.event.commits[11].author.name }, which may be controlled by an external user. |
+| .github/workflows/push.yml:10:12:10:57 | echo '$ ... age }}' | Potential injection from the ${ github.event.head_commit.message }, which may be controlled by an external user. |
+| .github/workflows/push.yml:11:12:11:62 | echo '$ ... ail }}' | Potential injection from the ${ github.event.head_commit.author.email }, which may be controlled by an external user. |
+| .github/workflows/push.yml:12:12:12:61 | echo '$ ... ame }}' | Potential injection from the ${ github.event.head_commit.author.name }, which may be controlled by an external user. |
+| .github/workflows/push.yml:13:12:13:65 | echo '$ ... ail }}' | Potential injection from the ${ github.event.head_commit.committer.email }, which may be controlled by an external user. |
+| .github/workflows/push.yml:14:12:14:64 | echo '$ ... ame }}' | Potential injection from the ${ github.event.head_commit.committer.name }, which may be controlled by an external user. |
+| .github/workflows/push.yml:15:12:15:65 | echo '$ ... ail }}' | Potential injection from the ${ github.event.commits[11].committer.email }, which may be controlled by an external user. |
+| .github/workflows/push.yml:16:12:16:64 | echo '$ ... ame }}' | Potential injection from the ${ github.event.commits[11].committer.name }, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:9:12:9:64 | echo '$ ... tle }}' | Potential injection from the ${ github.event.workflow_run.display_title }, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:10:12:10:70 | echo '$ ... age }}' | Potential injection from the ${ github.event.workflow_run.head_commit.message }, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:11:12:11:75 | echo '$ ... ail }}' | Potential injection from the ${ github.event.workflow_run.head_commit.author.email }, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:12:12:12:74 | echo '$ ... ame }}' | Potential injection from the ${ github.event.workflow_run.head_commit.author.name }, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:13:12:13:78 | echo '$ ... ail }}' | Potential injection from the ${ github.event.workflow_run.head_commit.committer.email }, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:14:12:14:77 | echo '$ ... ame }}' | Potential injection from the ${ github.event.workflow_run.head_commit.committer.name }, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:15:12:15:62 | echo '$ ... nch }}' | Potential injection from the ${ github.event.workflow_run.head_branch }, which may be controlled by an external user. |
+| .github/workflows/workflow_run.yml:16:12:16:78 | echo '$ ... ion }}' | Potential injection from the ${ github.event.workflow_run.head_repository.description }, which may be controlled by an external user. |
From 40b7910473176ec6aa97d0eda4c50faca6b819fd Mon Sep 17 00:00:00 2001
From: jarlob
Date: Wed, 5 Apr 2023 10:14:54 +0200
Subject: [PATCH 014/704] Fix QLDoc warnings
---
javascript/ql/lib/semmle/javascript/Actions.qll | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/javascript/ql/lib/semmle/javascript/Actions.qll b/javascript/ql/lib/semmle/javascript/Actions.qll
index 3567414650b..85b313af8a3 100644
--- a/javascript/ql/lib/semmle/javascript/Actions.qll
+++ b/javascript/ql/lib/semmle/javascript/Actions.qll
@@ -63,7 +63,7 @@ module Actions {
abstract string getName();
}
- /** Workflow level 'global' environment variable. */
+ /** A workflow level 'global' environment variable. */
class GlobalEnv extends Env {
string envName;
Workflow workflow;
@@ -77,7 +77,7 @@ module Actions {
override string getName() { result = envName }
}
- /** Job level environment variable. */
+ /** A job level environment variable. */
class JobEnv extends Env {
string envName;
Job job;
@@ -91,7 +91,7 @@ module Actions {
override string getName() { result = envName }
}
- /** Step level environment variable. */
+ /** A step level environment variable. */
class StepEnv extends Env {
string envName;
Step step;
From 9fba7d31f1cb8183e6617f24052efa7b6d1bef4c Mon Sep 17 00:00:00 2001
From: jarlob
Date: Wed, 5 Apr 2023 10:24:07 +0200
Subject: [PATCH 015/704] Improve documentation
---
.../src/Security/CWE-094/ExpressionInjection.qhelp | 14 +++++++++++++-
.../CWE-094/examples/comment_issue_bad_env.yml | 10 ++++++++++
2 files changed, 23 insertions(+), 1 deletion(-)
create mode 100644 javascript/ql/src/Security/CWE-094/examples/comment_issue_bad_env.yml
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp b/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp
index 6e248eb380e..dbc1196ae66 100644
--- a/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp
+++ b/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp
@@ -21,7 +21,7 @@
The best practice to avoid code injection vulnerabilities
in GitHub workflows is to set the untrusted input value of the expression
to an intermediate environment variable and then use the environment variable
- using the native syntax of the shell/script interpreter (i.e. NOT the ${{ env.VAR }}).
+ using the native syntax of the shell/script interpreter (i.e. NOT the ${{ env.VAR }}).
It is also recommended to limit the permissions of any tokens used
@@ -40,6 +40,18 @@
the environment variable and will prevent the attack:
+
+
+ The following example uses an environment variable, but
+ still allows injection because of the use of expression syntax:
+
+
+
+
+ The following example uses shell syntax to read
+ the environment variable and will prevent the attack:
+
+
diff --git a/javascript/ql/src/Security/CWE-094/examples/comment_issue_bad_env.yml b/javascript/ql/src/Security/CWE-094/examples/comment_issue_bad_env.yml
new file mode 100644
index 00000000000..b7698938de7
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-094/examples/comment_issue_bad_env.yml
@@ -0,0 +1,10 @@
+on: issue_comment
+
+jobs:
+ echo-body:
+ runs-on: ubuntu-latest
+ steps:
+ - env:
+ BODY: ${{ github.event.issue.body }}
+ run: |
+ echo '${{ env.BODY }}'
\ No newline at end of file
From 40635e60d1df85f4deeabe4fc82f4509c6e414a0 Mon Sep 17 00:00:00 2001
From: jarlob
Date: Wed, 5 Apr 2023 10:26:02 +0200
Subject: [PATCH 016/704] Improve documentation
---
.../ql/src/Security/CWE-094/ExpressionInjection.qhelp | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp b/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp
index dbc1196ae66..d010a75a46b 100644
--- a/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp
+++ b/javascript/ql/src/Security/CWE-094/ExpressionInjection.qhelp
@@ -35,15 +35,9 @@
-
- The following example uses shell syntax to read
- the environment variable and will prevent the attack:
-
-
-
The following example uses an environment variable, but
- still allows injection because of the use of expression syntax:
+ still allows the injection because of the use of expression syntax:
From 0a878d4db945f59f9ca8e9840e67b87a9712d21b Mon Sep 17 00:00:00 2001
From: jarlob
Date: Thu, 6 Apr 2023 19:07:38 +0200
Subject: [PATCH 017/704] Support yAml extensions
---
javascript/ql/lib/semmle/javascript/Actions.qll | 2 +-
.../.github/workflows/{issues.yml => issues.yaml} | 0
2 files changed, 1 insertion(+), 1 deletion(-)
rename javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/{issues.yml => issues.yaml} (100%)
diff --git a/javascript/ql/lib/semmle/javascript/Actions.qll b/javascript/ql/lib/semmle/javascript/Actions.qll
index 85b313af8a3..d8378ea347b 100644
--- a/javascript/ql/lib/semmle/javascript/Actions.qll
+++ b/javascript/ql/lib/semmle/javascript/Actions.qll
@@ -16,7 +16,7 @@ module Actions {
this.getLocation()
.getFile()
.getRelativePath()
- .regexpMatch("(^|.*/)\\.github/workflows/.*\\.yml$")
+ .regexpMatch("(^|.*/)\\.github/workflows/.*\\.y(?:a?)ml$")
}
}
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yaml
similarity index 100%
rename from javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yml
rename to javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/.github/workflows/issues.yaml
From baefeab2d13f204368f0ec4c706acc5e6a4f940b Mon Sep 17 00:00:00 2001
From: jarlob
Date: Thu, 6 Apr 2023 19:11:04 +0200
Subject: [PATCH 018/704] fix tests
---
.../ExpressionInjection/ExpressionInjection.expected | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
index 457cb815e6a..21248e7f5b2 100644
--- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
@@ -15,11 +15,11 @@
| .github/workflows/gollum.yml:8:12:8:53 | echo '$ ... tle }}' | Potential injection from the ${ github.event.pages[11].title }, which may be controlled by an external user. |
| .github/workflows/gollum.yml:9:12:9:56 | echo '$ ... ame }}' | Potential injection from the ${ github.event.pages[0].page_name }, which may be controlled by an external user. |
| .github/workflows/gollum.yml:10:12:10:59 | echo '$ ... ame }}' | Potential injection from the ${ github.event.pages[2222].page_name }, which may be controlled by an external user. |
-| .github/workflows/issues.yml:13:12:13:49 | echo '$ ... tle }}' | Potential injection from the ${ github.event.issue.title }, which may be controlled by an external user. |
-| .github/workflows/issues.yml:14:12:14:48 | echo '$ ... ody }}' | Potential injection from the ${ github.event.issue.body }, which may be controlled by an external user. |
-| .github/workflows/issues.yml:15:12:15:39 | echo '$ ... env }}' | Potential injection from the ${ env.global_env }, which may be controlled by an external user. |
-| .github/workflows/issues.yml:17:12:17:36 | echo '$ ... env }}' | Potential injection from the ${ env.job_env }, which may be controlled by an external user. |
-| .github/workflows/issues.yml:18:12:18:37 | echo '$ ... env }}' | Potential injection from the ${ env.step_env }, which may be controlled by an external user. |
+| .github/workflows/issues.yaml:13:12:13:49 | echo '$ ... tle }}' | Potential injection from the ${ github.event.issue.title }, which may be controlled by an external user. |
+| .github/workflows/issues.yaml:14:12:14:48 | echo '$ ... ody }}' | Potential injection from the ${ github.event.issue.body }, which may be controlled by an external user. |
+| .github/workflows/issues.yaml:15:12:15:39 | echo '$ ... env }}' | Potential injection from the ${ env.global_env }, which may be controlled by an external user. |
+| .github/workflows/issues.yaml:17:12:17:36 | echo '$ ... env }}' | Potential injection from the ${ env.job_env }, which may be controlled by an external user. |
+| .github/workflows/issues.yaml:18:12:18:37 | echo '$ ... env }}' | Potential injection from the ${ env.step_env }, which may be controlled by an external user. |
| .github/workflows/pull_request_review.yml:7:12:7:56 | echo '$ ... tle }}' | Potential injection from the ${ github.event.pull_request.title }, which may be controlled by an external user. |
| .github/workflows/pull_request_review.yml:8:12:8:55 | echo '$ ... ody }}' | Potential injection from the ${ github.event.pull_request.body }, which may be controlled by an external user. |
| .github/workflows/pull_request_review.yml:9:12:9:61 | echo '$ ... bel }}' | Potential injection from the ${ github.event.pull_request.head.label }, which may be controlled by an external user. |
From 9c7eecf547e805f400d3d0334e1ca154ad0c4f49 Mon Sep 17 00:00:00 2001
From: jarlob
Date: Thu, 6 Apr 2023 22:53:59 +0200
Subject: [PATCH 019/704] Add support for composite actions
---
.../ql/lib/semmle/javascript/Actions.qll | 69 +++++--
.../Security/CWE-094/ExpressionInjection.ql | 172 ++++++++++++------
.../ExpressionInjection.expected | 1 +
.../CWE-094/ExpressionInjection/action.yml | 14 ++
4 files changed, 184 insertions(+), 72 deletions(-)
create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/action.yml
diff --git a/javascript/ql/lib/semmle/javascript/Actions.qll b/javascript/ql/lib/semmle/javascript/Actions.qll
index d8378ea347b..6faeeb15987 100644
--- a/javascript/ql/lib/semmle/javascript/Actions.qll
+++ b/javascript/ql/lib/semmle/javascript/Actions.qll
@@ -10,16 +10,62 @@ import javascript
* See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions.
*/
module Actions {
- /** A YAML node in a GitHub Actions workflow file. */
+ /** A YAML node in a GitHub Actions workflow or custom action file. */
private class Node extends YamlNode {
Node() {
- this.getLocation()
- .getFile()
- .getRelativePath()
- .regexpMatch("(^|.*/)\\.github/workflows/.*\\.y(?:a?)ml$")
+ exists(File f |
+ f = this.getLocation().getFile() and
+ (
+ f.getRelativePath().regexpMatch("(^|.*/)\\.github/workflows/.*\\.y(?:a?)ml$")
+ or
+ f.getBaseName() = "action.yml"
+ )
+ )
}
}
+ /**
+ * A custom action. This is a mapping at the top level of an Actions YAML action file.
+ * See https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions.
+ */
+ class Action extends Node, YamlDocument, YamlMapping {
+ /** Gets the `runs` mapping. */
+ Runs getRuns() { result = this.lookup("runs") }
+ }
+
+ /**
+ * An `runs` mapping in a custom action YAML.
+ * See https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#runs
+ */
+ class Runs extends StepsContainer {
+ Action action;
+
+ Runs() { action.lookup("runs") = this }
+
+ /** Gets the action that this `runs` mapping is in. */
+ Action getAction() { result = action }
+ }
+
+ /**
+ * The parent class of the class that can contain `steps` mappings. (`Job` or `Runs` currently.)
+ */
+ abstract class StepsContainer extends YamlNode, YamlMapping {
+ /** Gets the sequence of `steps` within this YAML node. */
+ YamlSequence getSteps() { result = this.lookup("steps") }
+ }
+
+ /**
+ * A `using` mapping in a custom action YAML.
+ */
+ class Using extends YamlNode, YamlScalar {
+ Runs runs;
+
+ Using() { runs.lookup("using") = this }
+
+ /** Gets the `runs` mapping that this `using` mapping is in. */
+ Runs getRuns() { result = runs }
+ }
+
/**
* An Actions workflow. This is a mapping at the top level of an Actions YAML workflow file.
* See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions.
@@ -109,7 +155,7 @@ module Actions {
* An Actions job within a workflow.
* See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobs.
*/
- class Job extends YamlNode, YamlMapping {
+ class Job extends StepsContainer {
string jobId;
Workflow workflow;
@@ -136,9 +182,6 @@ module Actions {
/** Gets the step at the given index within this job. */
Step getStep(int index) { result.getJob() = this and result.getIndex() = index }
- /** Gets the sequence of `steps` within this job. */
- YamlSequence getSteps() { result = this.lookup("steps") }
-
/** Gets the `env` mapping in this job. */
YamlMapping getEnv() { result = this.lookup("env") }
@@ -184,15 +227,17 @@ module Actions {
*/
class Step extends YamlNode, YamlMapping {
int index;
- Job job;
+ StepsContainer parent;
- Step() { this = job.getSteps().getElement(index) }
+ Step() { this = parent.getSteps().getElement(index) }
/** Gets the 0-based position of this step within the sequence of `steps`. */
int getIndex() { result = index }
/** Gets the job this step belongs to. */
- Job getJob() { result = job }
+ Job getJob() { result = parent.(Job) }
+
+ Runs getRuns() { result = parent.(Runs) }
/** Gets the value of the `uses` field in this step, if any. */
Uses getUses() { result.getStep() = this }
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
index 056d7204551..696fe0a0186 100644
--- a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
+++ b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
@@ -103,70 +103,122 @@ private predicate isExternalUserControlledWorkflowRun(string context) {
)
}
-from YamlNode node, string injection, string context, Actions::On on
+/**
+ * The env variable name in `${{ env.name }}`
+ * is where the external user controlled value was assigned to.
+ */
+bindingset[injection]
+predicate isEnvTainted(Actions::Env env, string injection, string context) {
+ Actions::getEnvName(injection) = env.getName() and
+ Actions::getASimpleReferenceExpression(env) = context
+}
+
+/**
+ * Holds if the `run` contains any expression interpolation `${{ e }}`.
+ * Sets `context` to the initial untrusted value assignment in case of `${{ env... }}` interpolation
+ */
+predicate isRunInjectable(Actions::Run run, string injection, string context) {
+ Actions::getASimpleReferenceExpression(run) = injection and
+ (
+ injection = context
+ or
+ exists(Actions::Env env | isEnvTainted(env, injection, context))
+ )
+}
+
+/**
+ * Holds if the `actions/github-script` contains any expression interpolation `${{ e }}`.
+ * Sets `context` to the initial untrusted value assignment in case of `${{ env... }}` interpolation
+ */
+predicate isScriptInjectable(Actions::Script script, string injection, string context) {
+ exists(Actions::Step step, Actions::Uses uses |
+ script.getWith().getStep() = step and
+ uses.getStep() = step and
+ uses.getGitHubRepository() = "actions/github-script" and
+ Actions::getASimpleReferenceExpression(script) = injection and
+ (
+ injection = context
+ or
+ exists(Actions::Env env | isEnvTainted(env, injection, context))
+ )
+ )
+}
+
+from YamlNode node, string injection, string context
where
- (
- exists(Actions::Run run |
- node = run and
- Actions::getASimpleReferenceExpression(run) = injection and
- run.getStep().getJob().getWorkflow().getOn() = on and
- (
- injection = context
- or
- exists(Actions::Env env |
- Actions::getEnvName(injection) = env.getName() and
- Actions::getASimpleReferenceExpression(env) = context
- )
+ exists(Actions::Using u, Actions::Runs runs |
+ u.getValue() = "composite" and
+ u.getRuns() = runs and
+ (
+ exists(Actions::Run run |
+ isRunInjectable(run, injection, context) and
+ node = run and
+ run.getStep().getRuns() = runs
)
- )
- or
- exists(Actions::Script script, Actions::Step step, Actions::Uses uses |
- node = script and
- script.getWith().getStep().getJob().getWorkflow().getOn() = on and
- script.getWith().getStep() = step and
- uses.getStep() = step and
- uses.getGitHubRepository() = "actions/github-script" and
- Actions::getASimpleReferenceExpression(script) = injection and
- (
- injection = context
- or
- exists(Actions::Env env |
- Actions::getEnvName(injection) = env.getName() and
- Actions::getASimpleReferenceExpression(env) = context
- )
+ or
+ exists(Actions::Script script |
+ node = script and
+ script.getWith().getStep().getRuns() = runs and
+ isScriptInjectable(script, injection, context)
)
+ ) and
+ (
+ isExternalUserControlledIssue(context) or
+ isExternalUserControlledPullRequest(context) or
+ isExternalUserControlledReview(context) or
+ isExternalUserControlledComment(context) or
+ isExternalUserControlledGollum(context) or
+ isExternalUserControlledCommit(context) or
+ isExternalUserControlledDiscussion(context) or
+ isExternalUserControlledWorkflowRun(context)
+ )
+ )
+ or
+ exists(Actions::On on |
+ (
+ exists(Actions::Run run |
+ isRunInjectable(run, injection, context) and
+ node = run and
+ run.getStep().getJob().getWorkflow().getOn() = on
+ )
+ or
+ exists(Actions::Script script |
+ node = script and
+ script.getWith().getStep().getJob().getWorkflow().getOn() = on and
+ isScriptInjectable(script, injection, context)
+ )
+ ) and
+ (
+ exists(on.getNode("issues")) and
+ isExternalUserControlledIssue(context)
+ or
+ exists(on.getNode("pull_request_target")) and
+ isExternalUserControlledPullRequest(context)
+ or
+ exists(on.getNode("pull_request_review")) and
+ (isExternalUserControlledReview(context) or isExternalUserControlledPullRequest(context))
+ or
+ exists(on.getNode("pull_request_review_comment")) and
+ (isExternalUserControlledComment(context) or isExternalUserControlledPullRequest(context))
+ or
+ exists(on.getNode("issue_comment")) and
+ (isExternalUserControlledComment(context) or isExternalUserControlledIssue(context))
+ or
+ exists(on.getNode("gollum")) and
+ isExternalUserControlledGollum(context)
+ or
+ exists(on.getNode("push")) and
+ isExternalUserControlledCommit(context)
+ or
+ exists(on.getNode("discussion")) and
+ isExternalUserControlledDiscussion(context)
+ or
+ exists(on.getNode("discussion_comment")) and
+ (isExternalUserControlledDiscussion(context) or isExternalUserControlledComment(context))
+ or
+ exists(on.getNode("workflow_run")) and
+ isExternalUserControlledWorkflowRun(context)
)
- ) and
- (
- exists(on.getNode("issues")) and
- isExternalUserControlledIssue(context)
- or
- exists(on.getNode("pull_request_target")) and
- isExternalUserControlledPullRequest(context)
- or
- exists(on.getNode("pull_request_review")) and
- (isExternalUserControlledReview(context) or isExternalUserControlledPullRequest(context))
- or
- exists(on.getNode("pull_request_review_comment")) and
- (isExternalUserControlledComment(context) or isExternalUserControlledPullRequest(context))
- or
- exists(on.getNode("issue_comment")) and
- (isExternalUserControlledComment(context) or isExternalUserControlledIssue(context))
- or
- exists(on.getNode("gollum")) and
- isExternalUserControlledGollum(context)
- or
- exists(on.getNode("push")) and
- isExternalUserControlledCommit(context)
- or
- exists(on.getNode("discussion")) and
- isExternalUserControlledDiscussion(context)
- or
- exists(on.getNode("discussion_comment")) and
- (isExternalUserControlledDiscussion(context) or isExternalUserControlledComment(context))
- or
- exists(on.getNode("workflow_run")) and
- isExternalUserControlledWorkflowRun(context)
)
select node,
"Potential injection from the ${ " + injection +
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
index 21248e7f5b2..76ad174e16a 100644
--- a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.expected
@@ -62,3 +62,4 @@
| .github/workflows/workflow_run.yml:14:12:14:77 | echo '$ ... ame }}' | Potential injection from the ${ github.event.workflow_run.head_commit.committer.name }, which may be controlled by an external user. |
| .github/workflows/workflow_run.yml:15:12:15:62 | echo '$ ... nch }}' | Potential injection from the ${ github.event.workflow_run.head_branch }, which may be controlled by an external user. |
| .github/workflows/workflow_run.yml:16:12:16:78 | echo '$ ... ion }}' | Potential injection from the ${ github.event.workflow_run.head_repository.description }, which may be controlled by an external user. |
+| action.yml:14:12:14:50 | echo '$ ... ody }}' | Potential injection from the ${ github.event.comment.body }, which may be controlled by an external user. |
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/action.yml b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/action.yml
new file mode 100644
index 00000000000..e9a178cf3db
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/action.yml
@@ -0,0 +1,14 @@
+name: 'test'
+description: 'test'
+branding:
+ icon: 'test'
+ color: 'test'
+inputs:
+ test:
+ description: test
+ required: false
+ default: 'test'
+runs:
+ using: "composite"
+ steps:
+ - run: echo '${{ github.event.comment.body }}'
\ No newline at end of file
From af83d8af415fb6bae60cad0d728201af20cd347a Mon Sep 17 00:00:00 2001
From: jarlob
Date: Thu, 6 Apr 2023 22:59:09 +0200
Subject: [PATCH 020/704] Add comment
---
javascript/ql/lib/semmle/javascript/Actions.qll | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/javascript/ql/lib/semmle/javascript/Actions.qll b/javascript/ql/lib/semmle/javascript/Actions.qll
index 6faeeb15987..f0585b915b4 100644
--- a/javascript/ql/lib/semmle/javascript/Actions.qll
+++ b/javascript/ql/lib/semmle/javascript/Actions.qll
@@ -234,9 +234,10 @@ module Actions {
/** Gets the 0-based position of this step within the sequence of `steps`. */
int getIndex() { result = index }
- /** Gets the job this step belongs to. */
+ /** Gets the `job` this step belongs to. The step may belong to a `job` in a workflow or `runs` in a custom action. */
Job getJob() { result = parent.(Job) }
+ /** Gets the `runs` this step belongs to. The step may belong to a `job` in a workflow or `runs` in a custom action. */
Runs getRuns() { result = parent.(Runs) }
/** Gets the value of the `uses` field in this step, if any. */
From 3745ccceddaa0dd5dc2d34fa54f30bf08f9a89c1 Mon Sep 17 00:00:00 2001
From: jarlob
Date: Thu, 6 Apr 2023 23:02:08 +0200
Subject: [PATCH 021/704] Fix warnings
---
javascript/ql/lib/semmle/javascript/Actions.qll | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/javascript/ql/lib/semmle/javascript/Actions.qll b/javascript/ql/lib/semmle/javascript/Actions.qll
index f0585b915b4..50b11b18df6 100644
--- a/javascript/ql/lib/semmle/javascript/Actions.qll
+++ b/javascript/ql/lib/semmle/javascript/Actions.qll
@@ -10,7 +10,7 @@ import javascript
* See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions.
*/
module Actions {
- /** A YAML node in a GitHub Actions workflow or custom action file. */
+ /** A YAML node in a GitHub Actions workflow or a custom action file. */
private class Node extends YamlNode {
Node() {
exists(File f |
@@ -235,10 +235,10 @@ module Actions {
int getIndex() { result = index }
/** Gets the `job` this step belongs to. The step may belong to a `job` in a workflow or `runs` in a custom action. */
- Job getJob() { result = parent.(Job) }
+ Job getJob() { result = parent }
/** Gets the `runs` this step belongs to. The step may belong to a `job` in a workflow or `runs` in a custom action. */
- Runs getRuns() { result = parent.(Runs) }
+ Runs getRuns() { result = parent }
/** Gets the value of the `uses` field in this step, if any. */
Uses getUses() { result.getStep() = this }
From 7573c615f6dff90cb154285f1b8be0027ee87f48 Mon Sep 17 00:00:00 2001
From: jarlob
Date: Thu, 6 Apr 2023 23:07:22 +0200
Subject: [PATCH 022/704] Fix warnings
---
javascript/ql/src/Security/CWE-094/ExpressionInjection.ql | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
index 696fe0a0186..9e703d2d47c 100644
--- a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
+++ b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
@@ -104,7 +104,7 @@ private predicate isExternalUserControlledWorkflowRun(string context) {
}
/**
- * The env variable name in `${{ env.name }}`
+ * Holds if the env variable name in `${{ env.name }}`
* is where the external user controlled value was assigned to.
*/
bindingset[injection]
@@ -122,7 +122,7 @@ predicate isRunInjectable(Actions::Run run, string injection, string context) {
(
injection = context
or
- exists(Actions::Env env | isEnvTainted(env, injection, context))
+ isEnvTainted(_, injection, context)
)
}
@@ -139,7 +139,7 @@ predicate isScriptInjectable(Actions::Script script, string injection, string co
(
injection = context
or
- exists(Actions::Env env | isEnvTainted(env, injection, context))
+ isEnvTainted(_, injection, context)
)
)
}
From 72b66ffe97f153902d2b6483fec3e0bb9196c3bb Mon Sep 17 00:00:00 2001
From: jarlob
Date: Fri, 7 Apr 2023 10:01:14 +0200
Subject: [PATCH 023/704] Fix comment.
---
.../ql/src/experimental/Security/CWE-094/UntrustedCheckout.ql | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/javascript/ql/src/experimental/Security/CWE-094/UntrustedCheckout.ql b/javascript/ql/src/experimental/Security/CWE-094/UntrustedCheckout.ql
index a81c8c65d25..71171af6ead 100644
--- a/javascript/ql/src/experimental/Security/CWE-094/UntrustedCheckout.ql
+++ b/javascript/ql/src/experimental/Security/CWE-094/UntrustedCheckout.ql
@@ -53,7 +53,7 @@ class ProbableJob extends Actions::Job {
}
/**
- * on: pull_request_target
+ * The `on: pull_request_target`.
*/
class ProbablePullRequestTarget extends Actions::On, YamlMappingLikeNode {
ProbablePullRequestTarget() {
From 9e3d57d442551ed3695468ca9e1dd5a1a088a01e Mon Sep 17 00:00:00 2001
From: yoff
Date: Tue, 11 Apr 2023 14:34:40 +0200
Subject: [PATCH 024/704] Update
python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py
Co-authored-by: Rasmus Wriedt Larsen
---
.../ApiGraphs/py3/test_captured_flask.py | 14 +-------------
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py b/python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py
index 48e0202e5de..9bf9ee4d598 100644
--- a/python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py
+++ b/python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py
@@ -11,16 +11,4 @@ def create_app():
@app.route('/v2/user/', methods=['GET','PUT'])
def users(id):
- if 'Authorization-Token' not in request.headers:
- return make_response(jsonify({'Error':'Authorization-Token header is not set'}),403)
-
- token = request.headers.get('Authorization-Token')
- sid = check_token(token)
-
- #if we don't have a valid session send 403
- if not sid:
- return make_response(jsonify({'Error':'Token check failed: {0}'.format(sid)}))
- try:
- user = Users.query.filter_by(id=id).first() #$ use=moduleImport("flask_sqlalchemy").getMember("SQLAlchemy").getReturn().getMember("Model").getASubclass().getMember("query").getMember("filter_by")
- except Exception as e:
- return make_response(jsonify({'error':str(e)}),500)
+ Users.query.filter_by(id=id).first() #$ use=moduleImport("flask_sqlalchemy").getMember("SQLAlchemy").getReturn().getMember("Model").getASubclass().getMember("query").getMember("filter_by")
From 820db43945feb799928fbae4ed5253d030833dc7 Mon Sep 17 00:00:00 2001
From: Maiky <76447395+maikypedia@users.noreply.github.com>
Date: Thu, 13 Apr 2023 17:21:31 +0200
Subject: [PATCH 025/704] Add ERB Template Injection Sink
---
ruby/ql/lib/codeql/ruby/frameworks/Rails.qll | 16 ++++++++++++++++
.../TemplateInjection/ErbInjection.rb | 7 +++++++
.../TemplateInjection/TemplateInjection.expected | 3 +++
3 files changed, 26 insertions(+)
diff --git a/ruby/ql/lib/codeql/ruby/frameworks/Rails.qll b/ruby/ql/lib/codeql/ruby/frameworks/Rails.qll
index b2a9fef3c1c..42d038a303d 100644
--- a/ruby/ql/lib/codeql/ruby/frameworks/Rails.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/Rails.qll
@@ -400,3 +400,19 @@ private class AccessLocalsKeySummary extends SummarizedCallable {
preservesValue = true
}
}
+
+/** A call to `render inline: foo`, considered as a ERB template rendering. */
+private class RailsTemplateRendering extends TemplateRendering::Range, DataFlow::CallNode {
+ private DataFlow::Node template;
+
+ RailsTemplateRendering() {
+ (
+ this.asExpr().getExpr() instanceof Rails::RenderCall
+ or
+ this.asExpr().getExpr() instanceof Rails::RenderToCall
+ ) and
+ template = this.getKeywordArgument("inline")
+ }
+
+ override DataFlow::Node getTemplate() { result = template }
+}
diff --git a/ruby/ql/test/query-tests/experimental/TemplateInjection/ErbInjection.rb b/ruby/ql/test/query-tests/experimental/TemplateInjection/ErbInjection.rb
index f202fc146a7..41b9d706953 100644
--- a/ruby/ql/test/query-tests/experimental/TemplateInjection/ErbInjection.rb
+++ b/ruby/ql/test/query-tests/experimental/TemplateInjection/ErbInjection.rb
@@ -14,6 +14,10 @@ class FooController < ActionController::Base
# where name is unsanitized
template = ERB.new(bad_text).result(binding)
+ # BAD: user input is evaluated
+ # where name is unsanitized
+ render inline: bad_text
+
# Template with the source
good_text = "
@@ -22,6 +26,9 @@ class FooController < ActionController::Base
# GOOD: user input is not evaluated
template2 = ERB.new(good_text).result(binding)
+
+ # GOOD: user input is not evaluated
+ render inline: good_text
end
end
diff --git a/ruby/ql/test/query-tests/experimental/TemplateInjection/TemplateInjection.expected b/ruby/ql/test/query-tests/experimental/TemplateInjection/TemplateInjection.expected
index a3e20d71b20..d7a76ef930a 100644
--- a/ruby/ql/test/query-tests/experimental/TemplateInjection/TemplateInjection.expected
+++ b/ruby/ql/test/query-tests/experimental/TemplateInjection/TemplateInjection.expected
@@ -4,6 +4,7 @@ edges
| ErbInjection.rb:5:12:5:17 | call to params : | ErbInjection.rb:5:12:5:24 | ...[...] : |
| ErbInjection.rb:5:12:5:24 | ...[...] : | ErbInjection.rb:5:5:5:8 | name : |
| ErbInjection.rb:8:5:8:12 | bad_text : | ErbInjection.rb:15:24:15:31 | bad_text |
+| ErbInjection.rb:8:5:8:12 | bad_text : | ErbInjection.rb:19:20:19:27 | bad_text |
| ErbInjection.rb:8:16:11:14 | ... % ... : | ErbInjection.rb:8:5:8:12 | bad_text : |
| ErbInjection.rb:11:11:11:14 | name : | ErbInjection.rb:8:16:11:14 | ... % ... : |
| SlimInjection.rb:5:5:5:8 | name : | SlimInjection.rb:8:5:8:12 | bad_text : |
@@ -23,6 +24,7 @@ nodes
| ErbInjection.rb:8:16:11:14 | ... % ... : | semmle.label | ... % ... : |
| ErbInjection.rb:11:11:11:14 | name : | semmle.label | name : |
| ErbInjection.rb:15:24:15:31 | bad_text | semmle.label | bad_text |
+| ErbInjection.rb:19:20:19:27 | bad_text | semmle.label | bad_text |
| SlimInjection.rb:5:5:5:8 | name : | semmle.label | name : |
| SlimInjection.rb:5:12:5:17 | call to params : | semmle.label | call to params : |
| SlimInjection.rb:5:12:5:24 | ...[...] : | semmle.label | ...[...] : |
@@ -35,5 +37,6 @@ nodes
subpaths
#select
| ErbInjection.rb:15:24:15:31 | bad_text | ErbInjection.rb:5:12:5:17 | call to params : | ErbInjection.rb:15:24:15:31 | bad_text | This template depends on a $@. | ErbInjection.rb:5:12:5:17 | call to params | user-provided value |
+| ErbInjection.rb:19:20:19:27 | bad_text | ErbInjection.rb:5:12:5:17 | call to params : | ErbInjection.rb:19:20:19:27 | bad_text | This template depends on a $@. | ErbInjection.rb:5:12:5:17 | call to params | user-provided value |
| SlimInjection.rb:14:25:14:32 | bad_text | SlimInjection.rb:5:12:5:17 | call to params : | SlimInjection.rb:14:25:14:32 | bad_text | This template depends on a $@. | SlimInjection.rb:5:12:5:17 | call to params | user-provided value |
| SlimInjection.rb:23:25:23:33 | bad2_text | SlimInjection.rb:5:12:5:17 | call to params : | SlimInjection.rb:23:25:23:33 | bad2_text | This template depends on a $@. | SlimInjection.rb:5:12:5:17 | call to params | user-provided value |
From 64cf3adfd4e28441e11a2321f67b73e718ae6aba Mon Sep 17 00:00:00 2001
From: Maiky <76447395+maikypedia@users.noreply.github.com>
Date: Thu, 13 Apr 2023 17:29:14 +0200
Subject: [PATCH 026/704] Update examples
---
ruby/ql/src/experimental/template-injection/examples/SSTIBad.rb | 1 +
ruby/ql/src/experimental/template-injection/examples/SSTIGood.rb | 1 +
2 files changed, 2 insertions(+)
diff --git a/ruby/ql/src/experimental/template-injection/examples/SSTIBad.rb b/ruby/ql/src/experimental/template-injection/examples/SSTIBad.rb
index 6ea7f4ed8c2..c464dfae2bf 100644
--- a/ruby/ql/src/experimental/template-injection/examples/SSTIBad.rb
+++ b/ruby/ql/src/experimental/template-injection/examples/SSTIBad.rb
@@ -9,6 +9,7 @@ class BadERBController < ActionController::Base
Hello %s
" % name
template = ERB.new(html_text).result(binding)
+ render inline: html_text
end
end
diff --git a/ruby/ql/src/experimental/template-injection/examples/SSTIGood.rb b/ruby/ql/src/experimental/template-injection/examples/SSTIGood.rb
index 844f0115d7c..f2d33378968 100644
--- a/ruby/ql/src/experimental/template-injection/examples/SSTIGood.rb
+++ b/ruby/ql/src/experimental/template-injection/examples/SSTIGood.rb
@@ -9,6 +9,7 @@ class GoodController < ActionController::Base
Hello <%= name %>