Skip to content

Commit 8918839

Browse files
jeongsoolee09jeongsoolee09
authored
Merge pull request #208 from advanced-security/jeongsoolee09/restrict-cap-remoteflowsource-properties
Restrict `RemoteFlowSource` of CAP to only some properties and method calls on it
2 parents 08f7b1b + 139c259 commit 8918839

File tree

18 files changed

+562
-623
lines changed

18 files changed

+562
-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/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
});

‎javascript/frameworks/cap/test/models/cds/remoteflowsources/srv/service2.cds

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,8 @@
11
using { advanced_security.log_injection.sample_entities as db_schema } from '../db/schema';
22

3-
/* Uncomment the line below to make the service hidden */
4-
// @protocol: 'none'
53
service Service2 @(path: '/service-2') {
6-
/* Entity to send READ/GET about. */
74
entity Service2Entity as projection on db_schema.Entity2 excluding { Attribute4 }
85

9-
/* API to talk to Service2. */
106
action send2 (
117
messageToPass: String
128
) returns String;

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

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

33
module.exports = cds.service.impl(function () {
4-
/* Log upon receiving an "send2" event. */
54
this.on("send2", async (msg) => {
6-
const { messageToPass } = msg.data;
7-
/* Do something with the received data; customize below to individual needs. */
5+
const { messageToPass } = msg.data; // UNSAFE: Taint source, Exposed service
86
const doSomething = console.log;
97
doSomething(messageToPass);
108
});

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

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

0 commit comments

Comments
 (0)