-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add support for regex whitelist #12
base: master
Are you sure you want to change the base?
Conversation
@AsherST excellent, we are reviewing/testing! |
It's not the most elegant implementation, but it should get the job done |
@AsherST Coincidently are working on rolling out some other SEDATED changes very soon as well. So we are planning to pull your change down, do some testing, and we should have some results hopefully end of next week. Question for you, what is the specific use case (examples) wherein you'd want to whitelist? Knowing the exact scenario will help us be more thorough in our testing. @denniskennedy and I will be in touch! |
There are situations where there may be a variable for credentials that then gets substituted later on that always follows a set pattern such as |
@SimeonCloutier - Any update as we are facing similar issue? |
@denniskennedy and I have been working thru this change and here is what we are finding. Please refer to my comments on Issue #14. In short, allowing regex whitelisting will open us up to similar issues mentioned in #14. Again, this is why we are emphasizing the time/place and need for manual false positive review occasionally. I am curious though if you have thoughts on how to avoid these issues? Now, as a slightly alternative solution, we have been working through a big batch of regex updates. The new regexes are much better at recognizing environment variable injection. With that said, that is really the approach that we are focusing in on right now... because if SEDATED® can recognize the environment variable injection properly then regex whitelisting becomes less and less of a need. At the moment we are in a change freeze due to the Corona Virus and not able to fully test out the new regexes before rolling them out to public. But, as soon as we are able to properly get the regexes tested, and work through any tweaks, we will be releasing them (stay tuned). Please do let us know your thoughts on the above. If there is a way to introduce this feature but greatly reduce and/or eliminate the risk mentioned then I think we can proceed with it. Thanks! |
Thanks @SimeonCloutier for the feedback. May be slightly off topic and we can take it up in another issue but so far the false positives are as below
I will update more once I look into my notes. Any thoughts on how we could handle these? |
@sheshagirisrao Send over any examples you have for both of those. In the new version of the regexes we are working on now, we have a ton of updates and so we can take your examples and bounce them against the new regexes to see if they still are flagged. |
@AsherST We recently released SEDATED version 1.2.0 (below) which is a fairly substantial upgrade to the regexes. The new regexes may actually help to address some of the things you mentioned above. I'd like to go ahead and get this pull request taken care of one way or another (merged or closed), so in lieu of our new regexes and the concerns mentioned above about whitelisting, let us know your thoughts! Thanks https://github.com/OWASP/SEDATED/releases/tag/v1.2.0 |
Very basic support for a regex whitelist.