diff --git a/actions/ql/lib/codeql/Locations.qll b/actions/ql/lib/codeql/Locations.qll index 96b5d45f18e0..24c6ae9cda1d 100644 --- a/actions/ql/lib/codeql/Locations.qll +++ b/actions/ql/lib/codeql/Locations.qll @@ -70,8 +70,8 @@ class Location extends TLocation, TBaseLocation { /** * Holds if this element is at the specified location. - * The location spans column `startcolumn` of line `startline` to - * column `endcolumn` of line `endline` in file `filepath`. + * The location spans column `sc` of line `sl` to + * column `ec` of line `el` in file `p`. * For more information, see * [Providing locations in CodeQL queries](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/). */ diff --git a/actions/ql/lib/codeql/actions/Ast.qll b/actions/ql/lib/codeql/actions/Ast.qll index ae19a7a7e8ca..6e76e4cd665a 100644 --- a/actions/ql/lib/codeql/actions/Ast.qll +++ b/actions/ql/lib/codeql/actions/Ast.qll @@ -261,7 +261,7 @@ class If extends AstNode instanceof IfImpl { } /** - * An Environemnt node representing a deployment environment. + * An Environment node representing a deployment environment. */ class Environment extends AstNode instanceof EnvironmentImpl { string getName() { result = super.getName() } diff --git a/actions/ql/lib/codeql/actions/ast/internal/Ast.qll b/actions/ql/lib/codeql/actions/ast/internal/Ast.qll index b0cbb8a1d79e..b922214e21c5 100644 --- a/actions/ql/lib/codeql/actions/ast/internal/Ast.qll +++ b/actions/ql/lib/codeql/actions/ast/internal/Ast.qll @@ -125,12 +125,11 @@ abstract class AstNodeImpl extends TAstNode { * Gets the enclosing Step. */ StepImpl getEnclosingStep() { - if this instanceof StepImpl - then result = this - else - if this instanceof ScalarValueImpl - then result.getAChildNode*() = this.getParentNode() - else none() + this instanceof StepImpl and + result = this + or + this instanceof ScalarValueImpl and + result.getAChildNode*() = this.getParentNode() } /** @@ -1416,9 +1415,8 @@ class ExternalJobImpl extends JobImpl, UsesImpl { override string getVersion() { exists(YamlString name | n.lookup("uses") = name and - if not name.getValue().matches("\\.%") - then result = name.getValue().regexpCapture(repoUsesParser(), 4) - else none() + not name.getValue().matches("\\.%") and + result = name.getValue().regexpCapture(repoUsesParser(), 4) ) } } diff --git a/actions/ql/lib/codeql/actions/controlflow/BasicBlocks.qll b/actions/ql/lib/codeql/actions/controlflow/BasicBlocks.qll index af5e0f62552f..2dcfd81a47dc 100644 --- a/actions/ql/lib/codeql/actions/controlflow/BasicBlocks.qll +++ b/actions/ql/lib/codeql/actions/controlflow/BasicBlocks.qll @@ -286,7 +286,7 @@ private module Cached { /** * Holds if `cfn` is the `i`th node in basic block `bb`. * - * In other words, `i` is the shortest distance from a node `bb` + * In other words, `i` is the shortest distance from a node `bbStart` * that starts a basic block to `cfn` along the `intraBBSucc` relation. */ cached diff --git a/actions/ql/lib/codeql/actions/dataflow/ExternalFlow.qll b/actions/ql/lib/codeql/actions/dataflow/ExternalFlow.qll index 2914dac5f0a6..9667c6e525ea 100644 --- a/actions/ql/lib/codeql/actions/dataflow/ExternalFlow.qll +++ b/actions/ql/lib/codeql/actions/dataflow/ExternalFlow.qll @@ -63,10 +63,10 @@ predicate madSource(DataFlow::Node source, string kind, string fieldName) { ( if fieldName.trim().matches("env.%") then source.asExpr() = uses.getInScopeEnvVarExpr(fieldName.trim().replaceAll("env.", "")) - else - if fieldName.trim().matches("output.%") - then source.asExpr() = uses - else none() + else ( + fieldName.trim().matches("output.%") and + source.asExpr() = uses + ) ) ) } diff --git a/actions/ql/lib/codeql/actions/dataflow/FlowSources.qll b/actions/ql/lib/codeql/actions/dataflow/FlowSources.qll index df3d513d0050..18cc4322c81b 100644 --- a/actions/ql/lib/codeql/actions/dataflow/FlowSources.qll +++ b/actions/ql/lib/codeql/actions/dataflow/FlowSources.qll @@ -31,14 +31,14 @@ abstract class RemoteFlowSource extends SourceNode { class GitHubCtxSource extends RemoteFlowSource { string flag; string event; - GitHubExpression e; GitHubCtxSource() { - this.asExpr() = e and - // github.head_ref - e.getFieldName() = "head_ref" and - flag = "branch" and - ( + exists(GitHubExpression e | + this.asExpr() = e and + // github.head_ref + e.getFieldName() = "head_ref" and + flag = "branch" + | event = e.getATriggerEvent().getName() and event = "pull_request_target" or @@ -148,7 +148,6 @@ class GhCLICommandSource extends RemoteFlowSource, CommandSource { class GitHubEventPathSource extends RemoteFlowSource, CommandSource { string cmd; string flag; - string access_path; Run run; // Examples @@ -163,7 +162,7 @@ class GitHubEventPathSource extends RemoteFlowSource, CommandSource { run.getScript().getACommand() = cmd and cmd.matches("jq%") and cmd.matches("%GITHUB_EVENT_PATH%") and - exists(string regexp | + exists(string regexp, string access_path | untrustedEventPropertiesDataModel(regexp, flag) and not flag = "json" and access_path = "github.event" + cmd.regexpCapture(".*\\s+([^\\s]+)\\s+.*", 1) and diff --git a/actions/ql/lib/codeql/actions/security/ArgumentInjectionQuery.qll b/actions/ql/lib/codeql/actions/security/ArgumentInjectionQuery.qll index 679b8977cf91..1795e9493cb4 100644 --- a/actions/ql/lib/codeql/actions/security/ArgumentInjectionQuery.qll +++ b/actions/ql/lib/codeql/actions/security/ArgumentInjectionQuery.qll @@ -19,7 +19,6 @@ abstract class ArgumentInjectionSink extends DataFlow::Node { */ class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink { string command; - string argument; ArgumentInjectionFromEnvVarSink() { exists(Run run, string var | @@ -28,7 +27,7 @@ class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink { exists(run.getInScopeEnvVarExpr(var)) or var = "GITHUB_HEAD_REF" ) and - run.getScript().getAnEnvReachingArgumentInjectionSink(var, command, argument) + run.getScript().getAnEnvReachingArgumentInjectionSink(var, command, _) ) } @@ -44,13 +43,12 @@ class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink { */ class ArgumentInjectionFromCommandSink extends ArgumentInjectionSink { string command; - string argument; ArgumentInjectionFromCommandSink() { exists(CommandSource source, Run run | run = source.getEnclosingRun() and this.asExpr() = run.getScript() and - run.getScript().getACmdReachingArgumentInjectionSink(source.getCommand(), command, argument) + run.getScript().getACmdReachingArgumentInjectionSink(source.getCommand(), command, _) ) } diff --git a/actions/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll b/actions/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll index 76025a9ba0db..9f3ed33db961 100644 --- a/actions/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll +++ b/actions/ql/lib/codeql/actions/security/ArtifactPoisoningQuery.qll @@ -125,8 +125,6 @@ class LegitLabsDownloadArtifactActionStep extends UntrustedArtifactDownloadStep, } class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, UsesStep { - string script; - ActionsGitHubScriptDownloadStep() { // eg: // - uses: actions/github-script@v6 @@ -149,12 +147,14 @@ class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, Use // var fs = require('fs'); // fs.writeFileSync('${{github.workspace}}/test-results.zip', Buffer.from(download.data)); this.getCallee() = "actions/github-script" and - this.getArgument("script") = script and - script.matches("%listWorkflowRunArtifacts(%") and - script.matches("%downloadArtifact(%") and - script.matches("%writeFileSync(%") and - // Filter out artifacts that were created by pull-request. - not script.matches("%exclude_pull_requests: true%") + exists(string script | + this.getArgument("script") = script and + script.matches("%listWorkflowRunArtifacts(%") and + script.matches("%downloadArtifact(%") and + script.matches("%writeFileSync(%") and + // Filter out artifacts that were created by pull-request. + not script.matches("%exclude_pull_requests: true%") + ) } override string getPath() { @@ -171,10 +171,10 @@ class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, Use .getScript() .getACommand() .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 3))) - else - if this.getAFollowingStep().(Run).getScript().getACommand().regexpMatch(unzipRegexp()) - then result = "GITHUB_WORKSPACE/" - else none() + else ( + this.getAFollowingStep().(Run).getScript().getACommand().regexpMatch(unzipRegexp()) and + result = "GITHUB_WORKSPACE/" + ) } } @@ -207,12 +207,13 @@ class GHRunArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run { .getScript() .getACommand() .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 3))) - else - if + else ( + ( this.getAFollowingStep().(Run).getScript().getACommand().regexpMatch(unzipRegexp()) or this.getScript().getACommand().regexpMatch(unzipRegexp()) - then result = "GITHUB_WORKSPACE/" - else none() + ) and + result = "GITHUB_WORKSPACE/" + ) } } @@ -259,15 +260,15 @@ class DirectArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run { class ArtifactPoisoningSink extends DataFlow::Node { UntrustedArtifactDownloadStep download; - PoisonableStep poisonable; ArtifactPoisoningSink() { - download.getAFollowingStep() = poisonable and - // excluding artifacts downloaded to the temporary directory - not download.getPath().regexpMatch("^/tmp.*") and - not download.getPath().regexpMatch("^\\$\\{\\{\\s*runner\\.temp\\s*}}.*") and - not download.getPath().regexpMatch("^\\$RUNNER_TEMP.*") and - ( + exists(PoisonableStep poisonable | + download.getAFollowingStep() = poisonable and + // excluding artifacts downloaded to the temporary directory + not download.getPath().regexpMatch("^/tmp.*") and + not download.getPath().regexpMatch("^\\$\\{\\{\\s*runner\\.temp\\s*}}.*") and + not download.getPath().regexpMatch("^\\$RUNNER_TEMP.*") + | poisonable.(Run).getScript() = this.asExpr() and ( // Check if the poisonable step is a local script execution step diff --git a/actions/ql/lib/codeql/actions/security/ControlChecks.qll b/actions/ql/lib/codeql/actions/security/ControlChecks.qll index 244c04310d6d..41f512abbc34 100644 --- a/actions/ql/lib/codeql/actions/security/ControlChecks.qll +++ b/actions/ql/lib/codeql/actions/security/ControlChecks.qll @@ -159,11 +159,8 @@ abstract class CommentVsHeadDateCheck extends ControlCheck { /* Specific implementations of control checks */ class LabelIfCheck extends LabelCheck instanceof If { - string condition; - LabelIfCheck() { - condition = normalizeExpr(this.getCondition()) and - ( + exists(string condition | condition = normalizeExpr(this.getCondition()) | // eg: contains(github.event.pull_request.labels.*.name, 'safe to test') condition.regexpMatch(".*(^|[^!])contains\\(\\s*github\\.event\\.pull_request\\.labels\\b.*") or diff --git a/actions/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll b/actions/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll index 2022e3dca998..ea8a800ef3f6 100644 --- a/actions/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll +++ b/actions/ql/lib/codeql/actions/security/EnvVarInjectionQuery.qll @@ -55,12 +55,8 @@ class EnvVarInjectionFromFileReadSink extends EnvVarInjectionSink { * echo "COMMIT_MESSAGE=${COMMIT_MESSAGE}" >> $GITHUB_ENV */ class EnvVarInjectionFromCommandSink extends EnvVarInjectionSink { - CommandSource inCommand; - string injectedVar; - string command; - EnvVarInjectionFromCommandSink() { - exists(Run run | + exists(Run run, CommandSource inCommand, string injectedVar, string command | this.asExpr() = inCommand.getEnclosingRun().getScript() and run = inCommand.getEnclosingRun() and run.getScript().getACmdReachingGitHubEnvWrite(inCommand.getCommand(), injectedVar) and @@ -86,12 +82,8 @@ class EnvVarInjectionFromCommandSink extends EnvVarInjectionSink { * echo "FOO=$BODY" >> $GITHUB_ENV */ class EnvVarInjectionFromEnvVarSink extends EnvVarInjectionSink { - string inVar; - string injectedVar; - string command; - EnvVarInjectionFromEnvVarSink() { - exists(Run run | + exists(Run run, string inVar, string injectedVar, string command | run.getScript() = this.asExpr() and exists(run.getInScopeEnvVarExpr(inVar)) and run.getScript().getAnEnvReachingGitHubEnvWrite(inVar, injectedVar) and diff --git a/actions/ql/lib/codeql/actions/security/OutputClobberingQuery.qll b/actions/ql/lib/codeql/actions/security/OutputClobberingQuery.qll index c67d2876b091..4454a5496a2f 100644 --- a/actions/ql/lib/codeql/actions/security/OutputClobberingQuery.qll +++ b/actions/ql/lib/codeql/actions/security/OutputClobberingQuery.qll @@ -99,18 +99,14 @@ class OutputClobberingFromEnvVarSink extends OutputClobberingSink { * echo $BODY */ class WorkflowCommandClobberingFromEnvVarSink extends OutputClobberingSink { - string clobbering_var; - string clobbered_value; - WorkflowCommandClobberingFromEnvVarSink() { - exists(Run run, string workflow_cmd_stmt, string clobbering_stmt | + exists(Run run, string workflow_cmd_stmt, string clobbering_stmt, string clobbering_var | run.getScript() = this.asExpr() and run.getScript().getAStmt() = clobbering_stmt and clobbering_stmt.regexpMatch("echo\\s+(-e\\s+)?(\"|')?\\$(\\{)?" + clobbering_var + ".*") and exists(run.getInScopeEnvVarExpr(clobbering_var)) and run.getScript().getAStmt() = workflow_cmd_stmt and - clobbered_value = - trimQuotes(workflow_cmd_stmt.regexpCapture(".*::set-output\\s+name=.*::(.*)", 1)) + exists(trimQuotes(workflow_cmd_stmt.regexpCapture(".*::set-output\\s+name=.*::(.*)", 1))) ) } } diff --git a/actions/ql/lib/codeql/actions/security/UseOfUnversionedImmutableAction.qll b/actions/ql/lib/codeql/actions/security/UseOfUnversionedImmutableAction.qll index ef258fce2e5c..8595cd1086d6 100644 --- a/actions/ql/lib/codeql/actions/security/UseOfUnversionedImmutableAction.qll +++ b/actions/ql/lib/codeql/actions/security/UseOfUnversionedImmutableAction.qll @@ -1,10 +1,8 @@ import actions class UnversionedImmutableAction extends UsesStep { - string immutable_action; - UnversionedImmutableAction() { - isImmutableAction(this, immutable_action) and + isImmutableAction(this, _) and not isSemVer(this.getVersion()) } } diff --git a/actions/ql/src/experimental/Security/CWE-829/ArtifactPoisoningPathTraversal.ql b/actions/ql/src/experimental/Security/CWE-829/ArtifactPoisoningPathTraversal.ql index 519437ddb229..517a9d1eaad7 100644 --- a/actions/ql/src/experimental/Security/CWE-829/ArtifactPoisoningPathTraversal.ql +++ b/actions/ql/src/experimental/Security/CWE-829/ArtifactPoisoningPathTraversal.ql @@ -37,8 +37,6 @@ where ) or // upload artifact is not used in the same workflow - not exists(UsesStep upload | - download.getEnclosingWorkflow().getAJob().(LocalJob).getAStep() = upload - ) + not download.getEnclosingWorkflow().getAJob().(LocalJob).getAStep() instanceof UsesStep ) select download, "Potential artifact poisoning"