Merge pull request #15060 from am0o0/amammad-js-envinjection · github/codeql@db76896 · GitHub
Skip to content

Commit db76896

Browse files
erik-krogherik-krogh
authored
Merge pull request #15060 from am0o0/amammad-js-envinjection
JS: Env Injection query
2 parents 555d7e5 + 4e1f7a9 commit db76896

12 files changed

Lines changed: 300 additions & 0 deletions

File tree

‎javascript/ql/src/experimental/Security/CWE-099/EnvValueAndKeyInjection.qhelp‎

Lines changed: 36 additions & 0 deletions

‎javascript/ql/src/experimental/Security/CWE-099/EnvValueAndKeyInjection.ql‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
/**
2+
* @name User controlled arbitrary environment variable injection
3+
* @description creating arbitrary environment variables from user controlled data is not secure
4+
* @kind path-problem
5+
* @id js/env-key-and-value-injection
6+
* @problem.severity error
7+
* @security-severity 7.5
8+
* @precision medium
9+
* @tags security
10+
* external/cwe/cwe-089
11+
*/
12+
13+
import javascript
14+
import DataFlow::PathGraph
15+
16+
/** A taint tracking configuration for unsafe environment injection. */
17+
class Configuration extends TaintTracking::Configuration {
18+
Configuration() { this = "envInjection" }
19+
20+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
21+
22+
override predicate isSink(DataFlow::Node sink) {
23+
sink = keyOfEnv() or
24+
sink = valueOfEnv()
25+
}
26+
27+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
28+
exists(DataFlow::InvokeNode ikn |
29+
ikn = DataFlow::globalVarRef("Object").getAMemberInvocation("keys")
30+
|
31+
pred = ikn.getArgument(0) and
32+
(
33+
succ = ikn.getAChainedMethodCall(["filter", "map"]) or
34+
succ = ikn or
35+
succ = ikn.getAChainedMethodCall("forEach").getABoundCallbackParameter(0, 0)
36+
)
37+
)
38+
}
39+
}
40+
41+
DataFlow::Node keyOfEnv() {
42+
result =
43+
NodeJSLib::process().getAPropertyRead("env").getAPropertyWrite().getPropertyNameExpr().flow()
44+
}
45+
46+
DataFlow::Node valueOfEnv() {
47+
result = API::moduleImport("process").getMember("env").getAMember().asSink()
48+
}
49+
50+
private predicate readToProcessEnv(DataFlow::Node envKey, DataFlow::Node envValue) {
51+
exists(DataFlow::PropWrite env |
52+
env = NodeJSLib::process().getAPropertyRead("env").getAPropertyWrite()
53+
|
54+
envKey = env.getPropertyNameExpr().flow() and
55+
envValue = env.getRhs()
56+
)
57+
}
58+
59+
from
60+
Configuration cfgForValue, Configuration cfgForKey, DataFlow::PathNode source,
61+
DataFlow::PathNode envKey, DataFlow::PathNode envValue
62+
where
63+
cfgForValue.hasFlowPath(source, envKey) and
64+
envKey.getNode() = keyOfEnv() and
65+
cfgForKey.hasFlowPath(source, envValue) and
66+
envValue.getNode() = valueOfEnv() and
67+
readToProcessEnv(envKey.getNode(), envValue.getNode())
68+
select envKey.getNode(), source, envKey, "arbitrary environment variable assignment from this $@.",
69+
source.getNode(), "user controllable source"

‎javascript/ql/src/experimental/Security/CWE-099/EnvValueInjection.qhelp‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
8+
<p>
9+
Assigning Value to environment variables from user-controllable data is not safe.
10+
</p>
11+
12+
</overview>
13+
14+
<recommendation>
15+
16+
<p>
17+
Restrict this operation only to privileged users or only for some not important environment variables.
18+
</p>
19+
20+
</recommendation>
21+
22+
<example>
23+
24+
<p>
25+
The following example allows unauthorized users to assign a value to a critical environment variable.
26+
</p>
27+
28+
<sample src="examples/Bad_Value_Assignment.js" />
29+
30+
</example>
31+
32+
<references>
33+
<li><a href="https://huntr.com/bounties/00ec6847-125b-43e9-9658-d3cace1751d6/">Admin account TakeOver in mintplex-labs/anything-llm</a></li>
34+
</references>
35+
36+
</qhelp>

