From 8f7d3aee05d3220e39f113e2a3cb35fe764e8b53 Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Sat, 19 May 2018 18:15:13 -0500 Subject: [PATCH 1/3] Allow XRootD client to accept subjectAltNames. With this, if the CN doesn't follow the expected matching rules, then the client will iterate through the listed subjectAltNames to determine whether the certificate matches the current host. This includes support for CNs and SANs with wildcards. --- src/XrdCrypto/XrdCryptoX509.cc | 74 ++++++++++++++++++++++++++++++ src/XrdCrypto/XrdCryptoX509.hh | 11 +++++ src/XrdCrypto/XrdCryptosslX509.cc | 49 ++++++++++++++++++++ src/XrdCrypto/XrdCryptosslX509.hh | 3 ++ src/XrdSecgsi/XrdSecProtocolgsi.cc | 20 ++++---- 5 files changed, 147 insertions(+), 10 deletions(-) diff --git a/src/XrdCrypto/XrdCryptoX509.cc b/src/XrdCrypto/XrdCryptoX509.cc index bb01f6ee151..79580a67beb 100644 --- a/src/XrdCrypto/XrdCryptoX509.cc +++ b/src/XrdCrypto/XrdCryptoX509.cc @@ -252,3 +252,77 @@ int XrdCryptoX509::DumpExtensions(bool) ABSTRACTMETHOD("XrdCryptoX509::DumpExtensions"); return -1; } + +/** +--_____________________________________________________________________________ + * Compare two hostnames and see if they are the same, including wildcards. + * + * For example, + * + * - foo.example.com and foo.example.com are considered equal. + * - bar.example.com and foo.example.com are not equal. + * - *.example.com and foo.example.com are equal. + * - FOO.example.com and foo.EXAMPLE.COM are equal (comparison is not case sensitive). + * - F*.com and foo.com are equal + * + * Returns true if the hostnames are considered a match + */ +bool +XrdCryptoX509::MatchHostnames(const char * match_pattern, const char * hostname) +{ + char match_copy[256], host_copy[256]; + char *tok1, *tok2; + char *run1, *run2; + int idx; + + if ((match_pattern == NULL || hostname == NULL) || + ((strlen(match_pattern) > 255) || strlen(hostname) > 255)) + return false; + + // Create a lowercase copy of both hostnames + for (idx = 0; match_pattern[idx]; idx++) { + match_copy[idx] = tolower(match_pattern[idx]); + } + match_copy[idx] = '\0'; + for (idx = 0; hostname[idx]; idx++) { + host_copy[idx] = tolower(hostname[idx]); + } + host_copy[idx] = '\0'; + + // Split the strings by '.' character, iterate through each sub-component. + run1 = match_copy; + run2 = host_copy; + for (tok1 = strsep(&run1, "."), tok2 = strsep(&run2, "."); + tok1 && tok2; + tok1 = strsep(&run1, "."), tok2 = strsep(&run2, ".")) + { + // Match non-wildcard bits + while (*tok1 && *tok2 && *tok1 == *tok2) { + if (*tok2 == '*') + return false; + if (*tok1 == '*') + break; + tok1++; + tok2++; + } + + /** + * At this point, one of the following must be true: + * - We hit a wildcard. In this case, we accept the match as + * long as there is no non-wildcard after it. + * - We hit a character that doesn't match. + * - We hit the of at least one string. + */ + if (*tok1 == '*') { + tok1++; + // Non-wildcard after wildcard -- not acceptable. + if (*tok1 != '\0') + return false; + } + // Only accept the match if both components are at their end. + else if (*tok1 || *tok2) { + return false; + } + } + return !tok1 && !tok2; +} diff --git a/src/XrdCrypto/XrdCryptoX509.hh b/src/XrdCrypto/XrdCryptoX509.hh index fa8022364ee..fb87976248f 100644 --- a/src/XrdCrypto/XrdCryptoX509.hh +++ b/src/XrdCrypto/XrdCryptoX509.hh @@ -103,12 +103,23 @@ public: virtual const char *SubjectHash(int); // hash const char *SubjectHash() { return SubjectHash(0); } // hash + // Returns true if the certificate has a subject alt name which matches + // the given hostnem. + virtual bool MatchesSAN(const char * fqdn) = 0; + // Retrieve a given extension if there (in opaque form) virtual XrdCryptoX509data GetExtension(const char *oid); // Verify signature virtual bool Verify(XrdCryptoX509 *ref); + // Compare two hostnames, handling wildcards as appropriate. Necessary + // for support for accepting connections where the remote X509 certificate + // is a wildcard certificate. + // + // Returns true if the FQDN matches the specified pattern + static bool MatchHostnames(const char *match_pattern, const char *fqdn); + private: static const char *ctype[4]; // Names of types diff --git a/src/XrdCrypto/XrdCryptosslX509.cc b/src/XrdCrypto/XrdCryptosslX509.cc index 0a45c0e0f7b..e22b6b5ac54 100644 --- a/src/XrdCrypto/XrdCryptosslX509.cc +++ b/src/XrdCrypto/XrdCryptosslX509.cc @@ -1091,3 +1091,52 @@ int XrdCryptosslX509::Asn1PrintInfo(int tag, int xclass, int constructed, int in BIO_free(bp); return(0); } + +//____________________________________________________________________________ +bool XrdCryptosslX509::MatchesSAN(const char *fqdn) +{ + EPNAME("MatchesSAN"); + + // Statically allocated array for hostname lengths. RFC1035 limits + // valid lengths to 255 characters. + char san_fqdn[256]; + + if (!fqdn) + return false; + + // Only an EEC is usable as a host certificate. + if (type != kEEC) + return false; + + GENERAL_NAMES *gens = static_cast(X509_get_ext_d2i(cert, + NID_subject_alt_name, NULL, NULL)); + if (!gens) + return false; + + bool success = false; + for (int idx = 0; idx < sk_GENERAL_NAME_num(gens); idx++) { + GENERAL_NAME *gen; + ASN1_STRING *cstr; + gen = sk_GENERAL_NAME_value(gens, idx); + if (gen->type != GEN_DNS) + continue; + cstr = gen->d.dNSName; + if (ASN1_STRING_type(cstr) != V_ASN1_IA5STRING) + continue; + int san_fqdn_len = ASN1_STRING_length(cstr); + if (san_fqdn_len > 255) + continue; + memcpy(san_fqdn, ASN1_STRING_data(cstr), san_fqdn_len); + san_fqdn[san_fqdn_len] = '\0'; + if (strlen(san_fqdn) != static_cast(san_fqdn_len)) // Avoid embedded null's. + continue; + DEBUG("Comparing SAN " << san_fqdn << " with " << fqdn); + if (MatchHostnames(san_fqdn, fqdn)) { + DEBUG("SAN " << san_fqdn << " matches with " << fqdn); + success = true; + break; + } + } + sk_GENERAL_NAME_pop_free(gens, GENERAL_NAME_free); + return success; +} diff --git a/src/XrdCrypto/XrdCryptosslX509.hh b/src/XrdCrypto/XrdCryptosslX509.hh index 31190d1a051..ba917bfc056 100644 --- a/src/XrdCrypto/XrdCryptosslX509.hh +++ b/src/XrdCrypto/XrdCryptosslX509.hh @@ -98,6 +98,9 @@ public: const char *SubjectHash(int = 0); // get hash of subject name const char *IssuerHash(int = 0); // get hash of issuer name + // Check SANs + virtual bool MatchesSAN(const char *); + // Retrieve a given extension if there (in opaque form) XrdCryptoX509data GetExtension(const char *oid); diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index 31cf001a4af..cc3f4afe161 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -3048,7 +3048,10 @@ int XrdSecProtocolgsi::ClientDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, } // // Verify server identity - if (!ServerCertNameOK(hs->Chain->End()->Subject(), emsg)) { + // First, check the DN. On failure, we will iterate through + // the alternate names. + if (!ServerCertNameOK(hs->Chain->End()->Subject(), emsg) && + !hs->Chain->End()->MatchesSAN(Entity.host)) { return -1; } // @@ -5154,6 +5157,7 @@ XrdSecgsiVOMS_t XrdSecProtocolgsi::LoadVOMSFun(const char *plugin, return ep; } + //_____________________________________________________________________________ bool XrdSecProtocolgsi::ServerCertNameOK(const char *subject, XrdOucString &emsg) { @@ -5174,16 +5178,12 @@ bool XrdSecProtocolgsi::ServerCertNameOK(const char *subject, XrdOucString &emsg // Always check if the server CN is in the standard form "[*/][/*]" if (Entity.host) { - if (srvcn != (const char *) Entity.host) { - int ih = srvcn.find((const char *) Entity.host); - if (ih == 0 || (ih > 0 && srvcn[ih-1] == '/')) { - ih += strlen(Entity.host); - if (ih >= srvcn.length() || - srvcn[ih] == '\0' || srvcn[ih] == '/') allowed = 1; - } - } else { - allowed = 1; + size_t ih = srvcn.find("/"); + if (ih != std::string::npos) { + srvcn.erasefromstart(ih + 1); } + allowed = XrdCryptoX509::MatchHostnames(srvcn.c_str(), Entity.host); + // Update the error msg, if the case if (!allowed) { if (emsg.length() <= 0) { From cd8762fb2d3c08cad2e1ed701080e6e43164ec7e Mon Sep 17 00:00:00 2001 From: Gerardo Ganis Date: Fri, 1 Jun 2018 16:02:08 -0500 Subject: [PATCH 2/3] Replace C-style string manipulation with C++ equivalent. --- src/XrdCrypto/XrdCryptoX509.cc | 109 +++++++++++++-------------------- 1 file changed, 41 insertions(+), 68 deletions(-) diff --git a/src/XrdCrypto/XrdCryptoX509.cc b/src/XrdCrypto/XrdCryptoX509.cc index 79580a67beb..866937f4ae7 100644 --- a/src/XrdCrypto/XrdCryptoX509.cc +++ b/src/XrdCrypto/XrdCryptoX509.cc @@ -253,76 +253,49 @@ int XrdCryptoX509::DumpExtensions(bool) return -1; } -/** ---_____________________________________________________________________________ - * Compare two hostnames and see if they are the same, including wildcards. - * - * For example, - * - * - foo.example.com and foo.example.com are considered equal. - * - bar.example.com and foo.example.com are not equal. - * - *.example.com and foo.example.com are equal. - * - FOO.example.com and foo.EXAMPLE.COM are equal (comparison is not case sensitive). - * - F*.com and foo.com are equal - * - * Returns true if the hostnames are considered a match - */ -bool -XrdCryptoX509::MatchHostnames(const char * match_pattern, const char * hostname) +//_____________________________________________________________________________ +bool XrdCryptoX509::MatchHostnames(const char * match_pattern, const char * hostname) { - char match_copy[256], host_copy[256]; - char *tok1, *tok2; - char *run1, *run2; - int idx; - - if ((match_pattern == NULL || hostname == NULL) || - ((strlen(match_pattern) > 255) || strlen(hostname) > 255)) - return false; + // Compare two hostnames and see if they are the same, including wildcards. + // + // For example, + // + // - foo.example.com and foo.example.com are considered equal. + // - bar.example.com and foo.example.com are not equal. + // - *.example.com and foo.example.com are equal. + // - *.example.com and foo.bar.example.com are NOT equal (wildcard applies to a single label). + // - FOO.example.com and foo.EXAMPLE.COM are equal (comparison is not case sensitive). + // - F*.com and foo.com are equal + // + // Returns true if the hostnames are considered a match + + XrdOucString mpatt(match_pattern), hname(hostname); + + // Not empty + if (!mpatt.length() || !hname.length()) return false; // Create a lowercase copy of both hostnames - for (idx = 0; match_pattern[idx]; idx++) { - match_copy[idx] = tolower(match_pattern[idx]); - } - match_copy[idx] = '\0'; - for (idx = 0; hostname[idx]; idx++) { - host_copy[idx] = tolower(hostname[idx]); + mpatt.lower(0); + hname.lower(0); + + // Are they equal? + if (mpatt == hname) return true; + + bool theydomatch = false; + + // Get first token of both strings + int mfrom = -1, hfrom = -1; + XrdOucString mfirst, hfirst; + if (((mfrom = mpatt.tokenize(mfirst, mfrom, '.')) != -1) && + ((hfrom = hname.tokenize(hfirst, hfrom, '.')) != -1)) { + if (hfirst.matches(mfirst.c_str())) { + // First tokens matches, the rest should match without wildcards + mpatt.erasefromstart(mfrom); + hname.erasefromstart(hfrom); + if ((hname == mpatt) || + (!hname.length() && !mpatt.length())) theydomatch = true; + } } - host_copy[idx] = '\0'; - - // Split the strings by '.' character, iterate through each sub-component. - run1 = match_copy; - run2 = host_copy; - for (tok1 = strsep(&run1, "."), tok2 = strsep(&run2, "."); - tok1 && tok2; - tok1 = strsep(&run1, "."), tok2 = strsep(&run2, ".")) - { - // Match non-wildcard bits - while (*tok1 && *tok2 && *tok1 == *tok2) { - if (*tok2 == '*') - return false; - if (*tok1 == '*') - break; - tok1++; - tok2++; - } - - /** - * At this point, one of the following must be true: - * - We hit a wildcard. In this case, we accept the match as - * long as there is no non-wildcard after it. - * - We hit a character that doesn't match. - * - We hit the of at least one string. - */ - if (*tok1 == '*') { - tok1++; - // Non-wildcard after wildcard -- not acceptable. - if (*tok1 != '\0') - return false; - } - // Only accept the match if both components are at their end. - else if (*tok1 || *tok2) { - return false; - } - } - return !tok1 && !tok2; + + return theydomatch; } From 47eb688dc2131c30edd57eba525f95bb2ea842ba Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Sun, 3 Jun 2018 09:30:17 -0500 Subject: [PATCH 3/3] Use hostname, not reverse DNS, for address comparison. This changes XrdSecgsi to prefer to use the hostname for the purpose of matching a certificate to a hostname (as opposed to the prior behavior of a reverse DNS lookup). Relying on reverse DNS is considered insecure; note that all the other security mechanisms use the hostname. With the SAN changes allowing multiple potential patterns in the certificate, admins should be able to handle all the potential use cases. --- src/XrdSecgsi/XrdSecProtocolgsi.cc | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index cc3f4afe161..245c82a7126 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -48,6 +48,7 @@ #include "XrdOuc/XrdOucStream.hh" #include "XrdOuc/XrdOucEnv.hh" +#include "XrdNet/XrdNetAddr.hh" #include "XrdSut/XrdSutAux.hh" #include "XrdCrypto/XrdCryptoMsgDigest.hh" @@ -293,7 +294,15 @@ XrdSecProtocolgsi::XrdSecProtocolgsi(int opts, const char *hname, } // Set host name and address - Entity.host = strdup(endPoint.Name("*unknown*")); + // The hostname is critical for the GSI protocol; it must match the potential + // names on the remote EEC. However, as we may have been redirected to an IP + // address instead of an actual hostname, we must fallback to a reverse DNS lookup. + XrdNetAddr testAddr; + if (!hname || testAddr.Set(hname) == NULL) { + Entity.host = strdup(endPoint.Name("")); + } else { + Entity.host = strdup(hname); + } epAddr = endPoint; Entity.addrInfo = &epAddr;