Skip to content

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Aug 30, 2021

And improve+simplify the model for connect by basing it off the NodeJS model.

Adds source+sink for CVE-2020-7680.

Evaluation was uneventful.

@erik-krogh erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Aug 30, 2021
@erik-krogh erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Aug 31, 2021
@erik-krogh erik-krogh marked this pull request as ready for review August 31, 2021 19:54
@erik-krogh erik-krogh requested a review from a team as a code owner August 31, 2021 19:54
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.

LGTM. I have a few semantics-preserving suggestions.

@@ -156,10 +113,11 @@ module Connect {
* An access to a user-controlled Connect request input.
*/
private class RequestInputAccess extends HTTP::RequestInputAccess {
RequestExpr request;
NodeJSLib::RequestExpr request;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we keep hiding that this is just a NodeJSLib::RequestExpr reference under the hood?

Perhaps the deleted RequestExpr should just be an alias now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a class RequestExpr = NodeJSLib::RequestExpr;

Comment on lines +51 to +53
result = middleware.getAMemberCall(["push", "unshift"]).getArgument(0).getAFunctionValue()
or
result = middleware.(DataFlow::ArrayCreationNode).getAnElement().getAFunctionValue()
Copy link
Contributor

Choose a reason for hiding this comment

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

This middleware value is just an array in practice, right?
I expect that this push/unshift pattern could apply to all kinds of middleware setups that take arrays as arguments.

Should we (later?) add a utility class that would enable us to write the above as:

        result = middleware.(DataFlow::ArrayLikeNode).getAnElement().getAFunctionValue()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that. The same utility class could also be used in UnsafeShellCommandConstructionCustomizations.qll.

I'll put it on my list, and look at it later.

@codeql-ci codeql-ci merged commit 29bcd7c into github:main Sep 1, 2021
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