Skip to content

Commit 635fb4c

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#5685 from erik-krogh/markdownIt
Approved by asgerf
2 parents 3e4ff9e + 357e1c0 commit 635fb4c

File tree

5 files changed

+98
-0
lines changed

5 files changed

+98
-0
lines changed

‎javascript/change-notes/2021-04-15-markdownit.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
lgtm,codescanning
2+
* The security queries now track taint through markdown-it.
3+
Affected packages are
4+
[markdown-it](https://npmjs.com/package/markdown-it)

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,3 +118,43 @@ private class SnarkdownStep extends TaintTracking::SharedTaintStep {
118118
)
119119
}
120120
}
121+
122+
/**
123+
* Classes and predicates for modelling taint steps the `markdown-it` library.
124+
*/
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()
134+
|
135+
call.getParameter(0).getMember("html").getARhs().mayHaveBooleanValue(true) and
136+
result = call.getReturn()
137+
)
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).getAValueReachingRhs() =
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+
}
159+
}
160+
}

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +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: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 |
6984
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
7085
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
7186
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id |
@@ -212,6 +227,19 @@ edges
212227
| ReflectedXss.js:85:23:85:30 | req.body | ReflectedXss.js:85:12:85:31 | snarkdown2(req.body) |
213228
| ReflectedXss.js:85:23:85:30 | req.body | ReflectedXss.js:85:12:85:31 | snarkdown2(req.body) |
214229
| ReflectedXss.js:85:23:85:30 | req.body | ReflectedXss.js:85:12:85:31 | snarkdown2(req.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) |
215243
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
216244
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
217245
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
@@ -307,6 +335,10 @@ edges
307335
| 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 |
308336
| 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 |
309337
| 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 |
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 |
310342
| 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 |
311343
| 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 |
312344
| 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: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,3 +84,21 @@ app.get('/user/:id', function (req, res) {
8484
res.send(snarkdown(req.body)); // NOT OK
8585
res.send(snarkdown2(req.body)); // NOT OK
8686
});
87+
88+
const markdownIt = require('markdown-it')({
89+
html: true
90+
});
91+
const markdownIt2 = require('markdown-it')({});
92+
93+
const markdownIt3 = require('markdown-it')({html: true})
94+
.use(require('markdown-it-highlightjs'));
95+
96+
app.get('/user/:id', function (req, res) {
97+
res.send(req.body); // NOT OK
98+
res.send(markdownIt.render(req.body)); // NOT OK
99+
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
104+
});

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +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: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 |
1721
| 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 |
1822
| 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 |
1923
| 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)