Skip to content

Commit 028fcc7

Browse files
erik-krogherik-krogh
authored
Merge pull request github#11959 from erik-krogh/ssrfSan
JS: add encodeURIComponent as a sanitizer for request-forgery
2 parents a498936 + 49f5e89 commit 028fcc7

File tree

5 files changed

+55
-22
lines changed

5 files changed

+55
-22
lines changed

‎javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,4 +81,14 @@ module RequestForgery {
8181

8282
override string getKind() { result = "endpoint" }
8383
}
84+
85+
private import Xss as Xss
86+
87+
/**
88+
* A call to `encodeURI` or `encodeURIComponent`, viewed as a sanitizer for request forgery.
89+
* These calls will escape "/" to "%2F", which is not a problem for request forgery.
90+
* The result from calling `encodeURI` or `encodeURIComponent` is not a valid URL, and only makes sense
91+
* as a part of a URL.
92+
*/
93+
class UriEncodingSanitizer extends Sanitizer instanceof Xss::Shared::UriEncodingSanitizer { }
8494
}

‎javascript/ql/test/experimental/Security/CWE-918/SSRF.expected

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -15,20 +15,10 @@ nodes
1515
| check-path.js:19:13:19:43 | 'test.c ... tainted |
1616
| check-path.js:19:27:19:43 | req.query.tainted |
1717
| check-path.js:19:27:19:43 | req.query.tainted |
18-
| check-path.js:22:13:22:63 | 'test.c ... ainted) |
19-
| check-path.js:22:13:22:63 | 'test.c ... ainted) |
20-
| check-path.js:22:27:22:63 | encodeU ... ainted) |
21-
| check-path.js:22:46:22:62 | req.query.tainted |
22-
| check-path.js:22:46:22:62 | req.query.tainted |
2318
| check-path.js:23:13:23:45 | `/addre ... inted}` |
2419
| check-path.js:23:13:23:45 | `/addre ... inted}` |
2520
| check-path.js:23:27:23:43 | req.query.tainted |
2621
| check-path.js:23:27:23:43 | req.query.tainted |
27-
| check-path.js:24:13:24:65 | `/addre ... nted)}` |
28-
| check-path.js:24:13:24:65 | `/addre ... nted)}` |
29-
| check-path.js:24:27:24:63 | encodeU ... ainted) |
30-
| check-path.js:24:46:24:62 | req.query.tainted |
31-
| check-path.js:24:46:24:62 | req.query.tainted |
3222
| check-path.js:33:15:33:45 | 'test.c ... tainted |
3323
| check-path.js:33:15:33:45 | 'test.c ... tainted |
3424
| check-path.js:33:29:33:45 | req.query.tainted |
@@ -97,18 +87,10 @@ edges
9787
| check-path.js:19:27:19:43 | req.query.tainted | check-path.js:19:13:19:43 | 'test.c ... tainted |
9888
| check-path.js:19:27:19:43 | req.query.tainted | check-path.js:19:13:19:43 | 'test.c ... tainted |
9989
| check-path.js:19:27:19:43 | req.query.tainted | check-path.js:19:13:19:43 | 'test.c ... tainted |
100-
| check-path.js:22:27:22:63 | encodeU ... ainted) | check-path.js:22:13:22:63 | 'test.c ... ainted) |
101-
| check-path.js:22:27:22:63 | encodeU ... ainted) | check-path.js:22:13:22:63 | 'test.c ... ainted) |
102-
| check-path.js:22:46:22:62 | req.query.tainted | check-path.js:22:27:22:63 | encodeU ... ainted) |
103-
| check-path.js:22:46:22:62 | req.query.tainted | check-path.js:22:27:22:63 | encodeU ... ainted) |
10490
| check-path.js:23:27:23:43 | req.query.tainted | check-path.js:23:13:23:45 | `/addre ... inted}` |
10591
| check-path.js:23:27:23:43 | req.query.tainted | check-path.js:23:13:23:45 | `/addre ... inted}` |
10692
| check-path.js:23:27:23:43 | req.query.tainted | check-path.js:23:13:23:45 | `/addre ... inted}` |
10793
| check-path.js:23:27:23:43 | req.query.tainted | check-path.js:23:13:23:45 | `/addre ... inted}` |
108-
| check-path.js:24:27:24:63 | encodeU ... ainted) | check-path.js:24:13:24:65 | `/addre ... nted)}` |
109-
| check-path.js:24:27:24:63 | encodeU ... ainted) | check-path.js:24:13:24:65 | `/addre ... nted)}` |
110-
| check-path.js:24:46:24:62 | req.query.tainted | check-path.js:24:27:24:63 | encodeU ... ainted) |
111-
| check-path.js:24:46:24:62 | req.query.tainted | check-path.js:24:27:24:63 | encodeU ... ainted) |
11294
| check-path.js:33:29:33:45 | req.query.tainted | check-path.js:33:15:33:45 | 'test.c ... tainted |
11395
| check-path.js:33:29:33:45 | req.query.tainted | check-path.js:33:15:33:45 | 'test.c ... tainted |
11496
| check-path.js:33:29:33:45 | req.query.tainted | check-path.js:33:15:33:45 | 'test.c ... tainted |
@@ -167,9 +149,7 @@ edges
167149
| check-domain.js:26:15:26:27 | req.query.url | check-domain.js:26:15:26:27 | req.query.url | check-domain.js:26:15:26:27 | req.query.url | The URL of this request depends on a user-provided value. |
168150
| check-middleware.js:9:13:9:43 | "test.c ... tainted | check-middleware.js:9:27:9:43 | req.query.tainted | check-middleware.js:9:13:9:43 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
169151
| check-path.js:19:13:19:43 | 'test.c ... tainted | check-path.js:19:27:19:43 | req.query.tainted | check-path.js:19:13:19:43 | 'test.c ... tainted | The URL of this request depends on a user-provided value. |
170-
| check-path.js:22:13:22:63 | 'test.c ... ainted) | check-path.js:22:46:22:62 | req.query.tainted | check-path.js:22:13:22:63 | 'test.c ... ainted) | The URL of this request depends on a user-provided value. |
171152
| check-path.js:23:13:23:45 | `/addre ... inted}` | check-path.js:23:27:23:43 | req.query.tainted | check-path.js:23:13:23:45 | `/addre ... inted}` | The URL of this request depends on a user-provided value. |
172-
| check-path.js:24:13:24:65 | `/addre ... nted)}` | check-path.js:24:46:24:62 | req.query.tainted | check-path.js:24:13:24:65 | `/addre ... nted)}` | The URL of this request depends on a user-provided value. |
173153
| check-path.js:33:15:33:45 | 'test.c ... tainted | check-path.js:33:29:33:45 | req.query.tainted | check-path.js:33:15:33:45 | 'test.c ... tainted | The URL of this request depends on a user-provided value. |
174154
| check-path.js:37:15:37:45 | 'test.c ... tainted | check-path.js:37:29:37:45 | req.query.tainted | check-path.js:37:15:37:45 | 'test.c ... tainted | The URL of this request depends on a user-provided value. |
175155
| check-path.js:45:13:45:44 | `${base ... inted}` | check-path.js:45:26:45:42 | req.query.tainted | check-path.js:45:13:45:44 | `${base ... inted}` | The URL of this request depends on a user-provided value. |

