Refactor process.env handling in CleartextLogging and IndirectCommandInjection modules to use ThreatModelSource

Napalys · Napalys · commit d4b5ef6a6659 · 2025-05-01T11:14:15.000+02:00
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll
@@ -171,7 +171,7 @@ module CleartextLogging {
 
   /** An access to the sensitive object `process.env`. */
   class ProcessEnvSource extends Source {
-    ProcessEnvSource() { this = NodeJSLib::process().getAPropertyRead("env") }
+    ProcessEnvSource() { this.(ThreatModelSource).getThreatModel() = "environment" }
 
     override string describe() { result = "process environment" }
   }
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionCustomizations.qll
@@ -29,15 +29,15 @@ module IndirectCommandInjection {
    * A read of `process.env`, considered as a flow source for command injection.
    */
   private class ProcessEnvAsSource extends Source {
-    ProcessEnvAsSource() { this = NodeJSLib::process().getAPropertyRead("env") }
+    ProcessEnvAsSource() { this.(ThreatModelSource).getThreatModel() = "environment" }
 
     override string describe() { result = "environment variable" }
   }
 
   /** Gets a data flow node referring to `process.env`. */
   private DataFlow::SourceNode envObject(DataFlow::TypeTracker t) {
     t.start() and
-    result = NodeJSLib::process().getAPropertyRead("env")
+    result.(ThreatModelSource).getThreatModel() = "environment"
     or
     exists(DataFlow::TypeTracker t2 | result = envObject(t2).track(t2, t))
   }
diff --git a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql
@@ -37,7 +37,7 @@ DataFlow::ObjectLiteralNode tlsOptions() { result.flowsTo(tlsInvocation().getAnA
 from DataFlow::PropWrite disable
 where
   exists(DataFlow::SourceNode env |
-    env = NodeJSLib::process().getAPropertyRead("env") and
+    env.(ThreatModelSource).getThreatModel() = "environment" and
     disable = env.getAPropertyWrite("NODE_TLS_REJECT_UNAUTHORIZED") and
     disable.getRhs().mayHaveStringValue("0")
   )
diff --git a/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected b/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected
@@ -2,6 +2,9 @@
 | actions.js:4:6:4:29 | process ... _DATA'] | actions.js:4:6:4:16 | process.env | actions.js:4:6:4:29 | process ... _DATA'] | This command depends on an unsanitized $@. | actions.js:4:6:4:16 | process.env | environment variable |
 | actions.js:8:10:8:23 | e['TEST_DATA'] | actions.js:12:6:12:16 | process.env | actions.js:8:10:8:23 | e['TEST_DATA'] | This command depends on an unsanitized $@. | actions.js:12:6:12:16 | process.env | environment variable |
 | actions.js:14:6:14:21 | getInput('data') | actions.js:14:6:14:21 | getInput('data') | actions.js:14:6:14:21 | getInput('data') | This command depends on an unsanitized $@. | actions.js:14:6:14:21 | getInput('data') | GitHub Actions user input |
+| actions.js:18:10:18:40 | 'rm -rf ... 'SOME'] | actions.js:18:22:18:32 | shelljs.env | actions.js:18:10:18:40 | 'rm -rf ... 'SOME'] | This command depends on an unsanitized $@. | actions.js:18:22:18:32 | shelljs.env | environment variable |
+| actions.js:19:10:19:37 | 'rm -rf ... nv.SOME | actions.js:19:22:19:32 | shelljs.env | actions.js:19:10:19:37 | 'rm -rf ... nv.SOME | This command depends on an unsanitized $@. | actions.js:19:22:19:32 | shelljs.env | environment variable |
+| actions.js:20:10:20:32 | 'rm -rf ... ljs.env | actions.js:20:22:20:32 | shelljs.env | actions.js:20:10:20:32 | 'rm -rf ... ljs.env | This command depends on an unsanitized $@. | actions.js:20:22:20:32 | shelljs.env | environment variable |
 | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | command-line argument |
 | command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line argument |
 | command-line-parameter-command-injection.js:11:14:11:20 | args[0] | command-line-parameter-command-injection.js:10:13:10:24 | process.argv | command-line-parameter-command-injection.js:11:14:11:20 | args[0] | This command depends on an unsanitized $@. | command-line-parameter-command-injection.js:10:13:10:24 | process.argv | command-line argument |
@@ -44,6 +47,9 @@ edges
 | actions.js:7:15:7:15 | e | actions.js:8:10:8:10 | e | provenance |  |
 | actions.js:8:10:8:10 | e | actions.js:8:10:8:23 | e['TEST_DATA'] | provenance |  |
 | actions.js:12:6:12:16 | process.env | actions.js:7:15:7:15 | e | provenance |  |
+| actions.js:18:22:18:32 | shelljs.env | actions.js:18:10:18:40 | 'rm -rf ... 'SOME'] | provenance |  |
+| actions.js:19:22:19:32 | shelljs.env | actions.js:19:10:19:37 | 'rm -rf ... nv.SOME | provenance |  |
+| actions.js:20:22:20:32 | shelljs.env | actions.js:20:10:20:32 | 'rm -rf ... ljs.env | provenance |  |
 | command-line-parameter-command-injection.js:8:22:8:33 | process.argv | command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | provenance |  |
 | command-line-parameter-command-injection.js:10:6:10:33 | args | command-line-parameter-command-injection.js:11:14:11:17 | args | provenance |  |
 | command-line-parameter-command-injection.js:10:6:10:33 | args | command-line-parameter-command-injection.js:12:26:12:29 | args | provenance |  |
@@ -181,6 +187,12 @@ nodes
 | actions.js:8:10:8:23 | e['TEST_DATA'] | semmle.label | e['TEST_DATA'] |
 | actions.js:12:6:12:16 | process.env | semmle.label | process.env |
 | actions.js:14:6:14:21 | getInput('data') | semmle.label | getInput('data') |
+| actions.js:18:10:18:40 | 'rm -rf ... 'SOME'] | semmle.label | 'rm -rf ... 'SOME'] |
+| actions.js:18:22:18:32 | shelljs.env | semmle.label | shelljs.env |
+| actions.js:19:10:19:37 | 'rm -rf ... nv.SOME | semmle.label | 'rm -rf ... nv.SOME |
+| actions.js:19:22:19:32 | shelljs.env | semmle.label | shelljs.env |
+| actions.js:20:10:20:32 | 'rm -rf ... ljs.env | semmle.label | 'rm -rf ... ljs.env |
+| actions.js:20:22:20:32 | shelljs.env | semmle.label | shelljs.env |
 | command-line-parameter-command-injection.js:4:10:4:21 | process.argv | semmle.label | process.argv |
 | command-line-parameter-command-injection.js:8:10:8:36 | "cmd.sh ... argv[2] | semmle.label | "cmd.sh ... argv[2] |
 | command-line-parameter-command-injection.js:8:22:8:33 | process.argv | semmle.label | process.argv |
diff --git a/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/actions.js b/javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/actions.js
@@ -15,7 +15,7 @@ exec(getInput('data')); // $ Alert
 
 function test2(e) {
     const shelljs = require('shelljs');
-    exec('rm -rf ' + shelljs.env['SOME']); // $ MISSING: Alert
-    exec('rm -rf ' + shelljs.env.SOME); // $ MISSING: Alert
-    exec('rm -rf ' + shelljs.env); // $ MISSING: Alert
+    exec('rm -rf ' + shelljs.env['SOME']); // $ Alert
+    exec('rm -rf ' + shelljs.env.SOME); // $ Alert
+    exec('rm -rf ' + shelljs.env); // $ Alert
 }

Original file line number	Diff line number	Diff line change
`@@ -171,7 +171,7 @@ module CleartextLogging {`
`171`	`171`
`172`	`172`	/** An access to the sensitive object `process.env`. */
`173`	`173`	`class ProcessEnvSource extends Source {`
`174`		`- ProcessEnvSource() { this = NodeJSLib::process().getAPropertyRead("env") }`
	`174`	`+ ProcessEnvSource() { this.(ThreatModelSource).getThreatModel() = "environment" }`
`175`	`175`
`176`	`176`	`override string describe() { result = "process environment" }`
`177`	`177`	`}`
Original file line number	Diff line number	Diff line change
`@@ -37,7 +37,7 @@ DataFlow::ObjectLiteralNode tlsOptions() { result.flowsTo(tlsInvocation().getAnA`
`37`	`37`	`from DataFlow::PropWrite disable`
`38`	`38`	`where`
`39`	`39`	`exists(DataFlow::SourceNode env \|`
`40`		`- env = NodeJSLib::process().getAPropertyRead("env") and`
	`40`	`+ env.(ThreatModelSource).getThreatModel() = "environment" and`
`41`	`41`	`disable = env.getAPropertyWrite("NODE_TLS_REJECT_UNAUTHORIZED") and`
`42`	`42`	`disable.getRhs().mayHaveStringValue("0")`
`43`	`43`	`)`