JS: add query for useless use of cat by erik-krogh · Pull Request #2867 · github/codeql

erik-krogh · 2020-02-18T18:22:23Z

Adds a query that detects useless use of the unix command cat. For example:

child_process.execSync('cat /proc/cpuinfo').toString(); // NOT OK.

These calls can be replaced with calls to fs.readFile/fs.readFileSync, which are much safer and less error-prone (if e.g. spaces are used in the input).

The pattern is not an error in itself, but it is extremely error-prone, and can easily be replaced with a much safer alternative.

I searched GitHub for uses of cat to help decide what the query should flag.
I found some cases where cat is used in combination with wildcards and pipes, which cannot be trivially replaced by fs.readFile, so these uses of cat are not flagged by the query.

It looks like many uses of the pattern originate from people copy-pasting unix bash commands without understanding that there is a better way to do the same thing in node.

Here are some examples of what the query currently flags: https://lgtm.com/query/6505146813103244250/

This also flags CVE-2018-13797.

TODO:

esbena · 2020-02-18T21:11:31Z

I am quite interested in the more restrictive query which only flags the most blatantly useless use of cat with a dynamic file argument:

exec(`cat ${file}`, function (err, out) {	 
  ... out.trim().toLowerCase());	 ...
});

As that could trivially be replaced by:

readFile({file, function (err, out) {	 
  ... out.trim().toLowerCase());	 ...
});

…n to

erik-krogh · 2020-02-19T16:03:05Z

I am quite interested in the more restrictive query which only flags the most blatantly useless use of cat with a dynamic file argument:

I've re-written the query to only flag the results that can be trivially replaced with a call to fs.readFile, and I also output the call that would have to be made.

I still include reads from constant strings though (because constant-strings doesn't always stay constant as a program matures).

Here are some results: https://lgtm.com/query/344037768842122887/

esbena · 2020-02-20T08:07:00Z

Oh wow. That is actually more common than I expected, well done.

I think this query is more precise than the two siblings mentioned below. Neither of those queries have the security tag, but I agree that this query deserves that tag as it flags a giant code smell for code execution, a smell that may have leaked to nearby code.

We have a sibling query on LGTM.com where the results may be FPs in rare cases: https://github.com/Semmle/ql/blob/de66841263665ad15f64fffa13e9e0432735761a/javascript/ql/src/DOM/PseudoEval.ql#L2
We also have a sibling query that is not on LGTM.com as more of its results are FPs: https://github.com/Semmle/ql/blob/de66841263665ad15f64fffa13e9e0432735761a/javascript/ql/src/LanguageFeatures/Eval.ql#L2

I think you should have a look at https://github.com/Semmle/ql/blob/de66841263665ad15f64fffa13e9e0432735761a/javascript/ql/src/semmle/javascript/Concepts.qll#L13 to flag even more cases. You probably need to add an abstract isSync member predicate.

Some feedback on the alert messages:

Useless use of `cat`. Can be replaced with: fs.readFileSync(`${newpath}`)

${newpath} should be desugared to just newpath. A case analysis on the template strings should be enough.

The entire call is selected, which means that many lines are selected in the case of inlined callback functions. Use import semmle.javascript.RestrictedLocations and x.(FirstLineOf)

esbena · 2020-02-20T08:10:35Z

+      else extraArg = ""
+    ) and
+    if exists(cat.getCallback())
+    then callback = ", function(" + getCallbackArgs(cat.getCallback()) + ") {...}"


we should support arrow functions here as well.

erik-krogh · 2020-02-20T09:34:18Z

I think you should have a look at https://github.com/Semmle/ql/blob/de66841263665ad15f64fffa13e9e0432735761a/javascript/ql/src/semmle/javascript/Concepts.qll#L13 to flag even more cases. You probably need to add an abstract isSync member predicate.

My first iteration of the query actually used the SystemCommandExecution class. I'll try to do that again.

erik-krogh · 2020-02-20T13:42:19Z

Here are the results of the revised query on the same set of benchmarks: https://lgtm.com/query/8990960795230963842/

esbena

Hmm. The pretty printing makes this query quite complex.

I would like to see a major refactoring:
Can we separate all of the pretty printing into a separate PrettyPrint module and name of the predicates consistently? At the moment, we use "construct", "representation", "print", "get"(!) suffix and prefixes for the pretty printing predicates, that makes it very hard to separate the alerts from the alert messages.
Ideally, the pretty printing should not prevent any alerts from appearing, but that property is not obvious. It would be nice if the pretty printing module was only referred from the .ql file (or implemented therein), and if the alert message had a fallback string for the case where the pretty printing failed to produce a value.

