From 397eb2a762ae15ff89237c7b8db0f8443ef9fdb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Mon, 5 Aug 2024 23:44:20 +0200 Subject: [PATCH] Add getPath() to PRHeadCheckout and CacheWriting classes Add getPath() methods to get the path where a checkout step writes the code and where a Cache write reads the files from. --- .../actions/security/CachePoisoningQuery.qll | 34 ++++++++++++++++++- .../security/UntrustedCheckoutQuery.qll | 30 +++++++++++++++- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll index 29c0ed4feed..8c1a9ee0fd7 100644 --- a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll +++ b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll @@ -44,14 +44,28 @@ predicate runsOnDefaultBranch(Event e) { ) } -abstract class CacheWritingStep extends Step { } +abstract class CacheWritingStep extends Step { + abstract string getPath(); +} class CacheActionUsesStep extends CacheWritingStep, UsesStep { CacheActionUsesStep() { this.getCallee() = "actions/cache" } + + override string getPath() { + if exists(this.(UsesStep).getArgument("path")) + then result = this.(UsesStep).getArgument("path").splitAt("\n") + else result = "?" + } } class CacheActionSaveUsesStep extends CacheWritingStep, UsesStep { CacheActionSaveUsesStep() { this.getCallee() = "actions/cache/save" } + + override string getPath() { + if exists(this.(UsesStep).getArgument("path")) + then result = this.(UsesStep).getArgument("path").splitAt("\n") + else result = "?" + } } class SetupJavaUsesStep extends CacheWritingStep, UsesStep { @@ -62,6 +76,9 @@ class SetupJavaUsesStep extends CacheWritingStep, UsesStep { exists(this.getArgument("cache-dependency-path")) ) } + + // TODO: Try to get the actual path being cached + override string getPath() { result = "?" } } class SetupGoUsesStep extends CacheWritingStep, UsesStep { @@ -73,6 +90,9 @@ class SetupGoUsesStep extends CacheWritingStep, UsesStep { this.getArgument("cache") = "true" ) } + + // TODO: Try to get the actual path being cached + override string getPath() { result = "?" } } class SetupNodeUsesStep extends CacheWritingStep, UsesStep { @@ -83,6 +103,9 @@ class SetupNodeUsesStep extends CacheWritingStep, UsesStep { exists(this.getArgument("cache-dependency-path")) ) } + + // TODO: Try to get the actual path being cached + override string getPath() { result = "?" } } class SetupPythonUsesStep extends CacheWritingStep, UsesStep { @@ -93,6 +116,9 @@ class SetupPythonUsesStep extends CacheWritingStep, UsesStep { exists(this.getArgument("cache-dependency-path")) ) } + + // TODO: Try to get the actual path being cached + override string getPath() { result = "?" } } class SetupDotnetUsesStep extends CacheWritingStep, UsesStep { @@ -103,6 +129,9 @@ class SetupDotnetUsesStep extends CacheWritingStep, UsesStep { exists(this.getArgument("cache-dependency-path")) ) } + + // TODO: Try to get the actual path being cached + override string getPath() { result = "?" } } class SetupRubyUsesStep extends CacheWritingStep, UsesStep { @@ -110,4 +139,7 @@ class SetupRubyUsesStep extends CacheWritingStep, UsesStep { this.getCallee() = ["actions/setup-ruby", "ruby/setup-ruby"] and this.getArgument("bundler-cache") = "true" } + + // TODO: Try to get the actual path being cached + override string getPath() { result = "?" } } diff --git a/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll b/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll index fba33bb8bc8..7cfda4da49c 100644 --- a/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll +++ b/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll @@ -1,6 +1,12 @@ import actions import codeql.actions.DataFlow +string getStepCWD() { + // TODO: This should be the path of the git command. + // Read if from the step's CWD, workspace or look for a cd command. + result = "?" +} + bindingset[s] predicate containsPullRequestNumber(string s) { exists( @@ -68,7 +74,9 @@ predicate containsHeadRef(string s) { } /** Checkout of a Pull Request HEAD */ -abstract class PRHeadCheckoutStep extends Step { } +abstract class PRHeadCheckoutStep extends Step { + abstract string getPath(); +} /** Checkout of a Pull Request HEAD ref */ abstract class MutableRefCheckoutStep extends PRHeadCheckoutStep { } @@ -138,6 +146,12 @@ class ActionsMutableRefCheckout extends MutableRefCheckoutStep instanceof UsesSt ) ) } + + override string getPath() { + if exists(this.(UsesStep).getArgument("path")) + then result = this.(UsesStep).getArgument("path") + else result = "?" + } } /** Checkout of a Pull Request HEAD ref using actions/checkout action */ @@ -194,6 +208,12 @@ class ActionsSHACheckout extends SHACheckoutStep instanceof UsesStep { ) ) } + + override string getPath() { + if exists(this.(UsesStep).getArgument("path")) + then result = this.(UsesStep).getArgument("path") + else result = "?" + } } /** Checkout of a Pull Request HEAD ref using git within a Run step */ @@ -216,6 +236,8 @@ class GitMutableRefCheckout extends MutableRefCheckoutStep instanceof Run { ) ) } + + override string getPath() { result = getStepCWD() } } /** Checkout of a Pull Request HEAD ref using git within a Run step */ @@ -235,6 +257,8 @@ class GitSHACheckout extends SHACheckoutStep instanceof Run { ) ) } + + override string getPath() { result = getStepCWD() } } /** Checkout of a Pull Request HEAD ref using gh within a Run step */ @@ -256,6 +280,8 @@ class GhMutableRefCheckout extends MutableRefCheckoutStep instanceof Run { ) ) } + + override string getPath() { result = getStepCWD() } } /** Checkout of a Pull Request HEAD ref using gh within a Run step */ @@ -274,4 +300,6 @@ class GhSHACheckout extends SHACheckoutStep instanceof Run { ) ) } + + override string getPath() { result = getStepCWD() } }