diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md new file mode 100644 index 000000000000..7f9f2d9679c7 --- /dev/null +++ b/change-notes/1.19/analysis-javascript.md @@ -0,0 +1,22 @@ +# Improvements to JavaScript analysis + +## General improvements + +* Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features: + - file system access, for example through [fs-extra](https://github.com/jprichardson/node-fs-extra) + +## New queries + +| **Query** | **Tags** | **Purpose** | +|-----------------------------|-----------|--------------------------------------------------------------------| +| *@name of query (Query ID)* | *Tags* |*Aim of the new query and whether it is enabled by default or not* | + +## Changes to existing queries + +| **Query** | **Expected impact** | **Change** | +|--------------------------------|----------------------------|----------------------------------------------| +| Reflected cross-site scripting | More true-positive results | This rule now treats file names as a source. | + + +## Changes to QL libraries + diff --git a/javascript/ql/src/semmle/javascript/Concepts.qll b/javascript/ql/src/semmle/javascript/Concepts.qll index 1ab9f8cba2ed..ce470f882b74 100644 --- a/javascript/ql/src/semmle/javascript/Concepts.qll +++ b/javascript/ql/src/semmle/javascript/Concepts.qll @@ -29,6 +29,13 @@ abstract class FileSystemAccess extends DataFlow::Node { abstract DataFlow::Node getAPathArgument(); } +/** + * A data flow node that contains a file name or an array of file names from the local file system. + */ +abstract class FileNameSource extends DataFlow::Node { + +} + /** * A data flow node that performs a database access. */ diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll index 6a11bef5edc5..ca74091aeb1f 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll @@ -336,17 +336,26 @@ module NodeJSLib { ) } + /** + * A member `member` from module `fs` or its drop-in replacements `graceful-fs` or `fs-extra`. + */ + private DataFlow::SourceNode fsModuleMember(string member) { + exists (string moduleName | + moduleName = "fs" or + moduleName = "graceful-fs" or + moduleName = "fs-extra" | + result = DataFlow::moduleMember(moduleName, member) + ) + } /** - * A call to a method from module `fs` or `graceful-fs`. + * A call to a method from module `fs`, `graceful-fs` or `fs-extra`. */ private class NodeJSFileSystemAccess extends FileSystemAccess, DataFlow::CallNode { string methodName; NodeJSFileSystemAccess() { - exists (string moduleName | this = DataFlow::moduleMember(moduleName, methodName).getACall() | - moduleName = "fs" or moduleName = "graceful-fs" - ) + this = fsModuleMember(methodName).getACall() } override DataFlow::Node getAPathArgument() { @@ -356,6 +365,22 @@ module NodeJSLib { } } + /** + * A data flow node that contains one or more file names from the local file system. + */ + private class NodeJSFileNameSource extends FileNameSource { + + NodeJSFileNameSource() { + exists (string name | + name = "readdir" or + name = "realpath" | + this = fsModuleMember(name).getACall().getCallback([1..2]).getParameter(1) or + this = fsModuleMember(name + "Sync").getACall() + ) + } + + } + /** * A call to a method from module `child_process`. */ diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXss.qll index 7a4a61a00c08..1910d3975ab4 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ReflectedXss.qll @@ -53,6 +53,13 @@ module ReflectedXss { } } + /** A file name, considered as a flow source for reflected XSS. */ + class FileNameSourceAsSource extends Source { + FileNameSourceAsSource() { + this instanceof FileNameSource + } + } + /** * An expression that is sent as part of an HTTP response, considered as an XSS sink. * diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected index d2e638a855f7..b50f6ba9caa4 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected @@ -5,3 +5,7 @@ | promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value | | tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value | | tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value | +| xss-through-filenames.js:8:18:8:23 | files1 | Cross-site scripting vulnerability due to $@. | xss-through-filenames.js:7:43:7:48 | files1 | user-provided value | +| xss-through-filenames.js:26:19:26:24 | files1 | Cross-site scripting vulnerability due to $@. | xss-through-filenames.js:25:43:25:48 | files1 | user-provided value | +| xss-through-filenames.js:33:19:33:24 | files2 | Cross-site scripting vulnerability due to $@. | xss-through-filenames.js:25:43:25:48 | files1 | user-provided value | +| xss-through-filenames.js:37:19:37:24 | files3 | Cross-site scripting vulnerability due to $@. | xss-through-filenames.js:25:43:25:48 | files1 | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/xss-through-filenames.js b/javascript/ql/test/query-tests/Security/CWE-079/xss-through-filenames.js new file mode 100644 index 000000000000..c04e0d784efc --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/xss-through-filenames.js @@ -0,0 +1,40 @@ +var http = require('http'); +var fs = require('fs'); + +var express = require('express'); + +express().get('/', function(req, res) { + fs.readdir("/myDir", function (error, files1) { + res.send(files1); // NOT OK + }); +}); + +/** + * The essence of a real world vulnerability. + */ +http.createServer(function (req, res) { + + function format(files2) { + var files3 = []; + files2.sort(sort).forEach(function (file) { + files3.push('
  • ' + file + '
  • '); + }); + return files3.join(''); + } + + fs.readdir("/myDir", function (error, files1) { + res.write(files1); // NOT OK + + var dirs = []; + var files2 = []; + files1.forEach(function (file) { + files2.push(file); + }); + res.write(files2); // NOT OK + + var files3 = format(files2); + + res.write(files3); // NOT OK + + }); +});