Skip to content

Commit 373a437

Browse files
erik-krogherik-krogh
committed
add query to detect improperly sanitized code
1 parent b6e0e66 commit 373a437

5 files changed

Lines changed: 202 additions & 0 deletions

File tree

‎javascript/ql/src/Security/CWE-094/ImproperCodeSanitization.qhelp‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Placeholder
9+
</p>
10+
</overview>
11+
12+
<recommendation>
13+
<p>
14+
Placeholder
15+
</p>
16+
</recommendation>
17+
18+
<example>
19+
<p>
20+
Placeholder
21+
</p>
22+
23+
</example>
24+
25+
<references>
26+
<li>
27+
OWASP:
28+
<a href="https://www.owasp.org/index.php/Code_Injection">Code Injection</a>.
29+
</li>
30+
<li>
31+
MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects#Function_properties">Global functions</a>.
32+
</li>
33+
<li>
34+
MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function">Function constructor</a>.
35+
</li>
36+
</references>
37+
</qhelp>

‎javascript/ql/src/Security/CWE-094/ImproperCodeSanitization.ql‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/**
2+
* @name Improper code sanitization
3+
* @description Escaping code as HTML does not provide protection against code-injection.
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @precision high
7+
* @id js/bad-code-sanitization
8+
* @tags security
9+
* external/cwe/cwe-094
10+
* external/cwe/cwe-079
11+
* external/cwe/cwe-116
12+
*/
13+
14+
// TODO: Proper customizations module, Source class Sink class etc.
15+
import javascript
16+
import DataFlow::PathGraph
17+
private import semmle.javascript.heuristics.AdditionalSinks
18+
private import semmle.javascript.security.dataflow.CodeInjectionCustomizations
19+
20+
/**
21+
* A taint-tracking configuration for reasoning about improper code sanitization vulnerabilities.
22+
*/
23+
class Configuration extends TaintTracking::Configuration {
24+
Configuration() { this = "ImproperCodeSanitization" }
25+
26+
override predicate isSource(DataFlow::Node source) { source = source() }
27+
28+
override predicate isSink(DataFlow::Node sink) { sink = sink() }
29+
30+
override predicate isSanitizer(DataFlow::Node sanitizer) {
31+
sanitizer instanceof StringReplaceCall // any string-replace that happens after the bad-sanitizer, is assumed to be a good sanitizer.
32+
// TODO: Specialize? This regexp sanitizes: /[<>\b\f\n\r\t\0\u2028\u2029]/g
33+
}
34+
}
35+
36+
private DataFlow::Node source() {
37+
result instanceof HtmlSanitizerCall
38+
or
39+
result = DataFlow::globalVarRef("JSON").getAMemberCall("stringify")
40+
}
41+
42+
private StringOps::ConcatenationLeaf sink() {
43+
exists(StringOps::ConcatenationRoot root, int i |
44+
root.getOperand(i) = result and
45+
not exists(result.getStringValue()) and
46+
not root = endsInCodeInjectionSink()
47+
|
48+
exists(StringOps::ConcatenationLeaf functionLeaf |
49+
functionLeaf = root.getOperand(any(int j | j < i))
50+
|
51+
functionLeaf
52+
.getStringValue()
53+
.regexpMatch([".*function( )?([a-zA-Z0-9]+)?( )?\\(.*", ".*eval\\(.*",
54+
".*new Function\\(.*", "(^|.*[^a-zA-Z0-9])\\(.*\\)( )?=>.*"])
55+
)
56+
)
57+
}
58+
59+
private DataFlow::Node endsInCodeInjectionSink(DataFlow::TypeBackTracker t) {
60+
t.start() and
61+
result instanceof CodeInjection::Sink and
62+
not result instanceof StringOps::ConcatenationRoot // the heuristic CodeInjection sink looks for string-concats, we are not interrested in those here.
63+
or
64+
exists(DataFlow::TypeBackTracker t2 | t = t2.smallstep(result, endsInCodeInjectionSink(t2)))
65+
}
66+
67+
private DataFlow::Node endsInCodeInjectionSink() {
68+
result = endsInCodeInjectionSink(DataFlow::TypeBackTracker::end())
69+
}
70+
71+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
72+
where cfg.hasFlowPath(source, sink)
73+
select sink.getNode(), source, sink, "$@ flows to here and is used to construct code.",
74+
source.getNode(), "Improperly sanitized value"

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/ImproperCodeSanitization.expected‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
nodes
2+
| bad-code-sanitization.js:2:12:2:90 | /^[_$a- ... key)}]` |
3+
| bad-code-sanitization.js:2:65:2:90 | `[${JSO ... key)}]` |
4+
| bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) |
5+
| bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) |
6+
| bad-code-sanitization.js:6:11:6:25 | statements |
7+
| bad-code-sanitization.js:6:24:6:25 | [] |
8+
| bad-code-sanitization.js:7:21:7:70 | `${name ... key])}` |
9+
| bad-code-sanitization.js:7:31:7:43 | safeProp(key) |
10+
| bad-code-sanitization.js:8:27:8:36 | statements |
11+
| bad-code-sanitization.js:8:27:8:46 | statements.join(';') |
12+
| bad-code-sanitization.js:8:27:8:46 | statements.join(';') |
13+
| bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) |
14+
| bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) |
15+
| bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) |
16+
| bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) |
17+
| bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) |
18+
| bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) |
19+
| bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) |
20+
| bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) |
21+
| bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) |
22+
| bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) |
23+
| bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) |
24+
| bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) |
25+
edges
26+
| bad-code-sanitization.js:2:12:2:90 | /^[_$a- ... key)}]` | bad-code-sanitization.js:7:31:7:43 | safeProp(key) |
27+
| bad-code-sanitization.js:2:65:2:90 | `[${JSO ... key)}]` | bad-code-sanitization.js:2:12:2:90 | /^[_$a- ... key)}]` |
28+
| bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) | bad-code-sanitization.js:2:65:2:90 | `[${JSO ... key)}]` |
29+
| bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) | bad-code-sanitization.js:2:65:2:90 | `[${JSO ... key)}]` |
30+
| bad-code-sanitization.js:6:11:6:25 | statements | bad-code-sanitization.js:8:27:8:36 | statements |
31+
| bad-code-sanitization.js:6:24:6:25 | [] | bad-code-sanitization.js:6:11:6:25 | statements |
32+
| bad-code-sanitization.js:7:21:7:70 | `${name ... key])}` | bad-code-sanitization.js:6:24:6:25 | [] |
33+
| bad-code-sanitization.js:7:31:7:43 | safeProp(key) | bad-code-sanitization.js:7:21:7:70 | `${name ... key])}` |
34+
| bad-code-sanitization.js:8:27:8:36 | statements | bad-code-sanitization.js:8:27:8:46 | statements.join(';') |
35+
| bad-code-sanitization.js:8:27:8:36 | statements | bad-code-sanitization.js:8:27:8:46 | statements.join(';') |
36+
| bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) |
37+
| bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) | bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) |
38+
| bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) | bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) |
39+
| bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) |
40+
#select
41+
| bad-code-sanitization.js:8:27:8:46 | statements.join(';') | bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) | bad-code-sanitization.js:8:27:8:46 | statements.join(';') | $@ flows to here and is used to construct code. | bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) | Improperly sanitized value |
42+
| bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | $@ flows to here and is used to construct code. | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | Improperly sanitized value |
43+
| bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) | bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) | bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) | $@ flows to here and is used to construct code. | bad-code-sanitization.js:19:27:19:47 | JSON.st ... (input) | Improperly sanitized value |
44+
| bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) | bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) | bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) | $@ flows to here and is used to construct code. | bad-code-sanitization.js:40:23:40:43 | JSON.st ... (input) | Improperly sanitized value |
45+
| bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | $@ flows to here and is used to construct code. | bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | Improperly sanitized value |

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/ImproperCodeSanitization.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-094/ImproperCodeSanitization.ql

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/bad-code-sanitization.js‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
function safeProp(key) {
2+
return /^[_$a-zA-Z][_$a-zA-Z0-9]*$/.test(key) ? `.${key}` : `[${JSON.stringify(key)}]`;
3+
}
4+
5+
function test1() {
6+
const statements = [];
7+
statements.push(`${name}${safeProp(key)}=${stringify(thing[key])}`);
8+
return `(function(){${statements.join(';')}})` // NOT OK
9+
}
10+
11+
import htmlescape from 'htmlescape'
12+
13+
function test2(props) {
14+
const pathname = props.data.pathname;
15+
return `function(){return new Error('${htmlescape(pathname)}')}`; // NOT OK
16+
}
17+
18+
function test3(input) {
19+
return `(function(){${JSON.stringify(input)}))` // NOT OK
20+
}
21+
22+
function evenSaferProp(key) {
23+
return /^[_$a-zA-Z][_$a-zA-Z0-9]*$/.test(key) ? `.${key}` : `[${JSON.stringify(key)}]`.replace(/[<>\b\f\n\r\t\0\u2028\u2029]/g, '');
24+
}
25+
26+
function test4(input) {
27+
return `(function(){${evenSaferProp(input)}))` // OK
28+
}
29+
30+
function test4(input) {
31+
var foo = `(function(){${JSON.stringify(input)}))` // OK - for this query - we can type-track to a code-injection sink.
32+
setTimeout(foo);
33+
}
34+
35+
function test5(input) {
36+
console.log('methodName() => ' + JSON.stringify(input)); // OK
37+
}
38+
39+
function test6(input) {
40+
return `(() => {${JSON.stringify(input)})` // NOT OK
41+
}
42+
43+
function test7(input) {
44+
return `() => {${JSON.stringify(input)}` // NOT OK
45+
}

0 commit comments

Comments
 (0)