Use YamlMapping for modeling Env

JarLob · JarLob · commit 79218a394672 · 2023-04-14T00:56:51.000+02:00
diff --git a/javascript/ql/lib/semmle/javascript/Actions.qll b/javascript/ql/lib/semmle/javascript/Actions.qll
@@ -75,7 +75,7 @@ module Actions {
     YamlMapping getJobs() { result = this.lookup("jobs") }
 
     /** Gets the 'global' `env` mapping in this workflow. */
-    YamlMapping getEnv() { result = this.lookup("env") }
+    WorkflowEnv getEnv() { result = this.lookup("env") }
 
     /** Gets the name of the workflow. */
     string getName() { result = this.lookup("name").(YamlString).getValue() }
@@ -103,52 +103,36 @@ module Actions {
     Workflow getWorkflow() { result = workflow }
   }
 
-  /** An environment variable in 'env:' */
-  abstract class EnvVariable extends YamlNode, YamlString {
-    /** Gets the name of this environment variable. */
-    abstract string getName();
-  }
+  abstract class Env extends YamlNode, YamlMapping { }
 
-  /** A workflow level 'global' environment variable. */
-  class WorkflowEnvVariable extends EnvVariable {
-    string envName;
+  /** A workflow level `env` mapping. */
+  class WorkflowEnv extends Env {
     Workflow workflow;
 
-    WorkflowEnvVariable() { this = workflow.getEnv().lookup(envName) }
+    WorkflowEnv() { workflow.lookup("env") = this }
 
     /** Gets the workflow this field belongs to. */
     Workflow getWorkflow() { result = workflow }
-
-    /** Gets the name of this environment variable. */
-    override string getName() { result = envName }
   }
 
-  /** A job level environment variable. */
-  class JobEnvVariable extends EnvVariable {
-    string envName;
+  /** A job level `env` mapping. */
+  class JobEnv extends Env {
     Job job;
 
-    JobEnvVariable() { this = job.getEnv().lookup(envName) }
+    JobEnv() { job.lookup("env") = this }
 
     /** Gets the job this field belongs to. */
     Job getJob() { result = job }
-
-    /** Gets the name of this environment variable. */
-    override string getName() { result = envName }
   }
 
-  /** A step level environment variable. */
-  class StepEnvVariable extends EnvVariable {
-    string envName;
+  /** A step level `env` mapping. */
+  class StepEnv extends Env {
     Step step;
 
-    StepEnvVariable() { this = step.getEnv().lookup(envName) }
+    StepEnv() { step.lookup("env") = this }
 
     /** Gets the step this field belongs to. */
     Step getStep() { result = step }
-
-    /** Gets the name of this environment variable. */
-    override string getName() { result = envName }
   }
 
   /**
@@ -183,7 +167,7 @@ module Actions {
     Step getStep(int index) { result.getJob() = this and result.getIndex() = index }
 
     /** Gets the `env` mapping in this job. */
-    YamlMapping getEnv() { result = this.lookup("env") }
+    JobEnv getEnv() { result = this.lookup("env") }
 
     /** Gets the workflow this job belongs to. */
     Workflow getWorkflow() { result = workflow }
@@ -250,7 +234,7 @@ module Actions {
     StepIf getIf() { result.getStep() = this }
 
     /** Gets the value of the `env` field in this step, if any. */
-    YamlMapping getEnv() { result = this.lookup("env") }
+    StepEnv getEnv() { result = this.lookup("env") }
 
     /** Gets the ID of this step, if any. */
     string getId() { result = this.lookup("id").(YamlString).getValue() }
diff --git a/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql b/javascript/ql/src/Security/CWE-094/ExpressionInjection.ql
@@ -108,9 +108,12 @@ private predicate isExternalUserControlledWorkflowRun(string context) {
  * is where the external user controlled value was assigned to.
  */
 bindingset[injection]
-predicate isEnvTainted(Actions::EnvVariable env, string injection, string context) {
-  Actions::getEnvName(injection) = env.getName() and
-  Actions::getASimpleReferenceExpression(env) = context
+predicate isEnvTainted(string injection, string context) {
+  exists(Actions::Env env, string envName, YamlString envValue |
+    envValue = env.lookup(envName) and
+    Actions::getEnvName(injection) = envName and
+    Actions::getASimpleReferenceExpression(envValue) = context
+  )
 }
 
 /**
@@ -122,7 +125,7 @@ predicate isRunInjectable(Actions::Run run, string injection, string context) {
   (
     injection = context
     or
-    isEnvTainted(_, injection, context)
+    isEnvTainted(injection, context)
   )
 }
 
@@ -139,7 +142,7 @@ predicate isScriptInjectable(Actions::Script script, string injection, string co
     (
       injection = context
       or
-      isEnvTainted(_, injection, context)
+      isEnvTainted(injection, context)
     )
   )
 }

Original file line number	Diff line number	Diff line change
`@@ -108,9 +108,12 @@ private predicate isExternalUserControlledWorkflowRun(string context) {`
`108`	`108`	`* is where the external user controlled value was assigned to.`
`109`	`109`	`*/`
`110`	`110`	`bindingset[injection]`
`111`		`-predicate isEnvTainted(Actions::EnvVariable env, string injection, string context) {`
`112`		`- Actions::getEnvName(injection) = env.getName() and`
`113`		`- Actions::getASimpleReferenceExpression(env) = context`
	`111`	`+predicate isEnvTainted(string injection, string context) {`
	`112`	`+ exists(Actions::Env env, string envName, YamlString envValue \|`
	`113`	`+ envValue = env.lookup(envName) and`
	`114`	`+ Actions::getEnvName(injection) = envName and`
	`115`	`+ Actions::getASimpleReferenceExpression(envValue) = context`
	`116`	`+ )`
`114`	`117`	`}`
`115`	`118`
`116`	`119`	`/**`
`@@ -122,7 +125,7 @@ predicate isRunInjectable(Actions::Run run, string injection, string context) {`
`122`	`125`	`(`
`123`	`126`	`injection = context`
`124`	`127`	`or`
`125`		`- isEnvTainted(_, injection, context)`
	`128`	`+ isEnvTainted(injection, context)`
`126`	`129`	`)`
`127`	`130`	`}`
`128`	`131`
`@@ -139,7 +142,7 @@ predicate isScriptInjectable(Actions::Script script, string injection, string co`
`139`	`142`	`(`
`140`	`143`	`injection = context`
`141`	`144`	`or`
`142`		`- isEnvTainted(_, injection, context)`
	`145`	`+ isEnvTainted(injection, context)`
`143`	`146`	`)`
`144`	`147`	`)`
`145`	`148`	`}`