Skip to content

Commit 33e30b9

Browse files
jeongsoolee09jeongsoolee09
authored
Merge pull request #226 from advanced-security/jeongsoolee09/improve-cap-log-injection
Make CAP Log injection query more resilient and conservative
2 parents b48604e + d256a61 commit 33e30b9

File tree

8 files changed

+279
-94
lines changed

8 files changed

+279
-94
lines changed

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

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@ class CdsLogger extends MethodCallNode {
2222
string getName() { result = name }
2323
}
2424

25+
/**
26+
* A template literal that is not interpolated. It is basically a string literal
27+
* defined in backticks (\`\`).
28+
*/
2529
class ConstantOnlyTemplateLiteral extends TemplateLiteral {
2630
ConstantOnlyTemplateLiteral() {
2731
forall(Expr e | e = this.getAnElement() | e instanceof TemplateElement)
@@ -51,9 +55,26 @@ module CAPLogInjectionConfiguration implements DataFlow::ConfigSig {
5155
}
5256

5357
predicate isBarrier(DataFlow::Node node) {
58+
/*
59+
* This predicate includes cases such as:
60+
* 1. A CDS entity element lacking a type annotation.
61+
* - Possibly because it relies on a common aspect.
62+
* 2. A CDS entity element annotated with a non-string type listed above.
63+
*
64+
* Therefore, the data held by the handler parameter data (e.g. `req.data.X`)
65+
* has to be EXPLICITLY annotated as `String` or `LargeString` to be excluded
66+
* from the next condition.
67+
*/
68+
5469
exists(HandlerParameterData handlerParameterData |
5570
node = handlerParameterData and
56-
not handlerParameterData.getType() = ["cds.String", "cds.LargeString"]
71+
/* Note the use of `.. != ..` instead of `not .. = ..` below. */
72+
exists(string handlerParameterDataType |
73+
handlerParameterDataType = handlerParameterData.getType()
74+
|
75+
handlerParameterDataType != "cds.String" and
76+
handlerParameterDataType != "cds.LargeString"
77+
)
5778
)
5879
}
5980

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -184,12 +184,12 @@ class ServiceInstanceFromConstructor extends ServiceInstance {
184184
* const cds = require("@sap/cds");
185185
* module.exports = class SomeService extends cds.ApplicationService {
186186
* init() {
187-
* this.on("SomeEvent", (req) => { ... } )
187+
* this.on("SomeEvent", (req) => { ... } )
188188
* }
189189
* }
190190
* ```
191191
* This class captures the access to the `this` variable as in `this.on(...)`.
192-
*
192+
*
193193
* e.g.2. Given this code:
194194
* ``` javascript
195195
* const cds = require('@sap/cds');
@@ -424,13 +424,14 @@ class HandlerRegistration extends MethodCallNode {
424424
* this.after("SomeEvent", "SomeEntity", (req, next) => { ... });
425425
* }
426426
* ```
427-
* All parameters named `req` above are captured. Also see `HandlerParameterOfExposedService`
427+
* All parameters named `req` above are captured. Also see
428+
* `RemoteflowSources::HandlerParameterOfExposedService`
428429
* for a subset of this class that is only about handlers exposed to some protocol.
429430
*/
430431
class HandlerParameter extends ParameterNode {
431432
Handler handler;
432433

433-
HandlerParameter() { this = handler.getParameter(0) }
434+
HandlerParameter() { this = isHandlerParameter(handler) }
434435

435436
Handler getHandler() { result = handler }
436437
}

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,3 +39,12 @@ private SourceNode cdsApplicationServiceInstantiation(TypeTracker t) {
3939
SourceNode cdsApplicationServiceInstantiation() {
4040
result = cdsApplicationServiceInstantiation(TypeTracker::end())
4141
}
42+
43+
private SourceNode isHandlerParameter(TypeTracker t, Handler handler) {
44+
result = handler.getParameter(0) or
45+
exists(TypeTracker t2 | result = isHandlerParameter(t, handler).track(t2, t))
46+
}
47+
48+
SourceNode isHandlerParameter(Handler handler) {
49+
result = isHandlerParameter(TypeTracker::end(), handler)
50+
}

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

Lines changed: 40 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,43 @@
11
| server.js:7:32:7:34 | req |
22
| 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/service1.js:70:30:70:32 | req |
3+
| srv/service1.js:6:30:6:32 | req |
4+
| srv/service1.js:12:30:12:32 | req |
5+
| srv/service1.js:19:30:19:32 | req |
6+
| srv/service1.js:25:30:25:32 | req |
7+
| srv/service1.js:31:30:31:32 | req |
8+
| srv/service1.js:37:30:37:32 | req |
9+
| srv/service1.js:44:29:44:31 | req |
10+
| srv/service1.js:60:30:60:32 | req |
11+
| srv/service1.js:66:30:66:32 | req |
12+
| srv/service1.js:72:30:72:32 | req |
13+
| srv/service1.js:78:30:78:32 | req |
14+
| srv/service1.js:84:29:84:31 | req |
15+
| srv/service1.js:90:29:90:31 | req |
16+
| srv/service1.js:96:29:96:31 | req |
17+
| srv/service1.js:102:30:102:32 | req |
18+
| srv/service1.js:110:21:110:27 | request |
19+
| srv/service1.js:114:23:114:29 | request |
20+
| srv/service1.js:118:24:118:30 | request |
21+
| srv/service1.js:122:19:122:25 | request |
22+
| srv/service1.js:126:29:126:35 | request |
1323
| srv/service2.js:4:27:4:29 | msg |
14-
| srv/service3.js:5:29:5:31 | req |
15-
| srv/service3.js:11:29:11:31 | req |
16-
| srv/service3.js:17:29:17:31 | req |
17-
| srv/service3.js:23:29:23:31 | req |
18-
| srv/service3.js:39:29:39:31 | req |
19-
| srv/service3.js:45:29:45:31 | req |
20-
| srv/service3.js:51:29:51:31 | req |
21-
| srv/service3.js:57:29:57:31 | req |
22-
| srv/service3.js:63:29:63:31 | req |
23-
| srv/service3.js:69:30:69:32 | req |
24+
| srv/service3.js:5:30:5:32 | req |
25+
| srv/service3.js:11:30:11:32 | req |
26+
| srv/service3.js:18:30:18:32 | req |
27+
| srv/service3.js:24:30:24:32 | req |
28+
| srv/service3.js:30:30:30:32 | req |
29+
| srv/service3.js:36:30:36:32 | req |
30+
| srv/service3.js:43:29:43:31 | req |
31+
| srv/service3.js:59:30:59:32 | req |
32+
| srv/service3.js:65:30:65:32 | req |
33+
| srv/service3.js:71:30:71:32 | req |
34+
| srv/service3.js:77:30:77:32 | req |
35+
| srv/service3.js:83:29:83:31 | req |
36+
| srv/service3.js:89:29:89:31 | req |
37+
| srv/service3.js:95:29:95:31 | req |
38+
| srv/service3.js:101:30:101:32 | req |
39+
| srv/service3.js:111:21:111:27 | request |
40+
| srv/service3.js:115:23:115:29 | request |
41+
| srv/service3.js:119:24:119:30 | request |
42+
| srv/service3.js:123:19:123:25 | request |
43+
| srv/service3.js:127:29:127:35 | request |

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

Lines changed: 40 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,43 @@
11
| 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/service1.js:47:29:47:45 | req._queryOptions |
2+
| srv/service1.js:20:33:20:42 | req.params |
3+
| srv/service1.js:32:29:32:39 | req.headers |
4+
| srv/service1.js:45:30:45:56 | req.htt ... omeProp |
5+
| srv/service1.js:46:30:46:55 | req.htt ... omeProp |
6+
| srv/service1.js:47:30:47:57 | req.htt ... omeProp |
7+
| srv/service1.js:48:30:48:58 | req.htt ... omeProp |
8+
| srv/service1.js:49:30:49:58 | req.htt ... omeProp |
9+
| srv/service1.js:50:30:50:53 | req.htt ... inalUrl |
10+
| srv/service1.js:51:30:51:50 | req.htt ... ostname |
11+
| srv/service1.js:52:30:52:57 | req.htt ... eProp") |
12+
| srv/service1.js:53:30:53:56 | req.htt ... eProp") |
13+
| srv/service1.js:54:31:54:61 | req.htt ... eProp") |
14+
| srv/service1.js:55:31:55:60 | req.htt ... eProp") |
15+
| srv/service1.js:61:29:61:34 | req.id |
16+
| srv/service1.js:73:29:73:45 | req._queryOptions |
17+
| srv/service1.js:111:10:111:21 | request.data |
18+
| srv/service1.js:115:10:115:23 | request.params |
19+
| srv/service1.js:119:10:119:24 | request.headers |
20+
| srv/service1.js:123:10:123:19 | request.id |
21+
| srv/service1.js:127:10:127:30 | request ... Options |
1722
| srv/service2.js:5:31:5:38 | msg.data |
1823
| srv/service3.js:6:33:6:40 | req.data |
19-
| srv/service3.js:12:33:12:42 | req.params |
20-
| srv/service3.js:18:29:18:39 | req.headers |
21-
| srv/service3.js:24:30:24:56 | req.htt ... omeProp |
22-
| srv/service3.js:25:30:25:55 | req.htt ... omeProp |
23-
| srv/service3.js:26:30:26:57 | req.htt ... omeProp |
24-
| srv/service3.js:27:30:27:58 | req.htt ... omeProp |
25-
| srv/service3.js:28:30:28:58 | req.htt ... omeProp |
26-
| srv/service3.js:29:30:29:53 | req.htt ... inalUrl |
27-
| srv/service3.js:30:30:30:50 | req.htt ... ostname |
28-
| srv/service3.js:31:30:31:57 | req.htt ... eProp") |
29-
| srv/service3.js:32:30:32:56 | req.htt ... eProp") |
30-
| srv/service3.js:33:31:33:61 | req.htt ... eProp") |
31-
| srv/service3.js:34:31:34:60 | req.htt ... eProp") |
32-
| srv/service3.js:40:29:40:34 | req.id |
33-
| srv/service3.js:46:29:46:45 | req._queryOptions |
24+
| srv/service3.js:19:31:19:40 | req.params |
25+
| srv/service3.js:31:29:31:39 | req.headers |
26+
| srv/service3.js:44:30:44:56 | req.htt ... omeProp |
27+
| srv/service3.js:45:30:45:55 | req.htt ... omeProp |
28+
| srv/service3.js:46:30:46:57 | req.htt ... omeProp |
29+
| srv/service3.js:47:30:47:58 | req.htt ... omeProp |
30+
| srv/service3.js:48:30:48:58 | req.htt ... omeProp |
31+
| srv/service3.js:49:30:49:53 | req.htt ... inalUrl |
32+
| srv/service3.js:50:30:50:50 | req.htt ... ostname |
33+
| srv/service3.js:51:30:51:57 | req.htt ... eProp") |
34+
| srv/service3.js:52:30:52:56 | req.htt ... eProp") |
35+
| srv/service3.js:53:31:53:61 | req.htt ... eProp") |
36+
| srv/service3.js:54:31:54:60 | req.htt ... eProp") |
37+
| srv/service3.js:60:29:60:34 | req.id |
38+
| srv/service3.js:72:29:72:45 | req._queryOptions |
39+
| srv/service3.js:112:10:112:21 | request.data |
40+
| srv/service3.js:116:10:116:23 | request.params |
41+
| srv/service3.js:120:10:120:24 | request.headers |
42+
| srv/service3.js:124:10:124:19 | request.id |
43+
| srv/service3.js:128:10:128:30 | request ... Options |

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

Lines changed: 57 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,24 +3,44 @@ const cds = require("@sap/cds");
33
/* Emit a "Received1" event upon receiving a READ request on its entity. */
44
module.exports = class Service1 extends cds.ApplicationService {
55
init() {
6-
this.on("send1", async (req) => {
6+
this.on("send11", async (req) => {
77
const { messageToPass } = req.data; // UNSAFE: Taint source, Exposed service
88
const Service2 = await cds.connect.to("service-2");
99
Service2.send("send2", { messageToPass });
1010
});
1111

12-
this.on("send2", async (req) => {
12+
this.on("send12", async (req) => {
13+
const reqData = getReqData(req);
14+
const { messageToPass } = reqData;
15+
const Service2 = await cds.connect.to("service-2");
16+
Service2.send("send2", { messageToPass });
17+
});
18+
19+
this.on("send21", async (req) => {
1320
const [ messageToPass ] = req.params; // UNSAFE: Taint source, Exposed service
1421
const Service2 = await cds.connect.to("service-2");
1522
Service2.send("send2", { messageToPass });
1623
});
1724

18-
this.on("send3", async (req) => {
25+
this.on("send22", async (req) => {
26+
const [ messageToPass ] = getReqParams(req); // UNSAFE: Taint source, Exposed service
27+
const Service2 = await cds.connect.to("service-2");
28+
Service2.send("send2", { messageToPass });
29+
});
30+
31+
this.on("send31", async (req) => {
1932
const messageToPass = req.headers["user-agent"]; // UNSAFE: Taint source, Exposed service
2033
const Service2 = await cds.connect.to("service-2");
2134
Service2.send("send2", { messageToPass });
2235
});
2336

37+
this.on("send32", async (req) => {
38+
const reqHeaders = getReqHeaders(req);
39+
const messageToPass = reqHeaders["user-agent"]; // UNSAFE: Taint source, Exposed service
40+
const Service2 = await cds.connect.to("service-2");
41+
Service2.send("send2", { messageToPass });
42+
});
43+
2444
this.on("send4", async (req) => {
2545
const messageToPass1 = req.http.req.query.someProp; // UNSAFE: Taint source, Exposed service
2646
const messageToPass2 = req.http.req.body.someProp; // UNSAFE: Taint source, Exposed service
@@ -37,18 +57,30 @@ module.exports = class Service1 extends cds.ApplicationService {
3757
Service2.send("send2", { messageToPass1 });
3858
});
3959

40-
this.on("send5", async (req) => {
60+
this.on("send51", async (req) => {
4161
const messageToPass = req.id; // UNSAFE: Taint source, Exposed service
4262
const Service2 = await cds.connect.to("service-2");
4363
Service2.send("send2", { messageToPass });
4464
});
4565

46-
this.on("send6", async (req) => {
66+
this.on("send52", async (req) => {
67+
const messageToPass = getReqId(req); // UNSAFE: Taint source, Exposed service
68+
const Service2 = await cds.connect.to("service-2");
69+
Service2.send("send2", { messageToPass });
70+
});
71+
72+
this.on("send61", async (req) => {
4773
const messageToPass = req._queryOptions; // UNSAFE: Taint source, Exposed service
4874
const Service2 = await cds.connect.to("service-2");
4975
Service2.send("send2", { messageToPass });
5076
});
5177

78+
this.on("send62", async (req) => {
79+
const messageToPass = getReqQueryOptions(req); // UNSAFE: Taint source, Exposed service
80+
const Service2 = await cds.connect.to("service-2");
81+
Service2.send("send2", { messageToPass });
82+
});
83+
5284
this.on("send7", async (req) => {
5385
const messageToPass = req.locale; // SAFE: Not a taint source, Exposed service
5486
const Service2 = await cds.connect.to("service-2");
@@ -74,3 +106,23 @@ module.exports = class Service1 extends cds.ApplicationService {
74106
});
75107
}
76108
};
109+
110+
function getReqData(request) {
111+
return request.data; // UNSAFE: Taint source, Exposed service
112+
}
113+
114+
function getReqParams(request) {
115+
return request.params; // UNSAFE: Taint source, Exposed service
116+
}
117+
118+
function getReqHeaders(request) {
119+
return request.headers; // UNSAFE: Taint source, Exposed service
120+
}
121+
122+
function getReqId(request) {
123+
return request.id; // UNSAFE: Taint source, Exposed service
124+
}
125+
126+
function getReqQueryOptions(request) {
127+
return request._queryOptions; // UNSAFE: Taint source, Exposed service
128+
}

0 commit comments

Comments
 (0)