From 010d7df71d36ec98e41f03827fcc14fc949ad1b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 20 Feb 2024 11:58:54 +0100 Subject: [PATCH] feat(reusable-workflow-models): Reusable workflow MaD Add support to define sources/sinks/summaries for Reusable Workflows as MaD entries. --- .../actions/controlflow/internal/Cfg.qll | 16 ++------------- .../dataflow/internal/DataFlowPrivate.qll | 20 ++++++++++++++++++- .../dataflow/internal/DataFlowPublic.qll | 11 ++++++++++ ql/lib/ext/TEST-RW-MODELS.model.yml | 17 ++++++++++++++++ ql/lib/test/test.ql | 13 ++++++------ .../.github/workflows/calling_workflow.yml | 16 +++++++++++---- 6 files changed, 67 insertions(+), 26 deletions(-) create mode 100644 ql/lib/ext/TEST-RW-MODELS.model.yml diff --git a/ql/lib/codeql/actions/controlflow/internal/Cfg.qll b/ql/lib/codeql/actions/controlflow/internal/Cfg.qll index 8808fb0afe5..94a2c6a71e2 100644 --- a/ql/lib/codeql/actions/controlflow/internal/Cfg.qll +++ b/ql/lib/codeql/actions/controlflow/internal/Cfg.qll @@ -243,21 +243,9 @@ private class JobTree extends StandardPreOrderTree instanceof JobStmt { } } -private class StepUsesTree extends StandardPreOrderTree instanceof StepUsesExpr { - override ControlFlowTree getChildNode(int i) { - result = - rank[i](Expression child, Location l | - (child = super.getArgumentExpr(_) or child = super.getEnvExpr(_)) and - l = child.getLocation() - | - child - order by - l.getStartLine(), l.getStartColumn(), l.getEndColumn(), l.getEndLine(), child.toString() - ) - } -} +private class UsesExprTree extends LeafTree instanceof UsesExpr { } -private class JobUsesTree extends StandardPreOrderTree instanceof JobUsesExpr { +private class UsesTree extends StandardPreOrderTree instanceof UsesExpr { override ControlFlowTree getChildNode(int i) { result = rank[i](Expression child, Location l | diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll index 89f31983189..e1a3479cfc0 100644 --- a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll +++ b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll @@ -58,7 +58,7 @@ class DataFlowExpr extends Cfg::Node { } /** - * A call corresponds to a Uses steps where a 3rd party action or a reusable workflow gets called + * A call corresponds to a Uses steps where a 3rd party action or a reusable workflow get called */ class DataFlowCall instanceof Cfg::Node { DataFlowCall() { super.getAstNode() instanceof UsesExpr } @@ -180,6 +180,23 @@ predicate stepsCtxLocalStep(Node nodeFrom, Node nodeTo) { ) } +/** + * Holds if there is a local flow step between a ${{ needs.xxx.outputs.yyy }} expression accesing a job output field + * and the step output itself. But only for those cases where the job (needs) output is defined externally in a MaD Source + * specification. The reason for this is that we don't currently have a way to specify that a source starts with a + * non-empty access path so we cannot write a Source that stores the taint in a Content, we can only do that for steps + * (storeStep). The easiest thing is to add this local flow step that simulates a read step from the source node for a specific + * field name. + */ +predicate needsCtxLocalStep(Node nodeFrom, Node nodeTo) { + exists(UsesExpr astFrom, NeedsCtxAccessExpr astTo | + externallyDefinedSource(nodeFrom, _, "output." + astTo.getFieldName()) and + astFrom = nodeFrom.asExpr() and + astTo = nodeTo.asExpr() and + astTo.getRefExpr() = astFrom + ) +} + /** * Holds if there is a local flow step between a ${{}} expression accesing an input variable and the input itself * e.g. ${{ inputs.foo }} @@ -215,6 +232,7 @@ predicate envCtxLocalStep(Node nodeFrom, Node nodeTo) { pragma[nomagic] predicate localFlowStep(Node nodeFrom, Node nodeTo) { stepsCtxLocalStep(nodeFrom, nodeTo) or + needsCtxLocalStep(nodeFrom, nodeTo) or inputsCtxLocalStep(nodeFrom, nodeTo) or envCtxLocalStep(nodeFrom, nodeTo) } diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll index 8b62cccf30a..5fe3c741735 100644 --- a/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll +++ b/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll @@ -66,6 +66,17 @@ class ParameterNode extends ExprNode { InputExpr getInputExpr() { result = input } } +/** + * A call to a data flow callable (Uses). + */ +class CallNode extends ExprNode { + private DataFlowCall call; + + CallNode() { this.getCfgNode() instanceof DataFlowCall } + + string getCallee() { result = this.getCfgNode().(DataFlowCall).getName() } +} + /** * An argument to a Uses step (call). */ diff --git a/ql/lib/ext/TEST-RW-MODELS.model.yml b/ql/lib/ext/TEST-RW-MODELS.model.yml new file mode 100644 index 00000000000..7adbcd5adbd --- /dev/null +++ b/ql/lib/ext/TEST-RW-MODELS.model.yml @@ -0,0 +1,17 @@ +extensions: + - addsTo: + pack: githubsecuritylab/actions-all + extensible: summaryModel + data: + - ["octo-org/this-repo/.github/workflows/workflow.yml", "*", "input.config-path", "output.workflow-output", "taint"] + - ["octo-org/summary-repo/.github/workflows/workflow.yml", "*", "input.config-path", "output.workflow-output", "taint"] + - addsTo: + pack: githubsecuritylab/actions-all + extensible: sourceModel + data: + - ["octo-org/source-repo/.github/workflows/workflow.yml", "*", "output.workflow-output", "*", "Foo"] + - addsTo: + pack: githubsecuritylab/actions-all + extensible: sinkModel + data: + - ["octo-org/sink-repo/.github/workflows/workflow.yml", "*", "input.config-path", "expression-injection"] diff --git a/ql/lib/test/test.ql b/ql/lib/test/test.ql index 4b2be43bbda..168987284c3 100644 --- a/ql/lib/test/test.ql +++ b/ql/lib/test/test.ql @@ -43,14 +43,9 @@ query predicate nonOrphanVarAccesses(ExprAccessExpr va, string var, AstNode pare query predicate parentNodes(AstNode child, AstNode parent) { child.getParentNode() = parent } -query predicate cfgNodes(Cfg::Node n) { - //any() - n.getAstNode() instanceof OutputsStmt -} +query predicate cfgNodes(Cfg::Node n) { any() } -query predicate dfNodes(DataFlow::Node e) { - e.getLocation().getFile().getBaseName() = "argus_case_study.yml" -} +query predicate dfNodes(DataFlow::Node e) { any() } query predicate exprNodes(DataFlow::ExprNode e) { any() } @@ -69,3 +64,7 @@ query predicate sources(string action, string version, string output, string tri query predicate summaries(string action, string version, string input, string output, string kind) { summaryModel(action, version, input, output, kind) } + +query predicate calls(DataFlow::CallNode call, string callee) { callee = call.getCallee() } + +query predicate needs(DataFlow::ExprNode e) { e.asExpr() instanceof NeedsCtxAccessExpr } diff --git a/ql/src/test/.github/workflows/calling_workflow.yml b/ql/src/test/.github/workflows/calling_workflow.yml index 9aafe1189ef..7c2bfdf0348 100644 --- a/ql/src/test/.github/workflows/calling_workflow.yml +++ b/ql/src/test/.github/workflows/calling_workflow.yml @@ -8,17 +8,20 @@ jobs: uses: octo-org/this-repo/.github/workflows/reusable_workflow.yml@172239021f7ba04fe7327647b213799853a9eb89 with: config-path: ${{ github.event.pull_request.head.ref }} - secrets: inherit call2: uses: ./.github/workflows/reusable_workflow.yml with: config-path: ${{ github.event.pull_request.head.ref }} - secrets: inherit call3: - uses: octo-org/another-repo/.github/workflows/workflow.yml@v1 + uses: octo-org/summary-repo/.github/workflows/workflow.yml@v1 + with: + config-path: ${{ github.event.pull_request.head.ref }} + call4: + uses: octo-org/source-repo/.github/workflows/workflow.yml@v1 + call5: + uses: octo-org/sink-repo/.github/workflows/workflow.yml@v1 with: config-path: ${{ github.event.pull_request.head.ref }} - secrets: inherit job1: runs-on: ubuntu-latest @@ -36,3 +39,8 @@ jobs: needs: call3 steps: - run: echo ${{ needs.call3.outputs.workflow-output }} + job4: + runs-on: ubuntu-latest + needs: call4 + steps: + - run: echo ${{ needs.call4.outputs.workflow-output }}