Skip to content

Commit 5253def

Browse files
knewbury01knewbury01
committed
Add path injection query and patch path injection taint model
1 parent 56866cb commit 5253def

File tree

6 files changed

+269
-9
lines changed

6 files changed

+269
-9
lines changed

‎javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CAPPathInjectionQuery.qll‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,21 @@
77
import javascript
88
import advanced_security.javascript.frameworks.cap.CDSUtils
99

10-
abstract class UtilsAccessedPathSink extends DataFlow::Node { }
10+
abstract class UtilsSink extends DataFlow::Node {
11+
abstract string sinkType();
12+
}
13+
14+
abstract class UtilsAccessedPathSink extends UtilsSink {
15+
override string sinkType() { result = "unrestricted file read" }
16+
}
1117

12-
abstract class UtilsControlledDataSink extends DataFlow::Node { }
18+
abstract class UtilsControlledDataSink extends UtilsSink {
19+
override string sinkType() { result = "tainted data being written to a file" }
20+
}
1321

14-
abstract class UtilsControlledPathSink extends DataFlow::Node { }
22+
abstract class UtilsControlledPathSink extends UtilsSink {
23+
override string sinkType() { result = "unrestricted file operations" }
24+
}
1525

1626
abstract class UtilsExtraFlow extends DataFlow::Node { }
1727

