Skip to content

Commit 162c19c

Browse files
erik-krogherik-krogh
committed
changes based on review
1 parent 7637ebc commit 162c19c

File tree

6 files changed

+70
-42
lines changed

6 files changed

+70
-42
lines changed

‎javascript/ql/src/semmle/javascript/frameworks/Logging.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ abstract class LoggerCall extends DataFlow::CallNode {
1717
/**
1818
* Gets a log level name that is used in RFC5424, `npm`, `console`.
1919
*/
20-
private string getAStandardLoggerMethodName() {
20+
string getAStandardLoggerMethodName() {
2121
result = "crit" or
2222
result = "debug" or
2323
result = "error" or

‎javascript/ql/src/semmle/javascript/security/dataflow/ExceptionXss.qll‎

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -16,27 +16,21 @@ module ExceptionXss {
1616
* Gets the name of a method that does not leak taint from its arguments if an exception is thrown by the method.
1717
*/
1818
private string getAnUnlikelyToThrowMethodName() {
19-
result = "getElementById" or
20-
result = "indexOf" or
21-
result = "stringify" or
22-
result = "assign" or
23-
// fs methods. (The callback argument to the async functions are vulnerable, but its unlikely that the callback is the user-controlled part).
24-
result = "existsSync" or
25-
result = "exists" or
26-
result = "writeFileSync" or
27-
result = "writeFile" or
28-
result = "appendFile" or
29-
result = "appendFileSync" or
30-
result = "pick" or
31-
// log.info etc.
32-
result = "info" or
33-
result = "warn" or
34-
result = "error" or
35-
result = "join" or
19+
result = "getElementById" or // document.getElementById
20+
result = "indexOf" or // String.prototype.indexOf
21+
result = "assign" or // Object.assign
22+
result = "pick" or // _.pick
23+
result = getAStandardLoggerMethodName() or // log.info etc.
3624
result = "val" or // $.val
3725
result = "parse" or // JSON.parse
38-
result = "push" or // Array.prototype.push
39-
result = "test" // RegExp.prototype.test
26+
result = "stringify" or // JSON.stringify
27+
result = "test" or // RegExp.prototype.test
28+
result = "setItem" or // localStorage.setItem
29+
result = "existsSync" or
30+
// the "fs" methods are a mix of "this is safe" and "you have bigger problems".
31+
exists(ExternalMemberDecl decl | decl.hasQualifiedName("fs", result)) or
32+
// Array methods are generally exception safe.
33+
exists(ExternalMemberDecl decl | decl.hasQualifiedName("Array", result))
4034
}
4135

4236
/**
@@ -104,7 +98,7 @@ module ExceptionXss {
10498
}
10599

106100
/**
107-
* Get the parameter in the callback that contains an error.
101+
* Gets the parameter in the callback that contains an error.
108102
* In the current implementation this is always the first parameter.
109103
*/
110104
DataFlow::Node getErrorParam() { result = errorParameter }

‎javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll‎

Lines changed: 18 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -51,24 +51,6 @@ module Shared {
5151
)
5252
}
5353
}
54-
55-
/**
56-
* A property read from a safe property is considered a sanitizer.
57-
*/
58-
class SafePropertyReadSanitizer extends Sanitizer, DataFlow::Node {
59-
SafePropertyReadSanitizer() {
60-
exists(PropAccess pacc | pacc = this.asExpr() |
61-
isSafeLocationProperty(pacc)
62-
or
63-
// `$(location.hash)` is a fairly common and safe idiom
64-
// (because `location.hash` always starts with `#`),
65-
// so we mark `hash` as safe for the purposes of this query
66-
pacc.getPropertyName() = "hash"
67-
or
68-
pacc.getPropertyName() = "length"
69-
)
70-
}
71-
}
7254
}
7355

7456
/** Provides classes and predicates for the DOM-based XSS query. */
@@ -277,6 +259,24 @@ module DomBasedXss {
277259
}
278260
}
279261

