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
Regression: certificate host validation to strict if SAN present #875
Comments
Yeah, that change clearly didn't take other SAN field types into account properly. My bad. I'll write up a suggested fix. |
Reverting would be one option, but i decided to instead step forward and try to adhere to the RFC2818 better which says that a dNSName field MUST match if present. Not entirely what curl did previously but should be what it does with this patch and it should work for your case @databus23 . Would be great if you could verify before I merge this. From c07990025984ec7bb58f9e91a14d197f3e71f5c2 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Wed, 15 Jun 2016 15:36:40 +0200
Subject: [PATCH] openssl: fix cert check with non-DNS name fields present
Regression introduced in 5f5b62635 (released in 7.48.0)
Reported-by: Fabian Ruff
Fixes #875
---
lib/vtls/openssl.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index f702653..2f69790 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -1080,10 +1080,11 @@ static CURLcode verifyhost(struct connectdata *conn, X509 *server_cert)
struct in6_addr addr;
#else
struct in_addr addr;
#endif
CURLcode result = CURLE_OK;
+ bool dNSName = FALSE; /* if a dNSName field exists in the cert */
#ifdef ENABLE_IPV6
if(conn->bits.ipv6_ip &&
Curl_inet_pton(AF_INET6, conn->host.name, &addr)) {
target = GEN_IPADD;
@@ -1100,20 +1101,27 @@ static CURLcode verifyhost(struct connectdata *conn, X509 *server_cert)
altnames = X509_get_ext_d2i(server_cert, NID_subject_alt_name, NULL, NULL);
if(altnames) {
int numalts;
int i;
+ bool dnsmatched = FALSE;
+ bool ipmatched = FALSE;
/* get amount of alternatives, RFC2459 claims there MUST be at least
one, but we don't depend on it... */
numalts = sk_GENERAL_NAME_num(altnames);
- /* loop through all alternatives while none has matched */
- for(i=0; (i<numalts) && !matched; i++) {
+ /* loop through all alternatives - until a dnsmatch */
+ for(i=0; (i < numalts) && !dnsmatched; i++) {
/* get a handle to alternative name number i */
const GENERAL_NAME *check = sk_GENERAL_NAME_value(altnames, i);
+ /* If a subjectAltName extension of type dNSName is present, that MUST
+ be used as the identity. / RFC2818 section 3.1 */
+ if(check->type == GEN_DNS)
+ dNSName = TRUE;
+
/* only check alternatives of the same type the target is */
if(check->type == target) {
/* get data and length */
const char *altptr = (char *)ASN1_STRING_data(check->d.ia5);
size_t altlen = (size_t) ASN1_STRING_length(check->d.ia5);
@@ -1132,39 +1140,44 @@ static CURLcode verifyhost(struct connectdata *conn, X509 *server_cert)
*/
if((altlen == strlen(altptr)) &&
/* if this isn't true, there was an embedded zero in the name
string and we cannot match it. */
Curl_cert_hostcheck(altptr, conn->host.name)) {
- matched = TRUE;
+ dnsmatched = TRUE;
infof(data,
" subjectAltName: host \"%s\" matched cert's \"%s\"\n",
conn->host.dispname, altptr);
}
break;
case GEN_IPADD: /* IP address comparison */
/* compare alternative IP address if the data chunk is the same size
our server IP address is */
if((altlen == addrlen) && !memcmp(altptr, &addr, altlen)) {
- matched = TRUE;
+ ipmatched = TRUE;
infof(data,
" subjectAltName: host \"%s\" matched cert's IP address!\n",
conn->host.dispname);
}
break;
}
}
}
GENERAL_NAMES_free(altnames);
+
+ if(dnsmatched || (!dNSName && ipmatched)) {
+ /* count as a match if the dnsname matched or if there was no dnsname
+ fields at all AND there was an IP field match */
+ matched = TRUE;
+ }
}
if(matched)
/* an alternative name matched */
;
- else if(altnames) {
- /* an alternative name field existed, but didn't match and then we MUST
- fail */
+ else if(dNSName) {
+ /* an dNSName field existed, but didn't match and then we MUST fail */
infof(data, " subjectAltName does not match %s\n", conn->host.dispname);
failf(data, "SSL: no alternative certificate subject name matches "
"target host name '%s'", conn->host.dispname);
result = CURLE_PEER_FAILED_VERIFICATION;
}
--
2.8.1
|
I verified against latest master that the patch indeed fixes the reported issue. 👍 |
@bagder Thanks! Whats the current ETA for 7.50? |
The release calendar currently says July 13th, but I need to change that for personal reasons. So I don't know yet... |
Regarding the code with comment: |
Regarding the initial problem statement from @databus23, Common Name is still not checked for match on hostname for server certificates. Although this is no longer a requirement, it is allowed by RFC6125 Section 6.4.4, as databus23 observed, and this is still a problem for users with such server certificates. |
CN is checked, but only if there's no SAN field used. That's also what that the spec says we should do. I suppose you're saying there's some case when this doesn't work? If so, that's a bug and you should probably file an issue with all the details in it. This issue you're commenting in here has already been fixed. |
I did this
Server certificate contains this:
I expected the following
no problem fetching the url
curl/libcurl version
Problem
In recent versions of curl the
CN
is ignored when anySAN
is present in the certificate regardless of the SAN's type. I think this commit 5f5b626 is responsible for the change in behaviour.matched
used to be tristate (no match yet, mismatch, match) and was changed to a boolean. With this change the validation now fails even if the SAN contains no usable entries (e.g. email)If I read RFC6125 Section 6.4.4 the old behavior is at least allowed.
Was this change intentional? We are running a large internal coporate CA for years now which always imprints the requester as an
email SAN
in each certificate. Newer versions of curl don't work with our certificates anymore which only contain the hostname in the CN.The text was updated successfully, but these errors were encountered: