From 73a7d406a543cfaa63b21d3b80d1cafba133f80a Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 18 Feb 2020 18:54:27 +0100 Subject: [PATCH 01/37] add query for useless use of cat --- .../ql/src/Security/CWE-078/UselessCat.ql | 20 +++++ .../javascript/security/UselessUseOfCat.qll | 59 +++++++++++++++ .../CWE-078/CommandInjection.expected | 9 +++ .../Security/CWE-078/UselessCat.expected | 2 + .../Security/CWE-078/UselessCat.ql | 18 +++++ .../Security/CWE-078/uselesscat.js | 73 +++++++++++++++++++ 6 files changed, 181 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-078/UselessCat.ql create mode 100644 javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll create mode 100644 javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected create mode 100644 javascript/ql/test/query-tests/Security/CWE-078/UselessCat.ql create mode 100644 javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js diff --git a/javascript/ql/src/Security/CWE-078/UselessCat.ql b/javascript/ql/src/Security/CWE-078/UselessCat.ql new file mode 100644 index 000000000000..d2f0e13fe2e1 --- /dev/null +++ b/javascript/ql/src/Security/CWE-078/UselessCat.ql @@ -0,0 +1,20 @@ +/** + * @name Useless use of cat + * @description Using cat to simply read a file can lead to unintended bugs, and at worst security issues. + * @kind problem + * @problem.severity error + * @precision high + * @id js/useless-use-of-cat + * @tags correctness + * security + * external/cwe/cwe-078 + * external/cwe/cwe-088 + */ + +import javascript +import semmle.javascript.security.UselessUseOfCat + +from UselessCat cat +select cat.getCommand(), "Useless use of `cat` in $@.", cat, "command execution" + + diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll new file mode 100644 index 000000000000..caf6c2a666ad --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -0,0 +1,59 @@ +/** + * Provides predicates and classes for working with useless uses of `cat`. + */ + +import javascript +import semmle.javascript.security.dataflow.IndirectCommandArgument + +/** + * Gets the first string from a string/string-concatenation. + */ +private string getStartingString(DataFlow::Node node) { + node.mayHaveStringValue(result) or + node.(StringOps::ConcatenationRoot).getFirstLeaf().mayHaveStringValue(result) +} + +/** + * Gets a string from a string/string-concatenation. + */ +private string getAString(DataFlow::Node node) { + node.mayHaveStringValue(result) or + node.(StringOps::ConcatenationRoot).getALeaf().mayHaveStringValue(result) +} + +/** + * An command-line execution of `cat` that only reads a file. + */ +class UselessCat extends DataFlow::Node { + DataFlow::Node command; + + UselessCat() { + command = this.(SystemCommandExecution).getACommandArgument() and + exists(string cat | + cat = "cat" or cat = "/bin/cat" or cat = "sudo cat" or cat = "sudo /bin/cat" + | + exists(string commandString | + commandString = getStartingString(command).trim() and + (commandString = cat or commandString.regexpMatch(cat + " .*")) + ) and + // `cat` is OK in combination with pipes and wildcards. + not getAString(command).regexpMatch(".*(\\*|\\|).*") and + // It is OK just to spawn "cat" without any arguments. + not ( + command.mayHaveStringValue(cat) and + not exists( + this + .(SystemCommandExecution) + .getArgumentList() + .(DataFlow::ArrayCreationNode) + .getAnElement() + ) + ) + ) + } + + /** + * Gets the dataflow node determining the command executed. + */ + DataFlow::Node getCommand() { result = command } +} diff --git a/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected b/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected index 589deda96c96..3211c1196453 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected @@ -89,6 +89,10 @@ nodes | third-party-command-injection.js:5:20:5:26 | command | | third-party-command-injection.js:6:21:6:27 | command | | third-party-command-injection.js:6:21:6:27 | command | +| uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | +| uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | +| uselesscat.js:18:40:18:54 | req.params.data | +| uselesscat.js:18:40:18:54 | req.params.data | edges | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd | | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd | @@ -178,6 +182,10 @@ edges | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | +| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | +| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | +| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | +| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | #select | child_process-test.js:17:13:17:15 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:17:13:17:15 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value | | child_process-test.js:18:17:18:19 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:18:17:18:19 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value | @@ -211,3 +219,4 @@ edges | other.js:18:22:18:24 | cmd | other.js:5:25:5:31 | req.url | other.js:18:22:18:24 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value | | other.js:19:36:19:38 | cmd | other.js:5:25:5:31 | req.url | other.js:19:36:19:38 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value | | third-party-command-injection.js:6:21:6:27 | command | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | This command depends on $@. | third-party-command-injection.js:5:20:5:26 | command | a server-provided value | +| uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | This command depends on $@. | uselesscat.js:18:40:18:54 | req.params.data | a user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected new file mode 100644 index 000000000000..b19c48336703 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected @@ -0,0 +1,2 @@ +| False negative | uselesscat.js:69:42:69:69 | // NOT ... lagged] | +| False positive | uselesscat.js:18:70:18:118 | // OK [ ... jection | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.ql b/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.ql new file mode 100644 index 000000000000..bfb342f7ad15 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.ql @@ -0,0 +1,18 @@ +import javascript +import semmle.javascript.security.UselessUseOfCat + +from LineComment comment, string msg +where + comment.getFile().getAbsolutePath().regexpMatch(".*/uselesscat.js") and + ( + comment.getText().regexpMatch(".*NOT OK.*") and + not any(UselessCat cat).asExpr().getLocation().getStartLine() = + comment.getLocation().getStartLine() and + msg = "False negative" + or + comment.getText().regexpMatch(".* OK.*") and + not comment.getText().regexpMatch(".*NOT OK.*") and + any(UselessCat cat).asExpr().getLocation().getStartLine() = comment.getLocation().getStartLine() and + msg = "False positive" + ) +select msg, comment diff --git a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js new file mode 100644 index 000000000000..d613908e8d36 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js @@ -0,0 +1,73 @@ +var express = require('express'); +var child_process = require('child_process'); +var execSync = child_process.execSync; +var exec = child_process.exec; +var spawn = child_process.spawn; +var spawnSync = child_process.spawnSync; +var fs = require('fs'); +var app = express(); + +function readStatus(id) { + exec("cat /proc/" + id + "/status", function (err, out) { // NOT OK + console.log(out); + }); +}; + +var basePath = '/foo/bar'; +app.get('/:data', function (req, res) { + res.send(execSync('cat ' + basePath + req.params.data).toString()); // OK [but flagged] - caught by command-injection +}); + +spawn('cat', ['package.json']); // NOT OK + +spawn('/bin/cat', ['package.json']); // NOT + +spawn('/bin/cat', [someValue]); // NOT OK +spawn('sudo /bin/cat', [someValue]); // NOT OK +spawn('sudo cat', [someValue]); // NOT OK + +child_process.spawnSync('cat', [someValue]); // NOT OK + +child_process.execSync('cat package.json | wc -l'); // OK - pipes! + +const cat = child_process.spawn('cat') // OK - benign use (usually pipe to and from). + +spawn('cat', ['package.json']); // NOT OK. + +exec('cat *.js') // OK - wildcard use. +exec('cat *.js | wc -l') // OK - wildcard use and pipes! +exec('cat *.js | wc -l', { cwd: './' }, function () { }); // OK - wildcard and pipes + +spawnSync('cat', ['/proc/cpuinfo']) // NOT OK. + +exec(`cat ${path.join(__dirname, 'package.json')} | sort | uniq`, () => {}); // OK: pipes + +execSync('cat /proc/cpuinfo').toString(); // NOT OK. + +var cmd = "cat /proc/cpuinfo" +execSync(cmd); // NOT OK + +execSync("cat /proc/cpuinfo | grep -c '" + someValue + "'"); // OK - pipes + +function cat(file) { + return execSync('cat ' + file).toString(); // NOT OK [flagged] +} + +execSync(`cat ${files.join(' ')} > ${outFile}`); // NOT OK [flagged] + +var cmd = 'cat package.json | grep' +exec(cmd); // OK - pipes! + +execSync("sudo cat " + newpath + "*.js | grep foo").toString(); // OK - wildcard and pipes + +execSync(`cat ${newpath}`); // NOT OK + +exec("cat /proc/cpuinfo | grep name"); // OK - pipes + +execSync(`cat ${newpath} | ${othertool}`); // OK - pipes + +execSync("sh -c 'cat " + newpath + "'"); // NOT OK. [but not flagged] + +exec(` cat ${newpath}`) // NOT OK + +exec(` cat ${newpath} | grep foo`) // OK - pipes \ No newline at end of file From 344060e1395d56b6d33a13bd68c91aa6f82063c8 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 19 Feb 2020 10:12:24 +0100 Subject: [PATCH 02/37] accept IO redirections as OK --- .../javascript/security/UselessUseOfCat.qll | 4 ++-- .../Security/CWE-078/UselessCat.expected | 1 + .../query-tests/Security/CWE-078/uselesscat.js | 15 ++++++++++++--- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index caf6c2a666ad..e5c5e8a0a1ab 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -36,8 +36,8 @@ class UselessCat extends DataFlow::Node { commandString = getStartingString(command).trim() and (commandString = cat or commandString.regexpMatch(cat + " .*")) ) and - // `cat` is OK in combination with pipes and wildcards. - not getAString(command).regexpMatch(".*(\\*|\\|).*") and + // `cat` is OK in combination with pipes, wildcards, and redirections. + not getAString(command).regexpMatch(".*(\\*|\\||>|<).*") and // It is OK just to spawn "cat" without any arguments. not ( command.mayHaveStringValue(cat) and diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected index b19c48336703..67b59aa9d6ef 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected @@ -1,2 +1,3 @@ | False negative | uselesscat.js:69:42:69:69 | // NOT ... lagged] | | False positive | uselesscat.js:18:70:18:118 | // OK [ ... jection | +| False positive | uselesscat.js:82:80:82:128 | // OK ( ... / gid)) | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js index d613908e8d36..4fe7ccd75d2f 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js @@ -50,10 +50,10 @@ execSync(cmd); // NOT OK execSync("cat /proc/cpuinfo | grep -c '" + someValue + "'"); // OK - pipes function cat(file) { - return execSync('cat ' + file).toString(); // NOT OK [flagged] + return execSync('cat ' + file).toString(); // NOT OK } -execSync(`cat ${files.join(' ')} > ${outFile}`); // NOT OK [flagged] +execSync(`cat ${files.join(' ')} > ${outFile}`); // OK var cmd = 'cat package.json | grep' exec(cmd); // OK - pipes! @@ -70,4 +70,13 @@ execSync("sh -c 'cat " + newpath + "'"); // NOT OK. [but not flagged] exec(` cat ${newpath}`) // NOT OK -exec(` cat ${newpath} | grep foo`) // OK - pipes \ No newline at end of file +exec(` cat ${newpath} | grep foo`) // OK - pipes + +execSync('cat /proc/cpuinfo > foo/bar/baz').toString(); // OK. + +execSync(`cat ${newpath} > ${destpath}`).toString(); // OK. + +const Opts = {encoding: 'utf8'} +execSync(`cat foo/bar/${newpath}`, Opts).slice(0, 7); // NOT OK ("encoding" is used EXACTLY the same way in fs.readFileSync) + +execSync("/bin/cat /proc/cpuinfo", { uid: 1000, gid: 1000, encoding: 'utf8'}); // OK (fs.readFileSync cannot emulate uid / gid)) \ No newline at end of file From bdab9ee12b44c303e086fb1bf950745a93dcaf6d Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 19 Feb 2020 14:31:03 +0100 Subject: [PATCH 03/37] change useless cat query to only flag instances that can be re-written to --- .../{UselessCat.ql => UselessUseOfCat.ql} | 3 +- .../javascript/security/UselessUseOfCat.qll | 322 ++++++++++++++++-- .../Security/CWE-078/UselessCat.expected | 3 - .../Security/CWE-078/UselessUseOfCat.expected | 20 ++ .../{UselessCat.ql => UselessUseOfCat.ql} | 2 + .../Security/CWE-078/uselesscat.js | 94 ++--- 6 files changed, 366 insertions(+), 78 deletions(-) rename javascript/ql/src/Security/CWE-078/{UselessCat.ql => UselessUseOfCat.ql} (83%) delete mode 100644 javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected create mode 100644 javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected rename javascript/ql/test/query-tests/Security/CWE-078/{UselessCat.ql => UselessUseOfCat.ql} (89%) diff --git a/javascript/ql/src/Security/CWE-078/UselessCat.ql b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql similarity index 83% rename from javascript/ql/src/Security/CWE-078/UselessCat.ql rename to javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql index d2f0e13fe2e1..f48ba6b8e958 100644 --- a/javascript/ql/src/Security/CWE-078/UselessCat.ql +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql @@ -14,7 +14,8 @@ import javascript import semmle.javascript.security.UselessUseOfCat + from UselessCat cat -select cat.getCommand(), "Useless use of `cat` in $@.", cat, "command execution" +select cat, "Useless use of `cat`. Can be replaced with: " + createReadFileCall(cat) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index e5c5e8a0a1ab..f67fa2876853 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -6,54 +6,312 @@ import javascript import semmle.javascript.security.dataflow.IndirectCommandArgument /** - * Gets the first string from a string/string-concatenation. + * Gets a string representing an equivalent call to `fs.ReadFile` for a call to `cat`. */ -private string getStartingString(DataFlow::Node node) { - node.mayHaveStringValue(result) or - node.(StringOps::ConcatenationRoot).getFirstLeaf().mayHaveStringValue(result) +string createReadFileCall(UselsesCatCandidates::UselessCatCandicate cat) { + exists(string sync, string extraArg, string callback | + (if cat.isSync() then sync = "Sync" else sync = "") and + ( + if exists(cat.getOptionsArg()) + then extraArg = ", " + printOptionsArg(cat.getOptionsArg()) + ")" + else extraArg = "" + ) and + if exists(cat.getCallback()) + then callback = ", function(" + getCallbackArgs(cat.getCallback()) + ") {...}" + else callback = "" + | + result = "fs.readFile" + sync + "(" + cat.getFileArgument().trim() + extraArg + callback + ")" + ) } /** - * Gets a string from a string/string-concatenation. + * Gets a string concatenation of the parameters to a function. */ -private string getAString(DataFlow::Node node) { - node.mayHaveStringValue(result) or - node.(StringOps::ConcatenationRoot).getALeaf().mayHaveStringValue(result) +private string getCallbackArgs(DataFlow::FunctionNode func) { + result = concat(int i | i = [0 .. 2] | func.getParameter(i).getName(), ", ") } /** - * An command-line execution of `cat` that only reads a file. + * Gets a string representation of the options argument from an exec-like call. */ -class UselessCat extends DataFlow::Node { - DataFlow::Node command; +private string printOptionsArg(DataFlow::Node node) { + result = node.asExpr().(VarAccess).getVariable().getName() + or + // fall back to toString(), but ensure that we don't have dots in the middle. + result = node.(DataFlow::ObjectLiteralNode).toString() and not result.regexpMatch(".*\\.\\..*") +} + +/** + * A call to a useless use of `cat`. + */ +class UselessCat extends DataFlow::CallNode { + UselsesCatCandidates::UselessCatCandicate candidate; UselessCat() { - command = this.(SystemCommandExecution).getACommandArgument() and - exists(string cat | - cat = "cat" or cat = "/bin/cat" or cat = "sudo cat" or cat = "sudo /bin/cat" - | - exists(string commandString | - commandString = getStartingString(command).trim() and - (commandString = cat or commandString.regexpMatch(cat + " .*")) - ) and - // `cat` is OK in combination with pipes, wildcards, and redirections. - not getAString(command).regexpMatch(".*(\\*|\\||>|<).*") and - // It is OK just to spawn "cat" without any arguments. - not ( - command.mayHaveStringValue(cat) and - not exists( - this - .(SystemCommandExecution) - .getArgumentList() - .(DataFlow::ArrayCreationNode) - .getAnElement() + this = candidate and + // wildcards, pipes, redirections, and multiple files are OK. + // (The multiple files detection relies on the fileArgument not containing spaces anywhere) + not candidate.getFileArgument().regexpMatch(".*(\\*|\\||>|<| ).*") and + // Only acceptable option is "encoding", everything else is non-trivial to emulate with fs.readFile. + not exists(string prop | + not prop = "encoding" and + exists(candidate.getOptionsArg().getALocalSource().getAPropertyWrite(prop)) + ) and + exists(createReadFileCall(this)) and + // If there is a callback, then it must either have one or two arguments, or if there is a third argument it must be unused. + ( + not exists(candidate.getCallback()) + or + exists(DataFlow::FunctionNode func | func = candidate.getCallback() | + func.getNumParameter() = 1 + or + func.getNumParameter() = 2 + or + // `exec` can use 3 parameters, `readFile` can only use two, so it is OK to have a third parameter if it is unused, + func.getNumParameter() = 3 and + not exists(DataFlow::Node node | + not node = func.getParameter(2) and func.getParameter(2) = node.getALocalSource() ) ) ) } +} + +module UselsesCatCandidates { + /** + * A candidate for a useless use of `cat`. + * Subclasses of this class specify the structure of the `exec`-like call. + */ + abstract class UselessCatCandicate extends DataFlow::CallNode { + /** + * Holds if the call is synchronous (e.g. `execFileSync`). + */ + abstract predicate isSync(); + + /** + * Gets a string representation of the expression that determines what file is read. + */ + abstract string getFileArgument(); + + /** + * Gets the data-flow node for the options argument to the `exec`-like call. + */ + abstract DataFlow::Node getOptionsArg(); + + /** + * Gets the callback used for the `exec` like call (if it exists). + */ + abstract DataFlow::FunctionNode getCallback(); + } + + /** + * Create a string representation of a string concatenation. + */ + private string createConcatRepresentation(StringOps::ConcatenationRoot root) { + // String concat + not exists(root.getStringValue()) and + not root.asExpr() instanceof TemplateLiteral and + forall(Expr e | e = root.getALeaf().asExpr() | exists(createLeafRepresentation(e))) and + result = + concat(Expr leaf | + leaf = root.getALeaf().asExpr() + | + createLeafRepresentation(leaf), "+" order by leaf.getFirstToken().getIndex() + ) + or + // Template string + exists(TemplateLiteral template | template = root.asExpr() | + forall(Expr e | e = template.getAChild() | exists(createTemplateElementRepresentation(e))) and + result = + "`" + + concat(int i | + i = [0 .. template.getNumChild() - 1] + | + createTemplateElementRepresentation(template.getChild(i)) order by i + ) + "`" + ) + } /** - * Gets the dataflow node determining the command executed. + * Gets a string representing the expression needed to re-create the value for a leaf in a string-concatenation. */ - DataFlow::Node getCommand() { result = command } + private string createLeafRepresentation(Expr e) { + result = "\"" + e.getStringValue() + "\"" or + result = e.(VarAccess).getVariable().getName() + } + + /** + * Gets a string representing the expression needed to re-create the value for an element of a template string. + */ + private string createTemplateElementRepresentation(Expr e) { + result = "${" + e.(VarAccess).getVariable().getName() + "}" + or + result = e.(TemplateElement).getValue() + } + + /** + * Gets a string used to call `cat`. + */ + private string cat() { + result = "cat" or result = "/bin/cat" or result = "sudo cat" or result = "sudo /bin/cat" + } + + /** + * Gets a string representing an expression that gets the file read by a call to `cat`. + * The input `arg` is the node that determines the commandline where `cat` is invoked. + */ + private string getFileArgumentWithoutCat(DataFlow::Node arg) { + exists(string cat | cat = cat() | + exists(string command | arg.mayHaveStringValue(command) | + command.prefix(cat.length()) = cat and + result = "\"" + command.suffix(cat.length()).trim() + "\"" + ) + or + exists(StringOps::ConcatenationRoot root, string printed, string quote | + root = arg and printed = createConcatRepresentation(root).suffix(1) // remove initial quote + | + (if root.asExpr() instanceof TemplateLiteral then quote = "`" else quote = "\"") and + root.getFirstLeaf().getStringValue().prefix(cat.length()) = cat and + // Remove an initial ""+ (e.g. in `""+file`) + exists(string rawConcat | rawConcat = quote + printed.suffix(cat.length()).trim() | + if rawConcat.prefix(3) = "\"\"+" then result = rawConcat.suffix(3) else result = rawConcat + ) + ) + ) + } + + /** + * A call to child_process.exec that might be a useless call to cat. + */ + private class ExecCall extends UselessCatCandicate { + string fileArgument; + boolean hasOptions; + + ExecCall() { + this = DataFlow::moduleImport("child_process").getAMemberCall("exec") and + ( + this.getNumArgument() = 2 and hasOptions = false + or + this.getNumArgument() = 3 and hasOptions = true + ) and + fileArgument = getFileArgumentWithoutCat(getArgument(0)) + } + + override predicate isSync() { none() } + + override string getFileArgument() { result = fileArgument } + + override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(1) } + + override DataFlow::FunctionNode getCallback() { result = getLastArgument() } + } + + /** + * A call to child_process.execSync that might be a useless call to cat. + */ + private class ExecSyncCall extends UselessCatCandicate { + string fileArgument; + boolean hasOptions; + + ExecSyncCall() { + this = DataFlow::moduleImport("child_process").getAMemberCall("execSync") and + ( + this.getNumArgument() = 1 and hasOptions = false + or + this.getNumArgument() = 2 and hasOptions = true + ) and + fileArgument = getFileArgumentWithoutCat(getArgument(0)) + } + + override predicate isSync() { any() } + + override string getFileArgument() { result = fileArgument } + + override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(1) } + + override DataFlow::FunctionNode getCallback() { none() } + } + + // TODO: No! + bindingset[str, quote] + string surroundInQuotes(string str, string quote) { + if not str.prefix(1) = quote and not str.suffix(str.length() - 1) = quote + then result = quote + str + quote + else + if not str.prefix(1) = quote + then result = str + quote + else + if not str.suffix(str.length() - 1) = quote + then result = quote + str + else result = str + } + + /** + * Gets the file that is read for a call to child_process.execFile/execFileSync. + */ + string getFileThatIsRead(DataFlow::CallNode call) { + exists(DataFlow::ArrayCreationNode array, DataFlow::Node element | + array = call.getArgument(1).(DataFlow::ArrayCreationNode) and + array.getSize() = 1 and + element = array.getElement(0) + | + result = element.asExpr().(VarAccess).getVariable().getName() or + result = "\"" + element.getStringValue() + "\"" or + result = createConcatRepresentation(element) + ) + } + + /** + * A call to child_process.execFile that might be a useless call to cat. + */ + private class ExecFileCall extends UselessCatCandicate { + string fileArgument; + boolean hasOptions; + + ExecFileCall() { + this.getArgument(0).mayHaveStringValue(cat()) and + this = DataFlow::moduleImport("child_process").getAMemberCall("execFile") and + ( + this.getNumArgument() = 3 and hasOptions = false + or + this.getNumArgument() = 4 and hasOptions = true + ) and + fileArgument = getFileThatIsRead(this) + } + + override predicate isSync() { none() } + + override string getFileArgument() { result = fileArgument } + + override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(2) } + + override DataFlow::FunctionNode getCallback() { result = getLastArgument() } + } + + /** + * A call to child_process.execFileSync that might be a useless call to cat. + */ + private class ExecFileSyncCall extends UselessCatCandicate { + string fileArgument; + boolean hasOptions; + + ExecFileSyncCall() { + this.getArgument(0).mayHaveStringValue(cat()) and + this = DataFlow::moduleImport("child_process").getAMemberCall("execFileSync") and + ( + this.getNumArgument() = 2 and hasOptions = false + or + this.getNumArgument() = 3 and hasOptions = true + ) and + fileArgument = getFileThatIsRead(this) + } + + override predicate isSync() { any() } + + override string getFileArgument() { result = fileArgument } + + override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(2) } + + override DataFlow::FunctionNode getCallback() { none() } + } } diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected deleted file mode 100644 index 67b59aa9d6ef..000000000000 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.expected +++ /dev/null @@ -1,3 +0,0 @@ -| False negative | uselesscat.js:69:42:69:69 | // NOT ... lagged] | -| False positive | uselesscat.js:18:70:18:118 | // OK [ ... jection | -| False positive | uselesscat.js:82:80:82:128 | // OK ( ... / gid)) | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected new file mode 100644 index 000000000000..f4b22cd0803f --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected @@ -0,0 +1,20 @@ +readFile +| uselesscat.js:10:1:10:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) | +| uselesscat.js:12:1:14:2 | exec("c ... ut);\\n}) | fs.readFile("/proc/"+id+"/status", function(err, out) {...}) | +| uselesscat.js:16:1:16:29 | execSyn ... uinfo') | fs.readFileSync("/proc/cpuinfo") | +| uselesscat.js:18:1:18:26 | execSyn ... path}`) | fs.readFileSync(`${newpath}`) | +| uselesscat.js:32:1:32:34 | execSyn ... path}`) | fs.readFileSync(`foo/bar/${newpath}`) | +| uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | fs.readFileSync(`foo/bar/${newpath}`, {encoding: 'utf8'})) | +| uselesscat.js:51:9:51:31 | execSyn ... + file) | fs.readFileSync(file) | +| uselesscat.js:59:1:62:2 | execFil ... ut);\\n}) | fs.readFile("pom.xml", function(error, stderr, stdout) {...}) | +| uselesscat.js:69:1:72:2 | execFil ... ut);\\n}) | fs.readFile("pom.xml", {encoding: 'utf8'}), function(error, stderr, stdout) {...}) | +| uselesscat.js:74:1:74:60 | execFil ... utf8'}) | fs.readFileSync("pom.xml", {encoding: 'utf8'})) | +| uselesscat.js:76:1:76:39 | execFil ... xml' ]) | fs.readFileSync("pom.xml") | +| uselesscat.js:79:1:79:46 | execFil ... opts) | fs.readFileSync("pom.xml", opts)) | +| uselesscat.js:82:1:82:90 | execFil ... String) | fs.readFileSync("pom.xml", anOptsFileNameThatIsTooLongToBePrintedByToString)) | +| uselesscat.js:86:1:86:75 | execFil ... utf8'}) | fs.readFileSync("foo/"+newPath+"bar", {encoding: 'utf8'})) | +| uselesscat.js:88:1:88:35 | execSyn ... + foo) | fs.readFileSync("/proc/cpuinfo"+foo) | +| uselesscat.js:90:1:90:50 | execFil ... th}` ]) | fs.readFileSync(`foo/bar/${newpath}`) | +#select +| False negative | uselesscat.js:54:42:54:69 | // NOT ... lagged] | +| False negative | uselesscat.js:84:118:84:144 | // NOT ... lagged] | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.ql b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql similarity index 89% rename from javascript/ql/test/query-tests/Security/CWE-078/UselessCat.ql rename to javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql index bfb342f7ad15..edcf4987e3e8 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessCat.ql +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql @@ -16,3 +16,5 @@ where msg = "False positive" ) select msg, comment + +query string readFile(UselessCat cat) { result = createReadFileCall(cat) } diff --git a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js index 4fe7ccd75d2f..d35f3224d189 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js @@ -7,76 +7,86 @@ var spawnSync = child_process.spawnSync; var fs = require('fs'); var app = express(); -function readStatus(id) { - exec("cat /proc/" + id + "/status", function (err, out) { // NOT OK - console.log(out); - }); -}; - -var basePath = '/foo/bar'; -app.get('/:data', function (req, res) { - res.send(execSync('cat ' + basePath + req.params.data).toString()); // OK [but flagged] - caught by command-injection +exec("cat foo/bar", function (err, out) {}); // NOT OK + +exec("cat /proc/" + id + "/status", function (err, out) { // NOT OK + console.log(out); }); -spawn('cat', ['package.json']); // NOT OK +execSync('cat /proc/cpuinfo').toString(); // NOT OK. -spawn('/bin/cat', ['package.json']); // NOT +execSync(`cat ${newpath}`) // NOT OK -spawn('/bin/cat', [someValue]); // NOT OK -spawn('sudo /bin/cat', [someValue]); // NOT OK -spawn('sudo cat', [someValue]); // NOT OK +child_process.execSync('cat package.json | wc -l'); // OK - pipes! -child_process.spawnSync('cat', [someValue]); // NOT OK +execSync('cat /proc/cpuinfo /foo/bar').toString(); // OK multiple files. -child_process.execSync('cat package.json | wc -l'); // OK - pipes! +execSync(`cat ${newpath} /foo/bar`).toString(); // OK multiple files. -const cat = child_process.spawn('cat') // OK - benign use (usually pipe to and from). +exec(`cat ${newpath} | grep foo`, function (err, out) { }) // OK - pipes -spawn('cat', ['package.json']); // NOT OK. +execSync(`cat ${newpath}`, {uid: 1000}) // OK - non trivial options -exec('cat *.js') // OK - wildcard use. -exec('cat *.js | wc -l') // OK - wildcard use and pipes! -exec('cat *.js | wc -l', { cwd: './' }, function () { }); // OK - wildcard and pipes +exec('cat *.js | wc -l', { cwd: './' }, function (err, out) { }); // OK - wildcard and pipes -spawnSync('cat', ['/proc/cpuinfo']) // NOT OK. +execSync(`cat foo/bar/${newpath}`); // NOT OK ("encoding" is used EXACTLY the same way in fs.readFileSync) -exec(`cat ${path.join(__dirname, 'package.json')} | sort | uniq`, () => {}); // OK: pipes +execSync(`cat foo/bar/${newpath}`, {encoding: 'utf8'}); // NOT OK ("encoding" is used EXACTLY the same way in fs.readFileSync) -execSync('cat /proc/cpuinfo').toString(); // NOT OK. +execSync("/bin/cat /proc/cpuinfo", { uid: 1000, gid: 1000, encoding: 'utf8'}); // OK (fs.readFileSync cannot emulate uid / gid)) + +execSync('cat /proc/cpuinfo > foo/bar/baz').toString(); // OK. -var cmd = "cat /proc/cpuinfo" -execSync(cmd); // NOT OK +execSync(`cat ${newpath} > ${destpath}`).toString(); // OK. + +execSync(`cat ${files.join(' ')} > ${outFile}`); // OK + +execSync(`cat ${files.join(' ')}`); // OK - not just a simple file read + +exec("cat /proc/cpuinfo | grep name"); // OK - pipes -execSync("cat /proc/cpuinfo | grep -c '" + someValue + "'"); // OK - pipes +execSync(`cat ${newpath} | ${othertool}`); // OK - pipes function cat(file) { return execSync('cat ' + file).toString(); // NOT OK } -execSync(`cat ${files.join(' ')} > ${outFile}`); // OK +execSync("sh -c 'cat " + newpath + "'"); // NOT OK. [but not flagged] -var cmd = 'cat package.json | grep' -exec(cmd); // OK - pipes! +var execFile = child_process.execFile; +var execFileSync = child_process.execFileSync; -execSync("sudo cat " + newpath + "*.js | grep foo").toString(); // OK - wildcard and pipes +execFile('/bin/cat', [ 'pom.xml' ], function(error, stdout, stderr ) { // NOT OK + // Not using stderr + console.log(stdout); +}); -execSync(`cat ${newpath}`); // NOT OK +execFile('/bin/cat', [ 'pom.xml' ], function(error, stdout, stderr ) { // OK. - stderr is used. + console.log(stderr); +}); -exec("cat /proc/cpuinfo | grep name"); // OK - pipes -execSync(`cat ${newpath} | ${othertool}`); // OK - pipes +execFile('/bin/cat', [ 'pom.xml' ], {encoding: 'utf8'}, function(error, stdout, stderr ) { // NOT OK + // Not using stderr + console.log(stdout); +}); -execSync("sh -c 'cat " + newpath + "'"); // NOT OK. [but not flagged] +execFileSync('/bin/cat', [ 'pom.xml' ], {encoding: 'utf8'}); // NOT OK -exec(` cat ${newpath}`) // NOT OK +execFileSync('/bin/cat', [ 'pom.xml' ]); // NOT OK -exec(` cat ${newpath} | grep foo`) // OK - pipes +var opts = {encoding: 'utf8'}; +execFileSync('/bin/cat', [ 'pom.xml' ], opts); // NOT OK -execSync('cat /proc/cpuinfo > foo/bar/baz').toString(); // OK. +var anOptsFileNameThatIsTooLongToBePrintedByToString = {encoding: 'utf8'}; +execFileSync('/bin/cat', [ 'pom.xml' ], anOptsFileNameThatIsTooLongToBePrintedByToString); // NOT OK -execSync(`cat ${newpath} > ${destpath}`).toString(); // OK. +execFileSync('/bin/cat', [ 'pom.xml' ], {encoding: 'someEncodingValueThatIsCompletelyBogusAndTooLongForToString'}); // NOT OK [but not flagged] + +execFileSync('/bin/cat', [ "foo/" + newPath + "bar" ], {encoding: 'utf8'}); // NOT OK + +execSync('cat /proc/cpuinfo' + foo).toString(); // NOT OK. -const Opts = {encoding: 'utf8'} -execSync(`cat foo/bar/${newpath}`, Opts).slice(0, 7); // NOT OK ("encoding" is used EXACTLY the same way in fs.readFileSync) +execFileSync('/bin/cat', [ `foo/bar/${newpath}` ]); // NOT OK -execSync("/bin/cat /proc/cpuinfo", { uid: 1000, gid: 1000, encoding: 'utf8'}); // OK (fs.readFileSync cannot emulate uid / gid)) \ No newline at end of file +execFileSync('node', [ `foo/bar/${newpath}` ]); // OK - not a call to cat From 56f3e431f934952efa50deba7d2f913fbef88377 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 20 Feb 2020 10:28:53 +0100 Subject: [PATCH 04/37] update expected output --- .../Security/CWE-078/CommandInjection.expected | 9 --------- 1 file changed, 9 deletions(-) diff --git a/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected b/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected index 3211c1196453..589deda96c96 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected @@ -89,10 +89,6 @@ nodes | third-party-command-injection.js:5:20:5:26 | command | | third-party-command-injection.js:6:21:6:27 | command | | third-party-command-injection.js:6:21:6:27 | command | -| uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | -| uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | -| uselesscat.js:18:40:18:54 | req.params.data | -| uselesscat.js:18:40:18:54 | req.params.data | edges | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd | | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd | @@ -182,10 +178,6 @@ edges | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | -| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | -| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | -| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | -| uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | #select | child_process-test.js:17:13:17:15 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:17:13:17:15 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value | | child_process-test.js:18:17:18:19 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:18:17:18:19 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value | @@ -219,4 +211,3 @@ edges | other.js:18:22:18:24 | cmd | other.js:5:25:5:31 | req.url | other.js:18:22:18:24 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value | | other.js:19:36:19:38 | cmd | other.js:5:25:5:31 | req.url | other.js:19:36:19:38 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value | | third-party-command-injection.js:6:21:6:27 | command | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | This command depends on $@. | third-party-command-injection.js:5:20:5:26 | command | a server-provided value | -| uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | uselesscat.js:18:40:18:54 | req.params.data | uselesscat.js:18:20:18:54 | 'cat ' ... ms.data | This command depends on $@. | uselesscat.js:18:40:18:54 | req.params.data | a user-provided value | From d4e73df92f400fc6962363d594b1a4878d3aa87f Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 20 Feb 2020 10:39:16 +0100 Subject: [PATCH 05/37] remove dead predicate --- .../semmle/javascript/security/UselessUseOfCat.qll | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index f67fa2876853..3afe8dda8e68 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -232,20 +232,6 @@ module UselsesCatCandidates { override DataFlow::FunctionNode getCallback() { none() } } - // TODO: No! - bindingset[str, quote] - string surroundInQuotes(string str, string quote) { - if not str.prefix(1) = quote and not str.suffix(str.length() - 1) = quote - then result = quote + str + quote - else - if not str.prefix(1) = quote - then result = str + quote - else - if not str.suffix(str.length() - 1) = quote - then result = quote + str - else result = str - } - /** * Gets the file that is read for a call to child_process.execFile/execFileSync. */ From a5fdcb67f9b2b96228b02caf59003f970ada2c46 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 20 Feb 2020 10:43:41 +0100 Subject: [PATCH 06/37] restricts alerts to the first line --- javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql index f48ba6b8e958..e3fde814d666 100644 --- a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql @@ -13,9 +13,8 @@ import javascript import semmle.javascript.security.UselessUseOfCat +import semmle.javascript.RestrictedLocations from UselessCat cat -select cat, "Useless use of `cat`. Can be replaced with: " + createReadFileCall(cat) - - +select cat.asExpr().(FirstLineOf), "Useless use of `cat`. Can be replaced with: " + createReadFileCall(cat) \ No newline at end of file From 558beb72559e8c6ee97609ffd3f29beaaf06bc0a Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 20 Feb 2020 10:57:33 +0100 Subject: [PATCH 07/37] simplify the output file argument --- .../javascript/security/UselessUseOfCat.qll | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index 3afe8dda8e68..02eacb08a864 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -172,14 +172,28 @@ module UselsesCatCandidates { | (if root.asExpr() instanceof TemplateLiteral then quote = "`" else quote = "\"") and root.getFirstLeaf().getStringValue().prefix(cat.length()) = cat and - // Remove an initial ""+ (e.g. in `""+file`) exists(string rawConcat | rawConcat = quote + printed.suffix(cat.length()).trim() | - if rawConcat.prefix(3) = "\"\"+" then result = rawConcat.suffix(3) else result = rawConcat + result = getSimplifiedStringConcat(rawConcat) ) ) ) } + /** + * Gets a simplified and equivalent string concatenation for a given string concatenation `str` + */ + bindingset[str] + private string getSimplifiedStringConcat(string str) { + // Remove an initial ""+ (e.g. in `""+file`) + if str.prefix(3) = "\"\"+" then + result = str.suffix(3) + // prettify `${newpath}` to just newpath + else if str.prefix(3) = "`${" and str.suffix(str.length() - 2) = "}`" and not str.suffix(3).matches("%{%") then + result = str.prefix(str.length() - 2).suffix(3) + else + result = str + } + /** * A call to child_process.exec that might be a useless call to cat. */ From a193cb110eb9960c56e93b9f3816e6d273077d88 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 20 Feb 2020 11:13:39 +0100 Subject: [PATCH 08/37] support arrow functions in the callbacks --- .../javascript/security/UselessUseOfCat.qll | 28 ++++++++++++++----- .../Security/CWE-078/UselessUseOfCat.expected | 5 +++- .../Security/CWE-078/uselesscat.js | 6 ++++ 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index 02eacb08a864..ac5338eb27ef 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -17,13 +17,24 @@ string createReadFileCall(UselsesCatCandidates::UselessCatCandicate cat) { else extraArg = "" ) and if exists(cat.getCallback()) - then callback = ", function(" + getCallbackArgs(cat.getCallback()) + ") {...}" + then callback = constructCallbackString(cat.getCallback()) else callback = "" | result = "fs.readFile" + sync + "(" + cat.getFileArgument().trim() + extraArg + callback + ")" ) } +string constructCallbackString(DataFlow::FunctionNode func) { + exists(string args | args = getCallbackArgs(func) | + if func.getFunction() instanceof ArrowFunctionExpr + then + if func.getFunction().getBody() instanceof Expr + then result = ", (" + args + ") => ..." + else result = ", (" + args + ") => {...}" + else result = ", function(" + args + ") {...}" + ) +} + /** * Gets a string concatenation of the parameters to a function. */ @@ -185,13 +196,16 @@ module UselsesCatCandidates { bindingset[str] private string getSimplifiedStringConcat(string str) { // Remove an initial ""+ (e.g. in `""+file`) - if str.prefix(3) = "\"\"+" then - result = str.suffix(3) - // prettify `${newpath}` to just newpath - else if str.prefix(3) = "`${" and str.suffix(str.length() - 2) = "}`" and not str.suffix(3).matches("%{%") then - result = str.prefix(str.length() - 2).suffix(3) + if str.prefix(3) = "\"\"+" + then result = str.suffix(3) else - result = str + // prettify `${newpath}` to just newpath + if + str.prefix(3) = "`${" and + str.suffix(str.length() - 2) = "}`" and + not str.suffix(3).matches("%{%") + then result = str.prefix(str.length() - 2).suffix(3) + else result = str } /** diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected index f4b22cd0803f..de1c6aa86fed 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected @@ -2,7 +2,7 @@ readFile | uselesscat.js:10:1:10:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) | | uselesscat.js:12:1:14:2 | exec("c ... ut);\\n}) | fs.readFile("/proc/"+id+"/status", function(err, out) {...}) | | uselesscat.js:16:1:16:29 | execSyn ... uinfo') | fs.readFileSync("/proc/cpuinfo") | -| uselesscat.js:18:1:18:26 | execSyn ... path}`) | fs.readFileSync(`${newpath}`) | +| uselesscat.js:18:1:18:26 | execSyn ... path}`) | fs.readFileSync(newpath) | | uselesscat.js:32:1:32:34 | execSyn ... path}`) | fs.readFileSync(`foo/bar/${newpath}`) | | uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | fs.readFileSync(`foo/bar/${newpath}`, {encoding: 'utf8'})) | | uselesscat.js:51:9:51:31 | execSyn ... + file) | fs.readFileSync(file) | @@ -15,6 +15,9 @@ readFile | uselesscat.js:86:1:86:75 | execFil ... utf8'}) | fs.readFileSync("foo/"+newPath+"bar", {encoding: 'utf8'})) | | uselesscat.js:88:1:88:35 | execSyn ... + foo) | fs.readFileSync("/proc/cpuinfo"+foo) | | uselesscat.js:90:1:90:50 | execFil ... th}` ]) | fs.readFileSync(`foo/bar/${newpath}`) | +| uselesscat.js:94:1:94:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) | +| uselesscat.js:96:1:96:53 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | +| uselesscat.js:98:1:98:55 | exec("c ... h(out)) | fs.readFile("foo/bar", (err, out) => ...) | #select | False negative | uselesscat.js:54:42:54:69 | // NOT ... lagged] | | False negative | uselesscat.js:84:118:84:144 | // NOT ... lagged] | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js index d35f3224d189..98c41513baa0 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js @@ -90,3 +90,9 @@ execSync('cat /proc/cpuinfo' + foo).toString(); // NOT OK. execFileSync('/bin/cat', [ `foo/bar/${newpath}` ]); // NOT OK execFileSync('node', [ `foo/bar/${newpath}` ]); // OK - not a call to cat + +exec("cat foo/bar", function (err, out) {}); // NOT OK + +exec("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK + +exec("cat foo/bar", (err, out) => doSomethingWith(out)); // NOT OK \ No newline at end of file From b5ef45e6c232b163ea95dd8911f8b0a0c61d68ed Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 20 Feb 2020 11:30:23 +0100 Subject: [PATCH 09/37] add isSync predicate to SystemCommandExecution --- .../ql/src/semmle/javascript/Concepts.qll | 3 ++ .../javascript/frameworks/NodeJSLib.qll | 4 ++ .../semmle/javascript/frameworks/ShellJS.qll | 2 + .../frameworks/SystemCommandExecutors.qll | 20 +++++++- .../Security/CWE-078/UselessUseOfCat.expected | 46 +++++++++++++++++++ .../Security/CWE-078/UselessUseOfCat.ql | 4 ++ 6 files changed, 78 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/Concepts.qll b/javascript/ql/src/semmle/javascript/Concepts.qll index 23bbbce3b163..4fef87f85854 100644 --- a/javascript/ql/src/semmle/javascript/Concepts.qll +++ b/javascript/ql/src/semmle/javascript/Concepts.qll @@ -22,6 +22,9 @@ abstract class SystemCommandExecution extends DataFlow::Node { * to the command. */ DataFlow::Node getArgumentList() { none() } + + /** Holds if the command execution happens synchronously. */ + abstract predicate isSync(); } /** diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll index 0ee7c10fa7ff..95600e072f66 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll @@ -623,6 +623,10 @@ module NodeJSLib { // all of the above methods take the argument list as their second argument result = getArgument(1) } + + override predicate isSync() { + "Sync" = methodName.suffix(methodName.length() - 4) + } } /** diff --git a/javascript/ql/src/semmle/javascript/frameworks/ShellJS.qll b/javascript/ql/src/semmle/javascript/frameworks/ShellJS.qll index 226054792444..e459fe9089c7 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ShellJS.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ShellJS.qll @@ -160,6 +160,8 @@ module ShellJS { override DataFlow::Node getACommandArgument() { result = getArgument(0) } override predicate isShellInterpreted(DataFlow::Node arg) { arg = getACommandArgument() } + + override predicate isSync() {none ()} } /** diff --git a/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll b/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll index 7cea87bb3f3a..e0664f93dd1a 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll @@ -9,6 +9,7 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I int cmdArg; boolean shell; + boolean sync; SystemCommandExecutors() { exists(string mod, DataFlow::SourceNode callee | @@ -31,9 +32,11 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I ) and cmdArg = 0 | - callee = DataFlow::moduleMember(mod, method) + callee = DataFlow::moduleMember(mod, method) and + sync = getSync(method) ) or + sync = false and ( shell = false and ( @@ -65,4 +68,19 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I override predicate isShellInterpreted(DataFlow::Node arg) { arg = getACommandArgument() and shell = true } + + override predicate isSync() { + sync = true + } } + +/** + * Gets a boolean reflecting if the name ends with "sync" or "Sync". + */ +bindingset[name] +private boolean getSync(string name) { + if name.suffix(name.length() - 4) = "Sync" or name.suffix(name.length() - 4) = "sync" then + result = true + else + result = false +} \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected index de1c6aa86fed..3acd23863d0e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected @@ -18,6 +18,52 @@ readFile | uselesscat.js:94:1:94:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) | | uselesscat.js:96:1:96:53 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | | uselesscat.js:98:1:98:55 | exec("c ... h(out)) | fs.readFile("foo/bar", (err, out) => ...) | +syncCommand +| child_process-test.js:9:5:9:22 | cp.execSync("foo") | +| child_process-test.js:11:5:11:26 | cp.exec ... ("foo") | +| child_process-test.js:13:5:13:23 | cp.spawnSync("foo") | +| child_process-test.js:18:5:18:20 | cp.execSync(cmd) | +| child_process-test.js:20:5:20:24 | cp.execFileSync(cmd) | +| child_process-test.js:22:5:22:21 | cp.spawnSync(cmd) | +| command-line-parameter-command-injection.js:11:2:11:21 | cp.execSync(args[0]) | +| command-line-parameter-command-injection.js:12:2:12:33 | cp.exec ... rgs[0]) | +| command-line-parameter-command-injection.js:15:2:15:26 | cp.exec ... rgs[0]) | +| command-line-parameter-command-injection.js:16:2:16:38 | cp.exec ... rgs[0]) | +| command-line-parameter-command-injection.js:19:2:19:18 | cp.execSync(arg0) | +| command-line-parameter-command-injection.js:20:2:20:30 | cp.exec ... + arg0) | +| command-line-parameter-command-injection.js:26:2:26:51 | cp.exec ... tion"`) | +| command-line-parameter-command-injection.js:27:2:27:58 | cp.exec ... tion"`) | +| other.js:7:5:7:36 | require ... nc(cmd) | +| other.js:9:5:9:35 | require ... nc(cmd) | +| other.js:12:5:12:30 | require ... nc(cmd) | +| third-party-command-injection.js:6:9:6:28 | cp.execSync(command) | +| tst_shell-command-injection-from-environment.js:4:2:4:62 | cp.exec ... emp")]) | +| tst_shell-command-injection-from-environment.js:5:2:5:54 | cp.exec ... temp")) | +| uselesscat.js:16:1:16:29 | execSyn ... uinfo') | +| uselesscat.js:18:1:18:26 | execSyn ... path}`) | +| uselesscat.js:20:1:20:50 | child_p ... wc -l') | +| uselesscat.js:22:1:22:38 | execSyn ... o/bar') | +| uselesscat.js:24:1:24:35 | execSyn ... o/bar`) | +| uselesscat.js:28:1:28:39 | execSyn ... 1000}) | +| uselesscat.js:32:1:32:34 | execSyn ... path}`) | +| uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | +| uselesscat.js:36:1:36:77 | execSyn ... utf8'}) | +| uselesscat.js:38:1:38:43 | execSyn ... r/baz') | +| uselesscat.js:40:1:40:40 | execSyn ... path}`) | +| uselesscat.js:42:1:42:47 | execSyn ... File}`) | +| uselesscat.js:44:1:44:34 | execSyn ... ' ')}`) | +| uselesscat.js:48:1:48:41 | execSyn ... tool}`) | +| uselesscat.js:51:9:51:31 | execSyn ... + file) | +| uselesscat.js:54:1:54:39 | execSyn ... + "'") | +| uselesscat.js:74:1:74:60 | execFil ... utf8'}) | +| uselesscat.js:76:1:76:39 | execFil ... xml' ]) | +| uselesscat.js:79:1:79:46 | execFil ... opts) | +| uselesscat.js:82:1:82:90 | execFil ... String) | +| uselesscat.js:84:1:84:115 | execFil ... ring'}) | +| uselesscat.js:86:1:86:75 | execFil ... utf8'}) | +| uselesscat.js:88:1:88:35 | execSyn ... + foo) | +| uselesscat.js:90:1:90:50 | execFil ... th}` ]) | +| uselesscat.js:92:1:92:46 | execFil ... th}` ]) | #select | False negative | uselesscat.js:54:42:54:69 | // NOT ... lagged] | | False negative | uselesscat.js:84:118:84:144 | // NOT ... lagged] | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql index edcf4987e3e8..f9a4fd2ea8c2 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql @@ -18,3 +18,7 @@ where select msg, comment query string readFile(UselessCat cat) { result = createReadFileCall(cat) } + +query SystemCommandExecution syncCommand() { + result.isSync() +} \ No newline at end of file From 12c0291dde44e65f39db09e6918eba6ee09b86bd Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 20 Feb 2020 11:35:11 +0100 Subject: [PATCH 10/37] require that an options object has a known set of properties --- .../semmle/javascript/security/UselessUseOfCat.qll | 13 +++++++++---- .../Security/CWE-078/UselessUseOfCat.expected | 1 + .../test/query-tests/Security/CWE-078/uselesscat.js | 4 +++- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index ac5338eb27ef..5558fdcb654a 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -60,15 +60,20 @@ class UselessCat extends DataFlow::CallNode { UselessCat() { this = candidate and + exists(createReadFileCall(this)) and // wildcards, pipes, redirections, and multiple files are OK. // (The multiple files detection relies on the fileArgument not containing spaces anywhere) not candidate.getFileArgument().regexpMatch(".*(\\*|\\||>|<| ).*") and // Only acceptable option is "encoding", everything else is non-trivial to emulate with fs.readFile. - not exists(string prop | - not prop = "encoding" and - exists(candidate.getOptionsArg().getALocalSource().getAPropertyWrite(prop)) + ( + not exists(candidate.getOptionsArg()) + or + forex(string prop | + exists(candidate.getOptionsArg().getALocalSource().getAPropertyWrite(prop)) + | + prop = "encoding" + ) ) and - exists(createReadFileCall(this)) and // If there is a callback, then it must either have one or two arguments, or if there is a third argument it must be unused. ( not exists(candidate.getCallback()) diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected index 3acd23863d0e..61def6726b38 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected @@ -64,6 +64,7 @@ syncCommand | uselesscat.js:88:1:88:35 | execSyn ... + foo) | | uselesscat.js:90:1:90:50 | execFil ... th}` ]) | | uselesscat.js:92:1:92:46 | execFil ... th}` ]) | +| uselesscat.js:100:1:100:56 | execFil ... ptions) | #select | False negative | uselesscat.js:54:42:54:69 | // NOT ... lagged] | | False negative | uselesscat.js:84:118:84:144 | // NOT ... lagged] | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js index 98c41513baa0..c860d21f6549 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js @@ -95,4 +95,6 @@ exec("cat foo/bar", function (err, out) {}); // NOT OK exec("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK -exec("cat foo/bar", (err, out) => doSomethingWith(out)); // NOT OK \ No newline at end of file +exec("cat foo/bar", (err, out) => doSomethingWith(out)); // NOT OK + +execFileSync('/bin/cat', [ 'pom.xml' ], unknownOptions); // OK - unknown options. \ No newline at end of file From b1cbfce50bdc96b53ea7feb5c34ca3f64604892e Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 20 Feb 2020 14:17:37 +0100 Subject: [PATCH 11/37] use SystemCommandExecution and a few small fixes --- .../javascript/security/UselessUseOfCat.qll | 180 ++++++------------ .../Security/CWE-078/UselessUseOfCat.expected | 4 +- 2 files changed, 57 insertions(+), 127 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index 5558fdcb654a..a77298f83767 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -3,10 +3,9 @@ */ import javascript -import semmle.javascript.security.dataflow.IndirectCommandArgument /** - * Gets a string representing an equivalent call to `fs.ReadFile` for a call to `cat`. + * Gets a string representation of an equivalent call to `fs.readFile` for a given command execution `cat`. */ string createReadFileCall(UselsesCatCandidates::UselessCatCandicate cat) { exists(string sync, string extraArg, string callback | @@ -24,6 +23,9 @@ string createReadFileCall(UselsesCatCandidates::UselessCatCandicate cat) { ) } +/** + * Constructs a string representing the callback `func`. + */ string constructCallbackString(DataFlow::FunctionNode func) { exists(string args | args = getCallbackArgs(func) | if func.getFunction() instanceof ArrowFunctionExpr @@ -36,20 +38,25 @@ string constructCallbackString(DataFlow::FunctionNode func) { } /** - * Gets a string concatenation of the parameters to a function. + * Gets a string concatenation of the parameter names in a function `func`. */ private string getCallbackArgs(DataFlow::FunctionNode func) { - result = concat(int i | i = [0 .. 2] | func.getParameter(i).getName(), ", ") + result = + concat(int i | + i = [0 .. func.getNumParameter()] + | + func.getParameter(i).getName(), ", " order by i + ) } /** - * Gets a string representation of the options argument from an exec-like call. + * Gets a string representation of the options argument `arg` from an exec-like call. */ -private string printOptionsArg(DataFlow::Node node) { - result = node.asExpr().(VarAccess).getVariable().getName() +private string printOptionsArg(DataFlow::Node arg) { + result = arg.asExpr().(VarAccess).getVariable().getName() or // fall back to toString(), but ensure that we don't have dots in the middle. - result = node.(DataFlow::ObjectLiteralNode).toString() and not result.regexpMatch(".*\\.\\..*") + result = arg.(DataFlow::ObjectLiteralNode).toString() and not result.regexpMatch(".*\\.\\..*") } /** @@ -60,6 +67,7 @@ class UselessCat extends DataFlow::CallNode { UselessCat() { this = candidate and + // We can create an equivalent `fs.readFile` call. exists(createReadFileCall(this)) and // wildcards, pipes, redirections, and multiple files are OK. // (The multiple files detection relies on the fileArgument not containing spaces anywhere) @@ -96,28 +104,56 @@ class UselessCat extends DataFlow::CallNode { module UselsesCatCandidates { /** * A candidate for a useless use of `cat`. - * Subclasses of this class specify the structure of the `exec`-like call. + * This class describes the structure of a call to cat + * This class does not determine whether it is a useless call, or even if it is a call to `cat`. */ - abstract class UselessCatCandicate extends DataFlow::CallNode { + class UselessCatCandicate extends DataFlow::CallNode { + SystemCommandExecution command; + + UselessCatCandicate() { this = command } + /** * Holds if the call is synchronous (e.g. `execFileSync`). */ - abstract predicate isSync(); + predicate isSync() { command.isSync() } /** - * Gets a string representation of the expression that determines what file is read. + * Holds if the executed command execution has an argument list as a separate argument. */ - abstract string getFileArgument(); + predicate hasArgumentList() { exists(command.getArgumentList()) } /** - * Gets the data-flow node for the options argument to the `exec`-like call. + * Gets a string representation of the expression that determines what file is read by `cat`. */ - abstract DataFlow::Node getOptionsArg(); + string getFileArgument() { + if hasArgumentList() + then + getArgument(0).mayHaveStringValue(cat()) and + result = getFileThatIsReadFromCommandList(this) + else result = getFileArgumentWithoutCat(getArgument(0)) + } /** - * Gets the callback used for the `exec` like call (if it exists). + * Gets the data-flow node (if it exists) for the options argument to the `exec`-like call. */ - abstract DataFlow::FunctionNode getCallback(); + DataFlow::Node getOptionsArg() { + exists(int n | + n >= 1 and + // If there is a command-list, then the options is at least the third argument. + (not exists(command.getArgumentList()) or n >= 2) and + // async calls have a callback as their last call. + if this.isSync() then n < getNumArgument() else n < getNumArgument() - 1 + | + result = getArgument(n) + ) + } + + /** + * Gets the callback (if it exists) for an async `exec` like call. + */ + DataFlow::FunctionNode getCallback() { + not this.isSync() and result = getLastArgument().getALocalSource() + } } /** @@ -214,61 +250,9 @@ module UselsesCatCandidates { } /** - * A call to child_process.exec that might be a useless call to cat. - */ - private class ExecCall extends UselessCatCandicate { - string fileArgument; - boolean hasOptions; - - ExecCall() { - this = DataFlow::moduleImport("child_process").getAMemberCall("exec") and - ( - this.getNumArgument() = 2 and hasOptions = false - or - this.getNumArgument() = 3 and hasOptions = true - ) and - fileArgument = getFileArgumentWithoutCat(getArgument(0)) - } - - override predicate isSync() { none() } - - override string getFileArgument() { result = fileArgument } - - override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(1) } - - override DataFlow::FunctionNode getCallback() { result = getLastArgument() } - } - - /** - * A call to child_process.execSync that might be a useless call to cat. - */ - private class ExecSyncCall extends UselessCatCandicate { - string fileArgument; - boolean hasOptions; - - ExecSyncCall() { - this = DataFlow::moduleImport("child_process").getAMemberCall("execSync") and - ( - this.getNumArgument() = 1 and hasOptions = false - or - this.getNumArgument() = 2 and hasOptions = true - ) and - fileArgument = getFileArgumentWithoutCat(getArgument(0)) - } - - override predicate isSync() { any() } - - override string getFileArgument() { result = fileArgument } - - override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(1) } - - override DataFlow::FunctionNode getCallback() { none() } - } - - /** - * Gets the file that is read for a call to child_process.execFile/execFileSync. + * Gets the file that is read for a call with an explicit command list (e.g. `child_process.execFile/execFileSync`). */ - string getFileThatIsRead(DataFlow::CallNode call) { + string getFileThatIsReadFromCommandList(DataFlow::CallNode call) { exists(DataFlow::ArrayCreationNode array, DataFlow::Node element | array = call.getArgument(1).(DataFlow::ArrayCreationNode) and array.getSize() = 1 and @@ -279,58 +263,4 @@ module UselsesCatCandidates { result = createConcatRepresentation(element) ) } - - /** - * A call to child_process.execFile that might be a useless call to cat. - */ - private class ExecFileCall extends UselessCatCandicate { - string fileArgument; - boolean hasOptions; - - ExecFileCall() { - this.getArgument(0).mayHaveStringValue(cat()) and - this = DataFlow::moduleImport("child_process").getAMemberCall("execFile") and - ( - this.getNumArgument() = 3 and hasOptions = false - or - this.getNumArgument() = 4 and hasOptions = true - ) and - fileArgument = getFileThatIsRead(this) - } - - override predicate isSync() { none() } - - override string getFileArgument() { result = fileArgument } - - override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(2) } - - override DataFlow::FunctionNode getCallback() { result = getLastArgument() } - } - - /** - * A call to child_process.execFileSync that might be a useless call to cat. - */ - private class ExecFileSyncCall extends UselessCatCandicate { - string fileArgument; - boolean hasOptions; - - ExecFileSyncCall() { - this.getArgument(0).mayHaveStringValue(cat()) and - this = DataFlow::moduleImport("child_process").getAMemberCall("execFileSync") and - ( - this.getNumArgument() = 2 and hasOptions = false - or - this.getNumArgument() = 3 and hasOptions = true - ) and - fileArgument = getFileThatIsRead(this) - } - - override predicate isSync() { any() } - - override string getFileArgument() { result = fileArgument } - - override DataFlow::Node getOptionsArg() { hasOptions = true and result = getArgument(2) } - - override DataFlow::FunctionNode getCallback() { none() } - } } diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected index 61def6726b38..f4ebbd0848ba 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected @@ -6,8 +6,8 @@ readFile | uselesscat.js:32:1:32:34 | execSyn ... path}`) | fs.readFileSync(`foo/bar/${newpath}`) | | uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | fs.readFileSync(`foo/bar/${newpath}`, {encoding: 'utf8'})) | | uselesscat.js:51:9:51:31 | execSyn ... + file) | fs.readFileSync(file) | -| uselesscat.js:59:1:62:2 | execFil ... ut);\\n}) | fs.readFile("pom.xml", function(error, stderr, stdout) {...}) | -| uselesscat.js:69:1:72:2 | execFil ... ut);\\n}) | fs.readFile("pom.xml", {encoding: 'utf8'}), function(error, stderr, stdout) {...}) | +| uselesscat.js:59:1:62:2 | execFil ... ut);\\n}) | fs.readFile("pom.xml", function(error, stdout, stderr) {...}) | +| uselesscat.js:69:1:72:2 | execFil ... ut);\\n}) | fs.readFile("pom.xml", {encoding: 'utf8'}), function(error, stdout, stderr) {...}) | | uselesscat.js:74:1:74:60 | execFil ... utf8'}) | fs.readFileSync("pom.xml", {encoding: 'utf8'})) | | uselesscat.js:76:1:76:39 | execFil ... xml' ]) | fs.readFileSync("pom.xml") | | uselesscat.js:79:1:79:46 | execFil ... opts) | fs.readFileSync("pom.xml", opts)) | From b2ccec28e076c3b93c56da15cce11acd3577f748 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 20 Feb 2020 14:34:50 +0100 Subject: [PATCH 12/37] require the file to be non-empty --- .../ql/src/semmle/javascript/security/UselessUseOfCat.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index a77298f83767..47a99b0bf4e5 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -69,6 +69,8 @@ class UselessCat extends DataFlow::CallNode { this = candidate and // We can create an equivalent `fs.readFile` call. exists(createReadFileCall(this)) and + // There is a file to read, and not just a pair of quotes. + candidate.getFileArgument().length() >= 3 and // wildcards, pipes, redirections, and multiple files are OK. // (The multiple files detection relies on the fileArgument not containing spaces anywhere) not candidate.getFileArgument().regexpMatch(".*(\\*|\\||>|<| ).*") and From 924272a7a508567b3622a1a317e3649bc2424a2d Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 20 Feb 2020 14:35:26 +0100 Subject: [PATCH 13/37] insert placeholder qhelp --- .../Security/CWE-078/UselessUseOfCat.qhelp | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp new file mode 100644 index 000000000000..452f630712d2 --- /dev/null +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp @@ -0,0 +1,30 @@ + + + +

+ Useless use of cat +

+ + + + +

+ TODO: This is a placeholder +

+ + + +

+

+ + + +
  • + OWASP: + Command Injection. +
    • + +     +     From 6ea14532ab155a8d7cc0cb2ab2f7c8552212e3f6 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 21 Feb 2020 10:27:57 +0100 Subject: [PATCH 14/37] small changes based on review --- .../src/Security/CWE-078/UselessUseOfCat.ql | 5 ++--- .../javascript/security/UselessUseOfCat.qll | 19 +++++++++---------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql index e3fde814d666..417b9d9f2e3e 100644 --- a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql @@ -1,14 +1,13 @@ /** * @name Useless use of cat - * @description Using cat to simply read a file can lead to unintended bugs, and at worst security issues. + * @description Using `cat`-process to simply read a file is unnecessarily complex, inefficient, unportable, can lead to subtle bugs, or even security vulnerabilities. * @kind problem * @problem.severity error * @precision high * @id js/useless-use-of-cat * @tags correctness * security - * external/cwe/cwe-078 - * external/cwe/cwe-088 + * maintainability */ import javascript diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index 47a99b0bf4e5..ca21f5d0bc65 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -11,13 +11,15 @@ string createReadFileCall(UselsesCatCandidates::UselessCatCandicate cat) { exists(string sync, string extraArg, string callback | (if cat.isSync() then sync = "Sync" else sync = "") and ( - if exists(cat.getOptionsArg()) - then extraArg = ", " + printOptionsArg(cat.getOptionsArg()) + ")" - else extraArg = "" + extraArg = ", " + printOptionsArg(cat.getOptionsArg()) + ")" + or + extraArg = "" and not exists(cat.getOptionsArg()) ) and - if exists(cat.getCallback()) - then callback = constructCallbackString(cat.getCallback()) - else callback = "" + ( + callback = constructCallbackString(cat.getCallback()) + or + callback = "" and not exists(cat.getCallback()) + ) | result = "fs.readFile" + sync + "(" + cat.getFileArgument().trim() + extraArg + callback + ")" ) @@ -94,10 +96,7 @@ class UselessCat extends DataFlow::CallNode { func.getNumParameter() = 2 or // `exec` can use 3 parameters, `readFile` can only use two, so it is OK to have a third parameter if it is unused, - func.getNumParameter() = 3 and - not exists(DataFlow::Node node | - not node = func.getParameter(2) and func.getParameter(2) = node.getALocalSource() - ) + func.getNumParameter() = 3 and not exists(SSA::definition(func.getParameter(2).getParameter())) ) ) } From 75410e5760c47f77313715f07ec333ca0f7083d1 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 21 Feb 2020 14:26:42 +0100 Subject: [PATCH 15/37] big refactor of UselessUseOfCal --- .../src/Security/CWE-078/UselessUseOfCat.ql | 8 +- .../javascript/security/UselessUseOfCat.qll | 320 +++++++++++------- .../Security/CWE-078/UselessUseOfCat.expected | 11 +- .../Security/CWE-078/UselessUseOfCat.ql | 2 +- .../Security/CWE-078/uselesscat.js | 40 ++- 5 files changed, 244 insertions(+), 137 deletions(-) diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql index 417b9d9f2e3e..4f080711719b 100644 --- a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql @@ -15,5 +15,9 @@ import semmle.javascript.security.UselessUseOfCat import semmle.javascript.RestrictedLocations -from UselessCat cat -select cat.asExpr().(FirstLineOf), "Useless use of `cat`. Can be replaced with: " + createReadFileCall(cat) \ No newline at end of file +from UselessCat cat, string message +where + message = " Can be replaced with: " + PrettyPrintCatCall::createReadFileCall(cat) + or + not exists(PrettyPrintCatCall::createReadFileCall(cat)) and message = "" +select cat.asExpr().(FirstLineOf), "Useless use of `cat`." + message \ No newline at end of file diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index ca21f5d0bc65..2633c84ce70c 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -1,160 +1,233 @@ /** - * Provides predicates and classes for working with useless uses of `cat`. + * Provides predicates and classes for working with useless uses of the unix command `cat`. */ import javascript +import Expressions.ExprHasNoEffect +import Declarations.UnusedVariable /** - * Gets a string representation of an equivalent call to `fs.readFile` for a given command execution `cat`. + * A call that executes a system command. + * This class provide utility predicates for reasoning about command execution calls. */ -string createReadFileCall(UselsesCatCandidates::UselessCatCandicate cat) { - exists(string sync, string extraArg, string callback | - (if cat.isSync() then sync = "Sync" else sync = "") and - ( - extraArg = ", " + printOptionsArg(cat.getOptionsArg()) + ")" - or - extraArg = "" and not exists(cat.getOptionsArg()) - ) and - ( - callback = constructCallbackString(cat.getCallback()) - or - callback = "" and not exists(cat.getCallback()) - ) - | - result = "fs.readFile" + sync + "(" + cat.getFileArgument().trim() + extraArg + callback + ")" - ) -} +private class CommandCall extends DataFlow::InvokeNode { + SystemCommandExecution command; -/** - * Constructs a string representing the callback `func`. - */ -string constructCallbackString(DataFlow::FunctionNode func) { - exists(string args | args = getCallbackArgs(func) | - if func.getFunction() instanceof ArrowFunctionExpr - then - if func.getFunction().getBody() instanceof Expr - then result = ", (" + args + ") => ..." - else result = ", (" + args + ") => {...}" - else result = ", function(" + args + ") {...}" - ) -} + CommandCall() { this = command } -/** - * Gets a string concatenation of the parameter names in a function `func`. - */ -private string getCallbackArgs(DataFlow::FunctionNode func) { - result = - concat(int i | - i = [0 .. func.getNumParameter()] + /** + * Holds if the call is synchronous (e.g. `execFileSync`). + */ + predicate isSync() { command.isSync() } + + /** + * Gets an argument to this command execution that specifies the argument list to the command. + */ + DataFlow::Node getArgumentList() { result = command.getArgumentList() } + + /** + * Gets the callback (if it exists) for an async `exec`-like call. + */ + DataFlow::FunctionNode getCallback() { + not this.isSync() and result = getLastArgument().getALocalSource() + } + + /** + * Holds if the executed command execution has an argument list as a separate argument. + */ + predicate hasArgumentList() { exists(command.getArgumentList()) } + + /** + * Gets the data-flow node (if it exists) for a options argument for an `exec`-like call. + */ + DataFlow::Node getOptionsArg() { + exists(int n | + n >= 1 and + // if there is a command-list, then the options is at least the third argument. + (not exists(command.getArgumentList()) or n >= 2) and + // async exec calls can have a callback as their last call. + if command.isSync() or not exists(getCallback()) + then n < getNumArgument() + else n < getNumArgument() - 1 | - func.getParameter(i).getName(), ", " order by i + result = getArgument(n) ) + or + // Fallback in case normal API conventions are broken. + result = getAnArgument() and + result.getALocalSource() instanceof DataFlow::ObjectLiteralNode + } + + /** + * Gets the constant-string parts that are not part of the command itself. + * E.g. for a command execution `exec("/bin/cat foo bar")` this predicate will have result `"foo bar"`. + */ + string getNonCommandConstantString() { + if this.hasArgumentList() + then + result = + getConstantStringParts(getArgumentList() + .getALocalSource() + .(DataFlow::ArrayCreationNode) + .getElement(_)) + else + exists(string commandString | commandString = getConstantStringParts(getArgument(0)) | + result = commandString.suffix(1 + commandString.indexOf(" ", 0, 0)) + ) + } + + /** + * Holds if this command execution invokes the executeable `name`. + */ + bindingset[name] + predicate isACallTo(string name) { + if this.hasArgumentList() + then getArgument(0).mayHaveStringValue(name) + else + exists(string arg | arg = getConstantStringParts(getArgument(0)) | + arg.prefix(name.length()) = name + ) + } } /** - * Gets a string representation of the options argument `arg` from an exec-like call. + * Gets the constant string parts from a data-flow node. + * Either the string is some constant */ -private string printOptionsArg(DataFlow::Node arg) { - result = arg.asExpr().(VarAccess).getVariable().getName() +private string getConstantStringParts(DataFlow::Node node) { + node.mayHaveStringValue(result) or - // fall back to toString(), but ensure that we don't have dots in the middle. - result = arg.(DataFlow::ObjectLiteralNode).toString() and not result.regexpMatch(".*\\.\\..*") + result = node.(StringOps::ConcatenationRoot).getConstantStringParts() } /** * A call to a useless use of `cat`. */ -class UselessCat extends DataFlow::CallNode { - UselsesCatCandidates::UselessCatCandicate candidate; - +class UselessCat extends CommandCall { UselessCat() { - this = candidate and - // We can create an equivalent `fs.readFile` call. - exists(createReadFileCall(this)) and + this = command and + isACallTo(getACatExecuteable()) and // There is a file to read, and not just a pair of quotes. - candidate.getFileArgument().length() >= 3 and - // wildcards, pipes, redirections, and multiple files are OK. - // (The multiple files detection relies on the fileArgument not containing spaces anywhere) - not candidate.getFileArgument().regexpMatch(".*(\\*|\\||>|<| ).*") and + ( + not exists(PrettyPrintCatCall::createFileArgument(this)) + or + exists(string fileArg | fileArg = PrettyPrintCatCall::createFileArgument(this) | + fileArg.length() >= 3 + ) + ) and + // wildcards, pipes, redirections, other bash features, and multiple files (spaces) are OK. + not getNonCommandConstantString().regexpMatch(".*(\\*|\\||>|<| |\\$|&|,|\\`).*") and // Only acceptable option is "encoding", everything else is non-trivial to emulate with fs.readFile. ( - not exists(candidate.getOptionsArg()) + not exists(getOptionsArg()) or - forex(string prop | - exists(candidate.getOptionsArg().getALocalSource().getAPropertyWrite(prop)) - | + forex(string prop | exists(getOptionsArg().getALocalSource().getAPropertyWrite(prop)) | prop = "encoding" ) ) and // If there is a callback, then it must either have one or two arguments, or if there is a third argument it must be unused. ( - not exists(candidate.getCallback()) + not exists(getCallback()) or - exists(DataFlow::FunctionNode func | func = candidate.getCallback() | + exists(DataFlow::FunctionNode func | func = getCallback() | func.getNumParameter() = 1 or func.getNumParameter() = 2 or // `exec` can use 3 parameters, `readFile` can only use two, so it is OK to have a third parameter if it is unused, - func.getNumParameter() = 3 and not exists(SSA::definition(func.getParameter(2).getParameter())) + func.getNumParameter() = 3 and + not exists(SSA::definition(func.getParameter(2).getParameter())) ) + ) and + // The process returned by an async call is unused. + ( + isSync() + or + inVoidContext(this.getEnclosingExpr()) + or + this.getEnclosingExpr() = any(UnusedLocal v).getAnAssignedExpr() ) } } -module UselsesCatCandidates { +/** + * Gets a string used to call `cat`. + */ +string getACatExecuteable() { + result = "cat" or result = "/bin/cat" or result = "sudo cat" or result = "sudo /bin/cat" +} + +/** + * Predicates for creating an equivalent call to `fs.readFile` from a command execution of `cat`. + */ +module PrettyPrintCatCall { /** - * A candidate for a useless use of `cat`. - * This class describes the structure of a call to cat - * This class does not determine whether it is a useless call, or even if it is a call to `cat`. + * Create a string representation of an equivalent call to `fs.readFile` for a given command execution `cat`. */ - class UselessCatCandicate extends DataFlow::CallNode { - SystemCommandExecution command; - - UselessCatCandicate() { this = command } - - /** - * Holds if the call is synchronous (e.g. `execFileSync`). - */ - predicate isSync() { command.isSync() } + string createReadFileCall(UselessCat cat) { + exists(string sync, string extraArg, string callback | + (if cat.isSync() then sync = "Sync" else sync = "") and + ( + extraArg = ", " + createOptionsArg(cat.getOptionsArg()) + ")" + or + extraArg = "" and not exists(cat.getOptionsArg()) + ) and + ( + callback = createCallbackString(cat.getCallback()) + or + callback = "" and not exists(cat.getCallback()) + ) + | + result = + "fs.readFile" + sync + "(" + createFileArgument(cat).trim() + extraArg + callback + ")" + ) + } - /** - * Holds if the executed command execution has an argument list as a separate argument. - */ - predicate hasArgumentList() { exists(command.getArgumentList()) } + /** + * Create a string representation of the expression that determines what file is read by `cat`. + */ + string createFileArgument(CommandCall cat) { + if cat.hasArgumentList() + then + cat.getArgument(0).mayHaveStringValue(getACatExecuteable()) and + result = createFileThatIsReadFromCommandList(cat) + else result = createFileArgumentWithoutCat(cat.getArgument(0)) + } - /** - * Gets a string representation of the expression that determines what file is read by `cat`. - */ - string getFileArgument() { - if hasArgumentList() + /** + * Create a string representing the callback `func`. + */ + string createCallbackString(DataFlow::FunctionNode func) { + exists(string args | args = createCallbackArgs(func) | + if func.getFunction() instanceof ArrowFunctionExpr then - getArgument(0).mayHaveStringValue(cat()) and - result = getFileThatIsReadFromCommandList(this) - else result = getFileArgumentWithoutCat(getArgument(0)) - } + if func.getFunction().getBody() instanceof Expr + then result = ", (" + args + ") => ..." + else result = ", (" + args + ") => {...}" + else result = ", function(" + args + ") {...}" + ) + } - /** - * Gets the data-flow node (if it exists) for the options argument to the `exec`-like call. - */ - DataFlow::Node getOptionsArg() { - exists(int n | - n >= 1 and - // If there is a command-list, then the options is at least the third argument. - (not exists(command.getArgumentList()) or n >= 2) and - // async calls have a callback as their last call. - if this.isSync() then n < getNumArgument() else n < getNumArgument() - 1 + /** + * Create a string concatenation of the parameter names in a function `func`. + */ + private string createCallbackArgs(DataFlow::FunctionNode func) { + result = + concat(int i | + i = [0 .. func.getNumParameter()] | - result = getArgument(n) + func.getParameter(i).getName(), ", " order by i ) - } + } - /** - * Gets the callback (if it exists) for an async `exec` like call. - */ - DataFlow::FunctionNode getCallback() { - not this.isSync() and result = getLastArgument().getALocalSource() - } + /** + * Create a string representation of the options argument `arg` from an exec-like call. + */ + private string createOptionsArg(DataFlow::Node arg) { + result = arg.asExpr().(VarAccess).getVariable().getName() + or + // fall back to toString(), but ensure that we don't have dots in the middle. + result = arg.(DataFlow::ObjectLiteralNode).toString() and not result.regexpMatch(".*\\.\\..*") } /** @@ -169,7 +242,7 @@ module UselsesCatCandidates { concat(Expr leaf | leaf = root.getALeaf().asExpr() | - createLeafRepresentation(leaf), "+" order by leaf.getFirstToken().getIndex() + createLeafRepresentation(leaf), " + " order by leaf.getFirstToken().getIndex() ) or // Template string @@ -186,7 +259,7 @@ module UselsesCatCandidates { } /** - * Gets a string representing the expression needed to re-create the value for a leaf in a string-concatenation. + * Create a string representing the expression needed to re-create the value for a leaf in a string-concatenation. */ private string createLeafRepresentation(Expr e) { result = "\"" + e.getStringValue() + "\"" or @@ -194,7 +267,7 @@ module UselsesCatCandidates { } /** - * Gets a string representing the expression needed to re-create the value for an element of a template string. + * Create a string representing the expression needed to re-create the value for an element of a template string. */ private string createTemplateElementRepresentation(Expr e) { result = "${" + e.(VarAccess).getVariable().getName() + "}" @@ -203,18 +276,11 @@ module UselsesCatCandidates { } /** - * Gets a string used to call `cat`. - */ - private string cat() { - result = "cat" or result = "/bin/cat" or result = "sudo cat" or result = "sudo /bin/cat" - } - - /** - * Gets a string representing an expression that gets the file read by a call to `cat`. + * Create a string representing an expression that gets the file read by a call to `cat`. * The input `arg` is the node that determines the commandline where `cat` is invoked. */ - private string getFileArgumentWithoutCat(DataFlow::Node arg) { - exists(string cat | cat = cat() | + private string createFileArgumentWithoutCat(DataFlow::Node arg) { + exists(string cat | cat = getACatExecuteable() | exists(string command | arg.mayHaveStringValue(command) | command.prefix(cat.length()) = cat and result = "\"" + command.suffix(cat.length()).trim() + "\"" @@ -226,20 +292,20 @@ module UselsesCatCandidates { (if root.asExpr() instanceof TemplateLiteral then quote = "`" else quote = "\"") and root.getFirstLeaf().getStringValue().prefix(cat.length()) = cat and exists(string rawConcat | rawConcat = quote + printed.suffix(cat.length()).trim() | - result = getSimplifiedStringConcat(rawConcat) + result = createSimplifiedStringConcat(rawConcat) ) ) ) } /** - * Gets a simplified and equivalent string concatenation for a given string concatenation `str` + * Create a simplified and equivalent string concatenation for a given string concatenation `str` */ bindingset[str] - private string getSimplifiedStringConcat(string str) { + private string createSimplifiedStringConcat(string str) { // Remove an initial ""+ (e.g. in `""+file`) - if str.prefix(3) = "\"\"+" - then result = str.suffix(3) + if str.prefix(5) = "\"\" + " + then result = str.suffix(5) else // prettify `${newpath}` to just newpath if @@ -251,11 +317,11 @@ module UselsesCatCandidates { } /** - * Gets the file that is read for a call with an explicit command list (e.g. `child_process.execFile/execFileSync`). + * Create the file that is read for a call with an explicit command list (e.g. `child_process.execFile/execFileSync`). */ - string getFileThatIsReadFromCommandList(DataFlow::CallNode call) { + string createFileThatIsReadFromCommandList(CommandCall call) { exists(DataFlow::ArrayCreationNode array, DataFlow::Node element | - array = call.getArgument(1).(DataFlow::ArrayCreationNode) and + array = call.getArgumentList().(DataFlow::ArrayCreationNode) and array.getSize() = 1 and element = array.getElement(0) | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected index f4ebbd0848ba..1490b8abb015 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected @@ -1,6 +1,6 @@ readFile | uselesscat.js:10:1:10:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) | -| uselesscat.js:12:1:14:2 | exec("c ... ut);\\n}) | fs.readFile("/proc/"+id+"/status", function(err, out) {...}) | +| uselesscat.js:12:1:14:2 | exec("c ... ut);\\n}) | fs.readFile("/proc/" + id + "/status", function(err, out) {...}) | | uselesscat.js:16:1:16:29 | execSyn ... uinfo') | fs.readFileSync("/proc/cpuinfo") | | uselesscat.js:18:1:18:26 | execSyn ... path}`) | fs.readFileSync(newpath) | | uselesscat.js:32:1:32:34 | execSyn ... path}`) | fs.readFileSync(`foo/bar/${newpath}`) | @@ -12,12 +12,14 @@ readFile | uselesscat.js:76:1:76:39 | execFil ... xml' ]) | fs.readFileSync("pom.xml") | | uselesscat.js:79:1:79:46 | execFil ... opts) | fs.readFileSync("pom.xml", opts)) | | uselesscat.js:82:1:82:90 | execFil ... String) | fs.readFileSync("pom.xml", anOptsFileNameThatIsTooLongToBePrintedByToString)) | -| uselesscat.js:86:1:86:75 | execFil ... utf8'}) | fs.readFileSync("foo/"+newPath+"bar", {encoding: 'utf8'})) | -| uselesscat.js:88:1:88:35 | execSyn ... + foo) | fs.readFileSync("/proc/cpuinfo"+foo) | +| uselesscat.js:86:1:86:75 | execFil ... utf8'}) | fs.readFileSync("foo/" + newPath + "bar", {encoding: 'utf8'})) | +| uselesscat.js:88:1:88:35 | execSyn ... + foo) | fs.readFileSync("/proc/cpuinfo" + foo) | | uselesscat.js:90:1:90:50 | execFil ... th}` ]) | fs.readFileSync(`foo/bar/${newpath}`) | | uselesscat.js:94:1:94:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) | | uselesscat.js:96:1:96:53 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | | uselesscat.js:98:1:98:55 | exec("c ... h(out)) | fs.readFile("foo/bar", (err, out) => ...) | +| uselesscat.js:121:12:121:64 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | +| uselesscat.js:127:14:127:66 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | syncCommand | child_process-test.js:9:5:9:22 | cp.execSync("foo") | | child_process-test.js:11:5:11:26 | cp.exec ... ("foo") | @@ -65,6 +67,7 @@ syncCommand | uselesscat.js:90:1:90:50 | execFil ... th}` ]) | | uselesscat.js:92:1:92:46 | execFil ... th}` ]) | | uselesscat.js:100:1:100:56 | execFil ... ptions) | +| uselesscat.js:104:1:104:31 | execFil ... cat` ]) | #select | False negative | uselesscat.js:54:42:54:69 | // NOT ... lagged] | -| False negative | uselesscat.js:84:118:84:144 | // NOT ... lagged] | +| False positive | uselesscat.js:44:37:44:85 | // OK [ ... le read | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql index f9a4fd2ea8c2..a287a57ff551 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql @@ -17,7 +17,7 @@ where ) select msg, comment -query string readFile(UselessCat cat) { result = createReadFileCall(cat) } +query string readFile(UselessCat cat) { result = PrettyPrintCatCall::createReadFileCall(cat) } query SystemCommandExecution syncCommand() { result.isSync() diff --git a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js index c860d21f6549..d64058c010df 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js @@ -41,7 +41,7 @@ execSync(`cat ${newpath} > ${destpath}`).toString(); // OK. execSync(`cat ${files.join(' ')} > ${outFile}`); // OK -execSync(`cat ${files.join(' ')}`); // OK - not just a simple file read +execSync(`cat ${files.join(' ')}`); // OK [but flagged] - not just a simple file read exec("cat /proc/cpuinfo | grep name"); // OK - pipes @@ -81,7 +81,7 @@ execFileSync('/bin/cat', [ 'pom.xml' ], opts); // NOT OK var anOptsFileNameThatIsTooLongToBePrintedByToString = {encoding: 'utf8'}; execFileSync('/bin/cat', [ 'pom.xml' ], anOptsFileNameThatIsTooLongToBePrintedByToString); // NOT OK -execFileSync('/bin/cat', [ 'pom.xml' ], {encoding: 'someEncodingValueThatIsCompletelyBogusAndTooLongForToString'}); // NOT OK [but not flagged] +execFileSync('/bin/cat', [ 'pom.xml' ], {encoding: 'someEncodingValueThatIsCompletelyBogusAndTooLongForToString'}); // NOT OK execFileSync('/bin/cat', [ "foo/" + newPath + "bar" ], {encoding: 'utf8'}); // NOT OK @@ -97,4 +97,38 @@ exec("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK exec("cat foo/bar", (err, out) => doSomethingWith(out)); // NOT OK -execFileSync('/bin/cat', [ 'pom.xml' ], unknownOptions); // OK - unknown options. \ No newline at end of file +execFileSync('/bin/cat', [ 'pom.xml' ], unknownOptions); // OK - unknown options. + +exec("node foo/bar", (err, out) => doSomethingWith(out)); // OK - Not a call to cat + +execFileSync('node', [ `cat` ]); // OK - not a call to cat + +exec("cat foo/bar&", function (err, out) {}); // OK - contains & +exec("cat foo/bar,", function (err, out) {}); // OK - contains , +exec("cat foo/bar$", function (err, out) {}); // OK - contains $ +exec("cat foo/bar`", function (err, out) {}); // OK - contains ` + +spawn('cat', { stdio: ['pipe', stdin, 'inherit'] }); // OK - Non trivial use. (But weird API use.) + +(function () { + const cat = spawn('cat', [filename]); // OK - non trivial use. + cat.stdout.on('data', (data) => { + res.write(data); + }); + cat.stdout.on('end', () => res.end()); +})(); + +var dead = exec("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK + +var notDead = exec("cat foo/bar", (err, out) => {console.log(out)}); // OK +console.log(notDead); + +(function () { + var dead = exec("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK + + someCall( + exec("cat foo/bar", (err, out) => {console.log(out)}) // OK - non-trivial use of returned proccess. + ); + + return exec("cat foo/bar", (err, out) => {console.log(out)}); // OK - non-trivial use of returned proccess. +})(); \ No newline at end of file From 44db0f4e5df8f0d1ae1d0c578583474c13487dde Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 21 Feb 2020 15:39:49 +0100 Subject: [PATCH 16/37] better printing of the options arg --- .../javascript/security/UselessUseOfCat.qll | 14 ++++++++++---- .../Security/CWE-078/UselessUseOfCat.expected | 15 +++++++++------ .../query-tests/Security/CWE-078/uselesscat.js | 6 +++++- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index 2633c84ce70c..1d2a13d960d0 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -59,9 +59,9 @@ private class CommandCall extends DataFlow::InvokeNode { } /** - * Gets the constant-string parts that are not part of the command itself. - * E.g. for a command execution `exec("/bin/cat foo bar")` this predicate will have result `"foo bar"`. - */ + * Gets the constant-string parts that are not part of the command itself. + * E.g. for a command execution `exec("/bin/cat foo bar")` this predicate will have result `"foo bar"`. + */ string getNonCommandConstantString() { if this.hasArgumentList() then @@ -168,7 +168,13 @@ module PrettyPrintCatCall { exists(string sync, string extraArg, string callback | (if cat.isSync() then sync = "Sync" else sync = "") and ( - extraArg = ", " + createOptionsArg(cat.getOptionsArg()) + ")" + exists(cat.getOptionsArg()) and + ( + extraArg = ", " + createOptionsArg(cat.getOptionsArg()) + or + not exists(createOptionsArg(cat.getOptionsArg())) and + extraArg = ", ..." + ) or extraArg = "" and not exists(cat.getOptionsArg()) ) and diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected index 1490b8abb015..c0de47ed9957 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected @@ -4,15 +4,16 @@ readFile | uselesscat.js:16:1:16:29 | execSyn ... uinfo') | fs.readFileSync("/proc/cpuinfo") | | uselesscat.js:18:1:18:26 | execSyn ... path}`) | fs.readFileSync(newpath) | | uselesscat.js:32:1:32:34 | execSyn ... path}`) | fs.readFileSync(`foo/bar/${newpath}`) | -| uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | fs.readFileSync(`foo/bar/${newpath}`, {encoding: 'utf8'})) | +| uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | fs.readFileSync(`foo/bar/${newpath}`, {encoding: 'utf8'}) | | uselesscat.js:51:9:51:31 | execSyn ... + file) | fs.readFileSync(file) | | uselesscat.js:59:1:62:2 | execFil ... ut);\\n}) | fs.readFile("pom.xml", function(error, stdout, stderr) {...}) | -| uselesscat.js:69:1:72:2 | execFil ... ut);\\n}) | fs.readFile("pom.xml", {encoding: 'utf8'}), function(error, stdout, stderr) {...}) | -| uselesscat.js:74:1:74:60 | execFil ... utf8'}) | fs.readFileSync("pom.xml", {encoding: 'utf8'})) | +| uselesscat.js:69:1:72:2 | execFil ... ut);\\n}) | fs.readFile("pom.xml", {encoding: 'utf8'}, function(error, stdout, stderr) {...}) | +| uselesscat.js:74:1:74:60 | execFil ... utf8'}) | fs.readFileSync("pom.xml", {encoding: 'utf8'}) | | uselesscat.js:76:1:76:39 | execFil ... xml' ]) | fs.readFileSync("pom.xml") | -| uselesscat.js:79:1:79:46 | execFil ... opts) | fs.readFileSync("pom.xml", opts)) | -| uselesscat.js:82:1:82:90 | execFil ... String) | fs.readFileSync("pom.xml", anOptsFileNameThatIsTooLongToBePrintedByToString)) | -| uselesscat.js:86:1:86:75 | execFil ... utf8'}) | fs.readFileSync("foo/" + newPath + "bar", {encoding: 'utf8'})) | +| uselesscat.js:79:1:79:46 | execFil ... opts) | fs.readFileSync("pom.xml", opts) | +| uselesscat.js:82:1:82:90 | execFil ... String) | fs.readFileSync("pom.xml", anOptsFileNameThatIsTooLongToBePrintedByToString) | +| uselesscat.js:84:1:84:115 | execFil ... ring'}) | fs.readFileSync("pom.xml", ...) | +| uselesscat.js:86:1:86:75 | execFil ... utf8'}) | fs.readFileSync("foo/" + newPath + "bar", {encoding: 'utf8'}) | | uselesscat.js:88:1:88:35 | execSyn ... + foo) | fs.readFileSync("/proc/cpuinfo" + foo) | | uselesscat.js:90:1:90:50 | execFil ... th}` ]) | fs.readFileSync(`foo/bar/${newpath}`) | | uselesscat.js:94:1:94:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) | @@ -20,6 +21,7 @@ readFile | uselesscat.js:98:1:98:55 | exec("c ... h(out)) | fs.readFile("foo/bar", (err, out) => ...) | | uselesscat.js:121:12:121:64 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | | uselesscat.js:127:14:127:66 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | +| uselesscat.js:136:17:138:2 | execSyn ... tf8'\\n}) | fs.readFileSync("/etc/dnsmasq.conf", ...) | syncCommand | child_process-test.js:9:5:9:22 | cp.execSync("foo") | | child_process-test.js:11:5:11:26 | cp.exec ... ("foo") | @@ -68,6 +70,7 @@ syncCommand | uselesscat.js:92:1:92:46 | execFil ... th}` ]) | | uselesscat.js:100:1:100:56 | execFil ... ptions) | | uselesscat.js:104:1:104:31 | execFil ... cat` ]) | +| uselesscat.js:136:17:138:2 | execSyn ... tf8'\\n}) | #select | False negative | uselesscat.js:54:42:54:69 | // NOT ... lagged] | | False positive | uselesscat.js:44:37:44:85 | // OK [ ... le read | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js index d64058c010df..c50e0fc1f5ee 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js @@ -131,4 +131,8 @@ console.log(notDead); ); return exec("cat foo/bar", (err, out) => {console.log(out)}); // OK - non-trivial use of returned proccess. -})(); \ No newline at end of file +})(); + +const stdout2 = execSync('cat /etc/dnsmasq.conf', { // NOT OK. + encoding: 'utf8' +}); \ No newline at end of file From 75c1852ee49e04aa5ddd9d57c5b9613ebbf2628b Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 24 Feb 2020 11:58:59 +0100 Subject: [PATCH 17/37] doc changes from review Co-Authored-By: Esben Sparre Andreasen --- .../ql/src/semmle/javascript/security/UselessUseOfCat.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index 1d2a13d960d0..cccf7937c558 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -8,7 +8,7 @@ import Declarations.UnusedVariable /** * A call that executes a system command. - * This class provide utility predicates for reasoning about command execution calls. + * This class provides utility predicates for reasoning about command execution calls. */ private class CommandCall extends DataFlow::InvokeNode { SystemCommandExecution command; @@ -38,7 +38,7 @@ private class CommandCall extends DataFlow::InvokeNode { predicate hasArgumentList() { exists(command.getArgumentList()) } /** - * Gets the data-flow node (if it exists) for a options argument for an `exec`-like call. + * Gets the data-flow node (if it exists) for an options argument for an `exec`-like call. */ DataFlow::Node getOptionsArg() { exists(int n | From 473787a4268a5e90b4a653ee8bcdb3681f7867ff Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 24 Feb 2020 12:59:20 +0100 Subject: [PATCH 18/37] refactor the getOptionsArg predicate into the SystemCommandExecution class --- .../ql/src/semmle/javascript/Concepts.qll | 5 ++++ .../javascript/frameworks/NodeJSLib.qll | 12 ++++++++ .../semmle/javascript/frameworks/ShellJS.qll | 7 +++++ .../frameworks/SystemCommandExecutors.qll | 29 ++++++++++++------- .../javascript/security/UselessUseOfCat.qll | 16 +--------- .../Security/CWE-078/UselessUseOfCat.expected | 17 +++++++++++ .../Security/CWE-078/UselessUseOfCat.ql | 4 +++ 7 files changed, 65 insertions(+), 25 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/Concepts.qll b/javascript/ql/src/semmle/javascript/Concepts.qll index 4fef87f85854..798747af8b8e 100644 --- a/javascript/ql/src/semmle/javascript/Concepts.qll +++ b/javascript/ql/src/semmle/javascript/Concepts.qll @@ -25,6 +25,11 @@ abstract class SystemCommandExecution extends DataFlow::Node { /** Holds if the command execution happens synchronously. */ abstract predicate isSync(); + + /** + * Gets the data-flow node (if it exists) for an options argument. + */ + abstract DataFlow::Node getOptionsArg(); } /** diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll index 95600e072f66..2eed3f1586b2 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll @@ -627,6 +627,18 @@ module NodeJSLib { override predicate isSync() { "Sync" = methodName.suffix(methodName.length() - 4) } + + override DataFlow::Node getOptionsArg() { + not result.getALocalSource() instanceof DataFlow::FunctionNode and // looks like callback + not result.getALocalSource() instanceof DataFlow::ArrayCreationNode and // looks like argumentlist + not result = getArgument(0) and + // fork/spawn and all sync methos always has options as the last argument + if methodName.regexpMatch("fork.*") or methodName.regexpMatch("spawn.*") or methodName.regexpMatch(".*Sync") then + result = getLastArgument() + else + // the rest (exec/execFile) has the options argument as their second last. + result = getArgument(this.getNumArgument() - 2) + } } /** diff --git a/javascript/ql/src/semmle/javascript/frameworks/ShellJS.qll b/javascript/ql/src/semmle/javascript/frameworks/ShellJS.qll index e459fe9089c7..6944b7e74a38 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ShellJS.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ShellJS.qll @@ -162,6 +162,13 @@ module ShellJS { override predicate isShellInterpreted(DataFlow::Node arg) { arg = getACommandArgument() } override predicate isSync() {none ()} + + override DataFlow::Node getOptionsArg() { + result = getLastArgument() and + not result = getArgument(0) and + not result.getALocalSource() instanceof DataFlow::FunctionNode and // looks like callback + not result.getALocalSource() instanceof DataFlow::ArrayCreationNode // looks like argumentlist + } } /** diff --git a/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll b/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll index e0664f93dd1a..466ed29ae0a2 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll @@ -7,6 +7,7 @@ import javascript private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::InvokeNode { int cmdArg; + int optionsArg; boolean shell; boolean sync; @@ -14,9 +15,9 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I SystemCommandExecutors() { exists(string mod, DataFlow::SourceNode callee | exists(string method | - mod = "cross-spawn" and method = "sync" and cmdArg = 0 and shell = false + mod = "cross-spawn" and method = "sync" and cmdArg = 0 and shell = false and optionsArg = -1 or - mod = "execa" and + mod = "execa" and optionsArg = -1 and ( shell = false and ( @@ -40,21 +41,21 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I ( shell = false and ( - mod = "cross-spawn" and cmdArg = 0 + mod = "cross-spawn" and cmdArg = 0 and optionsArg = -1 or - mod = "cross-spawn-async" and cmdArg = 0 + mod = "cross-spawn-async" and cmdArg = 0 and optionsArg = -1 or - mod = "exec-async" and cmdArg = 0 + mod = "exec-async" and cmdArg = 0 and optionsArg = -1 or - mod = "execa" and cmdArg = 0 + mod = "execa" and cmdArg = 0 and optionsArg = -1 ) or shell = true and ( - mod = "exec" and + mod = "exec" and optionsArg = -2 and cmdArg = 0 or - mod = "remote-exec" and cmdArg = 1 + mod = "remote-exec" and cmdArg = 1 and optionsArg = -1 ) ) and callee = DataFlow::moduleImport(mod) @@ -69,8 +70,16 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I arg = getACommandArgument() and shell = true } - override predicate isSync() { - sync = true + override predicate isSync() { sync = true } + + override DataFlow::Node getOptionsArg() { + (if optionsArg < 0 then + result = getArgument(getNumArgument() - optionsArg) + else + result = getArgument(optionsArg)) and + not result = getArgument(0) and + not result.getALocalSource() instanceof DataFlow::FunctionNode and // looks like callback + not result.getALocalSource() instanceof DataFlow::ArrayCreationNode // looks like argumentlist } } diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index cccf7937c558..b40c0cc3a893 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -41,21 +41,7 @@ private class CommandCall extends DataFlow::InvokeNode { * Gets the data-flow node (if it exists) for an options argument for an `exec`-like call. */ DataFlow::Node getOptionsArg() { - exists(int n | - n >= 1 and - // if there is a command-list, then the options is at least the third argument. - (not exists(command.getArgumentList()) or n >= 2) and - // async exec calls can have a callback as their last call. - if command.isSync() or not exists(getCallback()) - then n < getNumArgument() - else n < getNumArgument() - 1 - | - result = getArgument(n) - ) - or - // Fallback in case normal API conventions are broken. - result = getAnArgument() and - result.getALocalSource() instanceof DataFlow::ObjectLiteralNode + result = command.getOptionsArg() } /** diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected index c0de47ed9957..f78680f30edb 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected @@ -71,6 +71,23 @@ syncCommand | uselesscat.js:100:1:100:56 | execFil ... ptions) | | uselesscat.js:104:1:104:31 | execFil ... cat` ]) | | uselesscat.js:136:17:138:2 | execSyn ... tf8'\\n}) | +options +| child_process-test.js:53:5:53:59 | cp.spaw ... cmd])) | child_process-test.js:53:25:53:58 | ['/C', ... , cmd]) | +| child_process-test.js:54:5:54:50 | cp.spaw ... t(cmd)) | child_process-test.js:54:25:54:49 | ['/C', ... at(cmd) | +| child_process-test.js:64:3:64:21 | cp.spawn(cmd, args) | child_process-test.js:64:17:64:20 | args | +| uselesscat.js:28:1:28:39 | execSyn ... 1000}) | uselesscat.js:28:28:28:38 | {uid: 1000} | +| uselesscat.js:30:1:30:64 | exec('c ... t) { }) | uselesscat.js:30:26:30:38 | { cwd: './' } | +| uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | uselesscat.js:34:36:34:53 | {encoding: 'utf8'} | +| uselesscat.js:36:1:36:77 | execSyn ... utf8'}) | uselesscat.js:36:36:36:76 | { uid: ... 'utf8'} | +| uselesscat.js:69:1:72:2 | execFil ... ut);\\n}) | uselesscat.js:69:38:69:55 | {encoding: 'utf8'} | +| uselesscat.js:74:1:74:60 | execFil ... utf8'}) | uselesscat.js:74:42:74:59 | {encoding: 'utf8'} | +| uselesscat.js:79:1:79:46 | execFil ... opts) | uselesscat.js:79:42:79:45 | opts | +| uselesscat.js:82:1:82:90 | execFil ... String) | uselesscat.js:82:42:82:89 | anOptsF ... oString | +| uselesscat.js:84:1:84:115 | execFil ... ring'}) | uselesscat.js:84:42:84:114 | {encodi ... tring'} | +| uselesscat.js:86:1:86:75 | execFil ... utf8'}) | uselesscat.js:86:57:86:74 | {encoding: 'utf8'} | +| uselesscat.js:100:1:100:56 | execFil ... ptions) | uselesscat.js:100:42:100:55 | unknownOptions | +| uselesscat.js:111:1:111:51 | spawn(' ... it'] }) | uselesscat.js:111:14:111:50 | { stdio ... rit'] } | +| uselesscat.js:136:17:138:2 | execSyn ... tf8'\\n}) | uselesscat.js:136:51:138:1 | { // NO ... utf8'\\n} | #select | False negative | uselesscat.js:54:42:54:69 | // NOT ... lagged] | | False positive | uselesscat.js:44:37:44:85 | // OK [ ... le read | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql index a287a57ff551..a70f53016bda 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.ql @@ -21,4 +21,8 @@ query string readFile(UselessCat cat) { result = PrettyPrintCatCall::createReadF query SystemCommandExecution syncCommand() { result.isSync() +} + +query DataFlow::Node options(SystemCommandExecution sys) { + result = sys.getOptionsArg() } \ No newline at end of file From a768e937f0c64052881f4c4ef389507dca5ac0e8 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 24 Feb 2020 13:08:50 +0100 Subject: [PATCH 19/37] complete qldoc --- .../ql/src/semmle/javascript/security/UselessUseOfCat.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index b40c0cc3a893..80a0eb6398d1 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -78,7 +78,7 @@ private class CommandCall extends DataFlow::InvokeNode { /** * Gets the constant string parts from a data-flow node. - * Either the string is some constant + * Either the result is a constant string value that the node can hold, or the node is a string-concatenation and the result is the string parts from the concatenation. */ private string getConstantStringParts(DataFlow::Node node) { node.mayHaveStringValue(result) From 051de247b0fa54119453e36b348a870d696c9cb5 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 24 Feb 2020 13:11:30 +0100 Subject: [PATCH 20/37] change regexpMatch to regexpFind --- .../ql/src/semmle/javascript/security/UselessUseOfCat.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index 80a0eb6398d1..a19fce9027f3 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -102,7 +102,7 @@ class UselessCat extends CommandCall { ) ) and // wildcards, pipes, redirections, other bash features, and multiple files (spaces) are OK. - not getNonCommandConstantString().regexpMatch(".*(\\*|\\||>|<| |\\$|&|,|\\`).*") and + not exists(getNonCommandConstantString().regexpFind("\\*|\\||>|<| |\\$|&|,|\\`", _, _)) and // Only acceptable option is "encoding", everything else is non-trivial to emulate with fs.readFile. ( not exists(getOptionsArg()) From fb94af976414aec823514f8d1ee3e1abbf507e1d Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 24 Feb 2020 13:18:15 +0100 Subject: [PATCH 21/37] remove the last dependency on PrettyPrinting --- .../semmle/javascript/security/UselessUseOfCat.qll | 11 ++++------- .../Security/CWE-078/UselessUseOfCat.expected | 2 +- .../test/query-tests/Security/CWE-078/uselesscat.js | 8 ++++++-- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index a19fce9027f3..bcdaa2eff4c3 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -93,13 +93,10 @@ class UselessCat extends CommandCall { UselessCat() { this = command and isACallTo(getACatExecuteable()) and - // There is a file to read, and not just a pair of quotes. - ( - not exists(PrettyPrintCatCall::createFileArgument(this)) - or - exists(string fileArg | fileArg = PrettyPrintCatCall::createFileArgument(this) | - fileArg.length() >= 3 - ) + // There is a file to read, it's not just spawning `cat`. + not ( + not exists(getArgumentList()) and + getArgument(0).mayHaveStringValue(getACatExecuteable()) ) and // wildcards, pipes, redirections, other bash features, and multiple files (spaces) are OK. not exists(getNonCommandConstantString().regexpFind("\\*|\\||>|<| |\\$|&|,|\\`", _, _)) and diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected index f78680f30edb..9e53b8cc422a 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected @@ -45,7 +45,7 @@ syncCommand | tst_shell-command-injection-from-environment.js:5:2:5:54 | cp.exec ... temp")) | | uselesscat.js:16:1:16:29 | execSyn ... uinfo') | | uselesscat.js:18:1:18:26 | execSyn ... path}`) | -| uselesscat.js:20:1:20:50 | child_p ... wc -l') | +| uselesscat.js:20:1:20:36 | execSyn ... wc -l') | | uselesscat.js:22:1:22:38 | execSyn ... o/bar') | | uselesscat.js:24:1:24:35 | execSyn ... o/bar`) | | uselesscat.js:28:1:28:39 | execSyn ... 1000}) | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js index c50e0fc1f5ee..86647b9fc296 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js @@ -17,7 +17,7 @@ execSync('cat /proc/cpuinfo').toString(); // NOT OK. execSync(`cat ${newpath}`) // NOT OK -child_process.execSync('cat package.json | wc -l'); // OK - pipes! +execSync('cat package.json | wc -l'); // OK - pipes! execSync('cat /proc/cpuinfo /foo/bar').toString(); // OK multiple files. @@ -135,4 +135,8 @@ console.log(notDead); const stdout2 = execSync('cat /etc/dnsmasq.conf', { // NOT OK. encoding: 'utf8' -}); \ No newline at end of file +}); + +exec('/bin/cat', function (e, s) {}); // OK + +spawn("cat") // OK \ No newline at end of file From a779ae58a88b3307eec6dd9e26b7bc27a7476c22 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 24 Feb 2020 14:03:41 +0100 Subject: [PATCH 22/37] add qhelp --- .../Security/CWE-078/UselessUseOfCat.qhelp | 47 +++++++++++++------ .../CWE-078/examples/useless-cat-fixed.js | 5 ++ .../Security/CWE-078/examples/useless-cat.js | 5 ++ 3 files changed, 42 insertions(+), 15 deletions(-) create mode 100644 javascript/ql/src/Security/CWE-078/examples/useless-cat-fixed.js create mode 100644 javascript/ql/src/Security/CWE-078/examples/useless-cat.js diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp index 452f630712d2..85a7a2e49494 100644 --- a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp @@ -3,28 +3,45 @@ "qhelp.dtd"> -

    - Useless use of cat -

    - - +

    Using the unix command cat to simply read a file is a +unnecessarily complex way to achieve something that can be done simpler and +safer using the Node.js fs.readFile API. +

    +

    +The use of cat for simple file reads leads to code that is +unportable, inefficient, complex, and can lead to subtle bugs or even +security vulnerabilities. +

     -

    - TODO: This is a placeholder -

    - +

    +Use fs.readFile or fs.readFileSync to read files +from the file system. +

     -

    -

    + +

    The following example shows code that reads a file using cat:

    + + + +

    The code in the example will break if the input name contain +special characters (including space), the code does not work on windows, +and if the input is user controlled a command injection attack can happen.

    + +

    To avoid these potential issues the fs.readFile API can be +used instead:

    + + +     -
  • - OWASP: - Command Injection. -
    • +
  • +OWASP: Command Injection. +Node.js: File System API. +
    • +     diff --git a/javascript/ql/src/Security/CWE-078/examples/useless-cat-fixed.js b/javascript/ql/src/Security/CWE-078/examples/useless-cat-fixed.js new file mode 100644 index 000000000000..225fa1f58699 --- /dev/null +++ b/javascript/ql/src/Security/CWE-078/examples/useless-cat-fixed.js @@ -0,0 +1,5 @@ +var fs = require('fs'); + +module.exports = function (name) { + return fs.readFileSync(name).toString(); +}; diff --git a/javascript/ql/src/Security/CWE-078/examples/useless-cat.js b/javascript/ql/src/Security/CWE-078/examples/useless-cat.js new file mode 100644 index 000000000000..78f099d0e4c2 --- /dev/null +++ b/javascript/ql/src/Security/CWE-078/examples/useless-cat.js @@ -0,0 +1,5 @@ +var child_process = require('child_process'); + +module.exports = function (name) { + return child_process.execSync("cat " + name).toString(); +}; From b72404dc9990c56896e1ae06707d4b20ba519aa6 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 24 Feb 2020 14:07:49 +0100 Subject: [PATCH 23/37] add change note --- change-notes/1.24/analysis-javascript.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/change-notes/1.24/analysis-javascript.md b/change-notes/1.24/analysis-javascript.md index 47fb2dc590be..eaebf0c12510 100644 --- a/change-notes/1.24/analysis-javascript.md +++ b/change-notes/1.24/analysis-javascript.md @@ -39,6 +39,8 @@ | Missing await (`js/missing-await`) | correctness | Highlights expressions that operate directly on a promise object in a nonsensical way, instead of awaiting its result. Results are shown on LGTM by default. | | Prototype pollution in utility function (`js/prototype-pollution-utility`) | security, external/cwe/cwe-400, external/cwe/cwe-471 | Highlights recursive copying operations that are susceptible to prototype pollution. Results are shown on LGTM by default. | | Unsafe jQuery plugin (`js/unsafe-jquery-plugin`) | Highlights potential XSS vulnerabilities in unsafely designed jQuery plugins. Results are shown on LGTM by default. | +| Useless use of cat (`js/useless-use-of-cat`) | correctness, security, maintainability | Highlights command executions of cat where the fs API should be used instead. Results are shown on LGTM by default. | + ## Changes to existing queries From b20e8520f6cfe15fe5217daeaaea37450b987817 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 24 Feb 2020 14:52:08 +0100 Subject: [PATCH 24/37] add default message if not pretty printed call can be created --- .../ql/src/Security/CWE-078/UselessUseOfCat.ql | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql index 4f080711719b..9f28c5fb0cf4 100644 --- a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql @@ -14,10 +14,12 @@ import javascript import semmle.javascript.security.UselessUseOfCat import semmle.javascript.RestrictedLocations - from UselessCat cat, string message -where - message = " Can be replaced with: " + PrettyPrintCatCall::createReadFileCall(cat) - or - not exists(PrettyPrintCatCall::createReadFileCall(cat)) and message = "" +where + message = " Can be replaced with: " + PrettyPrintCatCall::createReadFileCall(cat) + or + not exists(PrettyPrintCatCall::createReadFileCall(cat)) and + if cat.isSync() + then message = " Can be replaced with a call to fs.readFileSync(..)." + else message = " Can be replaced with a call to fs.readFile(..)." select cat.asExpr().(FirstLineOf), "Useless use of `cat`." + message \ No newline at end of file From afd6ea262840811512b0b5a7fe62b3adbc342d9c Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 24 Feb 2020 17:54:29 +0100 Subject: [PATCH 25/37] small correction in doc + autoformat --- .../ql/src/semmle/javascript/security/UselessUseOfCat.qll | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index bcdaa2eff4c3..29b18ecd2c94 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -40,9 +40,7 @@ private class CommandCall extends DataFlow::InvokeNode { /** * Gets the data-flow node (if it exists) for an options argument for an `exec`-like call. */ - DataFlow::Node getOptionsArg() { - result = command.getOptionsArg() - } + DataFlow::Node getOptionsArg() { result = command.getOptionsArg() } /** * Gets the constant-string parts that are not part of the command itself. @@ -78,7 +76,7 @@ private class CommandCall extends DataFlow::InvokeNode { /** * Gets the constant string parts from a data-flow node. - * Either the result is a constant string value that the node can hold, or the node is a string-concatenation and the result is the string parts from the concatenation. + * Either the result is a constant string value that the node can hold, or the node is a string-concatenation and the result is the string parts from the concatenation. */ private string getConstantStringParts(DataFlow::Node node) { node.mayHaveStringValue(result) @@ -108,7 +106,7 @@ class UselessCat extends CommandCall { prop = "encoding" ) ) and - // If there is a callback, then it must either have one or two arguments, or if there is a third argument it must be unused. + // If there is a callback, then it must either have one or two parameters, or if there is a third parameter it must be unused. ( not exists(getCallback()) or From d540caecdddc3b6bff300f5be6f2691394c954df Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 25 Feb 2020 10:04:51 +0100 Subject: [PATCH 26/37] Apply suggestions from code review Co-Authored-By: Esben Sparre Andreasen --- .../ql/src/semmle/javascript/security/UselessUseOfCat.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index 29b18ecd2c94..1d6480f3b094 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -97,7 +97,7 @@ class UselessCat extends CommandCall { getArgument(0).mayHaveStringValue(getACatExecuteable()) ) and // wildcards, pipes, redirections, other bash features, and multiple files (spaces) are OK. - not exists(getNonCommandConstantString().regexpFind("\\*|\\||>|<| |\\$|&|,|\\`", _, _)) and + not exists(getNonCommandConstantString().regexpFind("\\*|\\||>|<| |\\$|&|,|\\`| ", _, _)) and // Only acceptable option is "encoding", everything else is non-trivial to emulate with fs.readFile. ( not exists(getOptionsArg()) @@ -135,7 +135,7 @@ class UselessCat extends CommandCall { * Gets a string used to call `cat`. */ string getACatExecuteable() { - result = "cat" or result = "/bin/cat" or result = "sudo cat" or result = "sudo /bin/cat" + result = "cat" or result = "/bin/cat" } /** From 87d283aa6c7089e1869c367c1c3d9941a326f976 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 25 Feb 2020 10:50:59 +0100 Subject: [PATCH 27/37] add tests for third party command execution libraries (and two small fixes) --- .../frameworks/SystemCommandExecutors.qll | 5 ++-- .../javascript/security/UselessUseOfCat.qll | 6 ++--- .../Security/CWE-078/UselessUseOfCat.expected | 19 ++++++++++++++ .../Security/CWE-078/uselesscat.js | 26 ++++++++++++++++++- 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll b/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll index 466ed29ae0a2..89a0929a182b 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll @@ -70,14 +70,15 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I arg = getACommandArgument() and shell = true } + override DataFlow::Node getArgumentList() { shell = false and result = getArgument(1) } + override predicate isSync() { sync = true } override DataFlow::Node getOptionsArg() { (if optionsArg < 0 then - result = getArgument(getNumArgument() - optionsArg) + result = getArgument(getNumArgument() + optionsArg) and getNumArgument() + optionsArg > cmdArg else result = getArgument(optionsArg)) and - not result = getArgument(0) and not result.getALocalSource() instanceof DataFlow::FunctionNode and // looks like callback not result.getALocalSource() instanceof DataFlow::ArrayCreationNode // looks like argumentlist } diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index 1d6480f3b094..7cd66f7981b9 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -21,9 +21,9 @@ private class CommandCall extends DataFlow::InvokeNode { predicate isSync() { command.isSync() } /** - * Gets an argument to this command execution that specifies the argument list to the command. + * Gets a list that specifies the arguments given to the command. */ - DataFlow::Node getArgumentList() { result = command.getArgumentList() } + DataFlow::ArrayCreationNode getArgumentList() { result = command.getArgumentList().getALocalSource() } /** * Gets the callback (if it exists) for an async `exec`-like call. @@ -35,7 +35,7 @@ private class CommandCall extends DataFlow::InvokeNode { /** * Holds if the executed command execution has an argument list as a separate argument. */ - predicate hasArgumentList() { exists(command.getArgumentList()) } + predicate hasArgumentList() { exists(getArgumentList()) } /** * Gets the data-flow node (if it exists) for an options argument for an `exec`-like call. diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected index 9e53b8cc422a..4f2dae3d7b1a 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected +++ b/javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected @@ -22,6 +22,18 @@ readFile | uselesscat.js:121:12:121:64 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | | uselesscat.js:127:14:127:66 | exec("c ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | | uselesscat.js:136:17:138:2 | execSyn ... tf8'\\n}) | fs.readFileSync("/etc/dnsmasq.conf", ...) | +| uselesscat.js:146:1:146:61 | shelljs ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | +| uselesscat.js:147:1:147:47 | shelljs ... utf8'}) | fs.readFile("foo/bar", {encoding: 'utf8'}) | +| uselesscat.js:148:1:148:81 | shelljs ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | +| uselesscat.js:151:1:151:48 | cspawn( ... tf8' }) | fs.readFile("foo/bar", { encoding: 'utf8' }) | +| uselesscat.js:152:1:152:82 | cspawn( ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | +| uselesscat.js:153:1:153:60 | cspawn( ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | +| uselesscat.js:154:1:154:26 | cspawn( ... /bar']) | fs.readFile("foo/bar") | +| uselesscat.js:158:16:158:46 | cspawn. ... /bar']) | fs.readFileSync("foo/bar") | +| uselesscat.js:159:16:159:68 | cspawn. ... tf8' }) | fs.readFileSync("foo/bar", { encoding: 'utf8' }) | +| uselesscat.js:162:1:162:56 | execmod ... (out)}) | fs.readFile("foo/bar", (err, out) => {...}) | +| uselesscat.js:163:1:163:42 | execmod ... utf8'}) | fs.readFile("foo/bar") | +| uselesscat.js:164:1:164:76 | execmod ... (out)}) | fs.readFile("foo/bar", {encoding: 'utf8'}, (err, out) => {...}) | syncCommand | child_process-test.js:9:5:9:22 | cp.execSync("foo") | | child_process-test.js:11:5:11:26 | cp.exec ... ("foo") | @@ -71,6 +83,8 @@ syncCommand | uselesscat.js:100:1:100:56 | execFil ... ptions) | | uselesscat.js:104:1:104:31 | execFil ... cat` ]) | | uselesscat.js:136:17:138:2 | execSyn ... tf8'\\n}) | +| uselesscat.js:158:16:158:46 | cspawn. ... /bar']) | +| uselesscat.js:159:16:159:68 | cspawn. ... tf8' }) | options | child_process-test.js:53:5:53:59 | cp.spaw ... cmd])) | child_process-test.js:53:25:53:58 | ['/C', ... , cmd]) | | child_process-test.js:54:5:54:50 | cp.spaw ... t(cmd)) | child_process-test.js:54:25:54:49 | ['/C', ... at(cmd) | @@ -88,6 +102,11 @@ options | uselesscat.js:100:1:100:56 | execFil ... ptions) | uselesscat.js:100:42:100:55 | unknownOptions | | uselesscat.js:111:1:111:51 | spawn(' ... it'] }) | uselesscat.js:111:14:111:50 | { stdio ... rit'] } | | uselesscat.js:136:17:138:2 | execSyn ... tf8'\\n}) | uselesscat.js:136:51:138:1 | { // NO ... utf8'\\n} | +| uselesscat.js:147:1:147:47 | shelljs ... utf8'}) | uselesscat.js:147:29:147:46 | {encoding: 'utf8'} | +| uselesscat.js:151:1:151:48 | cspawn( ... tf8' }) | uselesscat.js:151:28:151:47 | { encoding: 'utf8' } | +| uselesscat.js:156:1:156:35 | cspawn( ... tf8' }) | uselesscat.js:156:15:156:34 | { encoding: 'utf8' } | +| uselesscat.js:159:16:159:68 | cspawn. ... tf8' }) | uselesscat.js:159:48:159:67 | { encoding: 'utf8' } | +| uselesscat.js:164:1:164:76 | execmod ... (out)}) | uselesscat.js:164:24:164:41 | {encoding: 'utf8'} | #select | False negative | uselesscat.js:54:42:54:69 | // NOT ... lagged] | | False positive | uselesscat.js:44:37:44:85 | // OK [ ... le read | diff --git a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js index 86647b9fc296..ace4784b3806 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/uselesscat.js @@ -139,4 +139,28 @@ const stdout2 = execSync('cat /etc/dnsmasq.conf', { // NOT OK. exec('/bin/cat', function (e, s) {}); // OK -spawn("cat") // OK \ No newline at end of file +spawn("cat") // OK + + +var shelljs = require("shelljs"); +shelljs.exec("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK +shelljs.exec("cat foo/bar", {encoding: 'utf8'}); // NOT OK +shelljs.exec("cat foo/bar", {encoding: 'utf8'}, (err, out) => {console.log(out)}); // NOT OK + +let cspawn = require('cross-spawn'); +cspawn('cat', ['foo/bar'], { encoding: 'utf8' }); // NOT OK +cspawn('cat', ['foo/bar'], { encoding: 'utf8' }, (err, out) => {console.log(out)}); // NOT OK +cspawn('cat', ['foo/bar'], (err, out) => {console.log(out)}); // NOT OK +cspawn('cat', ['foo/bar']); // NOT OK +cspawn('cat', (err, out) => {console.log(out)}); // OK +cspawn('cat', { encoding: 'utf8' }); // OK + +let myResult = cspawn.sync('cat', ['foo/bar']); // NOT OK +let myResult = cspawn.sync('cat', ['foo/bar'], { encoding: 'utf8' }); // NOT OK + +var execmod = require('exec'); +execmod("cat foo/bar", (err, out) => {console.log(out)}); // NOT OK +execmod("cat foo/bar", {encoding: 'utf8'}); // NOT OK +execmod("cat foo/bar", {encoding: 'utf8'}, (err, out) => {console.log(out)}); // NOT OK + + \ No newline at end of file From 8d26f3219950f887ab3ccb27e7807e94deb17fce Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 25 Feb 2020 10:53:07 +0100 Subject: [PATCH 28/37] arg -> param --- .../src/semmle/javascript/security/UselessUseOfCat.qll | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index 7cd66f7981b9..674aa83e52b9 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -185,20 +185,20 @@ module PrettyPrintCatCall { * Create a string representing the callback `func`. */ string createCallbackString(DataFlow::FunctionNode func) { - exists(string args | args = createCallbackArgs(func) | + exists(string params | params = createCallbackParams(func) | if func.getFunction() instanceof ArrowFunctionExpr then if func.getFunction().getBody() instanceof Expr - then result = ", (" + args + ") => ..." - else result = ", (" + args + ") => {...}" - else result = ", function(" + args + ") {...}" + then result = ", (" + params + ") => ..." + else result = ", (" + params + ") => {...}" + else result = ", function(" + params + ") {...}" ) } /** * Create a string concatenation of the parameter names in a function `func`. */ - private string createCallbackArgs(DataFlow::FunctionNode func) { + private string createCallbackParams(DataFlow::FunctionNode func) { result = concat(int i | i = [0 .. func.getNumParameter()] From c83c27cbc4bbc4d4f8f4d1e999625449534ab071 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 25 Feb 2020 11:11:58 +0100 Subject: [PATCH 29/37] add extra sanity-check that the output looks good --- .../javascript/security/UselessUseOfCat.qll | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index 674aa83e52b9..a0dfaa26fb62 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -74,6 +74,14 @@ private class CommandCall extends DataFlow::InvokeNode { } } +/** + * Holds if the input `str` contains some character that might be interpreted in a non-trivial way by a shell. + */ +bindingset[str] +predicate containsNonTrivialBashChar(string str) { + exists(str.regexpFind("\\*|\\||>|<| |\\$|&|,|\\`| ", _, _)) +} + /** * Gets the constant string parts from a data-flow node. * Either the result is a constant string value that the node can hold, or the node is a string-concatenation and the result is the string parts from the concatenation. @@ -97,7 +105,7 @@ class UselessCat extends CommandCall { getArgument(0).mayHaveStringValue(getACatExecuteable()) ) and // wildcards, pipes, redirections, other bash features, and multiple files (spaces) are OK. - not exists(getNonCommandConstantString().regexpFind("\\*|\\||>|<| |\\$|&|,|\\`| ", _, _)) and + not containsNonTrivialBashChar(getNonCommandConstantString()) and // Only acceptable option is "encoding", everything else is non-trivial to emulate with fs.readFile. ( not exists(getOptionsArg()) @@ -146,7 +154,7 @@ module PrettyPrintCatCall { * Create a string representation of an equivalent call to `fs.readFile` for a given command execution `cat`. */ string createReadFileCall(UselessCat cat) { - exists(string sync, string extraArg, string callback | + exists(string sync, string extraArg, string callback, string fileArg | (if cat.isSync() then sync = "Sync" else sync = "") and ( exists(cat.getOptionsArg()) and @@ -163,10 +171,12 @@ module PrettyPrintCatCall { callback = createCallbackString(cat.getCallback()) or callback = "" and not exists(cat.getCallback()) - ) + ) and + fileArg = createFileArgument(cat).trim() and + not(containsNonTrivialBashChar(fileArg.regexpReplaceAll("\\$|\\`| ", ""))) // string concat might contain " ", template strings might contain "$" or `, and that is OK. | result = - "fs.readFile" + sync + "(" + createFileArgument(cat).trim() + extraArg + callback + ")" + "fs.readFile" + sync + "(" + fileArg + extraArg + callback + ")" ) } From bb911bbbf132e3c58f46d70c1ee34eedf6d6ee23 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 27 Feb 2020 12:38:06 +0100 Subject: [PATCH 30/37] Apply suggestions from code review Co-Authored-By: Esben Sparre Andreasen --- .../ql/src/semmle/javascript/security/UselessUseOfCat.qll | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index a0dfaa26fb62..c154885925f8 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -78,8 +78,8 @@ private class CommandCall extends DataFlow::InvokeNode { * Holds if the input `str` contains some character that might be interpreted in a non-trivial way by a shell. */ bindingset[str] -predicate containsNonTrivialBashChar(string str) { - exists(str.regexpFind("\\*|\\||>|<| |\\$|&|,|\\`| ", _, _)) +private predicate containsNonTrivialShellChar(string str) { + exists(str.regexpFind("\\*|\\||>|<| |\\$|&|,|\\`| |;", _, _)) } /** @@ -142,7 +142,7 @@ class UselessCat extends CommandCall { /** * Gets a string used to call `cat`. */ -string getACatExecuteable() { +private string getACatExecuteable() { result = "cat" or result = "/bin/cat" } @@ -173,6 +173,7 @@ module PrettyPrintCatCall { callback = "" and not exists(cat.getCallback()) ) and fileArg = createFileArgument(cat).trim() and + // sanity check in case of surprising `toString` results, other uses of `containsNonTrivialBashChar` should ensure that this conjunct will hold most of the time not(containsNonTrivialBashChar(fileArg.regexpReplaceAll("\\$|\\`| ", ""))) // string concat might contain " ", template strings might contain "$" or `, and that is OK. | result = From a872d7c5c52ef4b5d2d1cc1e23bc046f1feb7b65 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 27 Feb 2020 12:42:22 +0100 Subject: [PATCH 31/37] add comment about negative optionsArg --- .../src/semmle/javascript/frameworks/SystemCommandExecutors.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll b/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll index 89a0929a182b..dbb398be9537 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll @@ -7,7 +7,7 @@ import javascript private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::InvokeNode { int cmdArg; - int optionsArg; + int optionsArg; // either a positive number representing the n'th argument, or a negative number representing the n'th last argument (e.g. -2 is the second last argument). boolean shell; boolean sync; From 17f1974e05a6719dd0587aebd6152ec18402451a Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 28 Feb 2020 09:43:32 +0100 Subject: [PATCH 32/37] Apply suggestions from code review Co-Authored-By: mc <42146119+mchammer01@users.noreply.github.com> --- change-notes/1.24/analysis-javascript.md | 2 +- .../ql/src/Security/CWE-078/UselessUseOfCat.qhelp | 15 +++++++-------- .../ql/src/Security/CWE-078/UselessUseOfCat.ql | 4 ++-- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/change-notes/1.24/analysis-javascript.md b/change-notes/1.24/analysis-javascript.md index eaebf0c12510..9c1afc25c07a 100644 --- a/change-notes/1.24/analysis-javascript.md +++ b/change-notes/1.24/analysis-javascript.md @@ -39,7 +39,7 @@ | Missing await (`js/missing-await`) | correctness | Highlights expressions that operate directly on a promise object in a nonsensical way, instead of awaiting its result. Results are shown on LGTM by default. | | Prototype pollution in utility function (`js/prototype-pollution-utility`) | security, external/cwe/cwe-400, external/cwe/cwe-471 | Highlights recursive copying operations that are susceptible to prototype pollution. Results are shown on LGTM by default. | | Unsafe jQuery plugin (`js/unsafe-jquery-plugin`) | Highlights potential XSS vulnerabilities in unsafely designed jQuery plugins. Results are shown on LGTM by default. | -| Useless use of cat (`js/useless-use-of-cat`) | correctness, security, maintainability | Highlights command executions of cat where the fs API should be used instead. Results are shown on LGTM by default. | +| Useless use of cat (`js/useless-use-of-cat`) | correctness, security, maintainability | Highlights command executions of `cat` where the fs API should be used instead. Results are shown on LGTM by default. | ## Changes to existing queries diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp index 85a7a2e49494..51ac109ae6cd 100644 --- a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp @@ -3,9 +3,9 @@ "qhelp.dtd"> -

    Using the unix command cat to simply read a file is a -unnecessarily complex way to achieve something that can be done simpler and -safer using the Node.js fs.readFile API. +

    Using the unix command cat only to read a file is a +an unnecessarily complex way to achieve something that can be done in a simpler and +safer manner using the Node.js fs.readFile API.

    The use of cat for simple file reads leads to code that is @@ -25,12 +25,11 @@ from the file system. -

    The code in the example will break if the input name contain -special characters (including space), the code does not work on windows, -and if the input is user controlled a command injection attack can happen.

    +

    The code in the example will break if the input name contains +special characters (including space). Additionally, it does not work on Windows +and if the input is user-controlled, a command injection attack can happen.

    -

    To avoid these potential issues the fs.readFile API can be -used instead:

    +

    The fs.readFile API should be used to avoid these potential issues: diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql index 9f28c5fb0cf4..0c33fc8c06f6 100644 --- a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql @@ -1,6 +1,6 @@ /** * @name Useless use of cat - * @description Using `cat`-process to simply read a file is unnecessarily complex, inefficient, unportable, can lead to subtle bugs, or even security vulnerabilities. + * @description Using the `cat` process to read a file is unnecessarily complex, inefficient, unportable, and can lead to subtle bugs, or even security vulnerabilities. * @kind problem * @problem.severity error * @precision high @@ -22,4 +22,4 @@ where if cat.isSync() then message = " Can be replaced with a call to fs.readFileSync(..)." else message = " Can be replaced with a call to fs.readFile(..)." -select cat.asExpr().(FirstLineOf), "Useless use of `cat`." + message \ No newline at end of file +select cat.asExpr().(FirstLineOf), "Useless use of `cat`." + message From 922779e0491a484d456ba46be45409e798bd50de Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 28 Feb 2020 09:48:07 +0100 Subject: [PATCH 33/37] remove double a/an and adjust line lenghts --- javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp index 51ac109ae6cd..8afc06a861e2 100644 --- a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp @@ -3,9 +3,9 @@ "qhelp.dtd"> -

    Using the unix command cat only to read a file is a -an unnecessarily complex way to achieve something that can be done in a simpler and -safer manner using the Node.js fs.readFile API. +

    Using the unix command cat only to read a file is an +unnecessarily complex way to achieve something that can be done in a simpler +and safer manner using the Node.js fs.readFile API.

    The use of cat for simple file reads leads to code that is From d8a96dd7713045de9bc3d7f536d4b5e2fa11a38f Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 28 Feb 2020 09:55:15 +0100 Subject: [PATCH 34/37] change name to suggestion from previous code review --- .../ql/src/semmle/javascript/security/UselessUseOfCat.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index c154885925f8..4c8b38155868 100644 --- a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll +++ b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll @@ -105,7 +105,7 @@ class UselessCat extends CommandCall { getArgument(0).mayHaveStringValue(getACatExecuteable()) ) and // wildcards, pipes, redirections, other bash features, and multiple files (spaces) are OK. - not containsNonTrivialBashChar(getNonCommandConstantString()) and + not containsNonTrivialShellChar(getNonCommandConstantString()) and // Only acceptable option is "encoding", everything else is non-trivial to emulate with fs.readFile. ( not exists(getOptionsArg()) @@ -174,7 +174,7 @@ module PrettyPrintCatCall { ) and fileArg = createFileArgument(cat).trim() and // sanity check in case of surprising `toString` results, other uses of `containsNonTrivialBashChar` should ensure that this conjunct will hold most of the time - not(containsNonTrivialBashChar(fileArg.regexpReplaceAll("\\$|\\`| ", ""))) // string concat might contain " ", template strings might contain "$" or `, and that is OK. + not(containsNonTrivialShellChar(fileArg.regexpReplaceAll("\\$|\\`| ", ""))) // string concat might contain " ", template strings might contain "$" or `, and that is OK. | result = "fs.readFile" + sync + "(" + fileArg + extraArg + callback + ")" From 5e0ae7b4d08f8bce3f5ea9539b80764d93c4da4e Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 28 Feb 2020 10:23:03 +0100 Subject: [PATCH 35/37] add end

    tag --- javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp index 8afc06a861e2..aa8acaee700e 100644 --- a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp @@ -29,7 +29,7 @@ from the file system. special characters (including space). Additionally, it does not work on Windows and if the input is user-controlled, a command injection attack can happen.

    -

    The fs.readFile API should be used to avoid these potential issues: +

    The fs.readFile API should be used to avoid these potential issues:

    From 391b6a833c2927f460526f6d7ce759caade35b14 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 2 Mar 2020 12:28:51 +0100 Subject: [PATCH 36/37] add link to The Useless Use of Cat Award --- javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp index aa8acaee700e..5ef218bdf59b 100644 --- a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.qhelp @@ -36,10 +36,9 @@ and if the input is user-controlled, a command injection attack can happen.

    -
  • -OWASP: Command Injection. -Node.js: File System API. -
    • +
  • OWASP: Command Injection.
    • +
  • Node.js: File System API.
    • +
  • The Useless Use of Cat Award.
    •  From 019266e5370a5d5f978900f964543d2a552335f1 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 2 Mar 2020 13:06:08 +0100 Subject: [PATCH 37/37] change name of Useless cat --- change-notes/1.24/analysis-javascript.md | 2 +- javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/change-notes/1.24/analysis-javascript.md b/change-notes/1.24/analysis-javascript.md index abd072711402..dad8caf09bbe 100644 --- a/change-notes/1.24/analysis-javascript.md +++ b/change-notes/1.24/analysis-javascript.md @@ -46,7 +46,7 @@ | Polynomial regular expression used on uncontrolled data (`js/polynomial-redos`) | security, external/cwe/cwe-730, external/cwe/cwe-400 | Highlights expensive regular expressions that may be used on malicious input. Results are shown on LGTM by default. | | Prototype pollution in utility function (`js/prototype-pollution-utility`) | security, external/cwe/cwe-400, external/cwe/cwe-471 | Highlights recursive copying operations that are susceptible to prototype pollution. Results are shown on LGTM by default. | | Unsafe jQuery plugin (`js/unsafe-jquery-plugin`) | Highlights potential XSS vulnerabilities in unsafely designed jQuery plugins. Results are shown on LGTM by default. | -| Useless use of cat (`js/useless-use-of-cat`) | correctness, security, maintainability | Highlights command executions of `cat` where the fs API should be used instead. Results are shown on LGTM by default. | +| Unnecessary use of `cat` process (`js/unnecessary-use-of-cat`) | correctness, security, maintainability | Highlights command executions of `cat` where the fs API should be used instead. Results are shown on LGTM by default. | ## Changes to existing queries diff --git a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql index 0c33fc8c06f6..6b0ed59e6323 100644 --- a/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql +++ b/javascript/ql/src/Security/CWE-078/UselessUseOfCat.ql @@ -1,10 +1,10 @@ /** - * @name Useless use of cat + * @name Unnecessary use of `cat` process * @description Using the `cat` process to read a file is unnecessarily complex, inefficient, unportable, and can lead to subtle bugs, or even security vulnerabilities. * @kind problem * @problem.severity error * @precision high - * @id js/useless-use-of-cat + * @id js/unnecessary-use-of-cat * @tags correctness * security * maintainability @@ -22,4 +22,4 @@ where if cat.isSync() then message = " Can be replaced with a call to fs.readFileSync(..)." else message = " Can be replaced with a call to fs.readFile(..)." -select cat.asExpr().(FirstLineOf), "Useless use of `cat`." + message +select cat.asExpr().(FirstLineOf), "Unnecessary use of `cat` process." + message