Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 42 additions & 22 deletions actions/ql/lib/codeql/actions/dataflow/FlowSources.qll
Original file line number Diff line number Diff line change
Expand Up @@ -52,29 +52,41 @@ class GitHubCtxSource extends RemoteFlowSource {
override string getEventName() { result = event }
}

bindingset[expression]
private predicate untrustedEventProperty(Expression expression, string kind) {
exists(string regexp |
untrustedEventPropertiesDataModel(regexp, kind) and
not kind = "json" and
normalizeExpr(expression.getExpression()).regexpMatch("(?i)\\s*" + wrapRegexp(regexp) + ".*")
)
}

bindingset[expression, event]
private predicate expressionContainsEventContext(Expression expression, string event) {
exists(string contextPrefix |
contextTriggerDataModel(event, contextPrefix) and
normalizeExpr(expression.getExpression()).matches("%" + contextPrefix + "%")
)
}

class GitHubEventCtxSource extends RemoteFlowSource {
string flag;
string context;
string event;

GitHubEventCtxSource() {
exists(Expression e, string regexp |
exists(Expression e |
this.asExpr() = e and
context = e.getExpression() and
(
// the context is available for the job trigger events
event = e.getATriggerEvent().getName() and
exists(string context_prefix |
contextTriggerDataModel(event, context_prefix) and
normalizeExpr(context).matches("%" + context_prefix + "%")
)
expressionContainsEventContext(e, event)
or
not exists(e.getATriggerEvent()) and
event = "unknown"
) and
untrustedEventPropertiesDataModel(regexp, flag) and
not flag = "json" and
normalizeExpr(context).regexpMatch("(?i)\\s*" + wrapRegexp(regexp) + ".*")
untrustedEventProperty(e, flag)
)
}

Expand Down Expand Up @@ -177,32 +189,40 @@ class GitHubEventPathSource extends RemoteFlowSource, CommandSource {
override Run getEnclosingRun() { result = run }
}

bindingset[expression, event]
private predicate jsonSourceForEvent(Expression expression, string event) {
exists(string context, string regexp, string contextPrefix |
context = expression.getExpression() and
untrustedEventPropertiesDataModel(regexp, _) and
contextTriggerDataModel(event, contextPrefix) and
normalizeExpr(context).matches("%" + contextPrefix + "%") and
normalizeExpr(context).regexpMatch("(?i).*" + wrapJsonRegexp(regexp) + ".*")
)
or
exists(string context |
context = expression.getExpression() and
untrustedEventPropertiesDataModel(_, _) and
contextTriggerDataModel(event, _) and
normalizeExpr(context).regexpMatch("(?i).*" + wrapJsonRegexp("\\bgithub.event\\b") + ".*")
)
}

class GitHubEventJsonSource extends RemoteFlowSource {
string flag;
string event;

GitHubEventJsonSource() {
exists(Expression e, string context, string regexp |
exists(Expression e |
this.asExpr() = e and
context = e.getExpression() and
untrustedEventPropertiesDataModel(regexp, _) and
(
// only contexts for the triggering events are considered tainted.
// eg: for `pull_request`, we only consider `github.event.pull_request`
event = e.getEnclosingWorkflow().getATriggerEvent().getName() and
exists(string context_prefix |
contextTriggerDataModel(event, context_prefix) and
normalizeExpr(context).matches("%" + context_prefix + "%")
) and
normalizeExpr(context).regexpMatch("(?i).*" + wrapJsonRegexp(regexp) + ".*")
or
// github.event is tainted for all triggers
event = e.getEnclosingWorkflow().getATriggerEvent().getName() and
contextTriggerDataModel(e.getEnclosingWorkflow().getATriggerEvent().getName(), _) and
normalizeExpr(context).regexpMatch("(?i).*" + wrapJsonRegexp("\\bgithub.event\\b") + ".*")
jsonSourceForEvent(e, event)
or
not exists(e.getATriggerEvent()) and
event = "unknown"
event = "unknown" and
untrustedEventPropertiesDataModel(_, _)
) and
flag = "json"
)
Expand Down
31 changes: 29 additions & 2 deletions actions/ql/lib/codeql/actions/security/CachePoisoningQuery.qll
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ string defaultBranchTriggerEvent() {
[
"check_run", "check_suite", "delete", "discussion", "discussion_comment", "fork", "gollum",
"issue_comment", "issues", "label", "milestone", "project", "project_card", "project_column",
"public", "pull_request_comment", "pull_request_target", "repository_dispatch", "schedule",
"watch", "workflow_run"
"public", "pull_request_comment", "pull_request_target", "repository_dispatch",
"registry_package", "page_build", "schedule", "watch", "workflow_dispatch", "workflow_run"
]
}

Expand Down Expand Up @@ -42,6 +42,33 @@ predicate runsOnDefaultBranch(Event e) {
)
}

private string defaultBranchCacheWriteEvent() {
result =
[
"push", "workflow_dispatch", "repository_dispatch", "delete", "registry_package",
"page_build", "schedule"
]
}

private predicate eventHasDefaultBranchCacheWriteAccess(Event event) {
runsOnDefaultBranch(event) and event.getName() = defaultBranchCacheWriteEvent()
}

/** Holds if `job` can write to the cache scope of the default branch for `event`. */
predicate hasDefaultBranchCacheWriteAccess(LocalJob job, Event event) {
job.getATriggerEvent() = event and
(
eventHasDefaultBranchCacheWriteAccess(event)
or
// the workflow caller runs in the context of the default branch
event.getName() = "workflow_call" and
exists(ExternalJob caller |
job.getEnclosingWorkflow().(ReusableWorkflow).getACaller() = caller and
eventHasDefaultBranchCacheWriteAccess(caller.getATriggerEvent())
)
)
}

abstract class CacheWritingStep extends Step {
abstract string getPath();
}
Expand Down
14 changes: 1 addition & 13 deletions actions/ql/lib/codeql/actions/security/CodeInjectionQuery.qll
Original file line number Diff line number Diff line change
Expand Up @@ -29,22 +29,10 @@ Event getRelevantCachePoisoningEventForSink(DataFlow::Node sink) {
exists(LocalJob job |
job = sink.asExpr().getEnclosingJob() and
job.getATriggerEvent() = result and
// job can be triggered by an external user
result.isExternallyTriggerable() and
// excluding privileged workflows since they can be exploited in easier circumstances
// which is covered by `actions/code-injection/critical`
not job.isPrivilegedExternallyTriggerable(result) and
(
// the workflow runs in the context of the default branch
runsOnDefaultBranch(result)
or
// the workflow caller runs in the context of the default branch
result.getName() = "workflow_call" and
exists(ExternalJob caller |
caller.getCallee() = job.getLocation().getFile().getRelativePath() and
runsOnDefaultBranch(caller.getATriggerEvent())
)
)
hasDefaultBranchCacheWriteAccess(job, result)
)
}

Expand Down
31 changes: 19 additions & 12 deletions actions/ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,48 +34,55 @@ Due to the above design, if something is cached in the context of the default br

## Example

GitHub gives workflows triggered by low-trust events, such as `issue_comment`,
`pull_request_target`, and `workflow_run`, read-only access to the default branch cache scope.
This query therefore reports only workflows whose trigger can write to that scope.

### Incorrect Usage

The following workflow is vulnerable to code injection in a non-privileged job but in the context of the default branch.
The following workflow interpolates a commit message directly into a script on a push to the
default branch. A commit message originating from a merged contribution may contain shell syntax,
which can expose the cache write token and allow the default branch cache to be poisoned.

```yaml
name: Vulnerable Workflow
on:
issue_comment:
types: [created]
push:
branches: [main]

jobs:
pr-comment:
build:
permissions: {}
runs-on: ubuntu-latest
steps:
- run: |
echo ${{ github.event.comment.body }}
echo ${{ github.event.head_commit.message }}
```

### Correct Usage

The following workflow is not vulnerable to code injections even if it runs in the context of the default branch.
The following workflow passes the commit message through an environment variable, so the shell
does not interpret its contents as code.

```yaml
name: Secure Workflow
on:
issue_comment:
types: [created]
push:
branches: [main]

jobs:
pr-comment:
build:
permissions: {}
runs-on: ubuntu-latest
steps:
- env:
BODY: ${{ github.event.comment.body }}
MESSAGE: ${{ github.event.head_commit.message }}
run: |
echo "$BODY"
echo "$MESSAGE"
```

## References

- Adnan Khan's Blog: [The Monsters in Your Build Cache – GitHub Actions Cache Poisoning](https://adnanthekhan.com/2024/05/06/the-monsters-in-your-build-cache-github-actions-cache-poisoning/).
- GitHub Docs: [GitHub Actions Caching Documentation](https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows).
- GitHub Docs: [Cache access for low-trust workflow triggers](https://docs.github.com/actions/reference/workflows-and-actions/dependency-caching#cache-access-for-low-trust-workflow-triggers).
- Scribe Security Blog: [Cache Poisoning in GitHub Actions](https://scribesecurity.com/blog/github-cache-poisoning/).
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Event
where
CodeInjectionFlow::flowPath(source, sink) and
event = getRelevantCachePoisoningEventForSink(sink.getNode()) and
source.getNode().(RemoteFlowSource).getEventName() = event.getName() and
Comment thread
JarLob marked this conversation as resolved.
// the checkout is not controlled by an access check
not exists(ControlCheck check |
check.protects(source.getNode().asExpr(), event, "code-injection")
Expand Down
66 changes: 21 additions & 45 deletions actions/ql/src/Security/CWE-349/CachePoisoningViaDirectCache.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,35 +34,39 @@ Due to the above design, if something is cached in the context of the default br

## Example

GitHub gives workflows triggered by low-trust events, such as `issue_comment`,
`pull_request_target`, and `workflow_run`, read-only access to the default branch cache scope.
This query therefore reports only workflows whose trigger can write to that scope.

### Incorrect Usage

The following workflow is caching an attacker-controlled file (`large_file`) in the context of the default branch.
The following write-capable manually dispatched workflow accepts a revision without validation,
fetches files from it, and saves those files in the default branch cache. This is unsafe if an
untrusted integration or automation can influence the dispatch input.

```yaml
name: Vulnerable Workflow
on:
issue_comment:
types: [created]
workflow_dispatch:
inputs:
head_sha:
required: true

jobs:
pr-comment:
cache:
permissions: read-all
runs-on: ubuntu-latest
steps:
- uses: xt0rted/pull-request-comment-branch@v2
id: comment-branch
- uses: actions/checkout@v3
with:
ref: ${{ steps.comment-branch.outputs.head_sha }}
- name: Set up Python 3.10
uses: actions/setup-python@v5
- name: Cache pip dependencies
- env:
HEAD_SHA: ${{ github.event.inputs.head_sha }}
run: |
git fetch origin "$HEAD_SHA"
git checkout "$HEAD_SHA"
- name: Cache fetched files
uses: actions/cache@v4
id: cache-pip
with:
path: ~/.cache/pip
key: ${{ runner.os }}-pip-${{ hashFiles('**/pyproject.toml') }}
restore-keys: ${{ runner.os }}-pip-
path: .
key: dispatched-${{ github.event.inputs.head_sha }}
```

### Correct Usage
Expand Down Expand Up @@ -91,36 +95,8 @@ jobs:
restore-keys: ${{ runner.os }}-pip-
```

Note, that the example above doesn't allow using secrets if the Pull Request originates from a fork. In case secrets are needed, `pull_request_target` with labels as `safe to test` can be used, but the code in Pull Request must be manually reviewed before applying the label.

```yaml
name: Secure Workflow
on:
pull_request_target:
types: [labeled]

jobs:
pr-comment:
if: contains(github.event.pull_request.labels.*.name, 'safe to test')
permissions: read-all
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
with:
ref: ${{ github.event.pull_request.head.sha}}
- name: Set up Python 3.10
uses: actions/setup-python@v5
- name: Cache pip dependencies
uses: actions/cache@v4
id: cache-pip
with:
path: ~/.cache/pip
key: ${{ runner.os }}-pip-${{ hashFiles('**/pyproject.toml') }}
restore-keys: ${{ runner.os }}-pip-
```

## References

- Adnan Khan's Blog: [The Monsters in Your Build Cache – GitHub Actions Cache Poisoning](https://adnanthekhan.com/2024/05/06/the-monsters-in-your-build-cache-github-actions-cache-poisoning/).
- GitHub Docs: [GitHub Actions Caching Documentation](https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows).
- GitHub Docs: [Cache access for low-trust workflow triggers](https://docs.github.com/actions/reference/workflows-and-actions/dependency-caching#cache-access-for-low-trust-workflow-triggers).
- Scribe Security Blog: [Cache Poisoning in GitHub Actions](https://scribesecurity.com/blog/github-cache-poisoning/).
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,7 @@ where
job.getATriggerEvent() = event and
// job can be triggered by an external user
event.isExternallyTriggerable() and
(
// the workflow runs in the context of the default branch
runsOnDefaultBranch(event)
or
// the workflow's caller runs in the context of the default branch
event.getName() = "workflow_call" and
exists(ExternalJob caller |
caller.getCallee() = job.getLocation().getFile().getRelativePath() and
runsOnDefaultBranch(caller.getATriggerEvent())
)
) and
hasDefaultBranchCacheWriteAccess(job, event) and
// the job writes to the cache
// (No need to follow the checkout/download step since the cache is normally write after the job completes)
job.getAStep() = step and
Expand Down
Loading
Loading