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

Add hostname validation feature #23

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add hostname validation feature #23

wants to merge 4 commits into from

Conversation

selvamKrish
Copy link

As per openssl wiki page https://wiki.openssl.org/index.php/Hostname_validation, openssl version prior to 1.0.2 did not perform hostname validation. So users of nopoll should handle the hostname validation part during conn handshake in the client end.
This code can help users to enable/disable hostname validation feature based on the options they are providing during the tls connect. As we could see in the computer security this man in the middle attack is most common security threats faced by the current world, So it is better to enable hostname validation by default and we did the same here. And also user may not be aware of this hostname validation is not handled in their openssl in case if they are using the openssl version < 1.0.2.

@schmidtw
Copy link
Contributor

@francisbrosnan This change looks like a solid & needed improvement to eliminate a discovered "man in the middle" attack vector. Do you have any concerns merging it?

@francisbrosnan
Copy link
Member

Hello Selvam,

Sorry for the delay. I was looking for a moment to thank you for the work and effort done (you have even created a regression test) and also to send you some notes to review some issues that the patch has.

Main concern is copyright assignment: accepting the patch as it is will create us problems with some
customers/users. In this context, I see you have included into the patch several files that you can't assign copyright...so that's a barrier we have to review to work around.

From a technical perspective, there are several issues with the naming (ie. functions like hostmatch or inet_pton4 will clash with other's code for sure given noPoll's library nature and C lack of namespaces). Code convention is another issue (under this subject there is work to do).

Besides, there are several code sections that will require checking/valgrinding to ensure we don't introduce problems inside base code that can be triggered with carefully crafted input...that I'm sure you have tested but I need to review them.

I'll try to review all these things and reply you as soon as possible,
Thanks Selvam for taking your time and effort to report this patch,

bill1600 pushed a commit to bill1600/nopoll that referenced this pull request Jan 16, 2019
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.

3 participants