Legacy TLS hostname validation (OpenSSL < 1.0.2) #2734
Conversation
| def self.validate_hostname(hostname : String, server_cert : LibCrypto::X509) | ||
| return Result::Error if server_cert.null? | ||
| result = matches_subject_alternative_name(hostname, server_cert) | ||
| result = matches_common_name(hostname, server_cert) if result.no_san_present? |
There was a problem hiding this comment.
Are we sure that we need to require the hostname in the SANs if they're present? So far my understanding was that while it's recommended to repeat the CN in the SANs, it's not required.
There was a problem hiding this comment.
Apparently, yes, this is what cURL does:
an alternative name field existed, but didn't match and then we MUST fail
https://github.com/curl/curl/blob/6cabd78531f80d5c6cd942ed1aa97eaa5ec080df/lib/x509asn1.c#L1137
There was a problem hiding this comment.
Looks like that's a fairly recent change http://stackoverflow.com/a/5937270/2199687
But okay with me then.
|
Looks good overall, thank you! The biggest work left here is indeed generating or including some self signed certificates and test against them, which I would very much like to see. The ruby testsuite I linked in #2717 does exactly that. |
ysbaddaden
force-pushed
the
openssl-legacy-verify-certificate-hostname
branch
from
6b057a6 to
c7fdbeb
Compare
| pattern = pattern.chomp('.').downcase | ||
| hostname = hostname.chomp('.').downcase | ||
|
|
||
| if ::Socket.ip?(hostname) |
There was a problem hiding this comment.
This is wrong: this should be in matches_subject_alternative_name since the SAN will be either DNS (2) or IP (7), and I only take DNS (ie. domain name), and CN only contains a domain name.
ysbaddaden
force-pushed
the
openssl-legacy-verify-certificate-hostname
branch
from
c7fdbeb to
4100dbb
Compare
|
Here comes a better (and hopefully final) rework, with basic OpenSSL::X509 wrapper classes (Certificate, Name, Extension) in order to write proper specs to verify a hostname against a certificate (SAN, CN). The X509 wrapper classes are marked as |
| OpenSSL::SSL::HostnameValidation.validate_hostname("::1", openssl_create_cert(san: "IP:::2")).should eq(OpenSSL::SSL::HostnameValidation::Result::MatchNotFound) | ||
| OpenSSL::SSL::HostnameValidation.validate_hostname("0:0:0:0:0:0:0:1", openssl_create_cert(san: "IP:::1")).should eq(OpenSSL::SSL::HostnameValidation::Result::MatchFound) | ||
| OpenSSL::SSL::HostnameValidation.validate_hostname("fe80:0:0:0:0:0:0:1", openssl_create_cert(san: "IP:fe80::1")).should eq(OpenSSL::SSL::HostnameValidation::Result::MatchFound) | ||
| OpenSSL::SSL::HostnameValidation.validate_hostname("fe80:0:0:0:0:0:0:2", openssl_create_cert(san: "IP:fe80::1")).should eq(OpenSSL::SSL::HostnameValidation::Result::MatchNotFound) |
There was a problem hiding this comment.
Should for example fe80::1 validate against itself too? Or for a more curious case, should fe80:0::1 validate against fe80::0:1? Talking about different valid representations of the same IP here.
ysbaddaden
force-pushed
the
openssl-legacy-verify-certificate-hostname
branch
from
4100dbb to
ed89de8
Compare
| # | ||
| # Required for OpenSSL <= 1.0.1 only. Newer versions should use | ||
| # `LibSSL.x509_verify_param_set1_ip_asc` and | ||
| # `LibSSL.x509_verify_param_set1_host` instead. |
There was a problem hiding this comment.
Might be worth to mention that OpenSSL::SSL::Socket does use either automatically.
There was a problem hiding this comment.
I made the method protected, since it's very unlikely to be used directly.
ysbaddaden
force-pushed
the
openssl-legacy-verify-certificate-hostname
branch
2 times, most recently
from
ed48d40 to
d0c052d
Compare
ysbaddaden
force-pushed
the
openssl-legacy-verify-certificate-hostname
branch
from
d0c052d to
f050e96
Compare
|
Done? |
|
I think so yes, just waiting for Travis :) |
| OpenSSL::SSL::HostnameValidation.validate_hostname("0:0:0:0:0:0:0:1", openssl_create_cert(san: "IP:::1")).should eq(OpenSSL::SSL::HostnameValidation::Result::MatchFound) | ||
| OpenSSL::SSL::HostnameValidation.validate_hostname("fe80:0:0:0:0:0:0:1", openssl_create_cert(san: "IP:fe80::1")).should eq(OpenSSL::SSL::HostnameValidation::Result::MatchFound) | ||
| OpenSSL::SSL::HostnameValidation.validate_hostname("fe80:0:0:0:0:0:0:2", openssl_create_cert(san: "IP:fe80::1")).should eq(OpenSSL::SSL::HostnameValidation::Result::MatchNotFound) | ||
| OpenSSL::SSL::HostnameValidation.validate_hostname("fe80:0:1", openssl_create_cert(san: "IP:fe80:0::1")).should eq(OpenSSL::SSL::HostnameValidation::Result::MatchNotFound) |
There was a problem hiding this comment.
Oh wait, while this one is properly a good one too, fe80:0:1 is not actually valid, right? I was more looking towards different non-expanded formattings of the same IP, to repeat for example fe80:0::1 and fe80::0:1, which both are a representation for fe80:0:0:0:0:0:0:1
There was a problem hiding this comment.
Do you want to correct it or should I just do it after the merge? Nvm, I'll do.
…icate-hostname Legacy TLS hostname validation (OpenSSL < 1.0.2)
|
Thank you ❤️ |
|
yay 🙌 |
|
@jhass @ysbaddaden So much love for you, guys! 🙇 |
closes #2717
This went a little too well: the manual badssl specs passed first time. This is weird. Also, there is no verification that the Common Name match does work, indeed. Maybe we could add specs with a few certificates with or without SAN entries to verify the methods actually work correctly (in isolation).
Note: I introduced a
Socket.ip?(str)method to validate IPv4 and IPv6 addresses. Maybe we'd like a better format. The implementation relies on the POSIXinet_ptonfunction, which I expect to be faster than a regular expression.