Skip to content

Commit 62dfd1f

Browse files
erik-krogherik-krogh
committed
improve the markdown-it model
1 parent 19c5889 commit 62dfd1f

File tree

4 files changed

+76
-25
lines changed

4 files changed

+76
-25
lines changed

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

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -120,17 +120,41 @@ private class SnarkdownStep extends TaintTracking::SharedTaintStep {
120120
}
121121

122122
/**
123-
* A taint step for the `markdown-it` library.
123+
* Classes and predicates for modelling taint steps the `markdown-it` library.
124124
*/
125-
private class MarkdownItStep extends TaintTracking::SharedTaintStep {
126-
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
127-
exists(API::CallNode renderer, API::CallNode call |
128-
renderer = API::moduleImport("markdown-it").getACall() and
129-
renderer.getParameter(0).getMember("html").getARhs().mayHaveBooleanValue(true) and
130-
call = renderer.getReturn().getMember(["render", "renderInline"]).getACall()
125+
private module MarkdownIt {
126+
/**
127+
* The creation of a parser from `markdown-it`.
128+
*/
129+
private API::Node markdownIt() {
130+
exists(API::InvokeNode call |
131+
call = API::moduleImport("markdown-it").getAnInvocation()
132+
or
133+
call = API::moduleImport("markdown-it").getMember("Markdown").getAnInvocation()
131134
|
132-
succ = call and
133-
pred = call.getArgument(0)
135+
call.getParameter(0).getMember("html").getARhs().mayHaveBooleanValue(true) and
136+
result = call.getReturn()
134137
)
138+
or
139+
exists(API::CallNode call |
140+
call = markdownIt().getMember(["use", "set", "configure", "enable", "disable"]).getACall() and
141+
result = call.getReturn() and
142+
not call.getParameter(0).getARhs().getALocalSource() =
143+
DataFlow::moduleImport("markdown-it-sanitizer")
144+
)
145+
}
146+
147+
/**
148+
* A taint step for the `markdown-it` library.
149+
*/
150+
private class MarkdownItStep extends TaintTracking::SharedTaintStep {
151+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
152+
exists(API::CallNode call |
153+
call = markdownIt().getMember(["render", "renderInline"]).getACall()
154+
|
155+
succ = call and
156+
pred = call.getArgument(0)
157+
)
158+
}
135159
}
136160
}

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

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -66,13 +66,21 @@ nodes
6666
| ReflectedXss.js:85:12:85:31 | snarkdown2(req.body) |
6767
| ReflectedXss.js:85:23:85:30 | req.body |
6868
| ReflectedXss.js:85:23:85:30 | req.body |
69-
| ReflectedXss.js:94:12:94:19 | req.body |
70-
| ReflectedXss.js:94:12:94:19 | req.body |
71-
| ReflectedXss.js:94:12:94:19 | req.body |
72-
| ReflectedXss.js:95:12:95:38 | markdow ... q.body) |
73-
| ReflectedXss.js:95:12:95:38 | markdow ... q.body) |
74-
| ReflectedXss.js:95:30:95:37 | req.body |
75-
| ReflectedXss.js:95:30:95:37 | req.body |
69+
| ReflectedXss.js:97:12:97:19 | req.body |
70+
| ReflectedXss.js:97:12:97:19 | req.body |
71+
| ReflectedXss.js:97:12:97:19 | req.body |
72+
| ReflectedXss.js:98:12:98:38 | markdow ... q.body) |
73+
| ReflectedXss.js:98:12:98:38 | markdow ... q.body) |
74+
| ReflectedXss.js:98:30:98:37 | req.body |
75+
| ReflectedXss.js:98:30:98:37 | req.body |
76+
| ReflectedXss.js:100:12:100:39 | markdow ... q.body) |
77+
| ReflectedXss.js:100:12:100:39 | markdow ... q.body) |
78+
| ReflectedXss.js:100:31:100:38 | req.body |
79+
| ReflectedXss.js:100:31:100:38 | req.body |
80+
| ReflectedXss.js:103:12:103:84 | markdow ... q.body) |
81+
| ReflectedXss.js:103:12:103:84 | markdow ... q.body) |
82+
| ReflectedXss.js:103:76:103:83 | req.body |
83+
| ReflectedXss.js:103:76:103:83 | req.body |
7684
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
7785
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
7886
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id |
@@ -219,11 +227,19 @@ edges
219227
| ReflectedXss.js:85:23:85:30 | req.body | ReflectedXss.js:85:12:85:31 | snarkdown2(req.body) |
220228
| ReflectedXss.js:85:23:85:30 | req.body | ReflectedXss.js:85:12:85:31 | snarkdown2(req.body) |
221229
| ReflectedXss.js:85:23:85:30 | req.body | ReflectedXss.js:85:12:85:31 | snarkdown2(req.body) |
222-
| ReflectedXss.js:94:12:94:19 | req.body | ReflectedXss.js:94:12:94:19 | req.body |
223-
| ReflectedXss.js:95:30:95:37 | req.body | ReflectedXss.js:95:12:95:38 | markdow ... q.body) |
224-
| ReflectedXss.js:95:30:95:37 | req.body | ReflectedXss.js:95:12:95:38 | markdow ... q.body) |
225-
| ReflectedXss.js:95:30:95:37 | req.body | ReflectedXss.js:95:12:95:38 | markdow ... q.body) |
226-
| ReflectedXss.js:95:30:95:37 | req.body | ReflectedXss.js:95:12:95:38 | markdow ... q.body) |
230+
| ReflectedXss.js:97:12:97:19 | req.body | ReflectedXss.js:97:12:97:19 | req.body |
231+
| ReflectedXss.js:98:30:98:37 | req.body | ReflectedXss.js:98:12:98:38 | markdow ... q.body) |
232+
| ReflectedXss.js:98:30:98:37 | req.body | ReflectedXss.js:98:12:98:38 | markdow ... q.body) |
233+
| ReflectedXss.js:98:30:98:37 | req.body | ReflectedXss.js:98:12:98:38 | markdow ... q.body) |
234+
| ReflectedXss.js:98:30:98:37 | req.body | ReflectedXss.js:98:12:98:38 | markdow ... q.body) |
235+
| ReflectedXss.js:100:31:100:38 | req.body | ReflectedXss.js:100:12:100:39 | markdow ... q.body) |
236+
| ReflectedXss.js:100:31:100:38 | req.body | ReflectedXss.js:100:12:100:39 | markdow ... q.body) |
237+
| ReflectedXss.js:100:31:100:38 | req.body | ReflectedXss.js:100:12:100:39 | markdow ... q.body) |
238+
| ReflectedXss.js:100:31:100:38 | req.body | ReflectedXss.js:100:12:100:39 | markdow ... q.body) |
239+
| ReflectedXss.js:103:76:103:83 | req.body | ReflectedXss.js:103:12:103:84 | markdow ... q.body) |
240+
| ReflectedXss.js:103:76:103:83 | req.body | ReflectedXss.js:103:12:103:84 | markdow ... q.body) |
241+
| ReflectedXss.js:103:76:103:83 | req.body | ReflectedXss.js:103:12:103:84 | markdow ... q.body) |
242+
| ReflectedXss.js:103:76:103:83 | req.body | ReflectedXss.js:103:12:103:84 | markdow ... q.body) |
227243
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
228244
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
229245
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
@@ -319,8 +335,10 @@ edges
319335
| ReflectedXss.js:83:12:83:19 | req.body | ReflectedXss.js:83:12:83:19 | req.body | ReflectedXss.js:83:12:83:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:83:12:83:19 | req.body | user-provided value |
320336
| ReflectedXss.js:84:12:84:30 | snarkdown(req.body) | ReflectedXss.js:84:22:84:29 | req.body | ReflectedXss.js:84:12:84:30 | snarkdown(req.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:84:22:84:29 | req.body | user-provided value |
321337
| ReflectedXss.js:85:12:85:31 | snarkdown2(req.body) | ReflectedXss.js:85:23:85:30 | req.body | ReflectedXss.js:85:12:85:31 | snarkdown2(req.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:85:23:85:30 | req.body | user-provided value |
322-
| ReflectedXss.js:94:12:94:19 | req.body | ReflectedXss.js:94:12:94:19 | req.body | ReflectedXss.js:94:12:94:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:94:12:94:19 | req.body | user-provided value |
323-
| ReflectedXss.js:95:12:95:38 | markdow ... q.body) | ReflectedXss.js:95:30:95:37 | req.body | ReflectedXss.js:95:12:95:38 | markdow ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:95:30:95:37 | req.body | user-provided value |
338+
| ReflectedXss.js:97:12:97:19 | req.body | ReflectedXss.js:97:12:97:19 | req.body | ReflectedXss.js:97:12:97:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:97:12:97:19 | req.body | user-provided value |
339+
| ReflectedXss.js:98:12:98:38 | markdow ... q.body) | ReflectedXss.js:98:30:98:37 | req.body | ReflectedXss.js:98:12:98:38 | markdow ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:98:30:98:37 | req.body | user-provided value |
340+
| ReflectedXss.js:100:12:100:39 | markdow ... q.body) | ReflectedXss.js:100:31:100:38 | req.body | ReflectedXss.js:100:12:100:39 | markdow ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:100:31:100:38 | req.body | user-provided value |
341+
| ReflectedXss.js:103:12:103:84 | markdow ... q.body) | ReflectedXss.js:103:76:103:83 | req.body | ReflectedXss.js:103:12:103:84 | markdow ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:103:76:103:83 | req.body | user-provided value |
324342
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | user-provided value |
325343
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
326344
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,15 @@ const markdownIt = require('markdown-it')({
9090
});
9191
const markdownIt2 = require('markdown-it')({});
9292