‎javascript/ql/src/experimental/Security/CWE-099/EnvValueInjection.ql‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/**
2+
* @name User controlled environment variable value injection
3+
* @description assigning important environment variables from user controlled data is not secure
4+
* @kind path-problem
5+
* @id js/env-value-injection
6+
* @problem.severity error
7+
* @security-severity 7.5
8+
* @precision medium
9+
* @tags security
10+
* external/cwe/cwe-089
11+
*/
12+
13+
import javascript
14+
import DataFlow::PathGraph
15+
16+
/** A taint tracking configuration for unsafe environment injection. */
17+
class Configuration extends TaintTracking::Configuration {
18+
Configuration() { this = "envInjection" }
19+
20+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
21+
22+
override predicate isSink(DataFlow::Node sink) {
23+
sink = API::moduleImport("process").getMember("env").getAMember().asSink()
24+
}
25+
}
26+
27+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
28+
where cfg.hasFlowPath(source, sink)
29+
select sink.getNode(), source, sink, "this environment variable assignment is $@.",
30+
source.getNode(), "user controllable"

‎javascript/ql/src/experimental/Security/CWE-099/examples/Bad_Value_And_Key_Assignment.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
const http = require('node:http');
2+
3+
http.createServer((req, res) => {
4+
const { EnvValue, EnvKey } = req.body;
5+
process.env[EnvKey] = EnvValue; // NOT OK
6+
7+
res.end('env has been injected!');
8+
});

‎javascript/ql/src/experimental/Security/CWE-099/examples/Bad_Value_Assignment.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
const http = require('node:http');
2+
3+
http.createServer((req, res) => {
4+
const { EnvValue } = req.body;
5+
process.env["A_Critical_Env"] = EnvValue; // NOT OK
6+
7+
res.end('env has been injected!');
8+
});

