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 d2f0e13fe2e..f48ba6b8e95 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 e5c5e8a0a1a..f67fa287685 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 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 + 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 ) ) ) } /** - * Gets the dataflow node determining the command executed. + * A call to child_process.exec that might be a useless call to cat. */ - DataFlow::Node getCommand() { result = command } + 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 67b59aa9d6e..00000000000 --- 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 00000000000..f4b22cd0803 --- /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 bfb342f7ad1..edcf4987e3e 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 4fe7ccd75d2..d35f3224d18 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); - }); -}; +exec("cat foo/bar", function (err, out) {}); // NOT OK -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 /proc/" + id + "/status", function (err, out) { // NOT OK + console.log(out); }); -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 ${newpath}`) // NOT OK -execSync("cat /proc/cpuinfo | grep -c '" + someValue + "'"); // OK - pipes +child_process.execSync('cat package.json | wc -l'); // OK - pipes! -function cat(file) { - return execSync('cat ' + file).toString(); // NOT OK -} +execSync('cat /proc/cpuinfo /foo/bar').toString(); // OK multiple files. -execSync(`cat ${files.join(' ')} > ${outFile}`); // OK +execSync(`cat ${newpath} /foo/bar`).toString(); // OK multiple files. -var cmd = 'cat package.json | grep' -exec(cmd); // OK - pipes! +exec(`cat ${newpath} | grep foo`, function (err, out) { }) // OK - pipes -execSync("sudo cat " + newpath + "*.js | grep foo").toString(); // OK - wildcard and pipes +execSync(`cat ${newpath}`, {uid: 1000}) // OK - non trivial options -execSync(`cat ${newpath}`); // NOT OK +exec('cat *.js | wc -l', { cwd: './' }, function (err, out) { }); // OK - wildcard and pipes -exec("cat /proc/cpuinfo | grep name"); // OK - pipes +execSync(`cat foo/bar/${newpath}`); // NOT OK ("encoding" is used EXACTLY the same way in fs.readFileSync) -execSync(`cat ${newpath} | ${othertool}`); // OK - pipes +execSync(`cat foo/bar/${newpath}`, {encoding: 'utf8'}); // NOT OK ("encoding" is used EXACTLY the same way in fs.readFileSync) -execSync("sh -c 'cat " + newpath + "'"); // NOT OK. [but not flagged] - -exec(` cat ${newpath}`) // NOT OK - -exec(` cat ${newpath} | grep foo`) // OK - pipes +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. 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(`cat ${files.join(' ')} > ${outFile}`); // 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 +execSync(`cat ${files.join(' ')}`); // OK - not just a simple file read + +exec("cat /proc/cpuinfo | grep name"); // OK - pipes + +execSync(`cat ${newpath} | ${othertool}`); // OK - pipes + +function cat(file) { + return execSync('cat ' + file).toString(); // NOT OK +} + +execSync("sh -c 'cat " + newpath + "'"); // NOT OK. [but not flagged] + +var execFile = child_process.execFile; +var execFileSync = child_process.execFileSync; + +execFile('/bin/cat', [ 'pom.xml' ], function(error, stdout, stderr ) { // NOT OK + // Not using stderr + console.log(stdout); +}); + +execFile('/bin/cat', [ 'pom.xml' ], function(error, stdout, stderr ) { // OK. - stderr is used. + console.log(stderr); +}); + + +execFile('/bin/cat', [ 'pom.xml' ], {encoding: 'utf8'}, function(error, stdout, stderr ) { // NOT OK + // Not using stderr + console.log(stdout); +}); + +execFileSync('/bin/cat', [ 'pom.xml' ], {encoding: 'utf8'}); // NOT OK + +execFileSync('/bin/cat', [ 'pom.xml' ]); // NOT OK + +var opts = {encoding: 'utf8'}; +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', [ "foo/" + newPath + "bar" ], {encoding: 'utf8'}); // NOT OK + +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