diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll index 61f2d8e91d7..0685b2fc14d 100644 --- a/ql/lib/codeql/actions/Ast.qll +++ b/ql/lib/codeql/actions/Ast.qll @@ -27,6 +27,25 @@ class Statement extends AstNode { } */ class Expression extends Statement { } +/** + * A composite action + */ +class CompositeActionStmt extends Statement instanceof Actions::CompositeAction { + RunsStmt getRunsStmt() { result = super.getRuns() } + + InputsStmt getInputsStmt() { result = this.(YamlMapping).lookup("inputs") } + + OutputsStmt getOutputsStmt() { result = this.(YamlMapping).lookup("outputs") } + + string getName() { result = this.getLocation().getFile().getRelativePath() } +} + +class RunsStmt extends Statement instanceof Actions::Runs { + StepStmt getAStepStmt() { result = super.getSteps().getElementNode(_) } + + StepStmt getStepStmt(int i) { result = super.getSteps().getElementNode(i) } +} + /** * A Github Actions Workflow */ @@ -43,67 +62,45 @@ class ReusableWorkflowStmt extends WorkflowStmt { this.(Actions::Workflow).getOn().getNode("workflow_call") = workflow_call } - ReusableWorkflowInputsStmt getInputsStmt() { - result = workflow_call.(YamlMapping).lookup("inputs") - } + InputsStmt getInputsStmt() { result = workflow_call.(YamlMapping).lookup("inputs") } - ReusableWorkflowOutputsStmt getOutputsStmt() { - result = workflow_call.(YamlMapping).lookup("outputs") - } + OutputsStmt getOutputsStmt() { result = workflow_call.(YamlMapping).lookup("outputs") } string getName() { result = this.getLocation().getFile().getRelativePath() } } -class ReusableWorkflowInputsStmt extends Statement instanceof YamlMapping { - ReusableWorkflowInputsStmt() { - exists(Actions::On on | on.getNode("workflow_call").(YamlMapping).lookup("inputs") = this) - } +class InputsStmt extends Statement instanceof YamlMapping { + YamlMapping parent; + + InputsStmt() { parent.lookup("inputs") = this } /** - * Gets a specific parameter expression (YamlMapping) by name. - * eg: - * on: - * workflow_call: - * inputs: - * config-path: - * required: true - * type: string - * secrets: - * token: - * required: true + * Gets a specific input expression (YamlMapping) by name. */ - ReusableWorkflowInputExpr getInputExpr(string name) { + InputExpr getInputExpr(string name) { result.(YamlString).getValue() = name and this.(YamlMapping).maps(result, _) } } -class ReusableWorkflowInputExpr extends Expression instanceof YamlString { } +class OutputsStmt extends Statement instanceof YamlMapping { + YamlMapping parent; -class ReusableWorkflowOutputsStmt extends Statement instanceof YamlMapping { - ReusableWorkflowOutputsStmt() { - exists(Actions::On on | on.getNode("workflow_call").(YamlMapping).lookup("outputs") = this) - } + OutputsStmt() { parent.lookup("outputs") = this } /** - * Gets a specific parameter expression (YamlMapping) by name. - * eg: - * on: - * workflow_call: - * outputs: - * firstword: - * description: "The first output string" - * value: ${{ jobs.example_job.outputs.output1 }} - * secondword: - * description: "The second output string" - * value: ${{ jobs.example_job.outputs.output2 }} + * Gets a specific output expression (YamlMapping) by name. */ - ReusableWorkflowOutputExpr getOutputExpr(string name) { + OutputExpr getOutputExpr(string name) { this.(YamlMapping).lookup(name).(YamlMapping).lookup("value") = result } } -class ReusableWorkflowOutputExpr extends Expression instanceof YamlString { } +// TODO: Needs a characteristic predicate otherwise anything is an output expression +class InputExpr extends Expression instanceof YamlString { } + +// TODO: Needs a characteristic predicate otherwise anything is an output expression +class OutputExpr extends Expression instanceof YamlString { } /** * A Job is a collection of steps that run in an execution environment. @@ -369,7 +366,7 @@ class StepOutputAccessExpr extends ExprAccessExpr { } override Expression getRefExpr() { - this.getJobStmt() = result.(StepStmt).getJobStmt() and + this.getLocation().getFile() = result.getLocation().getFile() and result.(StepStmt).getId() = stepId } } @@ -413,10 +410,10 @@ class JobOutputAccessExpr extends ExprAccessExpr { * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability * e.g. `${{ inputs.foo }}` */ -class ReusableWorkflowInputAccessExpr extends ExprAccessExpr { +class InputAccessExpr extends ExprAccessExpr { string paramName; - ReusableWorkflowInputAccessExpr() { + InputAccessExpr() { paramName = this.getExpression().regexpCapture("inputs\\.([A-Za-z0-9_-]+)", 1) } @@ -425,6 +422,11 @@ class ReusableWorkflowInputAccessExpr extends ExprAccessExpr { w.getLocation().getFile() = this.getLocation().getFile() and w.getInputsStmt().getInputExpr(paramName) = result ) + or + exists(CompositeActionStmt a | + a.getLocation().getFile() = this.getLocation().getFile() and + a.getInputsStmt().getInputExpr(paramName) = result + ) } } diff --git a/ql/lib/codeql/actions/controlflow/internal/Cfg.qll b/ql/lib/codeql/actions/controlflow/internal/Cfg.qll index 0dd34ff926f..bb0c25dbdf6 100644 --- a/ql/lib/codeql/actions/controlflow/internal/Cfg.qll +++ b/ql/lib/codeql/actions/controlflow/internal/Cfg.qll @@ -88,6 +88,8 @@ module CfgScope { abstract class CfgScope extends AstNode { } class WorkflowScope extends CfgScope instanceof WorkflowStmt { } + + class CompositeActionScope extends CfgScope instanceof CompositeActionStmt { } } private module Implementation implements CfgShared::InputSig { @@ -120,9 +122,15 @@ private module Implementation implements CfgShared::InputSig { int maxSplits() { result = 0 } - predicate scopeFirst(CfgScope scope, AstNode e) { first(scope.(WorkflowStmt), e) } + predicate scopeFirst(CfgScope scope, AstNode e) { + first(scope.(WorkflowStmt), e) or + first(scope.(CompositeActionStmt), e) + } - predicate scopeLast(CfgScope scope, AstNode e, Completion c) { last(scope.(WorkflowStmt), e, c) } + predicate scopeLast(CfgScope scope, AstNode e, Completion c) { + last(scope.(WorkflowStmt), e, c) or + last(scope.(CompositeActionStmt), e, c) + } predicate successorTypeIsSimple(SuccessorType t) { t instanceof NormalSuccessor } @@ -139,6 +147,28 @@ private import CfgImpl private import Completion private import CfgScope +private class CompositeActionTree extends StandardPreOrderTree instanceof CompositeActionStmt { + override ControlFlowTree getChildNode(int i) { + result = + rank[i](Expression child, Location l | + ( + child = this.(CompositeActionStmt).getInputsStmt() or + child = this.(CompositeActionStmt).getOutputsStmt() or + child = this.(CompositeActionStmt).getRunsStmt() + ) and + l = child.getLocation() + | + child + order by + l.getStartLine(), l.getStartColumn(), l.getEndColumn(), l.getEndLine(), child.toString() + ) + } +} + +private class RunsTree extends StandardPreOrderTree instanceof RunsStmt { + override ControlFlowTree getChildNode(int i) { result = super.getStepStmt(i) } +} + private class WorkflowTree extends StandardPreOrderTree instanceof WorkflowStmt { override ControlFlowTree getChildNode(int i) { if this instanceof ReusableWorkflowStmt @@ -169,8 +199,7 @@ private class WorkflowTree extends StandardPreOrderTree instanceof WorkflowStmt } } -private class ReusableWorkflowInputsTree extends StandardPreOrderTree instanceof ReusableWorkflowInputsStmt -{ +private class InputsTree extends StandardPreOrderTree instanceof InputsStmt { override ControlFlowTree getChildNode(int i) { result = rank[i](Expression child, Location l | @@ -183,10 +212,9 @@ private class ReusableWorkflowInputsTree extends StandardPreOrderTree instanceof } } -private class InputExprTree extends LeafTree instanceof ReusableWorkflowInputExpr { } +private class InputExprTree extends LeafTree instanceof InputExpr { } -private class ReusableWorkflowOutputsTree extends StandardPreOrderTree instanceof ReusableWorkflowOutputsStmt -{ +private class OutputsTree extends StandardPreOrderTree instanceof OutputsStmt { override ControlFlowTree getChildNode(int i) { result = rank[i](Expression child, Location l | @@ -199,7 +227,7 @@ private class ReusableWorkflowOutputsTree extends StandardPreOrderTree instanceo } } -private class OutputExprTree extends LeafTree instanceof ReusableWorkflowOutputExpr { } +private class OutputExprTree extends LeafTree instanceof OutputExpr { } private class JobTree extends StandardPreOrderTree instanceof JobStmt { override ControlFlowTree getChildNode(int i) { diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll index 120444863e5..fae6c74b0b3 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSources.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSources.qll @@ -161,3 +161,14 @@ private class ExternallyDefinedSource extends RemoteFlowSource { override string getSourceType() { result = soutceType } } + +/** + * Composite action input sources + */ +private class CompositeActionInputSource extends RemoteFlowSource { + CompositeActionStmt c; + + CompositeActionInputSource() { c.getInputsStmt().getInputExpr(_) = this.asExpr() } + + override string getSourceType() { result = "Composite action input" } +} diff --git a/ql/lib/codeql/actions/dataflow/FlowSteps.qll b/ql/lib/codeql/actions/dataflow/FlowSteps.qll index cafd6083276..750a4011320 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSteps.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSteps.qll @@ -83,7 +83,7 @@ predicate runEnvToScriptstep(DataFlow::Node pred, DataFlow::Node succ) { line = script.splitAt("\n") and ( line.regexpMatch(".*::set-output\\s+name.*") or - line.regexpMatch(".*>>\\s*$GITHUB_ENV.*") + line.regexpMatch(".*>>\\s*\\$GITHUB_OUTPUT.*") ) and script.indexOf("$" + ["", "{", "ENV{"] + varName) > 0 ) and diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll index ee59e25ab20..79bd48b395a 100644 --- a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll +++ b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll @@ -82,7 +82,10 @@ class DataFlowCallable instanceof Cfg::CfgScope { string getName() { if this instanceof ReusableWorkflowStmt then result = this.(ReusableWorkflowStmt).getName() - else none() + else + if this instanceof CompositeActionStmt + then result = this.(CompositeActionStmt).getName() + else none() } } @@ -190,11 +193,11 @@ predicate jobsCtxLocalStep(Node nodeFrom, Node nodeTo) { } /** - * Holds if there is a local flow step between a ${{}} expression accesing a reusable workflow input variable and the input itself + * Holds if there is a local flow step between a ${{}} expression accesing an input variable and the input itself * e.g. ${{ inputs.foo }} */ predicate inputsCtxLocalStep(Node nodeFrom, Node nodeTo) { - exists(Expression astFrom, ReusableWorkflowInputAccessExpr astTo | + exists(Expression astFrom, InputAccessExpr astTo | astFrom = nodeFrom.asExpr() and astTo = nodeTo.asExpr() and astTo.getRefExpr() = astFrom diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll index a14b0693874..d83608dc2b8 100644 --- a/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll +++ b/ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll @@ -48,7 +48,7 @@ class ExprNode extends Node, TExprNode { * Reusable workflow input nodes */ class ParameterNode extends ExprNode { - private ReusableWorkflowInputExpr parameter; + private InputExpr parameter; ParameterNode() { this.asExpr() = parameter and @@ -63,7 +63,7 @@ class ParameterNode extends ExprNode { override Location getLocation() { result = parameter.getLocation() } - ReusableWorkflowInputExpr getInputExpr() { result = parameter } + InputExpr getInputExpr() { result = parameter } } /** @@ -87,7 +87,8 @@ class ReturnNode extends ExprNode { ReturnNode() { this.getCfgNode() = node and - node.getAstNode() = any(ReusableWorkflowStmt w).getOutputsStmt().getOutputExpr(_) + (node.getAstNode() = any(ReusableWorkflowStmt w).getOutputsStmt().getOutputExpr(_) or + node.getAstNode() = any(CompositeActionStmt a).getOutputsStmt().getOutputExpr(_)) } ReturnKind getKind() { result = TNormalReturn() } diff --git a/ql/lib/test/test.ql b/ql/lib/test/test.ql index 36c268ecc99..4b2be43bbda 100644 --- a/ql/lib/test/test.ql +++ b/ql/lib/test/test.ql @@ -45,7 +45,7 @@ query predicate parentNodes(AstNode child, AstNode parent) { child.getParentNode query predicate cfgNodes(Cfg::Node n) { //any() - n.getAstNode() instanceof ReusableWorkflowOutputsStmt + n.getAstNode() instanceof OutputsStmt } query predicate dfNodes(DataFlow::Node e) { diff --git a/ql/src/Security/CWE-020/CompositeActionSummaries.ql b/ql/src/Security/CWE-020/CompositeActionSummaries.ql new file mode 100644 index 00000000000..46a7797e2b2 --- /dev/null +++ b/ql/src/Security/CWE-020/CompositeActionSummaries.ql @@ -0,0 +1,36 @@ +/** + * @name Composite Action Summaries + * @description Actions that pass user-controlled data to their output variables. + * @kind path-problem + * @problem.severity warning + * @security-severity 9.3 + * @precision high + * @id actions/composite-action-summaries + * @tags actions + * external/cwe/cwe-020 + */ + +import actions +import codeql.actions.TaintTracking +import codeql.actions.dataflow.FlowSources +import codeql.actions.dataflow.ExternalFlow + +private class OutputVariableSink extends DataFlow::Node { + OutputVariableSink() { exists(OutputsStmt s | s.getOutputExpr(_) = this.asExpr()) } +} + +private module MyConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + exists(CompositeActionStmt c | c.getInputsStmt().getInputExpr(_) = source.asExpr()) + } + + predicate isSink(DataFlow::Node sink) { sink instanceof OutputVariableSink } +} + +module MyFlow = TaintTracking::Global; + +import MyFlow::PathGraph + +from MyFlow::PathNode source, MyFlow::PathNode sink +where MyFlow::flowPath(source, sink) +select sink.getNode(), source, sink, "Summary" diff --git a/ql/src/Security/CWE-020/CompositeActionsSources.ql b/ql/src/Security/CWE-020/CompositeActionsSources.ql new file mode 100644 index 00000000000..09556ac1b78 --- /dev/null +++ b/ql/src/Security/CWE-020/CompositeActionsSources.ql @@ -0,0 +1,38 @@ +/** + * @name Composite Action Sources + * @description Actions that pass user-controlled data to their output variables. + * @kind path-problem + * @problem.severity warning + * @security-severity 9.3 + * @precision high + * @id actions/composite-action-sources + * @tags actions + * external/cwe/cwe-020 + */ + +import actions +import codeql.actions.TaintTracking +import codeql.actions.dataflow.FlowSources +import codeql.actions.dataflow.ExternalFlow + +private class OutputVariableSink extends DataFlow::Node { + OutputVariableSink() { exists(OutputsStmt s | s.getOutputExpr(_) = this.asExpr()) } +} + +private module MyConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + source instanceof RemoteFlowSource and + exists(CompositeActionStmt c | c.getAChildNode*() = source.asExpr()) and + not exists(CompositeActionStmt c | c.getInputsStmt().getInputExpr(_) = source.asExpr()) + } + + predicate isSink(DataFlow::Node sink) { sink instanceof OutputVariableSink } +} + +module MyFlow = TaintTracking::Global; + +import MyFlow::PathGraph + +from MyFlow::PathNode source, MyFlow::PathNode sink +where MyFlow::flowPath(source, sink) +select sink.getNode(), source, sink, "Source" diff --git a/ql/src/Security/CWE-094/ExpressionInjection.ql b/ql/src/Security/CWE-094/ExpressionInjection.ql index 7953c3b037c..6860f091d5e 100644 --- a/ql/src/Security/CWE-094/ExpressionInjection.ql +++ b/ql/src/Security/CWE-094/ExpressionInjection.ql @@ -6,7 +6,7 @@ * @problem.severity warning * @security-severity 9.3 * @precision high - * @id actions/command-injection + * @id actions/expression-injection * @tags actions * security * external/cwe/cwe-094 diff --git a/ql/src/test/.github/workflows/calling_composite.yml b/ql/src/test/.github/workflows/calling_composite.yml new file mode 100644 index 00000000000..79c2d072ef5 --- /dev/null +++ b/ql/src/test/.github/workflows/calling_composite.yml @@ -0,0 +1,14 @@ +on: [push] + +jobs: + hello_world_job: + runs-on: ubuntu-latest + name: A job to say hello + steps: + - uses: actions/checkout@v4 + - id: foo + uses: some-org/test-action@v1 + with: + who-to-greet: ${{ github.event.pull_request.head.ref }} + - run: echo ${{ steps.foo.outputs.reflected}} + - run: echo ${{ steps.foo.outputs.tainted}} diff --git a/ql/src/test/.github/workflows/changed-files.yml b/ql/src/test/.github/workflows/changed-files.yml index 0a47960517f..12bade510ba 100644 --- a/ql/src/test/.github/workflows/changed-files.yml +++ b/ql/src/test/.github/workflows/changed-files.yml @@ -13,8 +13,6 @@ jobs: - uses: actions/checkout@v4 with: fetch-depth: 0 - - # Example 1 - name: Get changed files id: changed-files uses: tj-actions/changed-files@v40 diff --git a/ql/src/test/composite-actions/action.yml b/ql/src/test/composite-actions/action.yml new file mode 100644 index 00000000000..c43d5fd6694 --- /dev/null +++ b/ql/src/test/composite-actions/action.yml @@ -0,0 +1,50 @@ +name: 'Hello World' +description: 'Greet someone' +inputs: + who-to-greet: # id of input + description: 'Who to greet' + required: true + default: 'World' +outputs: + reflected: + description: "Reflected input" + value: ${{ steps.reflector.outputs.reflected }} + tainted: + description: "Reflected input" + value: ${{ steps.source.outputs.tainted}} + +runs: + using: "composite" + steps: + - name: Secure Set Greeting + run: echo "Hello $INPUT_WHO_TO_GREET." + shell: bash + env: + INPUT_WHO_TO_GREET: ${{ inputs.who-to-greet }} + - name: Remove foo + id: replace + uses: mad9000/actions-find-and-replace-string@3 + with: + source: ${{ inputs.who-to-greet }} + find: 'foo' + replace: '' + - id: sink + run: echo ${{ steps.replace.outputs.value }} + shell: bash + - name: Vulnerable Set Greeting + run: echo "Hello ${{ inputs.who-to-greet }}." + shell: bash + - id: reflector + run: echo "reflected=$(echo $INPUT_WHO_TO_GREET)" >> $GITHUB_OUTPUT + shell: bash + env: + INPUT_WHO_TO_GREET: ${{ inputs.who-to-greet }} + - id: changed-files + uses: tj-actions/changed-files@v40 + - id: source + run: echo "tainted=$(echo $TAINTED)" >> $GITHUB_OUTPUT + shell: bash + env: + TAINTED: ${{ steps.changed-files.outputs.all_changed_files }} + +