Skip to content

Commit 78e9d85

Browse files
jeongsoolee09jeongsoolee09
authored
Merge pull request #224 from advanced-security/knewbury01/cds-util-path-traversal-query
Add CDS Utils path injection query
2 parents 76e0849 + 4c3f564 commit 78e9d85

File tree

10 files changed

+321
-76
lines changed

10 files changed

+321
-76
lines changed

‎.github/workflows/javascript.sarif.expected‎

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

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

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +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+
}
1113

12-
abstract class UtilsControlledDataSink extends DataFlow::Node { }
14+
abstract class UtilsAccessedPathSink extends UtilsSink {
15+
override string sinkType() { result = "unrestricted file read" }
16+
}
1317

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

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

1826
/**
1927
* This represents the data in calls as follows:
@@ -67,21 +75,3 @@ class ControlledInputPath extends UtilsControlledPathSink {
6775
exists(DirectoryReaders dr | dr.getPath() = this)
6876
}
6977
}
70-
71-
/**
72-
* This represents calls where the taint flows through the call. e.g.
73-
* ```javascript
74-
* let dir = isdir ('app')
75-
* ```
76-
*/
77-
class AdditionalFlowStep extends UtilsExtraFlow {
78-
AdditionalFlowStep() {
79-
exists(PathConverters pc | pc.getPath() = this)
80-
or
81-
exists(PathPredicates pr | pr.getPath() = this)
82-
}
83-
84-
DataFlow::CallNode getOutgoingNode() { result = this }
85-
86-
DataFlow::Node getIngoingNode() { result = this.(DataFlow::CallNode).getAnArgument() }
87-
}

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

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
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+
The sanitization stragety can vary depending on what types of paths are satisfactory as user-provided input. A simple approach to sanitization is to check user-provided input against an allow list. Other potential approaches include checking components of paths or normalizing them to make sure that the path does not escape the expected root folder.
12+
13+
Normalization techniques should be carefully considered and simple naive replacement strategies will not be sufficient, for example replacing any match of a parent directory reference (`../`) in the sample `.../...//` will still result in the path `../` being used which could escape the intended directory.
14+
15+
## Examples
16+
17+
This CAP service directly uses user-provided input to construct a path.
18+
19+
``` javascript
20+
const cds = require("@sap/cds");
21+
const { rm } = cds.utils
22+
23+
module.exports = class Service1 extends cds.ApplicationService {
24+
25+
init() {
26+
this.on("send1", async (req) => {
27+
let userinput = req.data
28+
await rm(userinput, 'db', 'data') // Path injection alert
29+
}
30+
}
31+
}
32+
```
33+
34+
This CAP service directly uses user-provided input to add content to a file.
35+
36+
``` javascript
37+
const cds = require("@sap/cds");
38+
const { rm } = cds.utils
39+
40+
module.exports = class Service1 extends cds.ApplicationService {
41+
init() {
42+
this.on("send1", async (req) => {
43+
let userinput = req.data
44+
await write(userinput).to('db/data') // Path injection alert
45+
46+
// GOOD: the path can not be controlled by an attacker
47+
let allowedDirectories = [
48+
'this-is-a-safe-directory'
49+
];
50+
if (allowedDirectories.includes(userinput)) {
51+
await rm(userinput) // sanitized - No Path injection alert
52+
}
53+
}
54+
}
55+
}
56+
```
57+
58+
## References
59+
60+
- OWASP 2021: [Injection](https://owasp.org/Top10/A03_2021-Injection/).
61+
- SAP CAP CDS Utils : [Documentation](https://cap.cloud.sap/docs/node.js/cds-utils).
62+
- Common Weakness Enumeration: [CWE-020](https://cwe.mitre.org/data/definitions/20.html).
63+
- Common Weakness Enumeration: [CWE-022](https://cwe.mitre.org/data/definitions/22.html).

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/**
2+
* @name Use of user controlled input in CAP CDS file system utilities
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+
private import semmle.javascript.security.dataflow.TaintedPathCustomizations
20+
private import semmle.javascript.security.dataflow.TaintedPathQuery as tq
21+
22+
module PathInjectionConfig implements DataFlow::ConfigSig {
23+
predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
24+
25+
predicate isSink(DataFlow::Node sink) { sink instanceof UtilsSink }
26+
27+
predicate isAdditionalFlowStep(DataFlow::Node nodein, DataFlow::Node nodeout) {
28+
exists(PathConverters pc | pc.getPath() = nodein and nodeout = pc)
29+
or
30+
exists(PathPredicates pr | pr.getPath() = nodein and nodeout = pr)
31+
}
32+
33+
predicate isBarrier(DataFlow::Node node) {
34+
node instanceof TaintedPath::Sanitizer
35+
or
36+
tq::TaintedPathConfig::isBarrier(node)
37+
}
38+
}
39+
40+
module PathInjectionConfigFlow = TaintTracking::Global<PathInjectionConfig>;
41+
42+
import PathInjectionConfigFlow::PathGraph
43+
44+
from PathInjectionConfigFlow::PathNode source, PathInjectionConfigFlow::PathNode sink
45+
where PathInjectionConfigFlow::flowPath(source, sink)
46+
select sink, source, sink,
47+
"This CDS utils usage relies on user-provided value and can result in " +
48+
sink.getNode().(UtilsSink).sinkType() + "."

‎javascript/frameworks/cap/test/models/cds/utils/utils.expected‎

Lines changed: 33 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,38 +1,33 @@
1-
| utils.js:5:21:5:30 | "%E0%A4%A" | "%E0%A4%A": additional flow step |
2-
| utils.js:7:31:7:40 | "%E0%A4%A" | "%E0%A4%A": additional flow step |
3-
| utils.js:9:18:9:27 | "%E0%A4%A" | "%E0%A4%A": additional flow step |
4-
| utils.js:13:17:13:21 | 'app' | 'app': additional flow step |
5-
| utils.js:15:19:15:32 | 'package.json' | 'package.json': additional flow step |
6-
| utils.js:17:22:17:35 | 'package.json' | 'package.json': controlled path sink |
7-
| utils.js:19:26:19:39 | 'package.json' | 'package.json': controlled path sink |
8-
| utils.js:21:20:21:33 | 'package.json' | 'package.json': controlled path sink |
9-
| utils.js:23:20:23:33 | 'package.json' | 'package.json': controlled path sink |
10-
| utils.js:25:14:25:22 | 'db/data' | 'db/data': controlled data sink |
11-
| utils.js:25:28:25:41 | 'dist/db/data' | 'dist/db/data': controlled path sink |
12-
| utils.js:26:14:26:22 | 'db/data' | 'db/data': controlled path sink |
13-
| utils.js:26:25:26:38 | 'dist/db/data' | 'dist/db/data': controlled data sink |
14-
| utils.js:28:12:28:20 | 'db/data' | 'db/data': accessed path sink |
15-
| utils.js:28:26:28:39 | 'dist/db/data' | 'dist/db/data': controlled path sink |
16-
| utils.js:29:12:29:20 | 'db/data' | 'db/data': accessed path sink |
17-
| utils.js:29:23:29:36 | 'dist/db/data' | 'dist/db/data': controlled path sink |
18-
| utils.js:31:13:31:26 | { foo: 'bar' } | { foo: 'bar' }: controlled data sink |
19-
| utils.js:31:32:31:47 | 'some/file.json' | 'some/file.json': controlled path sink |
20-
| utils.js:32:13:32:28 | 'some/file.json' | 'some/file.json': controlled path sink |
21-
| utils.js:32:31:32:44 | { foo: 'bar' } | { foo: 'bar' }: controlled data sink |
22-
| utils.js:34:14:34:19 | 'dist' | 'dist': controlled path sink |
23-
| utils.js:34:22:34:25 | 'db' | 'db': controlled path sink |
24-
| utils.js:34:28:34:33 | 'data' | 'data': controlled path sink |
25-
| utils.js:35:14:35:27 | 'dist/db/data' | 'dist/db/data': controlled path sink |
26-
| utils.js:37:13:37:18 | 'dist' | 'dist': controlled path sink |
27-
| utils.js:37:21:37:24 | 'db' | 'db': controlled path sink |
28-
| utils.js:37:27:37:32 | 'data' | 'data': controlled path sink |
29-
| utils.js:38:13:38:26 | 'dist/db/data' | 'dist/db/data': controlled path sink |
30-
| utils.js:40:14:40:19 | 'dist' | 'dist': controlled path sink |
31-
| utils.js:40:22:40:25 | 'db' | 'db': controlled path sink |
32-
| utils.js:40:28:40:33 | 'data' | 'data': controlled path sink |
33-
| utils.js:41:14:41:27 | 'dist/db/data' | 'dist/db/data': controlled path sink |
34-
| utils.js:43:10:43:15 | 'dist' | 'dist': controlled path sink |
35-
| utils.js:43:18:43:21 | 'db' | 'db': controlled path sink |
36-
| utils.js:43:24:43:29 | 'data' | 'data': controlled path sink |
37-
| utils.js:44:10:44:23 | 'dist/db/data' | 'dist/db/data': controlled path sink |
38-
| utils.js:52:20:52:28 | 'db/data' | 'db/data': controlled data sink |
1+
| utils.js:5:22:5:35 | 'package.json' | 'package.json': controlled path sink |
2+
| utils.js:7:26:7:39 | 'package.json' | 'package.json': controlled path sink |
3+
| utils.js:9:20:9:33 | 'package.json' | 'package.json': controlled path sink |
4+
| utils.js:11:20:11:33 | 'package.json' | 'package.json': controlled path sink |
5+
| utils.js:13:14:13:22 | 'db/data' | 'db/data': controlled data sink |
6+
| utils.js:13:28:13:41 | 'dist/db/data' | 'dist/db/data': controlled path sink |
7+
| utils.js:14:14:14:22 | 'db/data' | 'db/data': controlled path sink |
8+
| utils.js:14:25:14:38 | 'dist/db/data' | 'dist/db/data': controlled data sink |
9+
| utils.js:16:12:16:20 | 'db/data' | 'db/data': accessed path sink |
10+
| utils.js:16:26:16:39 | 'dist/db/data' | 'dist/db/data': controlled path sink |
11+
| utils.js:17:12:17:20 | 'db/data' | 'db/data': accessed path sink |
12+
| utils.js:17:23:17:36 | 'dist/db/data' | 'dist/db/data': controlled path sink |
13+
| utils.js:19:13:19:26 | { foo: 'bar' } | { foo: 'bar' }: controlled data sink |
14+
| utils.js:19:32:19:47 | 'some/file.json' | 'some/file.json': controlled path sink |
15+
| utils.js:20:13:20:28 | 'some/file.json' | 'some/file.json': controlled path sink |
16+
| utils.js:20:31:20:44 | { foo: 'bar' } | { foo: 'bar' }: controlled data sink |
17+
| utils.js:22:14:22:19 | 'dist' | 'dist': controlled path sink |
18+
| utils.js:22:22:22:25 | 'db' | 'db': controlled path sink |
19+
| utils.js:22:28:22:33 | 'data' | 'data': controlled path sink |
20+
| utils.js:23:14:23:27 | 'dist/db/data' | 'dist/db/data': controlled path sink |
21+
| utils.js:25:13:25:18 | 'dist' | 'dist': controlled path sink |
22+
| utils.js:25:21:25:24 | 'db' | 'db': controlled path sink |
23+
| utils.js:25:27:25:32 | 'data' | 'data': controlled path sink |
24+
| utils.js:26:13:26:26 | 'dist/db/data' | 'dist/db/data': controlled path sink |
25+
| utils.js:28:14:28:19 | 'dist' | 'dist': controlled path sink |
26+
| utils.js:28:22:28:25 | 'db' | 'db': controlled path sink |
27+
| utils.js:28:28:28:33 | 'data' | 'data': controlled path sink |
28+
| utils.js:29:14:29:27 | 'dist/db/data' | 'dist/db/data': controlled path sink |
29+
| utils.js:31:10:31:15 | 'dist' | 'dist': controlled path sink |
30+
| utils.js:31:18:31:21 | 'db' | 'db': controlled path sink |
31+
| utils.js:31:24:31:29 | 'data' | 'data': controlled path sink |
32+
| utils.js:32:10:32:23 | 'dist/db/data' | 'dist/db/data': controlled path sink |
33+
| utils.js:40:20:40:28 | 'db/data' | 'db/data': controlled data sink |

‎javascript/frameworks/cap/test/models/cds/utils/utils.js‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,6 @@
11
const cds = require("@sap/cds");
22

3-
const { decodeURI, decodeURIComponent, local, exists, isdir, isfile, read, readdir, append, write, copy, stat, find, mkdirp, rmdir, rimraf, rm } = cds.utils
4-
5-
let uri = decodeURI("%E0%A4%A") // taint step
6-
7-
let uri2 = decodeURIComponent("%E0%A4%A") // taint step
8-
9-
let uri3 = local("%E0%A4%A") // taint step
10-
11-
let uri4 = exists("%E0%A4%A") // NOT a taint step - returns a boolean
12-
13-
let dir = isdir('app') // taint step
14-
15-
let file = isfile('package.json') // taint step
3+
const { read, readdir, append, write, copy, stat, find, mkdirp, rmdir, rimraf, rm } = cds.utils
164

175
let pkg = await read('package.json') // sink
186

‎javascript/frameworks/cap/test/models/cds/utils/utils.ql‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,4 @@ where
88
node.(UtilsAccessedPathSink).toString() = str and strfull = str + ": accessed path sink"
99
or
1010
node.(UtilsControlledDataSink).toString() = str and strfull = str + ": controlled data sink"
11-
or
12-
node.(UtilsExtraFlow).toString() = str and strfull = str + ": additional flow step"
1311
select node, strfull

0 commit comments

Comments
 (0)