Skip to content

Commit 89f2dbf

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #195 from esben-semmle/js/reflected-xss-through-filenames
Approved by asger-semmle
2 parents 86fe0ce + bb48421 commit 89f2dbf

File tree

17 files changed

+411
-7
lines changed

17 files changed

+411
-7
lines changed

‎change-notes/1.19/analysis-javascript.md‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,15 @@
44

55
* Modelling of taint flow through array operations has been improved. This may give additional results for the security queries.
66

7+
* Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features:
8+
- file system access, for example through [fs-extra](https://github.com/jprichardson/node-fs-extra) or [globby](https://www.npmjs.com/package/globby)
9+
10+
711
## New queries
812

9-
| **Query** | **Tags** | **Purpose** |
10-
|-----------------------------|-----------|--------------------------------------------------------------------|
11-
| *@name of query (Query ID)* | *Tags* |*Aim of the new query and whether it is enabled by default or not* |
13+
| **Query** | **Tags** | **Purpose** |
14+
|-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
15+
| Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
1216

1317
## Changes to existing queries
1418

‎javascript/config/suites/javascript/security‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
+ semmlecode-javascript-queries/Security/CWE-022/TaintedPath.ql: /Security/CWE/CWE-022
33
+ semmlecode-javascript-queries/Security/CWE-078/CommandInjection.ql: /Security/CWE/CWE-078
44
+ semmlecode-javascript-queries/Security/CWE-079/ReflectedXss.ql: /Security/CWE/CWE-079
5+
+ semmlecode-javascript-queries/Security/CWE-079/StoredXss.ql: /Security/CWE/CWE-079
56
+ semmlecode-javascript-queries/Security/CWE-079/Xss.ql: /Security/CWE/CWE-079
67
+ semmlecode-javascript-queries/Security/CWE-089/SqlInjection.ql: /Security/CWE/CWE-089
78
+ semmlecode-javascript-queries/Security/CWE-094/CodeInjection.ql: /Security/CWE/CWE-094

‎javascript/ql/src/Security/CWE-079/StoredXss.qhelp‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
9+
Directly using uncontrolled stored value (for example, file names) to
10+
create HTML content without properly sanitizing the input first,
11+
allows for a cross-site scripting vulnerability.
12+
13+
</p>
14+
<p>
15+
16+
This kind of vulnerability is also called <i>stored</i> cross-site
17+
scripting, to distinguish it from other types of cross-site scripting.
18+
19+
</p>
20+
</overview>
21+
22+
<recommendation>
23+
<p>
24+
25+
To guard against cross-site scripting, consider using contextual
26+
output encoding/escaping before using uncontrolled stored values to
27+
create HTML content, or one of the other solutions that are mentioned
28+
in the references.
29+
30+
</p>
31+
</recommendation>
32+
33+
<example>
34+
<p>
35+
36+
The following example code writes file names directly to a HTTP
37+
response. This leaves the website vulnerable to cross-site scripting,
38+
if an attacker can choose the file names on the disk.
39+
40+
</p>
41+
<sample src="examples/StoredXss.js" />
42+
<p>
43+
Sanitizing the file names prevents the vulnerability:
44+
</p>
45+
<sample src="examples/StoredXssGood.js" />
46+
</example>
47+
48+
<references>
49+
<li>
50+
OWASP:
51+
<a href="https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet">XSS
52+
(Cross Site Scripting) Prevention Cheat Sheet</a>.
53+
</li>
54+
<li>
55+
OWASP
56+
<a href="https://www.owasp.org/index.php/Types_of_Cross-Site_Scripting">Types of Cross-Site
57+
Scripting</a>.
58+
</li>
59+
<li>
60+
Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
61+
</li>
62+
</references>
63+
</qhelp>

‎javascript/ql/src/Security/CWE-079/StoredXss.ql‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @name Stored cross-site scripting
3+
* @description Using uncontrolled stored values in HTML allows for
4+
* a stored cross-site scripting vulnerability.
5+
* @kind problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id js/stored-xss
9+
* @tags security
10+
* external/cwe/cwe-079
11+
* external/cwe/cwe-116
12+
*/
13+
14+
import javascript
15+
import semmle.javascript.security.dataflow.StoredXss::StoredXss
16+
17+
from Configuration xss, DataFlow::Node source, DataFlow::Node sink
18+
where xss.hasFlow(source, sink)
19+
select sink, "Stored cross-site scripting vulnerability due to $@.",
20+
source, "stored value"

‎javascript/ql/src/Security/CWE-079/examples/StoredXss.js‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
var express = require('express'),
2+
fs = require('fs');
3+
4+
express().get('/list-directory', function(req, res) {
5+
fs.readdir('/public', function (error, fileNames) {
6+
var list = '<ul>';
7+
fileNames.forEach(fileName => {
8+
// BAD: `fileName` can contain HTML elements
9+
list += '<li>' + fileName '</li>';
10+
});
11+
list += '</ul>'
12+
res.send(list);
13+
});
14+
});

‎javascript/ql/src/Security/CWE-079/examples/StoredXssGood.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
var express = require('express'),
2+
fs = require('fs'),
3+
escape = require('escape-html');
4+
5+
express().get('/list-directory', function(req, res) {
6+
fs.readdir('/public', function (error, fileNames) {
7+
var list = '<ul>';
8+
fileNames.forEach(fileName => {
9+
// GOOD: escaped `fileName` can not contain HTML elements
10+
list += '<li>' + escape(fileName) '</li>';
11+
});
12+
list += '</ul>'
13+
res.send(list);
14+
});
15+
});

‎javascript/ql/src/javascript.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ import semmle.javascript.frameworks.Credentials
5959
import semmle.javascript.frameworks.CryptoLibraries
6060
import semmle.javascript.frameworks.DigitalOcean
6161
import semmle.javascript.frameworks.Electron
62+
import semmle.javascript.frameworks.Files
6263
import semmle.javascript.frameworks.jQuery
6364
import semmle.javascript.frameworks.LodashUnderscore
6465
import semmle.javascript.frameworks.Logging

‎javascript/ql/src/semmle/javascript/Concepts.qll‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,13 @@ abstract class FileSystemAccess extends DataFlow::Node {
2929
abstract DataFlow::Node getAPathArgument();
3030
}
3131

32+
/**
33+
* A data flow node that contains a file name or an array of file names from the local file system.
34+
*/
35+
abstract class FileNameSource extends DataFlow::Node {
36+
37+
}
38+
3239
/**
3340
* A data flow node that performs a database access.
3441
*/

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

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
/**
2+
* Provides classes for working with file system libraries.
3+
*/
4+
5+
import javascript
6+
7+
/**
8+
* A file name from the `walk-sync` library.
9+
*/
10+
private class WalkSyncFileNameSource extends FileNameSource {
11+
12+
WalkSyncFileNameSource() {
13+
// `require('walkSync')()`
14+
this = DataFlow::moduleImport("walkSync").getACall()
15+
}
16+
17+
}
18+
19+
/**
20+
* A file name or an array of file names from the `walk` library.
21+
*/
22+
private class WalkFileNameSource extends FileNameSource {
23+
24+
WalkFileNameSource() {
25+
// `stats.name` in `require('walk').walk(_).on(_, (_, stats) => stats.name)`
26+
exists (DataFlow::FunctionNode callback |
27+
callback = DataFlow::moduleMember("walk", "walk").getACall().getAMethodCall("on").getCallback(1) |
28+
this = callback.getParameter(1).getAPropertyRead("name")
29+
)
30+
}
31+
32+
}
33+
34+
/**
35+
* A file name or an array of file names from the `glob` library.
36+
*/
37+
private class GlobFileNameSource extends FileNameSource {
38+
39+
GlobFileNameSource() {
40+
exists (string moduleName |
41+
moduleName = "glob" |
42+
// `require('glob').sync(_)`
43+
this = DataFlow::moduleMember(moduleName, "sync").getACall()
44+
or
45+
// `name` in `require('glob')(_, (e, name) => ...)`
46+
this = DataFlow::moduleImport(moduleName).getACall().getCallback([1..2]).getParameter(1)
47+
or
48+
exists (DataFlow::NewNode instance |
49+
instance = DataFlow::moduleMember(moduleName, "Glob").getAnInstantiation() |
50+
// `name` in `new require('glob').Glob(_, (e, name) => ...)`
51+
this = instance.getCallback([1..2]).getParameter(1) or
52+
// `new require('glob').Glob(_).found`
53+
this = instance.getAPropertyRead("found")
54+
)
55+
)
56+
}
57+
58+
}
59+
60+
/**
61+
* A file name or an array of file names from the `globby` library.
62+
*/
63+
private class GlobbyFileNameSource extends FileNameSource {
64+
65+
GlobbyFileNameSource() {
66+
exists (string moduleName |
67+
moduleName = "globby" |
68+
// `require('globby').sync(_)`
69+
this = DataFlow::moduleMember(moduleName, "sync").getACall()
70+
or
71+
// `files` in `require('globby')(_).then(files => ...)`
72+
this = DataFlow::moduleImport(moduleName).getACall().getAMethodCall("then").getCallback(0).getParameter(0)
73+
)
74+
}
75+
76+
}
77+
78+
/**
79+
* A file name or an array of file names from the `fast-glob` library.
80+
*/
81+
private class FastGlobFileNameSource extends FileNameSource {
82+
83+
FastGlobFileNameSource() {
84+
exists (string moduleName |
85+
moduleName = "fast-glob" |
86+
// `require('fast-glob').sync(_)`
87+
this = DataFlow::moduleMember(moduleName, "sync").getACall()
88+
or
89+
exists (DataFlow::SourceNode f |
90+
f = DataFlow::moduleImport(moduleName)
91+
or
92+
f = DataFlow::moduleMember(moduleName, "async") |
93+
// `files` in `require('fast-glob')(_).then(files => ...)` and
94+
// `files` in `require('fast-glob').async(_).then(files => ...)`
95+
this = f.getACall().getAMethodCall("then").getCallback(0).getParameter(0)
96+
)
97+
or
98+
// `file` in `require('fast-glob').stream(_).on(_, file => ...)`
99+
this = DataFlow::moduleMember(moduleName, "stream").getACall().getAMethodCall("on").getCallback(1).getParameter(0)
100+
)
101+
}
102+
103+
}

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

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -336,17 +336,26 @@ module NodeJSLib {
336336
)
337337
}
338338

