From ebaac5f5cb16ec9aba60e2fdc75bba13b08811ab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Wed, 14 Feb 2024 14:03:11 +0100
Subject: [PATCH] fix: enforce input,output,env prefixes in MaD

---
 .../codeql/actions/dataflow/ExternalFlow.qll  | 26 +++++++++++--------
 ql/lib/ext/PLACEHOLDER.model.yml              |  7 +++++
 .../frabert_replace-string-action.model.yml   |  4 +--
 ..._actions-find-and-replace-string.model.yml |  4 +--
 4 files changed, 26 insertions(+), 15 deletions(-)
 create mode 100644 ql/lib/ext/PLACEHOLDER.model.yml

diff --git a/ql/lib/codeql/actions/dataflow/ExternalFlow.qll b/ql/lib/codeql/actions/dataflow/ExternalFlow.qll
index 38b964110c7..6446fbb5572 100644
--- a/ql/lib/codeql/actions/dataflow/ExternalFlow.qll
+++ b/ql/lib/codeql/actions/dataflow/ExternalFlow.qll
@@ -50,22 +50,22 @@ predicate externallyDefinedSource(DataFlow::Node source, string sourceType, stri
     ) and
     (
       if fieldName.trim().matches("env.%")
-      then source.asExpr() = uses.getEnvExpr(fieldName.trim().replaceAll("env\\.", ""))
+      then source.asExpr() = uses.getEnvExpr(fieldName.trim().replaceAll("env.", ""))
       else
         if fieldName.trim().matches("output.%")
-        then
-          // 'output.' is the default qualifier
-          source.asExpr() = uses
+        then source.asExpr() = uses
         else none()
     ) and
     sourceType = kind
   )
 }
 
-predicate externallyDefinedSummary(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) {
+predicate externallyDefinedStoreStep(
+  DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c
+) {
   exists(UsesExpr uses, string action, string version, string input, string output |
-    c = any(DataFlow::FieldContent ct | ct.getName() = output.replaceAll("output\\.", "")) and
     summaryModel(action, version, input, output, "taint") and
+    c = any(DataFlow::FieldContent ct | ct.getName() = output.replaceAll("output.", "")) and
     uses.getCallee() = action.toLowerCase() and
     (
       if version.trim() = "*"
@@ -74,10 +74,11 @@ predicate externallyDefinedSummary(DataFlow::Node pred, DataFlow::Node succ, Dat
     ) and
     (
       if input.trim().matches("env.%")
-      then pred.asExpr() = uses.getEnvExpr(input.trim().replaceAll("env\\.", ""))
+      then pred.asExpr() = uses.getEnvExpr(input.trim().replaceAll("env.", ""))
       else
-        // 'input.' is the default qualifier
-        pred.asExpr() = uses.getArgumentExpr(input.trim().replaceAll("input\\.", ""))
+        if input.trim().matches("input.%")
+        then pred.asExpr() = uses.getArgumentExpr(input.trim().replaceAll("input.", ""))
+        else none()
     ) and
     succ.asExpr() = uses
   )
@@ -87,8 +88,11 @@ predicate externallyDefinedSink(DataFlow::ExprNode sink, string kind) {
   exists(UsesExpr uses, string action, string version, string input |
     (
       if input.trim().matches("env.%")
-      then sink.asExpr() = uses.getEnvExpr(input.trim().replaceAll("input\\.", ""))
-      else sink.asExpr() = uses.getArgumentExpr(input.trim())
+      then sink.asExpr() = uses.getEnvExpr(input.trim().replaceAll("env.", ""))
+      else
+        if input.trim().matches("input.%")
+        then sink.asExpr() = uses.getArgumentExpr(input.trim().replaceAll("input.", ""))
+        else none()
     ) and
     sinkModel(action, version, input, kind) and
     uses.getCallee() = action.toLowerCase() and
diff --git a/ql/lib/ext/PLACEHOLDER.model.yml b/ql/lib/ext/PLACEHOLDER.model.yml
new file mode 100644
index 00000000000..ef916067967
--- /dev/null
+++ b/ql/lib/ext/PLACEHOLDER.model.yml
@@ -0,0 +1,7 @@
+extensions:
+  - addsTo:
+      pack: codeql/actions-all
+      extensible: sinkModel
+    data:
+      - ["","","",""]
+
diff --git a/ql/lib/ext/frabert_replace-string-action.model.yml b/ql/lib/ext/frabert_replace-string-action.model.yml
index 76ce81b394e..79fd5c76e4a 100644
--- a/ql/lib/ext/frabert_replace-string-action.model.yml
+++ b/ql/lib/ext/frabert_replace-string-action.model.yml
@@ -3,5 +3,5 @@ extensions:
       pack: codeql/actions-all
       extensible: summaryModel
     data:
-      - ["frabert/replace-string-action", "*", "string", "replaced", "taint"]
-      - ["frabert/replace-string-action", "*", "replace-with", "replaced", "taint"]
+      - ["frabert/replace-string-action", "*", "input.string", "output.replaced", "taint"]
+      - ["frabert/replace-string-action", "*", "input.replace-with", "output.replaced", "taint"]
diff --git a/ql/lib/ext/mad9000_actions-find-and-replace-string.model.yml b/ql/lib/ext/mad9000_actions-find-and-replace-string.model.yml
index 46a577d2f7e..332527813a4 100644
--- a/ql/lib/ext/mad9000_actions-find-and-replace-string.model.yml
+++ b/ql/lib/ext/mad9000_actions-find-and-replace-string.model.yml
@@ -3,5 +3,5 @@ extensions:
       pack: codeql/actions-all
       extensible: summaryModel
     data:
-      - ["mad9000/actions-find-and-replace-string", "*", "source", "value", "taint"]
-      - ["mad9000/actions-find-and-replace-string", "*", "replace", "value", "taint"]
\ No newline at end of file
+      - ["mad9000/actions-find-and-replace-string", "*", "input.source", "output.value", "taint"]
+      - ["mad9000/actions-find-and-replace-string", "*", "input.replace", "output.value", "taint"]