‎javascript/ql/test/experimental/Security/CWE-099/EnvValueAndKeyInjection/EnvValueAndKeyInjection.expected‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
nodes
2+
| test.js:5:9:5:28 | { EnvValue, EnvKey } |
3+
| test.js:5:9:5:39 | EnvKey |
4+
| test.js:5:9:5:39 | EnvValue |
5+
| test.js:5:11:5:18 | EnvValue |
6+
| test.js:5:21:5:26 | EnvKey |
7+
| test.js:5:32:5:39 | req.body |
8+
| test.js:5:32:5:39 | req.body |
9+
| test.js:6:15:6:20 | EnvKey |
10+
| test.js:6:15:6:20 | EnvKey |
11+
| test.js:6:25:6:32 | EnvValue |
12+
| test.js:6:25:6:32 | EnvValue |
13+
| test.js:7:15:7:20 | EnvKey |
14+
| test.js:7:15:7:20 | EnvKey |
15+
| test.js:7:25:7:32 | EnvValue |
16+
| test.js:7:25:7:32 | EnvValue |
17+
| test.js:13:9:13:28 | { EnvValue, EnvKey } |
18+
| test.js:13:9:13:39 | EnvKey |
19+
| test.js:13:9:13:39 | EnvValue |
20+
| test.js:13:11:13:18 | EnvValue |
21+
| test.js:13:21:13:26 | EnvKey |
22+
| test.js:13:32:13:39 | req.body |
23+
| test.js:13:32:13:39 | req.body |
24+
| test.js:15:15:15:20 | EnvKey |
25+
| test.js:15:15:15:20 | EnvKey |
26+
| test.js:16:26:16:33 | EnvValue |
27+
| test.js:16:26:16:33 | EnvValue |
28+
edges
29+
| test.js:5:9:5:28 | { EnvValue, EnvKey } | test.js:5:11:5:18 | EnvValue |
30+
| test.js:5:9:5:28 | { EnvValue, EnvKey } | test.js:5:21:5:26 | EnvKey |
31+
| test.js:5:9:5:39 | EnvKey | test.js:6:15:6:20 | EnvKey |
32+
| test.js:5:9:5:39 | EnvKey | test.js:6:15:6:20 | EnvKey |
33+
| test.js:5:9:5:39 | EnvKey | test.js:7:15:7:20 | EnvKey |
34+
| test.js:5:9:5:39 | EnvKey | test.js:7:15:7:20 | EnvKey |
35+
| test.js:5:9:5:39 | EnvValue | test.js:6:25:6:32 | EnvValue |
36+
| test.js:5:9:5:39 | EnvValue | test.js:6:25:6:32 | EnvValue |
37+
| test.js:5:9:5:39 | EnvValue | test.js:7:25:7:32 | EnvValue |
38+
| test.js:5:9:5:39 | EnvValue | test.js:7:25:7:32 | EnvValue |
39+
| test.js:5:11:5:18 | EnvValue | test.js:5:9:5:39 | EnvValue |
40+
| test.js:5:21:5:26 | EnvKey | test.js:5:9:5:39 | EnvKey |
41+
| test.js:5:32:5:39 | req.body | test.js:5:9:5:28 | { EnvValue, EnvKey } |
42+
| test.js:5:32:5:39 | req.body | test.js:5:9:5:28 | { EnvValue, EnvKey } |
43+
| test.js:13:9:13:28 | { EnvValue, EnvKey } | test.js:13:11:13:18 | EnvValue |
44+
| test.js:13:9:13:28 | { EnvValue, EnvKey } | test.js:13:21:13:26 | EnvKey |
45+
| test.js:13:9:13:39 | EnvKey | test.js:15:15:15:20 | EnvKey |
46+
| test.js:13:9:13:39 | EnvKey | test.js:15:15:15:20 | EnvKey |
47+
| test.js:13:9:13:39 | EnvValue | test.js:16:26:16:33 | EnvValue |
48+
| test.js:13:9:13:39 | EnvValue | test.js:16:26:16:33 | EnvValue |
49+
| test.js:13:11:13:18 | EnvValue | test.js:13:9:13:39 | EnvValue |
50+
| test.js:13:21:13:26 | EnvKey | test.js:13:9:13:39 | EnvKey |
51+
| test.js:13:32:13:39 | req.body | test.js:13:9:13:28 | { EnvValue, EnvKey } |
52+
| test.js:13:32:13:39 | req.body | test.js:13:9:13:28 | { EnvValue, EnvKey } |
53+
#select
54+
| test.js:6:15:6:20 | EnvKey | test.js:5:32:5:39 | req.body | test.js:6:15:6:20 | EnvKey | arbitrary environment variable assignment from this $@. | test.js:5:32:5:39 | req.body | user controllable source |
55+
| test.js:7:15:7:20 | EnvKey | test.js:5:32:5:39 | req.body | test.js:7:15:7:20 | EnvKey | arbitrary environment variable assignment from this $@. | test.js:5:32:5:39 | req.body | user controllable source |

‎javascript/ql/test/experimental/Security/CWE-099/EnvValueAndKeyInjection/EnvValueAndKeyInjection.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE-099/EnvValueAndKeyInjection.ql

‎javascript/ql/test/experimental/Security/CWE-099/EnvValueAndKeyInjection/test.js‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
const http = require('node:http');
2+
3+
4+
http.createServer((req, res) => {
5+
const { EnvValue, EnvKey } = req.body;
6+
process.env[EnvKey] = EnvValue; // NOT OK
7+
process.env[EnvKey] = EnvValue; // NOT OK
8+
9+
res.end('env has been injected!');
10+
});
11+
12+
http.createServer((req, res) => {
13+
const { EnvValue, EnvKey } = req.body;
14+
15+
process.env[EnvKey] = "constant" // OK
16+
process.env.constant = EnvValue // OK
17+
18+
res.end('env has been injected!');
19+
});

‎javascript/ql/test/experimental/Security/CWE-099/EnvValueInjection/EnvValueInjection.expected‎

Lines changed: 27 additions & 0 deletions

0 commit comments

Comments
 (0)