‎javascript/ql/test/experimental/Security/CWE-918/check-path.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,9 @@ app.get('/check-with-axios', req => {
1919
axios.get('test.com/' + req.query.tainted); // SSRF
2020
axios.get('test.com/' + Number(req.query.tainted)); // OK
2121
axios.get('test.com/' + req.user.id); // OK
22-
axios.get('test.com/' + encodeURIComponent(req.query.tainted)); // SSRF
22+
axios.get('test.com/' + encodeURIComponent(req.query.tainted)); // OK
2323
axios.get(`/addresses/${req.query.tainted}`); // SSRF
24-
axios.get(`/addresses/${encodeURIComponent(req.query.tainted)}`); // SSRF
24+
axios.get(`/addresses/${encodeURIComponent(req.query.tainted)}`); // OK
2525

2626
if (Number.isInteger(req.query.tainted)) {
2727
axios.get('test.com/' + req.query.tainted); // OK

‎javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,19 @@ nodes
8888
| serverSide.js:115:25:115:35 | request.url |
8989
| serverSide.js:117:27:117:29 | url |
9090
| serverSide.js:117:27:117:29 | url |
91+
| serverSide.js:123:9:123:52 | tainted |
92+
| serverSide.js:123:19:123:42 | url.par ... , true) |
93+
| serverSide.js:123:19:123:48 | url.par ... ).query |
94+
| serverSide.js:123:19:123:52 | url.par ... ery.url |
95+
| serverSide.js:123:29:123:35 | req.url |
96+
| serverSide.js:123:29:123:35 | req.url |
97+
| serverSide.js:127:14:127:20 | tainted |
98+
| serverSide.js:127:14:127:20 | tainted |
99+
| serverSide.js:130:9:130:45 | myUrl |
100+
| serverSide.js:130:17:130:45 | `${some ... inted}` |
101+
| serverSide.js:130:37:130:43 | tainted |
102+
| serverSide.js:131:15:131:19 | myUrl |
103+
| serverSide.js:131:15:131:19 | myUrl |
91104
edges
92105
| serverSide.js:14:9:14:52 | tainted | serverSide.js:18:13:18:19 | tainted |
93106
| serverSide.js:14:9:14:52 | tainted | serverSide.js:18:13:18:19 | tainted |
@@ -172,6 +185,18 @@ edges
172185
| serverSide.js:115:17:115:42 | new URL ... , base) | serverSide.js:115:11:115:42 | url |
173186
| serverSide.js:115:25:115:35 | request.url | serverSide.js:115:17:115:42 | new URL ... , base) |
174187
| serverSide.js:115:25:115:35 | request.url | serverSide.js:115:17:115:42 | new URL ... , base) |
188+
| serverSide.js:123:9:123:52 | tainted | serverSide.js:127:14:127:20 | tainted |
189+
| serverSide.js:123:9:123:52 | tainted | serverSide.js:127:14:127:20 | tainted |
190+
| serverSide.js:123:9:123:52 | tainted | serverSide.js:130:37:130:43 | tainted |
191+
| serverSide.js:123:19:123:42 | url.par ... , true) | serverSide.js:123:19:123:48 | url.par ... ).query |
192+
| serverSide.js:123:19:123:48 | url.par ... ).query | serverSide.js:123:19:123:52 | url.par ... ery.url |
193+
| serverSide.js:123:19:123:52 | url.par ... ery.url | serverSide.js:123:9:123:52 | tainted |
194+
| serverSide.js:123:29:123:35 | req.url | serverSide.js:123:19:123:42 | url.par ... , true) |
195+
| serverSide.js:123:29:123:35 | req.url | serverSide.js:123:19:123:42 | url.par ... , true) |
196+
| serverSide.js:130:9:130:45 | myUrl | serverSide.js:131:15:131:19 | myUrl |
197+
| serverSide.js:130:9:130:45 | myUrl | serverSide.js:131:15:131:19 | myUrl |
198+
| serverSide.js:130:17:130:45 | `${some ... inted}` | serverSide.js:130:9:130:45 | myUrl |
199+
| serverSide.js:130:37:130:43 | tainted | serverSide.js:130:17:130:45 | `${some ... inted}` |
175200
#select
176201
| serverSide.js:18:5:18:20 | request(tainted) | serverSide.js:14:29:14:35 | req.url | serverSide.js:18:13:18:19 | tainted | The $@ of this request depends on a $@. | serverSide.js:18:13:18:19 | tainted | URL | serverSide.js:14:29:14:35 | req.url | user-provided value |
177202
| serverSide.js:20:5:20:24 | request.get(tainted) | serverSide.js:14:29:14:35 | req.url | serverSide.js:20:17:20:23 | tainted | The $@ of this request depends on a $@. | serverSide.js:20:17:20:23 | tainted | URL | serverSide.js:14:29:14:35 | req.url | user-provided value |
@@ -195,3 +220,5 @@ edges
195220
| serverSide.js:100:5:100:26 | new Web ... ainted) | serverSide.js:98:29:98:35 | req.url | serverSide.js:100:19:100:25 | tainted | The $@ of this request depends on a $@. | serverSide.js:100:19:100:25 | tainted | URL | serverSide.js:98:29:98:35 | req.url | user-provided value |
196221
| serverSide.js:109:20:109:30 | new ws(url) | serverSide.js:108:17:108:27 | request.url | serverSide.js:109:27:109:29 | url | The $@ of this request depends on a $@. | serverSide.js:109:27:109:29 | url | URL | serverSide.js:108:17:108:27 | request.url | user-provided value |
197222
| serverSide.js:117:20:117:30 | new ws(url) | serverSide.js:115:25:115:35 | request.url | serverSide.js:117:27:117:29 | url | The $@ of this request depends on a $@. | serverSide.js:117:27:117:29 | url | URL | serverSide.js:115:25:115:35 | request.url | user-provided value |
223+
| serverSide.js:125:5:128:6 | axios({ ... \\n }) | serverSide.js:123:29:123:35 | req.url | serverSide.js:127:14:127:20 | tainted | The $@ of this request depends on a $@. | serverSide.js:127:14:127:20 | tainted | URL | serverSide.js:123:29:123:35 | req.url | user-provided value |
224+
| serverSide.js:131:5:131:20 | axios.get(myUrl) | serverSide.js:123:29:123:35 | req.url | serverSide.js:131:15:131:19 | myUrl | The $@ of this request depends on a $@. | serverSide.js:131:15:131:19 | myUrl | URL | serverSide.js:123:29:123:35 | req.url | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-918/serverSide.js

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,3 +117,19 @@ new ws.Server({ port: 8080 }).on('connection', function (socket, request) {
117117
const socket = new ws(url);
118118
});
119119
});
120+
121+
122+
var server2 = http.createServer(function(req, res) {
123+
var tainted = url.parse(req.url, true).query.url;
124+
125+
axios({
126+
method: 'get',
127+
url: tainted // NOT OK
128+
})
129+
130+
var myUrl = `${something}/bla/${tainted}`;
131+
axios.get(myUrl); // NOT OK
132+
133+
var myEncodedUrl = `${something}/bla/${encodeURIComponent(tainted)}`;
134+
axios.get(myEncodedUrl); // OK
135+
})

0 commit comments

Comments
 (0)