-
Notifications
You must be signed in to change notification settings - Fork 68
security: reject symlinks in PR-supplied Firebase preview artifact #3654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,13 +66,34 @@ runs: | |
| - name: Extracting workflow artifact into Firebase public directory. | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| extractDir="$RUNNER_TEMP/artifact-unpack" | ||
| publicDir='${{inputs.firebase-public-dir}}' | ||
|
|
||
| mkdir -p '${{inputs.firebase-public-dir}}' | ||
| mkdir -p "$publicDir" | ||
| mkdir -p "$extractDir" | ||
|
|
||
| unzip unsafe-artifact.zip -d "$extractDir" | ||
| tar -xvzf "$extractDir/deploy-artifact.tar.gz" -C '${{inputs.firebase-public-dir}}' | ||
| tar -xvzf "$extractDir/deploy-artifact.tar.gz" -C "$publicDir" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider removing the tar -xzf "$extractDir/deploy-artifact.tar.gz" -C "$publicDir" |
||
|
|
||
| # Defense-in-depth: fail the deploy if the extracted artifact contains | ||
| # symlinks. The artifact is attacker-influenced (PR-supplied build | ||
| # output); a symlink such as `public/leak -> /proc/self/environ` or | ||
| # `public/leak -> ~/.config/gcloud/application_default_credentials.json` | ||
| # would otherwise be followed by downstream tooling that walks | ||
| # `firebase-public-dir` and reads each entry, ending up on the | ||
| # public Firebase Hosting CDN. There is no legitimate reason for a | ||
| # `public` artifact destined for static hosting to contain | ||
| # symlinks, so refusing them outright is the safest default. | ||
| # See https://securitylab.github.com/research/github-actions-preventing-pwn-requests/. | ||
| symlinks=$(find "$publicDir" -type l 2>/dev/null || true) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The symlinks=$(find "$publicDir" -type l) |
||
| if [ -n "$symlinks" ]; then | ||
| echo "::error title=Symlinks rejected in artifact::The deploy artifact contains symlinks, which are not permitted in a Hosting public directory for security reasons (a symlink can leak the contents of files outside the source tree onto the public Firebase Hosting CDN)." | ||
| echo "Symlinks found:" | ||
| echo "$symlinks" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When printing the list of found symlinks, it is safer to indent the output. This prevents a malicious filename containing a newline from being interpreted as a GitHub Actions workflow command if it happens to start at the beginning of a line. Using printf '%s\n' "$symlinks" | sed 's/^/ /' |
||
| exit 1 | ||
| fi | ||
|
|
||
| - name: Extracting artifact metadata | ||
| id: artifact-info | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interpolating
${{ inputs.firebase-public-dir }}directly into the shell script is vulnerable to shell injection if the input contains single quotes (e.g.,path'with'quotes). Although this input is marked asTRUSTEDin this repository, it is a security best practice in GitHub Actions to pass inputs via environment variables to avoid them being interpreted as part of the script body.