diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll index 31cf33782b0..9f91af470b2 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSources.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSources.qll @@ -1,4 +1,5 @@ private import codeql.actions.security.ArtifactPoisoningQuery +private import codeql.actions.security.UntrustedCheckoutQuery private import codeql.actions.config.Config private import codeql.actions.dataflow.ExternalFlow @@ -112,6 +113,15 @@ private class ArtifactSource extends RemoteFlowSource { override string getSourceType() { result = "artifact" } } +/** + * A file from an untrusted checkout. + */ +private class CheckoutSource extends RemoteFlowSource { + CheckoutSource() { this.asExpr() instanceof PRHeadCheckoutStep } + + override string getSourceType() { result = "artifact" } +} + /** * A list of file names returned by dorny/paths-filter. */ diff --git a/ql/lib/codeql/actions/dataflow/FlowSteps.qll b/ql/lib/codeql/actions/dataflow/FlowSteps.qll index 3caf80b7ca8..e16bc00f8ea 100644 --- a/ql/lib/codeql/actions/dataflow/FlowSteps.qll +++ b/ql/lib/codeql/actions/dataflow/FlowSteps.qll @@ -8,6 +8,7 @@ private import codeql.actions.DataFlow private import codeql.actions.dataflow.FlowSources private import codeql.actions.dataflow.ExternalFlow private import codeql.actions.security.ArtifactPoisoningQuery +private import codeql.actions.security.UntrustedCheckoutQuery /** * A unit class for adding additional taint steps. @@ -161,7 +162,11 @@ predicate envToEnvStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow:: * echo "::set-output name=id::$foo */ predicate artifactToOutputStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) { - exists(Run run, UntrustedArtifactDownloadStep download, string content, string key, string value | + exists(Run run, Step artifact, string content, string key, string value | + ( + artifact instanceof UntrustedArtifactDownloadStep or + artifact instanceof PRHeadCheckoutStep + ) and ( // A file is read and its content is assigned to an env var // - run: | @@ -185,7 +190,7 @@ predicate artifactToOutputStoreStep(DataFlow::Node pred, DataFlow::Node succ, Da outputsPartialFileContent(value) ) and c = any(DataFlow::FieldContent ct | ct.getName() = key) and - download.getAFollowingStep() = run and + artifact.getAFollowingStep() = run and pred.asExpr() = run.getScriptScalar() and succ.asExpr() = run ) @@ -199,7 +204,11 @@ predicate artifactToOutputStoreStep(DataFlow::Node pred, DataFlow::Node succ, Da * echo "bar=${foo}" >> "$GITHUB_ENV" */ predicate artifactToEnvStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) { - exists(Run run, string content, string key, string value, UntrustedArtifactDownloadStep download | + exists(Run run, string content, string key, string value, Step artifact | + ( + artifact instanceof UntrustedArtifactDownloadStep or + artifact instanceof PRHeadCheckoutStep + ) and ( // A file is read and its content is assigned to an env var // - run: | @@ -223,7 +232,7 @@ predicate artifactToEnvStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataF outputsPartialFileContent(value) ) and c = any(DataFlow::FieldContent ct | ct.getName() = key) and - download.getAFollowingStep() = run and + artifact.getAFollowingStep() = run and pred.asExpr() = run.getScriptScalar() and // we store the taint on the enclosing job since there may not be an implicit env attribute succ.asExpr() = run.getEnclosingJob() @@ -234,10 +243,14 @@ predicate artifactToEnvStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataF * A download artifact step followed by a step that may use downloaded artifacts. */ predicate artifactDownloadToRunStep(DataFlow::Node pred, DataFlow::Node succ) { - exists(UntrustedArtifactDownloadStep download, Run run | - pred.asExpr() = download and + exists(Step artifact, Run run | + ( + artifact instanceof UntrustedArtifactDownloadStep or + artifact instanceof PRHeadCheckoutStep + ) and + pred.asExpr() = artifact and succ.asExpr() = run.getScriptScalar() and - download.getAFollowingStep() = run + artifact.getAFollowingStep() = run ) } @@ -245,11 +258,15 @@ predicate artifactDownloadToRunStep(DataFlow::Node pred, DataFlow::Node succ) { * A download artifact step followed by a envvar-injection uses step . */ predicate artifactDownloadToUsesStep(DataFlow::Node pred, DataFlow::Node succ) { - exists(UntrustedArtifactDownloadStep download, Uses uses | + exists(Step artifact, Uses uses | + ( + artifact instanceof UntrustedArtifactDownloadStep or + artifact instanceof PRHeadCheckoutStep + ) and madSink(succ, "envvar-injection") and - pred.asExpr() = download and + pred.asExpr() = artifact and succ.asExpr() = uses and - download.getAFollowingStep() = uses + artifact.getAFollowingStep() = uses ) } diff --git a/ql/lib/qlpack.yml b/ql/lib/qlpack.yml index 6b17e77e063..75d8cd5d2e0 100644 --- a/ql/lib/qlpack.yml +++ b/ql/lib/qlpack.yml @@ -2,7 +2,7 @@ library: true warnOnImplicitThis: true name: github/actions-all -version: 0.1.21 +version: 0.1.22 dependencies: codeql/util: ^1.0.1 codeql/yaml: ^1.0.1 diff --git a/ql/src/Security/CWE-349/CachePoisoning.ql b/ql/src/Security/CWE-349/CachePoisoning.ql index 3b69110ed12..6609dae2b7f 100644 --- a/ql/src/Security/CWE-349/CachePoisoning.ql +++ b/ql/src/Security/CWE-349/CachePoisoning.ql @@ -12,6 +12,7 @@ */ import actions +import codeql.actions.security.ArtifactPoisoningQuery import codeql.actions.security.UntrustedCheckoutQuery import codeql.actions.security.CachePoisoningQuery import codeql.actions.security.PoisonableSteps @@ -19,13 +20,17 @@ import codeql.actions.security.ControlChecks query predicate edges(Step a, Step b) { a.getNextStep() = b } -from LocalJob j, Event e, PRHeadCheckoutStep checkout, Step s +from LocalJob j, Event e, Step artifact, Step s where + ( + artifact instanceof PRHeadCheckoutStep or + artifact instanceof UntrustedArtifactDownloadStep + ) and j.getATriggerEvent() = e and // job can be triggered by an external user e.isExternallyTriggerable() and // the checkout is not controlled by an access check - not exists(ControlCheck check | check.protects(checkout, j.getATriggerEvent())) and + not exists(ControlCheck check | check.protects(artifact, j.getATriggerEvent())) and ( // the workflow runs in the context of the default branch runsOnDefaultBranch(e) @@ -38,8 +43,7 @@ where ) ) and // the job checkouts untrusted code from a pull request - // TODO: Consider adding artifact downloads as a potential source of cache poisoning - j.getAStep() = checkout and + j.getAStep() = artifact and ( // the job writes to the cache // (No need to follow the checkout step as the cache writing is normally done after the job completes) @@ -49,9 +53,9 @@ where or // the job executes checked-out code // (The cache specific token can be leaked even for non-privileged workflows) - checkout.getAFollowingStep() = s and + artifact.getAFollowingStep() = s and s instanceof PoisonableStep and // excluding privileged workflows since they can be exploited in easier circumstances not j.isPrivileged() ) -select s, checkout, s, "Potential cache poisoning in the context of the default branch" +select s, artifact, s, "Potential cache poisoning in the context of the default branch" diff --git a/ql/src/qlpack.yml b/ql/src/qlpack.yml index d17bc34b9ab..ce8ab4c24dd 100644 --- a/ql/src/qlpack.yml +++ b/ql/src/qlpack.yml @@ -1,7 +1,7 @@ --- library: false name: github/actions-queries -version: 0.1.21 +version: 0.1.22 groups: [actions, queries] suites: codeql-suites extractor: javascript diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/untrusted_checkout1.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/untrusted_checkout1.yml new file mode 100644 index 00000000000..8f691ed759d --- /dev/null +++ b/ql/test/query-tests/Security/CWE-094/.github/workflows/untrusted_checkout1.yml @@ -0,0 +1,15 @@ +on: + pull_request_target + +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + with: + ref: ${{ github.event.pull_request.head.sha }} + - id: artifact + run: | + echo "::set-output name=pr_number::$(