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

Fixed IPv4 address parsing when port is present at end of address #1110

Closed
wants to merge 1 commit into from

Conversation

culyerr
Copy link

@culyerr culyerr commented Mar 30, 2016

Relates to #1109

@zimmerle
Copy link
Contributor

zimmerle commented Apr 5, 2016

Hi @culyerr,

Thanks for you patch. It is currently being tested by our buildbots.

@zimmerle zimmerle added this to the v2.9.2 milestone Apr 5, 2016
@zimmerle
Copy link
Contributor

zimmerle commented Apr 5, 2016

Hi @culyerr,

This function "tree_contains_ip" is not only used by IIS. It is used by our different ports: for Apache, NGINX, and others. We may also expect to receive an IPv6 as input of that function.

I believe that this bug that you want to address, may related to the fact that the IP address should never ever come with the port at that stage. Most likely the update needs to be made in the IIS specific code. So that we can get rid of the port in the earlier stages of ModSecurity processing.

Testing the patch I got 2582 unit tests failing. Please, have a look at:
http://www.modsecurity.org/developers/buildbot/builders/Linux32%20-%20Nginx%20%28Stable%29/builds/255/steps/unit%20test_1/logs/stdio

Thanks for the patch!

@zimmerle zimmerle closed this Apr 5, 2016
@zimmerle
Copy link
Contributor

zimmerle commented Apr 5, 2016

@culyerr if you want to discuss it, please reach me (zimmerle) or csanders on #modsecurity at FreeNode.

@culyerr
Copy link
Author

culyerr commented Sep 27, 2016

Thanks. I am picking this up again now and will look to implement the check in the IIS specific code.

@zimmerle
Copy link
Contributor

@culyerr Nice ;) if you need something we are at: https://modsecurity.slack.com/

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.

2 participants