93+
const markdownIt3 = require('markdown-it')({html: true})
94+
.use(require('markdown-it-highlightjs'));
95+
9396
app.get('/user/:id', function (req, res) {
9497
res.send(req.body); // NOT OK
9598
res.send(markdownIt.render(req.body)); // NOT OK
9699
res.send(markdownIt2.render(req.body)); // OK - no html
100+
res.send(markdownIt3.render(req.body)); // NOT OK
101+
102+
res.send(markdownIt.use(require('markdown-it-sanitizer')).render(req.body)); // OK - HTML is sanitized.
103+
res.send(markdownIt.use(require('markdown-it-abbr')).use(unknown).render(req.body)); // NOT OK
97104
});

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,10 @@
1414
| ReflectedXss.js:83:12:83:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:83:12:83:19 | req.body | user-provided value |
1515
| ReflectedXss.js:84:12:84:30 | snarkdown(req.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:84:22:84:29 | req.body | user-provided value |
1616
| ReflectedXss.js:85:12:85:31 | snarkdown2(req.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:85:23:85:30 | req.body | user-provided value |
17-
| ReflectedXss.js:94:12:94:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:94:12:94:19 | req.body | user-provided value |
18-
| ReflectedXss.js:95:12:95:38 | markdow ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:95:30:95:37 | req.body | user-provided value |
17+
| ReflectedXss.js:97:12:97:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:97:12:97:19 | req.body | user-provided value |
18+
| ReflectedXss.js:98:12:98:38 | markdow ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:98:30:98:37 | req.body | user-provided value |
19+
| ReflectedXss.js:100:12:100:39 | markdow ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:100:31:100:38 | req.body | user-provided value |
20+
| ReflectedXss.js:103:12:103:84 | markdow ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:103:76:103:83 | req.body | user-provided value |
1921
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | user-provided value |
2022
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
2123
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |

0 commit comments

Comments
 (0)