search

ASPLes / nopoll

OpenSource WebSocket toolkit
http://www.aspl.es/nopoll
GNU Lesser General Public License v2.1
126 stars 73 forks source link

Add hostname validation feature #23

Open selvamKrish opened 7 years ago

selvamKrish commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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,