fixes

Author: JarLob

ql/src/Security/CWE-088/ArgumentInjectionCritical.md

@@ -4,7 +4,7 @@
 
 Passing user-controlled arguments to certain commands in the context of `Run` steps may lead to arbitrary code execution.
 
-Argument injection in GitHub Actions may allow an attacker to exfiltrate any secrets used in the workflow and the temporary GitHub repository authorization token. The token might have write access to the repository, allowing an attacker to use the token to make changes to the repository.
+Argument injection in GitHub Actions may allow an attacker to exfiltrate any secrets used in the workflow and the temporary GitHub repository authorization token. The token may have write access to the repository, allowing the attacker to make changes to the repository.
 
 ## Recommendations
 

ql/src/Security/CWE-088/ArgumentInjectionMedium.md

@@ -4,7 +4,7 @@
 
 Passing user-controlled arguments to certain commands in the context of `Run` steps may lead to arbitrary code execution.
 
-Argument injection in GitHub Actions may allow an attacker to exfiltrate any secrets used in the workflow and the temporary GitHub repository authorization token. The token might have write access to the repository, allowing an attacker to use the token to make changes to the repository.
+Argument injection in GitHub Actions may allow an attacker to exfiltrate any secrets used in the workflow and the temporary GitHub repository authorization token. The token may have write access to the repository, allowing the attacker to make changes to the repository.
 
 ## Recommendations
 

ql/src/Security/CWE-094/CodeInjectionCritical.md

@@ -4,7 +4,7 @@
 
 Using user-controlled input in GitHub Actions may lead to code injection in contexts like _run:_ or _script:_.
 
-Code injection in GitHub Actions may allow an attacker to exfiltrate any secrets used in the workflow and the temporary GitHub repository authorization token. The token might have write access to the repository, allowing an attacker to use the token to make changes to the repository.
+Code injection in GitHub Actions may allow an attacker to exfiltrate any secrets used in the workflow and the temporary GitHub repository authorization token. The token may have write access to the repository, allowing an attacker to make changes to the repository.
 
 ## Recommendations
 
@@ -16,7 +16,7 @@ It is also recommended to limit the permissions of any tokens used by a workflow
 
 ### Incorrect Usage
 
-The following example lets a user inject an arbitrary shell command:
+The following example lets attackers inject an arbitrary shell command:
 
 ```yaml
 on: issue_comment

ql/src/Security/CWE-094/CodeInjectionMedium.md

@@ -4,7 +4,7 @@
 
 Using user-controlled input in GitHub Actions may lead to code injection in contexts like _run:_ or _script:_.
 
-Code injection in GitHub Actions may allow an attacker to exfiltrate any secrets used in the workflow and the temporary GitHub repository authorization token. The token might have write access to the repository, allowing an attacker to use the token to make changes to the repository.
+Code injection in GitHub Actions may allow an attacker to exfiltrate any secrets used in the workflow and the temporary GitHub repository authorization token. The token may have write access to the repository, allowing an attacker to make changes to the repository.
 
 ## Recommendations
 
@@ -16,7 +16,7 @@ It is also recommended to limit the permissions of any tokens used by a workflow
 
 ### Incorrect Usage
 
-The following example lets a user inject an arbitrary shell command:
+The following example lets attackers inject an arbitrary shell command:
 
 ```yaml
 on: issue_comment

ql/src/Security/CWE-275/MissingActionsPermissions.md

@@ -2,12 +2,11 @@
 
 ## Description
 
-A GitHub Actions job or workflow hasn't set explicit permissions to restrict privileges to the workflow job.
-A workflow job by default without the `permissions` key or a root workflow `permissions` will run with the default permissions defined at the repository level. For organizations created before February 2023, including many significant OSS projects and corporations, the default permissions grant read-write access to repositories, and new repositories inherit these old, insecure permissions.
+If a GitHub Actions job or workflow has no explicit permissions set, then the repository permissions are used. Repositories created under organizations inherit the organization permissions. The organizations or repositories created before February 2023 have the default permissions set to read-write. Often these permissions do not adhere to the principle of least privilege and can be reduced to read-only, leaving the `write` permission only to a specific types as `issues: write` or `pull-requests: write`.
 
 ## Recommendations
 