@@ -74,14 +84,16 @@ class ControlledInputPath extends UtilsControlledPathSink {
7484
* let dir = isdir ('app')
7585
* ```
7686
*/
77-
class AdditionalFlowStep extends UtilsExtraFlow {
78-
AdditionalFlowStep() {
79-
exists(PathConverters pc | pc.getPath() = this)
87+
class CDSAdditionalFlowStep extends UtilsExtraFlow {
88+
DataFlow::CallNode outNode;
89+
90+
CDSAdditionalFlowStep() {
91+
exists(PathConverters pc | pc.getPath() = this and outNode = pc)
8092
or
81-
exists(PathPredicates pr | pr.getPath() = this)
93+
exists(PathPredicates pr | pr.getPath() = this and outNode = pr)
8294
}
8395

84-
DataFlow::CallNode getOutgoingNode() { result = this }
96+
DataFlow::CallNode getOutgoingNode() { result = outNode }
8597

86-
DataFlow::Node getIngoingNode() { result = this.(DataFlow::CallNode).getAnArgument() }
98+
DataFlow::Node getIngoingNode() { result = this }
8799
}

‎javascript/frameworks/cap/src/path-traversal/PathInjection.md‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
# CAP CDS Utils used with user-controlled sources
2+
3+
If a path is constructed from user-provided input without sufficient sanitization, a malicious user may be able to manipulate the contents of the filesystem without proper authorization.
4+
5+
Additionally if user-provided input is used to create file contents this can also result in a malicious user manipulating the filesystem in an unchecked way.
6+
7+
## Recommendation
8+
9+
CAP applications using CDS Utils should not use user-provided input without sanitization.
10+
11+
## Examples
12+
13+
This CAP service directly uses user-provided input to construct a path.
14+
15+
``` javascript
16+
const cds = require("@sap/cds");
17+
const { rm } = cds.utils
18+
19+
module.exports = class Service1 extends cds.ApplicationService {
20+
21+
init() {
22+
this.on("send1", async (req) => {
23+
let userinput = req.data
24+
await rm(userinput, 'db', 'data') // Path injection alert
25+
}
26+
}
27+
}
28+
```
29+
30+
This CAP service directly uses user-provided input to add content to a file.
31+
32+
``` javascript
33+
const cds = require("@sap/cds");
34+
const { rm } = cds.utils
35+
36+
module.exports = class Service1 extends cds.ApplicationService {
37+
38+
init() {
39+
this.on("send1", async (req) => {
40+
let userinput = req.data
41+
await write(userinput).to('db/data') // Path injection alert
42+
}
43+
}
44+
}
45+
46+
```
47+
48+
## References
49+
50+
- OWASP 2021: [Injection](https://owasp.org/Top10/A03_2021-Injection/).
51+
- SAP CAP CDS Utils : [Documentation](https://cap.cloud.sap/docs/node.js/cds-utils).

‎javascript/frameworks/cap/src/path-traversal/PathInjection.ql‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
/**
2+
* @name Use of user controlled input in CAP CDS file system utilies
3+
* @description Using unchecked user controlled values can allow an
4+
* attacker to affect paths constructed and accessed in
5+
* the filesystem.
6+
* @kind path-problem
7+
* @problem.severity warning
8+
* @security-severity 7.5
9+
* @precision medium
10+
* @id js/cap-path-injection
11+
* @tags security
12+
* external/cwe/cwe-020
13+
* external/cwe/cwe-022
14+
*/
15+
16+
import javascript
17+
import advanced_security.javascript.frameworks.cap.CAPPathInjectionQuery
18+
import advanced_security.javascript.frameworks.cap.RemoteFlowSources
19+
20+
module PathInjectionConfig implements DataFlow::ConfigSig {
21+
predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
22+
23+
predicate isSink(DataFlow::Node sink) { sink instanceof UtilsSink }
24+
25+
predicate isAdditionalFlowStep(DataFlow::Node nodein, DataFlow::Node nodeout) {
26+
exists(CDSAdditionalFlowStep step |
27+
step.getIngoingNode() = nodein and
28+
step.getOutgoingNode() = nodeout
29+
)
30+
}
31+
}
32+
33+
module PathInjectionConfigFlow = TaintTracking::Global<PathInjectionConfig>;
34+
35+
import PathInjectionConfigFlow::PathGraph
36+
37+
from PathInjectionConfigFlow::PathNode source, PathInjectionConfigFlow::PathNode sink
38+
where PathInjectionConfigFlow::flowPath(source, sink)
39+
select sink, source, sink,
40+
"This CDS utils usage relies on user-provided value and can result in " +
41+
sink.getNode().(UtilsSink).sinkType() + "."

‎javascript/frameworks/cap/test/queries/path-traversal/pathinjection.expected‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
edges
2+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:31:26:31:34 | userinput | provenance | |
3+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:33:38:33:46 | userinput | provenance | |
4+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:34:24:34:32 | userinput | provenance | |
5+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:36:44:36:52 | userinput | provenance | |
6+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:38:25:38:33 | userinput | provenance | |
7+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:40:26:40:34 | userinput | provenance | |
8+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:41:26:41:34 | userinput | provenance | |
9+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:43:25:43:33 | userinput | provenance | |
10+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:44:25:44:33 | userinput | provenance | |
11+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:46:26:46:34 | userinput | provenance | |
12+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:47:26:47:34 | userinput | provenance | |
13+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:49:22:49:30 | userinput | provenance | |
14+
| pathinjection.js:8:19:8:38 | userinput | pathinjection.js:50:22:50:30 | userinput | provenance | |
15+
| pathinjection.js:8:31:8:38 | req.data | pathinjection.js:8:19:8:38 | userinput | provenance | |
16+
| pathinjection.js:9:19:9:44 | userinputtwo | pathinjection.js:37:25:37:36 | userinputtwo | provenance | |
17+
| pathinjection.js:9:34:9:44 | req.headers | pathinjection.js:9:19:9:44 | userinputtwo | provenance | |
18+
| pathinjection.js:10:19:10:45 | userinputthree | pathinjection.js:12:38:12:51 | userinputthree | provenance | |
19+
| pathinjection.js:10:19:10:45 | userinputthree | pathinjection.js:14:47:14:60 | userinputthree | provenance | |
20+
| pathinjection.js:10:19:10:45 | userinputthree | pathinjection.js:16:34:16:47 | userinputthree | provenance | |
21+
| pathinjection.js:10:19:10:45 | userinputthree | pathinjection.js:18:34:18:47 | userinputthree | provenance | |
22+
| pathinjection.js:10:19:10:45 | userinputthree | pathinjection.js:20:35:20:48 | userinputthree | provenance | |
23+
| pathinjection.js:10:36:10:45 | req.params | pathinjection.js:10:19:10:45 | userinputthree | provenance | |
24+
| pathinjection.js:12:19:12:52 | taint1 | pathinjection.js:22:36:22:41 | taint1 | provenance | |
25+
| pathinjection.js:12:28:12:52 | decodeU ... tthree) | pathinjection.js:12:19:12:52 | taint1 | provenance | |
26+
| pathinjection.js:12:38:12:51 | userinputthree | pathinjection.js:12:28:12:52 | decodeU ... tthree) | provenance | Config |
27+
| pathinjection.js:14:19:14:61 | taint2 | pathinjection.js:24:40:24:45 | taint2 | provenance | |
28+
| pathinjection.js:14:28:14:61 | decodeU ... tthree) | pathinjection.js:14:19:14:61 | taint2 | provenance | |
29+
| pathinjection.js:14:47:14:60 | userinputthree | pathinjection.js:14:28:14:61 | decodeU ... tthree) | provenance | Config |
30+
| pathinjection.js:16:19:16:48 | taint3 | pathinjection.js:26:34:26:39 | taint3 | provenance | |
31+
| pathinjection.js:16:28:16:48 | local(u ... tthree) | pathinjection.js:16:19:16:48 | taint3 | provenance | |
32+
| pathinjection.js:16:34:16:47 | userinputthree | pathinjection.js:16:28:16:48 | local(u ... tthree) | provenance | Config |
33+
| pathinjection.js:18:19:18:48 | taint4 | pathinjection.js:28:34:28:39 | taint4 | provenance | |
34+
| pathinjection.js:18:28:18:48 | isdir(u ... tthree) | pathinjection.js:18:19:18:48 | taint4 | provenance | |
35+
| pathinjection.js:18:34:18:47 | userinputthree | pathinjection.js:18:28:18:48 | isdir(u ... tthree) | provenance | Config |
36+
| pathinjection.js:20:19:20:49 | taint5 | pathinjection.js:30:40:30:45 | taint5 | provenance | |
37+
| pathinjection.js:20:28:20:49 | isfile( ... tthree) | pathinjection.js:20:19:20:49 | taint5 | provenance | |
38+
| pathinjection.js:20:35:20:48 | userinputthree | pathinjection.js:20:28:20:49 | isfile( ... tthree) | provenance | Config |
39+
nodes
40+
| pathinjection.js:8:19:8:38 | userinput | semmle.label | userinput |
41+
| pathinjection.js:8:31:8:38 | req.data | semmle.label | req.data |
42+
| pathinjection.js:9:19:9:44 | userinputtwo | semmle.label | userinputtwo |
43+
| pathinjection.js:9:34:9:44 | req.headers | semmle.label | req.headers |
44+
| pathinjection.js:10:19:10:45 | userinputthree | semmle.label | userinputthree |
45+
| pathinjection.js:10:36:10:45 | req.params | semmle.label | req.params |
46+
| pathinjection.js:12:19:12:52 | taint1 | semmle.label | taint1 |
47+
| pathinjection.js:12:28:12:52 | decodeU ... tthree) | semmle.label | decodeU ... tthree) |
48+
| pathinjection.js:12:38:12:51 | userinputthree | semmle.label | userinputthree |
49+
| pathinjection.js:14:19:14:61 | taint2 | semmle.label | taint2 |
50+
| pathinjection.js:14:28:14:61 | decodeU ... tthree) | semmle.label | decodeU ... tthree) |
51+
| pathinjection.js:14:47:14:60 | userinputthree | semmle.label | userinputthree |
52+
| pathinjection.js:16:19:16:48 | taint3 | semmle.label | taint3 |
53+
| pathinjection.js:16:28:16:48 | local(u ... tthree) | semmle.label | local(u ... tthree) |
54+
| pathinjection.js:16:34:16:47 | userinputthree | semmle.label | userinputthree |
55+
| pathinjection.js:18:19:18:48 | taint4 | semmle.label | taint4 |
56+
| pathinjection.js:18:28:18:48 | isdir(u ... tthree) | semmle.label | isdir(u ... tthree) |
57+
| pathinjection.js:18:34:18:47 | userinputthree | semmle.label | userinputthree |
58+
| pathinjection.js:20:19:20:49 | taint5 | semmle.label | taint5 |
59+
| pathinjection.js:20:28:20:49 | isfile( ... tthree) | semmle.label | isfile( ... tthree) |
60+
| pathinjection.js:20:35:20:48 | userinputthree | semmle.label | userinputthree |
61+
| pathinjection.js:22:36:22:41 | taint1 | semmle.label | taint1 |
62+
| pathinjection.js:24:40:24:45 | taint2 | semmle.label | taint2 |
63+
| pathinjection.js:26:34:26:39 | taint3 | semmle.label | taint3 |
64+
| pathinjection.js:28:34:28:39 | taint4 | semmle.label | taint4 |
65+
| pathinjection.js:30:40:30:45 | taint5 | semmle.label | taint5 |
66+
| pathinjection.js:31:26:31:34 | userinput | semmle.label | userinput |
67+
| pathinjection.js:33:38:33:46 | userinput | semmle.label | userinput |
68+
| pathinjection.js:34:24:34:32 | userinput | semmle.label | userinput |
69+
| pathinjection.js:36:44:36:52 | userinput | semmle.label | userinput |
70+
| pathinjection.js:37:25:37:36 | userinputtwo | semmle.label | userinputtwo |
71+
| pathinjection.js:38:25:38:33 | userinput | semmle.label | userinput |
72+
| pathinjection.js:40:26:40:34 | userinput | semmle.label | userinput |
73+
| pathinjection.js:41:26:41:34 | userinput | semmle.label | userinput |
74+
| pathinjection.js:43:25:43:33 | userinput | semmle.label | userinput |
75+
| pathinjection.js:44:25:44:33 | userinput | semmle.label | userinput |
76+
| pathinjection.js:46:26:46:34 | userinput | semmle.label | userinput |
77+
| pathinjection.js:47:26:47:34 | userinput | semmle.label | userinput |
78+
| pathinjection.js:49:22:49:30 | userinput | semmle.label | userinput |
79+
| pathinjection.js:50:22:50:30 | userinput | semmle.label | userinput |
80+
subpaths
81+
#select
82+
| pathinjection.js:22:36:22:41 | taint1 | pathinjection.js:10:36:10:45 | req.params | pathinjection.js:22:36:22:41 | taint1 | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
83+
| pathinjection.js:24:40:24:45 | taint2 | pathinjection.js:10:36:10:45 | req.params | pathinjection.js:24:40:24:45 | taint2 | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
84+
| pathinjection.js:26:34:26:39 | taint3 | pathinjection.js:10:36:10:45 | req.params | pathinjection.js:26:34:26:39 | taint3 | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
85+
| pathinjection.js:28:34:28:39 | taint4 | pathinjection.js:10:36:10:45 | req.params | pathinjection.js:28:34:28:39 | taint4 | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
86+
| pathinjection.js:30:40:30:45 | taint5 | pathinjection.js:10:36:10:45 | req.params | pathinjection.js:30:40:30:45 | taint5 | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
87+
| pathinjection.js:31:26:31:34 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:31:26:31:34 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
88+
| pathinjection.js:33:38:33:46 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:33:38:33:46 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
89+
| pathinjection.js:34:24:34:32 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:34:24:34:32 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file read. |
90+
| pathinjection.js:36:44:36:52 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:36:44:36:52 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
91+
| pathinjection.js:37:25:37:36 | userinputtwo | pathinjection.js:9:34:9:44 | req.headers | pathinjection.js:37:25:37:36 | userinputtwo | This CDS utils usage relies on user-provided value and can result in tainted data being written to a file. |
92+
| pathinjection.js:38:25:38:33 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:38:25:38:33 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
93+
| pathinjection.js:40:26:40:34 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:40:26:40:34 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
94+
| pathinjection.js:41:26:41:34 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:41:26:41:34 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
95+
| pathinjection.js:43:25:43:33 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:43:25:43:33 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
96+
| pathinjection.js:44:25:44:33 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:44:25:44:33 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
97+
| pathinjection.js:46:26:46:34 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:46:26:46:34 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
98+
| pathinjection.js:47:26:47:34 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:47:26:47:34 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
99+
| pathinjection.js:49:22:49:30 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:49:22:49:30 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |
100+
| pathinjection.js:50:22:50:30 | userinput | pathinjection.js:8:31:8:38 | req.data | pathinjection.js:50:22:50:30 | userinput | This CDS utils usage relies on user-provided value and can result in unrestricted file operations. |

‎javascript/frameworks/cap/test/queries/path-traversal/pathinjection.js‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
const cds = require("@sap/cds");
2+
const { decodeURI, decodeURIComponent, local, isdir, isfile, read, readdir, append, write, copy, stat, find, mkdirp, rmdir, rimraf, rm } = cds.utils
3+
4+
module.exports = class Service1 extends cds.ApplicationService {
5+
6+
init() {
7+
this.on("send1", async (req) => {
8+
const userinput = req.data
9+
const userinputtwo = req.headers
10+
const userinputthree = req.params
11+
12+
const taint1 = decodeURI(userinputthree) // taint step
13+
14+
const taint2 = decodeURIComponent(userinputthree) // taint step
15+
16+
const taint3 = local(userinputthree) // taint step
17+
18+
const taint4 = isdir(userinputthree) // taint step
19+
20+
const taint5 = isfile(userinputthree) // taint step
21+
22+
const pkg = await read(taint1) // sink
23+
24+
const pdir = await readdir(taint2) // sink
25+
26+
const s = await stat(taint3) // sink
27+
28+
const f = await find(taint4) // sink
29+
30+
await append('db/data').to(taint5) // sink
31+
await append(userinput, 'dist/db/data') // sink
32+
33+
await copy('db/data').to(userinput) // sink
34+
await copy(userinput, 'dist/db/data') // sink
35+
36+
await write({ foo: 'bar' }).to(userinput) // sink
37+
await write(userinputtwo).to('db/data') // sink
38+
await write(userinput, { foo: 'bar' }) // sink
39+
40+
await mkdirp(userinput, 'db', 'data') // sink
41+
await mkdirp(userinput) // sink
42+
43+
await rmdir(userinput, 'db', 'data') // sink
44+
await rmdir(userinput) // sink
45+
46+
await rimraf(userinput, 'db', 'data') // sink
47+
await rimraf(userinput) // sink
48+
49+
await rm(userinput, 'db', 'data') // sink
50+
await rm(userinput) // sink
51+
});
52+
53+
super.init();
54+
}
55+
};

‎javascript/frameworks/cap/test/queries/path-traversal/pathinjection.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
path-traversal/PathInjection.ql

0 commit comments

Comments
 (0)