Skip to content

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Mar 17, 2021

Gets a TP for CVE-2020-8902 (after #5419 is merged).

Evaluation shows a (spurious) speedup.

@erik-krogh erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Mar 17, 2021
@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 erik-krogh marked this pull request as ready for review March 18, 2021 08:55
@erik-krogh erik-krogh requested a review from a team as a code owner March 18, 2021 08:55
@erik-krogh erik-krogh added the JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis label Mar 18, 2021

RoutedRouteHandler() {
router = DataFlow::moduleImport(["@koa/router", "koa-router"]).getAnInvocation() and
call = router.getAMethodCall*() and
Copy link
Contributor

Choose a reason for hiding this comment

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

sanity check: I think the * can be a + here. Is that correct?

override DataFlow::SourceNode getRouteHandlerRegistration() {
result = call
or
result = router.getAMethodCall("routes")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this model the mentioned app.use(router.routes()) bit above?
Shouldn't the registration be the app.use(...) call instead then? My intuition is that router.routes() simply is a getter.

Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

Great work!

Not for this PR, but seeing this reinforced my conviction that we need to migrate HTTP to API graphs. E.g. I thought the .routes() call should just be another step in the type-tracking of route handlers, but it seems Koa doesn't type-tracking its route handlers.

RoutedRouteHandler() {
router = DataFlow::moduleImport(["@koa/router", "koa-router"]).getAnInvocation() and
call = router.getAMethodCall*() and
call.getMethodName() =
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use getAChainedMethodCall here, but I'll leave it up to you if you want to use it. It's unclear to me what the performance impact would be.

Copy link
Contributor Author

@erik-krogh erik-krogh Mar 18, 2021

Choose a reason for hiding this comment

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

Thanks, I had forgotten about getAChainedMethodCall.

I think the performance impact is negligible, it will likely unfold to the same thing anyway.
I can do another smoke-test evaluation if you want one.

Co-authored-by: Asger F <asgerf@github.com>
@codeql-ci codeql-ci merged commit 3415b64 into github:main Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants