Skip to content

JS: Extra taint-steps to support CVE#4231

Merged
codeql-ci merged 6 commits intogithub:mainfrom
erik-krogh:CVE767
Sep 15, 2020
Merged

JS: Extra taint-steps to support CVE#4231
codeql-ci merged 6 commits intogithub:mainfrom
erik-krogh:CVE767

Conversation

@erik-krogh
Copy link
Copy Markdown
Contributor

@erik-krogh erik-krogh commented Sep 8, 2020

This PR almost gets a TP/TN for CVE-2020-15152 (one callgraph related step missing).

Does the following:

  • Add support for direct callbacks to require("net").createServer((socket) => {...}).
    Previously only require("net").createServer().on("connection", (socket) => {...}) was supported.
    (Also adds tls as another module that has the same createServer method).

  • Adds (new require("net").Socket()).connect(..) as a ClientRequest.
    Neither getAResponseDataNode nor getADataNode are needed for the CVE, but I added them for completeness.

  • Drive-by refactorization of arrayFunctionTaintStep.

  • Taint step for require("bluebird").mapSeries().
    (mapSeries is unique to bluebird).
    Only the argument -> parameter step is needed for the CVE, but I added the return -> call step for completeness.

  • Adds lots of taint-steps for underscore methods.
    I took a quick walk through here: https://underscorejs.org/#each, and added taint-steps for the methods that had obvious taint-steps.
    Only _.compact(array) is needed for the CVE.

An evaluation on nightly looks fine fine.
(UnsafeJQueryPlugin.ql failed for azure-sdk-for-node on both branches - that was after my recent PR - I'm investigating).

@github-actions github-actions bot added the JS label Sep 8, 2020
@erik-krogh erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Sep 8, 2020
@erik-krogh erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Sep 10, 2020
@erik-krogh erik-krogh marked this pull request as ready for review September 10, 2020 08:39
@erik-krogh erik-krogh requested a review from a team as a code owner September 10, 2020 08:39
@erik-krogh erik-krogh added the WIP This is a work-in-progress, do not merge yet! label Sep 10, 2020
@erik-krogh erik-krogh marked this pull request as draft September 10, 2020 12:56
@erik-krogh erik-krogh removed the WIP This is a work-in-progress, do not merge yet! label Sep 10, 2020
@erik-krogh erik-krogh marked this pull request as ready for review September 10, 2020 12:58
}

/**
* Holds if there is a taint-step involving a (non-function) underscore method from `pred` to `succ`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you mean by "non-function"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I mean that I don't include any of the Underscore methods that involve functions. (E.g. _.bind(), _.partial(), _.throttle()).
They are called Function Functions in the Underscore documentation.

Should I update the docstring to something else?

@codeql-ci codeql-ci merged commit 951e309 into github:main Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants