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.

Sign up for GitHub

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

Jump to bottom

Check ip adress #445

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Check ip adress #445

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92cfbb1
It is now possible to validate certificate with ip addresses
peterphonic Feb 28, 2023
7528b0a
remove file added previously by error
peterphonic Feb 28, 2023
2e2a255
add test for the new function isValidIpAddress
peterphonic Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 64 additions & 6 deletions ixwebsocket/IXSocketOpenSSL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,23 @@
#include <vector>
#ifdef _WIN32
#include <shlwapi.h>
//To avoid the problem with X509_NAME collision between macro in wincrypt.h and x509v3.h
//It has to be undef before x509v3.h and after wincrypt.h
#undef X509_NAME
#else
#include <fnmatch.h>
#endif
#if OPENSSL_VERSION_NUMBER < 0x10100000L

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty positive that old openssl installation will not have this x509v3.h header, so we should keep this OPENSSL_VERSION_NUMBER check or we will lose compatibility with old openssl.

Copy link
Author

@peterphonic peterphonic Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsergean , I am just a little bit confused here.

Before my change, the code was like this :

#if OPENSSL_VERSION_NUMBER < 0x10100000L
#include "openssl/x509v3.h"
#endif

The way I read this is we do include x509v3.h for version below 0x10100000L . Version higher doesn't include x509v3.h

With my change, I need x509v3.h
So, would that make sense to do this instead

#if OPENSSL_VERSION_NUMBER < 0x10100000L || OPENSSL_VERSION_NUMBER >= 0x30000000L

In this case, we keep the actual functionality and x509v3.h is added only on version higher or equal to 3.0.0

#include <openssl/x509v3.h>
#endif

#define socketerrno errno

#ifdef _WIN32
// For manipulating the certificate store
#include <wincrypt.h>
// To avoid the problem with X509_NAME collision between macro in wincrypt.h and x509v3.h
// It has to be undef before x509v3.h and after wincrypt.h
#undef X509_NAME
#endif

#ifdef _WIN32
Expand Down Expand Up @@ -77,6 +83,43 @@ namespace

return true;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add unittest for this new function ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess so, where should I add this? Is is the reason I got message that test failed under linux, linux_asan, and mac_tsan... ?

bool isValidIpAddress(const std::string& s)
{
std::vector<int> v;
int n = s.size();
int start = 0, end = 0;
while (end < n)
{
while (end < n && s[end] != '.')
{
++end;
}
if (start == end) {
return false;
}
int val = 0;
for (int i = start; i < end; ++i)
{
if (s[i] < '0' || s[i] > '9')
{
return false;
}
val = val * 10 + (s[i] - '0');
}
if (val < 0 || val > 255) {
return false;
}
v.push_back(val);
start = ++end;
}
if (v.size() != 4)
{
return false;
}
return true;
}

} // namespace
#endif

Expand Down Expand Up @@ -763,10 +806,25 @@ namespace ix
// (The docs say that this should work from 1.0.2, and is the default from
// 1.1.0, but it does not. To be on the safe side, the manual test
// below is enabled for all versions prior to 1.1.0.)
if (!_tlsOptions.disable_hostname_validation)
{
X509_VERIFY_PARAM* param = SSL_get0_param(_ssl_connection);
X509_VERIFY_PARAM_set1_host(param, host.c_str(), host.size());
if (isValidIpAddress(host))
{
// We are connecting to an IP address. let OpenSSL validate the
// IP address in SAN
X509_VERIFY_PARAM *param = SSL_get0_param(_ssl_connection);
X509_VERIFY_PARAM_set1_host(param, NULL, 0);
X509_VERIFY_PARAM_set1_ip_asc(param, host.c_str());
}
else if (!_tlsOptions.disable_hostname_validation)
{
SSL_set1_host(_ssl_connection, host.c_str());
// Both CN-ID and partial wildcards are deprecated
// Optionally, reject all wildcards via:
// X509_CHECK_FLAG_NO_WILDCARDS
// See X509_check_host(3).
//
SSL_set_hostflags(_ssl_connection,
X509_CHECK_FLAG_NEVER_CHECK_SUBJECT |
X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS);
}
#endif
handshakeSuccessful = openSSLClientHandshake(host, errMsg, isCancellationRequested);
Expand Down