Skip to content

JS: Loosen the source in js/shell-command-constructed-from-input #5439

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Mar 25, 2021
Merged

JS: Loosen the source in js/shell-command-constructed-from-input #5439

merged 8 commits into from
Mar 25, 2021

Conversation

erik-krogh
Copy link
Contributor

Gets a TP/TN for CVE-2021-23326

Three changes:

  • Flag library input in sub-packages to better support mono-repos where the "sub-packages" are published independently.
  • Look for a main module in index.js and src/index.js, instead of just the former.
  • Add a link to the source in the alert message (I used it while debugging backwards flow, and I decided to keep it).

@erik-krogh erik-krogh added Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish no-change-note-required This PR does not need a change note JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis labels Mar 18, 2021
@erik-krogh erik-krogh requested a review from a team as a code owner March 18, 2021 13:07
@github-actions github-actions bot added the JS label Mar 18, 2021
esbena
esbena reviewed Mar 18, 2021
View reviewed changes
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some concerns about the FPs we may introduce. But you are the authority on those.

javascript/ql/src/semmle/javascript/NodeModuleResolutionImpl.qll Outdated
tryExtensions(pkg.getFile().getParentContainer(), "index", priority - prioritiesPerCandidate())
exists(Folder folder | folder = pkg.getFile().getParentContainer() |
result =
tryExtensions([folder, folder.getChildContainer("src")], "index",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is quite common to name the src folder lib in JavaScript projects. But perhaps that is not the case with lib/index.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lib is less common than src.
It seems reasonable to add it, so I'll do that.

javascript/ql/test/query-tests/Security/CWE-078/lib/subLib/index.js
@@ -1,7 +1,7 @@
var cp = require("child_process")

module.exports = function (name) {
cp.exec("rm -rf " + name); // OK - this file belongs in a sub-"module", and is not the primary exported module.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. We explicitly decided not to flag this kind of pattern earlier. Should we do a run-on-all to ensure that we are not forgetting something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. We explicitly decided not to flag this kind of pattern earlier. Should we do a run-on-all to ensure that we are not forgetting something?

Lets do that.

I think the reason I added the filter was from seeing noise in various sub-packages.
Often these sub-packages were just copy-pasted third party libraries, or only used internally.

But they're not FPs, they're just benign results (and people should stop copy-pasting code).

I'm running an evaluation now, lets wait for that before we do a run-on-all.

javascript/ql/src/semmle/javascript/PackageExports.qll
* The value is either directly the `module.exports` value, a nested property of `module.exports`, or a method on an exported class.
*/
private DataFlow::Node getAValueExportedByPackage() {
result = getAnExportFromModule(getTopmostPackageJSON().getMainModule())
result =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 this is a nice simplification! Let's hope we don't need it.

@erik-krogh erik-krogh mentioned this pull request Mar 18, 2021
Merged
@erik-krogh erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Mar 18, 2021
@erik-krogh
Copy link
Contributor Author

Evaluation came back, and there's something to look at (new results).

I think the performance regression is spurious.
I only evaluated with two queries, and I couldn't replicate the biggest regression locally (the relevant predicates didn't even show up in the clause timing reports).

@erik-krogh
Copy link
Contributor Author

Evaluation came back, and there's something to look at (new results).

The unsafe-shell-command results are TPs from copy-pasted code-bases (the package.json points to another repo).

And the poly-redos results are mostly from various sub-packages.
Most of the ones I inspected looked like they could be published independently.

So I think it looks safe.
But a run-on-all would likely be a good idea.
(Waiting for #5449 before I do a run-on-all).

@erik-krogh
Copy link
Contributor Author

But a run-on-all would likely be a good idea.
(Waiting for #5449 before I do a run-on-all).

Run on all is done: #5449 (comment)
The FP rate looks OK.

@codeql-ci codeql-ci merged commit e90035a into github:main Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esbena esbena esbena approved these changes

Assignees
No one assigned
Labels
JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis JS no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@erik-krogh @esbena @codeql-ci