JS: provide command execution sinks for execa package #14294
Conversation
There was a problem hiding this comment.
Here is a first round of comments, it will take a while before we can get this ready to merge.
This is not an exhaustive review, but it should give you plenty to work on.
I suggest you only continue with the other library models after we've gotten this PR into a mergable state.
The tests in this PR are in general very confusing to read.
You're mixing file-system access with system-command execution, and nothing in the code/comments in tst.js tells me which is which.
You also have plenty of "GOOD" and "BAD" comments, but I can't figure out what they actually mean.
Just as an example: Line 7 and 9 of tst.js are annotated with "GOOD" and "BAD" respectively.
But they both work only on string constants, and are thus completely harmless, and the only thing you're testing is that they are both recognized as system-command-executions (which they should be).
Basically: there is no connection between the test outcomes (the .expected file) and the comments in the test.
Also, the tests are not testing very much.
E.g. your test_SystemCommandExecution is only testing that a command-execution is recognized, not what the command-argument or argument-list of that command-execution is.
Adding some parameters to the test predicates can help.
Another thing that would be really useful is to add some end-to-end tests.
E.g. adding a new JavaScript file in javascript/ql/test/query-tests/Security/CWE-078/CommandInjection/, where test specifically for command-execution, and annotate each line with // OK / // NOT OK similar to how other files in that directory do it.
A reason for second-order-command-injection not working is that you haven't implemented the optional getArgumentList predicate on the relevant class. When I add an implemtation of that predicate to ExecaExec I can get this example to work just fine: execa('git', ['ls-remote', unsafeValue]); // NOT OK.
| // GOOD | ||
| await $`echo example1`.pipeStderr(`tmp`); | ||
| // BAD argument injection | ||
| await $`ssh ${"example2"}`.pipeStderr(`tmp`); |
There was a problem hiding this comment.
There is nothing vulnerable on this line? It's all just constant strings getting concatenated.
| * The system command execution nodes for `execa.$` or `execa.$.sync` tag functions | ||
| */ | ||
| class ExecaScriptEec extends SystemCommandExecution, ExecaScriptExpr { | ||
| ExecaScriptEec() { name = ["Sync", "ASync"] } |
There was a problem hiding this comment.
This charpred does nothing. name can only ever be "Sync" or ASync".
Also, a boolean called e.g. isSync is better suited than a string called name, because you're really recording whether the method is sync or not, strings just make it confusing.
| isExecaShellEnable(this.getParameter(1)) | ||
| } | ||
|
|
||
| override predicate isSync() { name = "execaSync" } |
There was a problem hiding this comment.
Maybe use a boolean instead of name.
(Same comment throghout).
| * The Execa input file option | ||
| */ | ||
| class ExecaRead extends FileSystemReadAccess, DataFlow::Node { | ||
| API::Node execaNode; |
There was a problem hiding this comment.
A better name might be execaArg or something similar.
The Node part of the name doesn't really say anything.
| // data is the output of a command so IDK how it can be implemented | ||
| override DataFlow::Node getADataNode() { none() } |
There was a problem hiding this comment.
You can't implement that with other than none() here, so this implementation is fine.
| } | ||
|
|
||
| // Holds if left parameter is the left child of a template literal and returns the template literal | ||
| private TemplateLiteral templateLiteralChildAsSink(Expr left) { |
There was a problem hiding this comment.
The naming here is confusing. The way this predicate is used has something to do with sinks, but the predicate itself is not related to sinks.
Maybe something like isFirstTaggedTemplateParameter? And move the TemplateLiteral to be a parameter of the predicate.
|
|
||
| query predicate test_SystemCommandExecution(SystemCommandExecution exec) { any() } | ||
|
|
||
| query predicate test_FileNameSource(FileNameSource exec) { any() } |
There was a problem hiding this comment.
This test predicate has no results?
Was this supposed to have results?
| override DataFlow::Node getArgumentList() { | ||
| // $`${Can Not Be sink} ${sink} .. ${sink}` | ||
| exists(TemplateLiteral tmpL | templateLiteralChildAsSink(this.asExpr()) = tmpL | | ||
| result.asExpr() = tmpL.getAChildExpr+() and |
There was a problem hiding this comment.
I really don't like this +.
Consider something like $${foo} ${bar} .. ${baz(arg1, submethod(subarg))}``
With .getAChildExpr+() you are getting every sub-expression in the above.
So that includes `baz`, `arg1`, etc. Even those are definitely not relevant.
| override predicate isSync() { name = "Sync" } | ||
|
|
||
| override DataFlow::Node getOptionsArg() { | ||
| result = this.asExpr().getAChildExpr*().flow() and result.asExpr() instanceof ObjectExpr |
There was a problem hiding this comment.
I also really don't like that you have a * here.
Similar argument to above.
| /** | ||
| * The system command execution nodes for `execa.execaCommand` or `execa.execaCommandSync` functions | ||
| */ | ||
| class ExecaCommandExec2 extends SystemCommandExecution, DataFlow::CallNode { |
There was a problem hiding this comment.
Having a name like ExecaCommandExec2 means that something went wrong when you named your classes, because the difference between ExecaCommandExec2 and ExecaCommandExec is very much not clear from their names.
I think your naming might be easier if you convert some classes to predicate like I've suggested in another review comment.
Execa package before version 5 has already been modeled but newer versions up to 8 have many new APIs that I've implemented now.
@erik-krogh the
SecondOrderCommandInjectionQuerydoesn't work for me here. please look at the test for examples, I already put comments on which lines are vulnerable to second-order command injection.