-Add the `permissions` key to the job or workflow (applied to all jobs) and set the permissions to the least privilege required to complete the task:
+Add the `permissions` key to the job or the root of workflow (in this case it is applied to all jobs in the workflow that do not have their own `permissions` key) and assign the least privileges required to complete the task:
 
 ```yaml
 name: "My workflow"

ql/src/Security/CWE-285/ImproperAccessControl.md

@@ -2,17 +2,20 @@
 
 ## Description
 
-An authorization check may not be properly implemented, allowing an attacker to mutate the code after it has been reviewed.
+Sometimes labels are used to approve GitHub Actions. An authorization check may not be properly implemented, allowing an attacker to mutate the code after it has been reviewed and approved by label.
 
 ## Recommendations
 
-When using Label gates, make sure that the code cannot be modified after it has been reviewed and the label has been set.
+When using labels, make sure that the code cannot be modified after it has been reviewed and the label has been set.
 
 ## Examples
 
 ### Incorrect Usage
 
-The following example shows a job that requires the label `safe to test` to be set before running untrusted code. However, the workflow gets triggered on `synchronize` activity type and, therefore, it will get triggered every time there is a change in the Pull Request. An attacker can modify the code of the Pull Request after the code has been reviewed and the label has been set.
+The following example shows a job that requires the label `safe to test` to be set before running untrusted code. There are two problems with the code:
+
+1. The workflow gets triggered on `synchronize` activity type and, therefore, it will get triggered every time there is a change in the Pull Request. An attacker can modify the code of the Pull Request after the code has been reviewed and the label has been set. The workflow will be triggered every time a new change is added to the Pull Request.
+2. The workflow uses `ref: ${{ github.event.pull_request.head.ref }}` for checkout, which is a branch name of the Pull Request. There is a window of opportunity for the attacker to modify their branch after the Pull Request is labeled, but before the workflow starts and runs the checkout.
 
 ```yaml
 on:
@@ -33,7 +36,7 @@ jobs:
 
 ### Correct Usage
 
-Make sure that the workflow only gets triggered when the label is set and use an inmutable commit (`github.event.pull_request.head.sha`) instead of a mutable reference.
+Make sure that the workflow only gets triggered when the label is set and use an immutable commit (`github.event.pull_request.head.sha`) instead of a mutable reference.
 
 ```yaml
 on:

ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.md

@@ -2,14 +2,14 @@
 
 ## Description
 
-GitHub Actions cache poisoning is a technique that allows an attacker to inject malicious content into the Action's cache, potentially leading to code execution in privileged workflows.
+GitHub Actions cache poisoning is a technique that allows an attacker to inject malicious content into the Action's cache from unprivileged workflow, potentially leading to code execution in privileged workflows.
 
 An attacker with the ability to run code in the context of the default branch (e.g. through Code Injection or Execution of Untrusted Code) can exploit this to:
 
-1. Steal the cache access token and URL
-2. Fill the cache to trigger eviction of legitimate entries
-3. Poison cache entries with malicious payloads
-4. Achieve code execution in privileged workflows that restore the poisoned cache
+1. Steal the cache access token and URL.
+2. Overflow the cache to trigger eviction of legitimate entries.
+3. Poison cache entries with malicious payloads.
+4. Achieve code execution in privileged workflows that restore the poisoned cache.
 
 This allows lateral movement from low-privileged to high-privileged workflows within a repository.
 
@@ -27,11 +27,11 @@ Due to the above design, if something is cached in the context of the default br
 
 1. Avoid using caching in workflows that handle sensitive operations like releases.
 2. If caching must be used:
-   - Validate restored cache contents before use
-   - Use short-lived, workflow-specific cache keys
-   - Clear caches regularly
-3. Implement strict isolation between untrusted and privileged workflow execution:
-4. Never run untrusted code in the context of the default branch
+   - Validate restored cache contents before use.
+   - Use short-lived, workflow-specific cache keys.
+   - Clear caches regularly.
+3. Implement strict isolation between untrusted and privileged workflow execution.
+4. Never run untrusted code in the context of the default branch.
 5. Sign the cache value cryptographically and verify the signature before usage.
 
 ## Examples

ql/src/Security/CWE-349/CachePoisoningViaDirectCache.md

@@ -2,14 +2,14 @@
 
 ## Description
 
-GitHub Actions cache poisoning is a technique that allows an attacker to inject malicious content into the Action's cache, potentially leading to code execution in privileged workflows.
+GitHub Actions cache poisoning is a technique that allows an attacker to inject malicious content into the Action's cache from unprivileged workflow, potentially leading to code execution in privileged workflows.
 
 An attacker with the ability to run code in the context of the default branch (e.g. through Code Injection or Execution of Untrusted Code) can exploit this to:
 
-1. Steal the cache access token and URL
-2. Fill the cache to trigger eviction of legitimate entries
-3. Poison cache entries with malicious payloads
-4. Achieve code execution in privileged workflows that restore the poisoned cache
+1. Steal the cache access token and URL.
+2. Overflow the cache to trigger eviction of legitimate entries.
+3. Poison cache entries with malicious payloads.
+4. Achieve code execution in privileged workflows that restore the poisoned cache.
 
 This allows lateral movement from low-privileged to high-privileged workflows within a repository.
 
@@ -27,11 +27,11 @@ Due to the above design, if something is cached in the context of the default br
 
 1. Avoid using caching in workflows that handle sensitive operations like releases.
 2. If caching must be used:
-   - Validate restored cache contents before use
-   - Use short-lived, workflow-specific cache keys
-   - Clear caches regularly
-3. Implement strict isolation between untrusted and privileged workflow execution:
-4. Never run untrusted code in the context of the default branch
+   - Validate restored cache contents before use.
+   - Use short-lived, workflow-specific cache keys.
+   - Clear caches regularly.
+3. Implement strict isolation between untrusted and privileged workflow execution.
+4. Never run untrusted code in the context of the default branch.
 5. Sign the cache value cryptographically and verify the signature before usage.
 
 ## Examples
@@ -69,13 +69,12 @@ jobs:
 
 ### Correct Usage
 
-The following workflow is not checking out untrusted files and, therefore, is caching trusted files only.
+The following workflow checking out untrusted files, but the cache is scoped to the Pull Request.
 
 ```yaml
 name: Secure Workflow
 on:
-  issue_comment:
-    types: [created]
+  pull_request:
 
 jobs:
   pr-comment:
@@ -94,6 +93,34 @@ 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
 
 - [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/)

ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.md

@@ -2,14 +2,14 @@
 
 ## Description
 
-GitHub Actions cache poisoning is a technique that allows an attacker to inject malicious content into the Action's cache, potentially leading to code execution in privileged workflows.
+GitHub Actions cache poisoning is a technique that allows an attacker to inject malicious content into the Action's cache from unprivileged workflow, potentially leading to code execution in privileged workflows.
 
 An attacker with the ability to run code in the context of the default branch (e.g. through Code Injection or Execution of Untrusted Code) can exploit this to:
 
-1. Steal the cache access token and URL
-2. Fill the cache to trigger eviction of legitimate entries
-3. Poison cache entries with malicious payloads
-4. Achieve code execution in privileged workflows that restore the poisoned cache
+1. Steal the cache access token and URL.
+2. Overflow the cache to trigger eviction of legitimate entries.
+3. Poison cache entries with malicious payloads.
+4. Achieve code execution in privileged workflows that restore the poisoned cache.
 
 This allows lateral movement from low-privileged to high-privileged workflows within a repository.
 
@@ -27,11 +27,11 @@ Due to the above design, if something is cached in the context of the default br
 
 1. Avoid using caching in workflows that handle sensitive operations like releases.
 2. If caching must be used:
-   - Validate restored cache contents before use
-   - Use short-lived, workflow-specific cache keys
-   - Clear caches regularly
-3. Implement strict isolation between untrusted and privileged workflow execution:
-4. Never run untrusted code in the context of the default branch
+   - Validate restored cache contents before use.
+   - Use short-lived, workflow-specific cache keys.
+   - Clear caches regularly.
+3. Implement strict isolation between untrusted and privileged workflow execution.
+4. Never run untrusted code in the context of the default branch.
 5. Sign the cache value cryptographically and verify the signature before usage.
 
 ## Examples
@@ -59,7 +59,7 @@ jobs:
 
 ### Correct Usage
 
