Skip to content

Commit b9bbd42

Browse files
jeongsoolee09jeongsoolee09
authored
Merge branch 'main' into lcartey/update-dependencies
2 parents 6740358 + 8918839 commit b9bbd42

File tree

23 files changed

+693
-623
lines changed

23 files changed

+693
-623
lines changed

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

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -346,16 +346,26 @@ class HandlerRegistration extends MethodCallNode {
346346
}
347347

348348
/**
349-
* A parameter of a handler
349+
* The first parameter of a handler, representing the request object received either directly
350+
* from a user, or from another service that may be internal (defined in the same application)
351+
* or external (defined in another application, or even served from a different server).
352+
* e.g.
353+
* ``` javascript
354+
* module.exports = class Service1 extends cds.ApplicationService {
355+
* this.on("SomeEvent", "SomeEntity", (req) => { ... });
356+
* this.before("SomeEvent", "SomeEntity", (req, next) => { ... });
357+
* this.after("SomeEvent", "SomeEntity", (req, next) => { ... });
358+
* }
359+
* ```
360+
* All parameters named `req` above are captured. Also see `HandlerParameterOfExposedService`
361+
* for a subset of this class that is only about handlers exposed to some protocol.
350362
*/
351-
class HandlerParameter instanceof ParameterNode {
363+
class HandlerParameter extends ParameterNode {
352364
Handler handler;
353365

354366
HandlerParameter() { this = handler.getParameter(0) }
355367

356368
Handler getHandler() { result = handler }
357-
358-
string toString() { result = super.toString() }
359369
}
360370

361371
/**
@@ -832,7 +842,7 @@ class HandlerParameterData instanceof PropRead {
832842
string dataName;
833843

834844
HandlerParameterData() {
835-
this = handlerParameter.(SourceNode).getAPropertyRead("data").getAPropertyRead(dataName)
845+
this = handlerParameter.getAPropertyRead("data").getAPropertyRead(dataName)
836846
}
837847

838848
/**

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

Lines changed: 65 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,35 +2,81 @@ import javascript
22
import advanced_security.javascript.frameworks.cap.CDS
33

44
/**
5-
* Either of:
6-
* a parameter of a handler registered for an (exposed) service on an event. e.g.
7-
* ```javascript
8-
* this.on("SomeEvent", "SomeEntity", (req) => { ... });
9-
* this.before("SomeEvent", "SomeEntity", (req, next) => { ... });
10-
* SomeService.on("SomeEvent", "SomeEntity", (msg) => { ... });
11-
* SomeService.after("SomeEvent", "SomeEntity", (msg) => { ... });
5+
* The request parameter of a handler belonging to a service that is exposed to
6+
* a protocol. e.g. All parameters named `req` is captured in the below example.
7+
* ``` javascript
8+
* // srv/service1.js
9+
* module.exports = class Service1 extends cds.ApplicationService {
10+
* this.on("SomeEvent", "SomeEntity", (req) => { ... });
11+
* this.before("SomeEvent", "SomeEntity", (req, next) => { ... });
12+
* }
1213
* ```
13-
* OR
14-
* a handler parameter that is not connected to a service
15-
* possibly due to cds compilation failure
16-
* or non explicit service references in source. e.g.
17-
* ```javascript
18-
* cds.serve('./test-service').with((srv) => {
19-
* srv.after('READ', req => req.target.data) //req
20-
* })
14+
* ``` cds
15+
* // srv/service1.cds
16+
* service Service1 @(path: '/service-1') { ... }
2117
* ```
18+
*
19+
* NOTE: CDS extraction can fail for various reasons, and if so the detection
20+
* logic falls back on overapproximating on the parameters and assume they are
21+
* exposed.
2222
*/
23-
class HandlerParameterOfExposedService extends RemoteFlowSource, HandlerParameter {
23+
class HandlerParameterOfExposedService extends HandlerParameter {
2424
HandlerParameterOfExposedService() {
25+
/* 1. The CDS definition is there and we can determine it is exposed. */
2526
this.getHandler().getHandlerRegistration().getService().getDefinition().isExposed()
2627
or
27-
/* no precise service definition is known */
28+
/*
29+
* 2. (Fallback) The CDS definition is not there, so no precise service definition
30+
* is known.
31+
*/
32+
2833
not exists(this.getHandler().getHandlerRegistration().getService().getDefinition())
2934
}
35+
}
36+
37+
/**
38+
* Reads of property belonging to a request parameter that is exposed to a protocol.
39+
* It currently models the following access paths:
40+
* - `req.data` (from `cds.Event.data`)
41+
* - `req.params` (from `cds.Request.params`)
42+
* - `req.headers` (from `cds.Event.headers`)
43+
* - `req.http.req.*` (from `cds.EventContext.http.req`)
44+
* - `req.id` (from `cds.EventContext.id`)
45+
*
46+
* Note that `req.http.req` has type `require("@express").Request`, so their uses are
47+
* completely identical. Subsequently, the models for this access path follow Express'
48+
* API descriptions (as far back as 3.x). Also see `Express::RequestInputAccess`,
49+
* `Express::RequestHeaderAccess`, and `Express::RequestBodyAccess` of the standard
50+
* library.
51+
*/
52+
class UserProvidedPropertyReadOfHandlerParameterOfExposedService extends RemoteFlowSource {
53+
HandlerParameterOfExposedService handlerParameterOfExposedService;
54+
55+
UserProvidedPropertyReadOfHandlerParameterOfExposedService() {
56+
/* 1. `req.(data|params|headers|id)` */
57+
this = handlerParameterOfExposedService.getAPropertyRead(["data", "params", "headers", "id"])
58+
or
59+
/* 2. APIs stemming from `req.http.req`: Defined by Express.js */
60+
exists(PropRead reqHttpReq |
61+
reqHttpReq = handlerParameterOfExposedService.getAPropertyRead("http").getAPropertyRead("req")
62+
|
63+
this = reqHttpReq.getAPropertyRead(["originalUrl", "hostname"]) or
64+
this =
65+
reqHttpReq
66+
.getAPropertyRead(["query", "body", "params", "headers", "cookies"])
67+
.getAPropertyRead() or
68+
this = reqHttpReq.getAMemberCall(["get", "is", "header", "param"])
69+
)
70+
}
71+
72+
HandlerParameterOfExposedService getHandlerParameter() {
73+
result = handlerParameterOfExposedService
74+
}
3075

31-
override string toString() { result = HandlerParameter.super.toString() }
76+
Handler getHandler() { result = handlerParameterOfExposedService.getHandler() }
3277

3378
override string getSourceType() {
34-
result = "Parameter of an event handler belonging to an exposed service"
79+
result =
80+
"Tainted property read of the request parameter of an event handler belonging to an exposed service"
3581
}
3682
}

‎javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposureHeuristicSource.md

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
# CAP Insertion of Sensitive Information into Log File
2+
3+
If sensitive information is written to a log entry using the CAP Node.js logging API, a malicious user may be able to gain access to user data.
4+
5+
Data that may expose system information such as full path names, system information, usernames and passwords should not be logged.
6+
7+
This query is similar to `js/cap-sensitive-log` in that the sinks are CAP logging facilities. The sources however are the same (exclusively) as the out of the box CodeQL query for [clear text logging](https://codeql.github.com/codeql-query-help/javascript/js-clear-text-logging/).
8+
9+
## Recommendation
10+
11+
CAP applications should not log sensitive information. Sensitive information can include: full path names, system information, usernames, passwords or any personally identifiable information. Make sure to log only information that is not sensitive, or obfuscate/encrypt sensitive information any time that it is logged.
12+
13+
## Examples
14+
15+
This CAP service directly logs the sensitive information. Potential attackers may gain access to this sensitive information when the log output is displayed or when the attacker gains access to the log, and the info is not obfuscated or encrypted.
16+
17+
``` javascript
18+
import cds from '@sap/cds'
19+
const LOG = cds.log("logger");
20+
21+
class SampleVulnService extends cds.ApplicationService {
22+
init() {
23+
LOG.info(`[INFO] Environment: ${JSON.stringify(process.env)}`); // CAP log exposure alert
24+
var obj = {
25+
x: password
26+
};
27+
28+
LOG.info(obj); // CAP log exposure alert
29+
30+
LOG.info(obj.x.replace(/./g, "*")); // NO CAP log exposure alert - replace call acts as sanitizer
31+
32+
var user = {
33+
password: encryptLib.encryptPassword(password)
34+
};
35+
LOG.info(user); // NO CAP log exposure alert - the data is encrypted
36+
}
37+
}
38+
```
39+
40+
## References
41+
42+
- OWASP 2021: [Security Logging and Monitoring Failures](https://owasp.org/Top10/A09_2021-Security_Logging_and_Monitoring_Failures/).
43+
- OWASP: [Logging Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Logging_Cheat_Sheet.html).
44+
- OWASP: [User Privacy Protection Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/User_Privacy_Protection_Cheat_Sheet.html).

‎javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposureHeuristicSource.ql

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/**
2+
* @name Insertion of sensitive information into log files
3+
* @description Writing heuristically sensitive information to log files can allow that
4+
* information to be leaked to an attacker more easily.
5+
* @kind path-problem
6+
* @problem.severity warning
7+
* @security-severity 7.5
8+
* @precision low
9+
* @id js/cap-sensitive-log-heurisitic-source
10+
* @tags security
11+
* external/cwe/cwe-532
12+
*/
13+
14+
import javascript
15+
import advanced_security.javascript.frameworks.cap.CDS
16+
import advanced_security.javascript.frameworks.cap.CAPLogInjectionQuery
17+
private import semmle.javascript.security.dataflow.CleartextLoggingCustomizations::CleartextLogging as CleartextLogging
18+
19+
module SensitiveLogExposureConfig implements DataFlow::ConfigSig {
20+
predicate isSource(DataFlow::Node source) { source instanceof CleartextLogging::Source }
21+
22+
predicate isSink(DataFlow::Node sink) { sink instanceof CdsLogSink }
23+
24+
predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg) {
25+
CleartextLogging::isAdditionalTaintStep(src, trg)
26+
}
27+
28+
predicate isBarrier(DataFlow::Node sink) { sink instanceof CleartextLogging::Barrier }
29+
30+
/**
31+
* This predicate is an intentional cartesian product of any sink node and any content that represents a property.
32+
* Normally Cartesian products are bad but in this case it is what we want, to capture all properties of objects that make their way to sinks.
33+
*/
34+
predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet contents) {
35+
// Assume all properties of a logged object are themselves logged.
36+
contents = DataFlow::ContentSet::anyProperty() and
37+
isSink(node)
38+
}
39+
}
40+
41+
module SensitiveLogExposureFlow = TaintTracking::Global<SensitiveLogExposureConfig>;
42+
43+
import SensitiveLogExposureFlow::PathGraph
44+
45+
from SensitiveLogExposureFlow::PathNode source, SensitiveLogExposureFlow::PathNode sink
46+
where SensitiveLogExposureFlow::flowPath(source, sink)
47+
select sink, source, sink, "This logs sensitive data returned by $@ as clear text.",
48+
source.getNode(), source.getNode().(CleartextLogging::Source).describe()

‎javascript/frameworks/cap/test/models/cds/remoteflowsources/ExposedServices.expected

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
| server.js:7:32:7:34 | req |
2+
| server.js:11:32:11:34 | req |
3+
| srv/service1.js:6:29:6:31 | req |
4+
| srv/service1.js:12:29:12:31 | req |
5+
| srv/service1.js:18:29:18:31 | req |
6+
| srv/service1.js:24:29:24:31 | req |
7+
| srv/service1.js:40:29:40:31 | req |
8+
| srv/service1.js:46:29:46:31 | req |
9+
| srv/service1.js:52:29:52:31 | req |
10+
| srv/service1.js:58:29:58:31 | req |
11+
| srv/service1.js:64:29:64:31 | req |
12+
| srv/service2.js:4:27:4:29 | msg |
13+
| srv/service3.js:5:29:5:31 | req |
14+
| srv/service3.js:11:29:11:31 | req |
15+
| srv/service3.js:17:29:17:31 | req |
16+
| srv/service3.js:23:29:23:31 | req |
17+
| srv/service3.js:39:29:39:31 | req |
18+
| srv/service3.js:45:29:45:31 | req |
19+
| srv/service3.js:51:29:51:31 | req |
20+
| srv/service3.js:57:29:57:31 | req |
21+
| srv/service3.js:63:29:63:31 | req |

‎javascript/frameworks/cap/test/models/cds/remoteflowsources/ExposedServices.ql

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import javascript
2+
import advanced_security.javascript.frameworks.cap.RemoteFlowSources
3+
4+
from HandlerParameterOfExposedService handlerParameterOfExposedService
5+
select handlerParameterOfExposedService

‎javascript/frameworks/cap/test/models/cds/remoteflowsources/remoteflowsource.expected

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,31 @@
1-
| srv/service1.js:6:29:6:31 | req |
2-
| srv/service2.js:5:27:5:29 | msg |
3-
| srv/service3nocds.js:6:43:6:45 | req |
4-
| srv/service3nocds.js:7:34:7:36 | req |
5-
| srv/service3nocds.js:11:28:11:30 | req |
6-
| srv/service3nocds.js:12:26:12:28 | req |
7-
| srv/service3nocds.js:19:34:19:36 | req |
8-
| srv/service4withcds.js:5:38:5:40 | req |
9-
| srv/service4withcds.js:6:43:6:45 | req |
10-
| srv/service4withcds.js:14:33:14:35 | req |
11-
| srv/service4withcds.js:15:38:15:40 | req |
12-
| srv/service4withcds.js:16:23:16:25 | req |
1+
| srv/service1.js:7:33:7:40 | req.data |
2+
| srv/service1.js:13:33:13:42 | req.params |
3+
| srv/service1.js:19:29:19:39 | req.headers |
4+
| srv/service1.js:25:30:25:56 | req.htt ... omeProp |
5+
| srv/service1.js:26:30:26:55 | req.htt ... omeProp |
6+
| srv/service1.js:27:30:27:57 | req.htt ... omeProp |
7+
| srv/service1.js:28:30:28:58 | req.htt ... omeProp |
8+
| srv/service1.js:29:30:29:58 | req.htt ... omeProp |
9+
| srv/service1.js:30:30:30:53 | req.htt ... inalUrl |
10+
| srv/service1.js:31:30:31:50 | req.htt ... ostname |
11+
| srv/service1.js:32:30:32:57 | req.htt ... eProp") |
12+
| srv/service1.js:33:30:33:56 | req.htt ... eProp") |
13+
| srv/service1.js:34:31:34:61 | req.htt ... eProp") |
14+
| srv/service1.js:35:31:35:60 | req.htt ... eProp") |
15+
| srv/service1.js:41:29:41:34 | req.id |
16+
| srv/service2.js:5:31:5:38 | msg.data |
17+
| srv/service3.js:6:33:6:40 | req.data |
18+
| srv/service3.js:12:33:12:42 | req.params |
19+
| srv/service3.js:18:29:18:39 | req.headers |
20+
| srv/service3.js:24:30:24:56 | req.htt ... omeProp |
21+
| srv/service3.js:25:30:25:55 | req.htt ... omeProp |
22+
| srv/service3.js:26:30:26:57 | req.htt ... omeProp |
23+
| srv/service3.js:27:30:27:58 | req.htt ... omeProp |
24+
| srv/service3.js:28:30:28:58 | req.htt ... omeProp |
25+
| srv/service3.js:29:30:29:53 | req.htt ... inalUrl |
26+
| srv/service3.js:30:30:30:50 | req.htt ... ostname |
27+
| srv/service3.js:31:30:31:57 | req.htt ... eProp") |
28+
| srv/service3.js:32:30:32:56 | req.htt ... eProp") |
29+
| srv/service3.js:33:31:33:61 | req.htt ... eProp") |
30+
| srv/service3.js:34:31:34:60 | req.htt ... eProp") |
31+
| srv/service3.js:40:29:40:34 | req.id |

‎javascript/frameworks/cap/test/models/cds/remoteflowsources/server.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,11 @@ const cds = require("@sap/cds");
22
const app = require("express")();
33

44
cds.serve("all").in(app);
5+
6+
cds.serve('Service1').with((srv) => {
7+
srv.before('READ', 'Books', (req) => req.reply([])) // SAFE: Exposed service (fallback), but not a taint source
8+
})
9+
10+
cds.serve('Service3').with((srv) => {
11+
srv.before('READ', 'Books', (req) => req.reply([])) // SAFE: Exposed service (fallback), but not a taint source
12+
})

‎javascript/frameworks/cap/test/models/cds/remoteflowsources/srv/service1.js

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,65 @@ const cds = require("@sap/cds");
44
module.exports = class Service1 extends cds.ApplicationService {
55
init() {
66
this.on("send1", async (req) => {
7-
const { messageToPass } = req.data;
7+
const { messageToPass } = req.data; // UNSAFE: Taint source, Exposed service
8+
const Service2 = await cds.connect.to("service-2");
9+
Service2.send("send2", { messageToPass });
10+
});
11+
12+
this.on("send2", async (req) => {
13+
const [ messageToPass ] = req.params; // UNSAFE: Taint source, Exposed service
14+
const Service2 = await cds.connect.to("service-2");
15+
Service2.send("send2", { messageToPass });
16+
});
17+
18+
this.on("send3", async (req) => {
19+
const messageToPass = req.headers["user-agent"]; // UNSAFE: Taint source, Exposed service
20+
const Service2 = await cds.connect.to("service-2");
21+
Service2.send("send2", { messageToPass });
22+
});
23+
24+
this.on("send4", async (req) => {
25+
const messageToPass1 = req.http.req.query.someProp; // UNSAFE: Taint source, Exposed service
26+
const messageToPass2 = req.http.req.body.someProp; // UNSAFE: Taint source, Exposed service
27+
const messageToPass3 = req.http.req.params.someProp; // UNSAFE: Taint source, Exposed service
28+
const messageToPass4 = req.http.req.headers.someProp; // UNSAFE: Taint source, Exposed service
29+
const messageToPass5 = req.http.req.cookies.someProp; // UNSAFE: Taint source, Exposed service
30+
const messageToPass6 = req.http.req.originalUrl; // UNSAFE: Taint source, Exposed service
31+
const messageToPass7 = req.http.req.hostname; // UNSAFE: Taint source, Exposed service
32+
const messageToPass8 = req.http.req.get("someProp"); // UNSAFE: Taint source, Exposed service
33+
const messageToPass9 = req.http.req.is("someProp"); // UNSAFE: Taint source, Exposed service
34+
const messageToPass10 = req.http.req.header("someProp"); // UNSAFE: Taint source, Exposed service
35+
const messageToPass11 = req.http.req.param("someProp"); // UNSAFE: Taint source, Exposed service
36+
const Service2 = await cds.connect.to("service-2"); // UNSAFE: Taint source, Exposed service
37+
Service2.send("send2", { messageToPass1 });
38+
});
39+
40+
this.on("send5", async (req) => {
41+
const messageToPass = req.id; // UNSAFE: Taint source, Exposed service
42+
const Service2 = await cds.connect.to("service-2");
43+
Service2.send("send2", { messageToPass });
44+
});
45+
46+
this.on("send6", async (req) => {
47+
const messageToPass = req.locale; // SAFE: Not a taint source, Exposed service
48+
const Service2 = await cds.connect.to("service-2");
49+
Service2.send("send2", { messageToPass });
50+
});
51+
52+
this.on("send7", async (req) => {
53+
const messageToPass = req.tenant; // SAFE: Not a taint source, Exposed service
54+
const Service2 = await cds.connect.to("service-2");
55+
Service2.send("send2", { messageToPass });
56+
});
57+
58+
this.on("send8", async (req) => {
59+
const messageToPass = req.timestamp; // SAFE: Not a taint source, Exposed service
60+
const Service2 = await cds.connect.to("service-2");
61+
Service2.send("send2", { messageToPass });
62+
});
63+
64+
this.on("send9", async (req) => {
65+
const messageToPass = req.user; // SAFE: Not a taint source, Exposed service
866
const Service2 = await cds.connect.to("service-2");
967
Service2.send("send2", { messageToPass });
1068
});

0 commit comments

Comments
 (0)