262+
/**
263+
* A property read from a safe property is considered a sanitizer.
264+
*/
265+
class SafePropertyReadSanitizer extends Sanitizer, DataFlow::Node {
266+
SafePropertyReadSanitizer() {
267+
exists(PropAccess pacc | pacc = this.asExpr() |
268+
isSafeLocationProperty(pacc)
269+
or
270+
// `$(location.hash)` is a fairly common and safe idiom
271+
// (because `location.hash` always starts with `#`),
272+
// so we mark `hash` as safe for the purposes of this query
273+
pacc.getPropertyName() = "hash"
274+
or
275+
pacc.getPropertyName() = "length"
276+
)
277+
}
278+
}
279+
280280
/**
281281
* A regexp replacement involving an HTML meta-character, viewed as a sanitizer for
282282
* XSS vulnerabilities.
@@ -287,8 +287,6 @@ module DomBasedXss {
287287
private class MetacharEscapeSanitizer extends Sanitizer, Shared::MetacharEscapeSanitizer { }
288288

289289
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
290-
291-
private class SafePropertyReadSanitizer extends Sanitizer, Shared::SafePropertyReadSanitizer {}
292290
}
293291

294292
/** Provides classes and predicates for the reflected XSS query. */

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ nodes
99
| etherpad.js:9:16:9:53 | req.que ... e + ")" |
1010
| etherpad.js:11:12:11:19 | response |
1111
| etherpad.js:11:12:11:19 | response |
12+
| exception-xss.js:190:12:190:24 | req.params.id |
13+
| exception-xss.js:190:12:190:24 | req.params.id |
14+
| exception-xss.js:190:12:190:24 | req.params.id |
1215
| formatting.js:4:9:4:29 | evil |
1316
| formatting.js:4:16:4:29 | req.query.evil |
1417
| formatting.js:4:16:4:29 | req.query.evil |
@@ -77,6 +80,7 @@ edges
7780
| etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:9:16:9:53 | req.que ... e + ")" |
7881
| etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:9:16:9:53 | req.que ... e + ")" |
7982
| etherpad.js:9:16:9:53 | req.que ... e + ")" | etherpad.js:9:5:9:53 | response |
83+
| exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id |
8084
| formatting.js:4:9:4:29 | evil | formatting.js:6:43:6:46 | evil |
8185
| formatting.js:4:9:4:29 | evil | formatting.js:7:49:7:52 | evil |
8286
| formatting.js:4:16:4:29 | req.query.evil | formatting.js:4:9:4:29 | evil |
@@ -131,6 +135,7 @@ edges
131135
#select
132136
| ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
133137
| etherpad.js:11:12:11:19 | response | etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:11:12:11:19 | response | Cross-site scripting vulnerability due to $@. | etherpad.js:9:16:9:30 | req.query.jsonp | user-provided value |
138+
| exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id | Cross-site scripting vulnerability due to $@. | exception-xss.js:190:12:190:24 | req.params.id | user-provided value |
134139
| formatting.js:6:14:6:47 | util.fo ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
135140
| formatting.js:7:14:7:53 | require ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
136141
| partial.js:10:14:10:18 | x + y | partial.js:13:42:13:48 | req.url | partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
| ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
2+
| exception-xss.js:190:12:190:24 | req.params.id | Cross-site scripting vulnerability due to $@. | exception-xss.js:190:12:190:24 | req.params.id | user-provided value |
23
| formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
34
| formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
45
| partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/exception-xss.js‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,4 +183,34 @@ app.get('/user/:id', function (req, res) {
183183
}
184184
$('myId').html(res); // NOT OK!
185185
});
186-
});
186+
});
187+
188+
app.get('/user/:id', function (req, res) {
189+
try {
190+
res.send(req.params.id);
191+
} catch(err) {
192+
res.send(err); // OK (the above `res.send()` is already reported by js/xss)
193+
}
194+
});
195+
196+
var fs = require("fs");
197+
198+
(function () {
199+
var foo = document.location.search;
200+
201+
try {
202+
// A series of functions does not throw tainted exceptions.
203+
Object.assign(foo, foo)
204+
_.pick(foo, foo);
205+
[foo, foo].join(join);
206+
$.val(foo);
207+
JSON.parse(foo);
208+
/bla/.test(foo);
209+
console.log(foo);
210+
log.info(foo);
211+
localStorage.setItem(foo);
212+
} catch (e) {
213+
$('myId').html(e); // OK
214+
}
215+
216+
})();

0 commit comments

Comments
 (0)