339+
/**
340+
* A member `member` from module `fs` or its drop-in replacements `graceful-fs` or `fs-extra`.
341+
*/
342+
private DataFlow::SourceNode fsModuleMember(string member) {
343+
exists (string moduleName |
344+
moduleName = "fs" or
345+
moduleName = "graceful-fs" or
346+
moduleName = "fs-extra" |
347+
result = DataFlow::moduleMember(moduleName, member)
348+
)
349+
}
339350

340351
/**
341-
* A call to a method from module `fs` or `graceful-fs`.
352+
* A call to a method from module `fs`, `graceful-fs` or `fs-extra`.
342353
*/
343354
private class NodeJSFileSystemAccess extends FileSystemAccess, DataFlow::CallNode {
344355
string methodName;
345356

346357
NodeJSFileSystemAccess() {
347-
exists (string moduleName | this = DataFlow::moduleMember(moduleName, methodName).getACall() |
348-
moduleName = "fs" or moduleName = "graceful-fs"
349-
)
358+
this = fsModuleMember(methodName).getACall()
350359
}
351360

352361
override DataFlow::Node getAPathArgument() {
@@ -356,6 +365,22 @@ module NodeJSLib {
356365
}
357366
}
358367

368+
/**
369+
* A data flow node that contains a file name or an array of file names from the local file system.
370+
*/
371+
private class NodeJSFileNameSource extends FileNameSource {
372+
373+
NodeJSFileNameSource() {
374+
exists (string name |
375+
name = "readdir" or
376+
name = "realpath" |
377+
this = fsModuleMember(name).getACall().getCallback([1..2]).getParameter(1) or
378+
this = fsModuleMember(name + "Sync").getACall()
379+
)
380+
}
381+
382+
}
383+
359384
/**
360385
* A call to a method from module `child_process`.
361386
*/

0 commit comments

Comments
 (0)