-The following workflow runs untrusted code in a non-privileged job and in the context of a non-default branch.
+The following workflow runs untrusted code in a non-privileged job and the cache is scoped to the Pull Request branch.
 
 ```yaml
 name: Secure Workflow

ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.md

@@ -1,4 +1,4 @@
-# Untrusted Checkout TOCTOU
+# Untrusted Checkout TOCTOU (Time-of-check to time-of-use)
 
 ## Description
 
@@ -8,77 +8,11 @@ Untrusted Checkout is protected by a security check but the checked-out branch c
 
 Verify that the code has not been modified after the security check. This may be achieved differently depending on the type of check:
 
-- Issue Ops: Verify that Commit containing the code to be executed was commited **before** then date the of the comment.
 - Deployment Environment Approval: Make sure to use a non-mutable reference to the code to be executed. For example use a `sha` instead of a `ref`.
 - Label Gates: Make sure to use a non-mutable reference to the code to be executed. For example use a `sha` instead of a `ref`.
 
 ## Examples
 
-### Incorrect Usage (Issue Ops)
-
-The following workflow runs untrusted code after either a member or admin of the repository comments on a Pull Request with the text `/run-tests`. Although it may seem secure, the workflow is checking out a mutable reference (`${{ steps.comment-branch.outputs.head_ref }}`) and therefore the code can be mutated between the time of check (TOC) and the time of use (TOU).
-
-```yaml
-name: Comment Triggered Test
-on:
-  issue_comment:
-    types: [created]
-jobs:
-  benchmark:
-    name: Integration Tests
-    if: ${{ github.event.issue.pull_request && contains(fromJson('["MEMBER", "OWNER"]'), github.event.comment.author_association) && startsWith(github.event.comment.body, '/run-tests ') }}
-    permissions: "write-all"
-    runs-on: [ubuntu-latest]
-    steps:
-      - name: Get PR branch
-        uses: xt0rted/pull-request-comment-branch@v2
-        id: comment-branch
-      - name: Checkout PR branch
-        uses: actions/checkout@v3
-        with:
-          ref: ${{ steps.comment-branch.outputs.head_ref }}
-      - run: ./cmd
-```
-
-### Correct Usage (Issue Ops)
-
-In the following example, the workflow checks if the latest commit of the Pull Request head was commited **before** the comment on the Pull Request, therefore ensuring that it was not mutated after the check.
-
-```yaml
-name: Comment Triggered Test
-on:
-  issue_comment:
-    types: [created]
-jobs:
-  benchmark:
-    name: Integration Tests
-    if: ${{ github.event.issue.pull_request && contains(fromJson('["MEMBER", "OWNER"]'), github.event.comment.author_association) && startsWith(github.event.comment.body, '/run-tests ') }}
-    permissions: "write-all"
-    runs-on: [ubuntu-latest]
-    steps:
-      - name: Get PR Info
-        id: pr
-        env:
-          PR_NUMBER: ${{ github.event.issue.number }}
-          GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-          GH_REPO: ${{ github.repository }}
-          COMMENT_AT: ${{ github.event.comment.created_at }}
-        run: |
-          pr="$(gh api /repos/${GH_REPO}/pulls/${PR_NUMBER})"
-          head_sha="$(echo "$pr" | jq -r .head.sha)"
-          pushed_at="$(echo "$pr" | jq -r .pushed_at)"
-          if [[ $(date -d "$pushed_at" +%s) -gt $(date -d "$COMMENT_AT" +%s) ]]; then
-              echo "Updating is not allowed because the PR was pushed to (at $pushed_at) after the triggering comment was issued (at $COMMENT_AT)"
-              exit 1
-          fi
-          echo "head_sha=$head_sha" >> $GITHUB_OUTPUT
-      - name: Checkout PR branch
-        uses: actions/checkout@v3
-        with:
-          ref: ${{ steps.pr.outputs.head_sha }}
-      - run: ./cmd
-```
-
 ### Incorrect Usage (Deployment Environment Approval)
 
 The following workflow uses a Deployment Environment which may be configured to require an approval. However, it check outs the code pointed to by the Pull Request branch reference. At attacker could submit legitimate code for review and then change it once it gets approved.
@@ -102,7 +36,7 @@ jobs:
 
 ### Correct Usage (Deployment Environment Approval)
 
-Use inmutable references (Commit SHA) to make sure that the reviewd code does not change between the check and the use.
+Use immutable references (Commit SHA) to make sure that the reviewed code does not change between the check and the use.
 
 ```yml
 on:
@@ -144,7 +78,7 @@ jobs:
 
 ### Correct Usage (Label Gates)
 
-Use inmutable references (Commit SHA) to make sure that the reviewd code does not change between the check and the use.
+Use immutable references (Commit SHA) to make sure that the reviewed code does not change between the check and the use.
 
 ```yaml
 on: