diff --git a/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll b/javascript/ql/src/semmle/javascript/security/UselessUseOfCat.qll index ac5338eb27e..5558fdcb654 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 3acd23863d0..61def6726b3 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 98c41513baa..c860d21f654 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