Semantically, I am a bit concerned about https://lgtm.com/projects/g/nodyn/nodyn/snapshot/eb587c6244757699905b2b24b2e4e26384a3dd08/files/src/test/javascript/childProcessSpec.js?sort=name&dir=ASC&mode=heatmap#L13:

    var proc = child_process.spawn('/bin/cat', [ 'pom.xml' ]);
    proc.stdout.on('data', function(d) {
      content += d.toString();
    })
    proc.on('close', function() {
      expect( content.indexOf( '<project xmlns' ) ).toBeGreaterThan(0);
      expect( content.indexOf( '</project>' ) ).toBeGreaterThan(0);
      helper.testComplete(true);
    })

The proc.on usage indicates a non-trivial reading of the cat output, even though it is trivial in this case.

esbena · 2020-02-21T07:31:33Z

+string createReadFileCall(UselsesCatCandidates::UselessCatCandicate cat) {
+  exists(string sync, string extraArg, string callback |
+    (if cat.isSync() then sync = "Sync" else sync = "") and
+    (


The existence is implied in the true-branches, so this can be de-iffified to:

( extraArg = ", " + printOptionsArg(cat.getOptionsArg()) + ")" or extraArg = "" and not exists(cat.getOptionsArg()) ) and callback = constructCallbackString(cat.getCallback()) or callback = "" and not exists(cat.getCallback))

esbena · 2020-02-21T07:42:11Z

+    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


I would $, &, {, }, and the backtick as well.

esbena · 2020-02-21T07:46:15Z

+        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


See https://github.com/Semmle/ql/blob/2df3fe8f3658df4bdea88f0c1583e89804b385c7/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll#L40

esbena · 2020-02-21T08:01:34Z

@@ -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.


Can we turn this up a notch? We should also have a sentence for each of these bad properties in the qhelp.

Suggested change

* @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.

esbena · 2020-02-21T08:04:07Z

+ * @id js/useless-use-of-cat
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078


I think we should drop the cwes here, and add maintainability instead.

If we insist on cwes, then we should also have the ones for path-injection:

* external/cwe/cwe-022 * external/cwe/cwe-023 * external/cwe/cwe-036 * external/cwe/cwe-073 * external/cwe/cwe-099

erik-krogh · 2020-02-21T09:06:17Z

Semantically, I am a bit concerned about https://lgtm.com/projects/g/nodyn/nodyn/snapshot/eb587c6244757699905b2b24b2e4e26384a3dd08/files/src/test/javascript/childProcessSpec.js?sort=name&dir=ASC&mode=heatmap#L13:
    var proc = child_process.spawn('/bin/cat', [ 'pom.xml' ]);
    proc.stdout.on('data', function(d) {
      content += d.toString();
    })
    proc.on('close', function() {
      expect( content.indexOf( '<project xmlns' ) ).toBeGreaterThan(0);
      expect( content.indexOf( '</project>' ) ).toBeGreaterThan(0);
      helper.testComplete(true);
    })
The proc.on usage indicates a non-trivial reading of the cat output, even though it is trivial in this case.

I agree, I've found the same thing. I think the query should not flag when the process object is used.
Here is a good example in Node.

esbena

Getting closer (sorry for high jacking this Draft PR btw).
I haven't looked at the pretty printing implementation this time, but I note that it accounts for more than half of the query. I think we need to check thoroughly that it scales properly.

esbena · 2020-02-24T09:03:19Z

+  DataFlow::Node getOptionsArg() {
+    exists(int n |
+      n >= 1 and
+      // if there is a command-list, then the options is at least the third argument.


I agree that this probably holds in practice, but it seems safer to introduce abstract DataFlow::Node getOptionsArg() into SystemCommandExecution`.

esbena · 2020-02-24T09:05:42Z

+
+/**
+ * Gets the constant string parts from a data-flow node.
+ * Either the string is some constant


Partial docstring

esbena · 2020-02-24T09:08:01Z

+    isACallTo(getACatExecuteable()) and
+    // There is a file to read, and not just a pair of quotes.
+    (
+      not exists(PrettyPrintCatCall::createFileArgument(this))


This is what I was worried about. Can we avoid letting the query results (excluding the message) depend on the pretty printing implementation?

esbena · 2020-02-24T09:11:12Z

+      )
+    ) and
+    // wildcards, pipes, redirections, other bash features, and multiple files (spaces) are OK.
+    not getNonCommandConstantString().regexpMatch(".*(\\*|\\||>|<| |\\$|&|,|\\`).*") and


regexpFind allows us to leave out the .*s...

Co-Authored-By: Esben Sparre Andreasen <esbena@github.com>

…class

erik-krogh · 2020-02-24T14:47:54Z

The current results contain plenty of examples of how not to read a file, but it doesn't seem like there are exploitable vulnerabilities among the results.

This one is close, but it is saved by a sanitizer in the formidable library.

esbena

Another batch of feedback.

I have created https://github.com/github/codeql-javascript-team/issues/71 for the formidable library.

esbena · 2020-02-24T13:58:45Z

+    override DataFlow::Node getOptionsArg() {
+      result = getLastArgument() and
+      not result = getArgument(0) and
+      not result.getALocalSource() instanceof DataFlow::FunctionNode and // looks like callback


I am on the fence in suggestion that we move these two instanceof checks to the abstract class, or remove them completely. I assume you have encountered a problem when we did not have the checks? Or is this just a leftover from the catch-all heuristic we had prior to this commit?
At the very least, it would be nice with an explicitly test that exercises these instanceof cases.

I assume you have encountered a problem when we did not have the checks?

Yep.

All of these command executions methods have variations of the same API: exec(command[, options][, callback]).

The API is implemented using runtime detection of the types of the arguments, so we have to do something similar.

At the very least, it would be nice with an explicitly test that exercises these instanceof cases.

👍
(I found a bug or two while making those tests)

esbena · 2020-02-25T06:52:16Z

+   * Create a string representing the callback `func`.
+   */
+  string createCallbackString(DataFlow::FunctionNode func) {
+    exists(string args | args = createCallbackArgs(func) |


Nit: args should be params, ditto createCallbackArgs.

esbena · 2020-02-25T06:56:46Z

+    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(".*\\.\\..*")


So if the options argument is config.options, then the message of the alert will become the fallback message of fs.readFile(...), right?

Not quite.

Currently we don't flag any exec call where there is an options argument with unknown properties.
An options argument config.options will fail that test, so we will not flag it in the first place.

If config.options didn't fail that test, only the options argument part of the fs.readFile call would be replaced with "...".

Co-Authored-By: Esben Sparre Andreasen <esbena@github.com>

…fixes)

erik-krogh · 2020-02-25T10:15:00Z

A performance evaluation shows that the query adds a little execution time to all projects.

esbena

Final nits.
Ping @mchammer01 for a doc-review.

esbena · 2020-02-27T10:36:50Z


 private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::InvokeNode {
  int cmdArg;
+  int optionsArg;


Please explain the meaning of optionsArg = -1 and optionsArg = -2 in a comment here.

Co-Authored-By: Esben Sparre Andreasen <esbena@github.com>

mchammer01

@erik-krogh - I reviewed this PR from an editorial point of view. It looks good.
I have made a few comments, mainly about improving readability for users (and there was a tiny typo). Let me know what you think.

mchammer01 · 2020-02-27T20:57:59Z

@@ -0,0 +1,25 @@
+/**
+ * @name Useless use of cat


Could we find some synonyms here as there is use and useless in the same sentence.
Also should cat be in single quotes? (cat)?
ps - if Useless use is used in other query names, I am ok for us to leave it.

Useless is used a fair bit in query names, but Useless use is not used anywhere else.

How about Unnecessary use of cat?
I also think that fits the query better.

There's The Useless Use of Cat Award which inspired this query, so it would be nice to keep some semblance.

Perhaps we should have a more ordinary name for the query, and then add ("Useless Use of Cat") like we did for js/zip-slip.

* @name Arbitrary file write during zip extraction ("Zip Slip")

(I wrote that reply before seeing Erik's reply)

I leave the resolution of this to the two of you, I said what I had to say from an editorial point of view and as a non-developer 😃

@esbena how do you feel about Unnecessary use of cat?

Lets qualify it a bit more:

Unnecessary use of `cat` process

Co-Authored-By: mc <42146119+mchammer01@users.noreply.github.com>

esbena · 2020-02-28T09:03:56Z

+<references>
+
+<li>
+OWASP: <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.


Lets add a link to the award page: http://porkmail.org/era/unix/award.html#cat. Or is that too unofficial or not serious enough? @mchammer01, what is your opinion here?

Thanks for checking with me @esbena, I am fine with this 😉

esbena · 2020-02-28T09:13:15Z

@@ -0,0 +1,25 @@
+/**
+ * @name Useless use of cat


There's The Useless Use of Cat Award which inspired this query, so it would be nice to keep some semblance.

Perhaps we should have a more ordinary name for the query, and then add ("Useless Use of Cat") like we did for js/zip-slip.

* @name Arbitrary file write during zip extraction ("Zip Slip")

mchammer01 · 2020-03-02T12:08:17Z

Thanks for all the doc updates @erik-krogh - it looks great 🥇

add query for useless use of cat

73a7d40

erik-krogh added JS WIP This is a work-in-progress, do not merge yet! labels Feb 18, 2020

erik-krogh added 2 commits February 19, 2020 10:12

accept IO redirections as OK

344060e

change useless cat query to only flag instances that can be re-writte…

bdab9ee

…n to

esbena reviewed Feb 20, 2020

View reviewed changes

update expected output

56f3e43

erik-krogh added 9 commits February 20, 2020 10:39

remove dead predicate

d4e73df

restricts alerts to the first line

a5fdcb6

simplify the output file argument

558beb7

support arrow functions in the callbacks

a193cb1

add isSync predicate to SystemCommandExecution

b5ef45e

require that an options object has a known set of properties

12c0291

use SystemCommandExecution and a few small fixes

b1cbfce

require the file to be non-empty

b2ccec2

insert placeholder qhelp

924272a

esbena requested changes Feb 21, 2020

View reviewed changes

erik-krogh added 3 commits February 21, 2020 10:27

small changes based on review

6ea1453

big refactor of UselessUseOfCal

75410e5

better printing of the options arg

44db0f4

esbena reviewed Feb 24, 2020

View reviewed changes

erik-krogh and others added 3 commits February 24, 2020 11:58

doc changes from review

75c1852

Co-Authored-By: Esben Sparre Andreasen <esbena@github.com>

refactor the getOptionsArg predicate into the SystemCommandExecution …

473787a

…class

complete qldoc

a768e93

erik-krogh added 3 commits February 24, 2020 13:18

remove the last dependency on PrettyPrinting

fb94af9

add qhelp

a779ae5

add change note

b72404d

erik-krogh marked this pull request as ready for review February 24, 2020 13:08

erik-krogh requested a review from a team as a code owner February 24, 2020 13:08

add default message if not pretty printed call can be created

b20e852

small correction in doc + autoformat

afd6ea2

esbena requested changes Feb 25, 2020

View reviewed changes

erik-krogh and others added 4 commits February 25, 2020 10:04

Apply suggestions from code review

d540cae

Co-Authored-By: Esben Sparre Andreasen <esbena@github.com>

add tests for third party command execution libraries (and two small …

87d283a

…fixes)

arg -> param

8d26f32

add extra sanity-check that the output looks good

c83c27c

esbena requested changes Feb 27, 2020

View reviewed changes

erik-krogh and others added 2 commits February 27, 2020 12:38

Apply suggestions from code review

bb911bb

Co-Authored-By: Esben Sparre Andreasen <esbena@github.com>

add comment about negative optionsArg

a872d7c

mchammer01 requested changes Feb 27, 2020

View reviewed changes

erik-krogh and others added 4 commits February 28, 2020 09:43

Apply suggestions from code review

17f1974

Co-Authored-By: mc <42146119+mchammer01@users.noreply.github.com>

remove double a/an and adjust line lenghts

922779e

change name to suggestion from previous code review

d8a96dd

Merge remote-tracking branch 'upstream/master' into UselessCat

ce9cd53

esbena requested changes Feb 28, 2020

View reviewed changes

erik-krogh added 3 commits February 28, 2020 10:23

add end </p> tag

5e0ae7b

add link to The Useless Use of Cat Award

391b6a8

change name of Useless cat

019266e

esbena approved these changes Mar 3, 2020

View reviewed changes

erik-krogh removed the WIP This is a work-in-progress, do not merge yet! label Mar 3, 2020

semmle-qlci merged commit e1c5449 into github:master Mar 3, 2020

	* @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.

Uh oh!

Conversation

erik-krogh commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esbena commented Feb 18, 2020

Uh oh!

erik-krogh commented Feb 19, 2020

Uh oh!

esbena commented Feb 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Feb 20, 2020

Uh oh!

erik-krogh commented Feb 20, 2020

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Feb 21, 2020

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Feb 24, 2020

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erik-krogh commented Feb 25, 2020

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Feb 18, 2020 •

edited

Loading

erik-krogh Feb 28, 2020 •

edited

Loading

mchammer01 Mar 2, 2020 •

edited

Loading