Skip to content

JS: Add steps into static regexp capture group references #3824

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.

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

Merged

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Jun 27, 2020

Adds taint steps from regexp matching to a following use of RegExp.$1 or similar.

Note that we don't check if the regexp in the capture group is restrictive enough to sanitize the input. I've (re)created an issue for it here.

Also note that RegExp.$1 and friends are only assigned to on a successful match. If the match fails, the old values remain. I added test cases to explain this, but couldn't find any real-world examples where this mattered, so I kept it simple by just assuming every match erases the old values, regardless of whether there is a success check.

Fixes #3739.

@asgerf asgerf added the JS label Jun 27, 2020
@asgerf asgerf requested a review from a team as a code owner June 27, 2020 09:23
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

I think this is a better solution than the basic-block based idea I had to solve the same issue 👍

Just some small docstring comments from my end.

I'm also interested in seeing how it scales to larger benchmarks.

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
Comment on lines 760 to 764
exists(DataFlow::MethodCallNode replace |
replace.getMethodName() = "replace" and
getANodeReachingCaptureRef(succ) = replace.getCallback(1).getFunction().getEntry() and
pred = replace.getReceiver()
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
exists(DataFlow::MethodCallNode replace |
replace.getMethodName() = "replace" and
getANodeReachingCaptureRef(succ) = replace.getCallback(1).getFunction().getEntry() and
pred = replace.getReceiver()
)
exists(StringReplaceCall replace |
getANodeReachingCaptureRef(succ) = replace.getRawReplacement().getFunction().getEntry() and
pred = replace.getReceiver()
)

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'd need getALocalSource().(DataFlow::FunctionNode). in there as well, which was a bit verbose, so I added StringReplaceCall.getReplacementCallback() instead, PTAL.

@asgerf
Copy link
Contributor Author

asgerf commented Jun 29, 2020

Running an evaluation on big-apps.

@asgerf asgerf added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Jun 29, 2020
@asgerf
Copy link
Contributor Author

asgerf commented Jun 30, 2020

Big-apps evaluation went fine.

@asgerf asgerf removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Jun 30, 2020
@asgerf
Copy link
Contributor Author

asgerf commented Jun 30, 2020

@esbena can I get an approve? You're still requesting changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

do not support taint through the "static" $2 property
4 participants