Skip to content
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

Added event to prevent tainting. #5398

Merged
merged 16 commits into from
Mar 20, 2021
Merged

Conversation

mortenson
Copy link
Contributor

Per #5382 this PR adds a new event that allows plugins to prevent tainting in specific scenarios. I've also added an example plugin/test that shows how you can inspect the structure of an array to determine if it should be considered a taint source.

This is a WIP, so let me know if you want the event name or test changed - once that's decided I can dispatch the event from more analyzers if you'd like.

@muglug
Copy link
Collaborator

muglug commented Mar 16, 2021

Ok, extra complication: I think this plugin should instead inform these two arguments:

?array $added_taints = null,
?array $removed_taints = null
in the call here:

It may be the case that you want to remove the html taint but preserve the sql taint, for example. In this case you'd want to remove html taints.

So instead of being a boolean I think it wants to provide two separate arrays of added and removed taints at every node it's used on. If both those arrays are empty there's no net effect

@muglug
Copy link
Collaborator

muglug commented Mar 16, 2021

I've updated the PR along those lines! The main benefit is it allows people to institute their own custom taints in a smarter fashion.

@mortenson
Copy link
Contributor Author

@muglug That looks good to me - the test I added still works as expected so I think this would give me a path forward on the render array problem. Would you like me to rename the new test method and add this event dispatch to other analyzers? (If you have time to do this don't wait on me though)

@muglug
Copy link
Collaborator

muglug commented Mar 16, 2021

Would you like me to rename the new test method and add this event dispatch to other analyzers?

That would be great!

@mortenson
Copy link
Contributor Author

@muglug StaticCallAnalyzer, AtomicPropertyFetchAnalyzer, and ArrayFetchAnalyzer didn't have access to Context which is needed for this event, should I add $context as an optional parameter to the relevant public static methods on those classes?

All other analyzers that used that TaintedInput check you mentioned in the issue have been updated.

@mortenson
Copy link
Contributor Author

I did the above, feel free to clean up the new event invocations/calls as needed.

@mortenson mortenson changed the title WIP: Added event to prevent tainting. Added event to prevent tainting. Mar 19, 2021
@muglug muglug merged commit 4aabb41 into vimeo:master Mar 20, 2021
@muglug
Copy link
Collaborator

muglug commented Mar 20, 2021

Thanks, this is a great effort

@mortenson
Copy link
Contributor Author

Thank you @muglug !

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

Successfully merging this pull request may close these issues.

None yet

2 participants