From 00b21d86ad478a229ada873f48ae54078745c2bc Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Wed, 17 Oct 2018 07:15:35 -0500 Subject: [PATCH 1/2] Fallback to reverse DNS in all cases. Whenever we have enabled trust DNS, we should fall back to using reverse DNS in all cases, not just when an IP address is given. This provides backward compatibility with the case where the client uses a DNS alias directly but the server is missing the SAN for that DNS alias (instead the server only has the canonical name for the IP address in the DN). --- src/XrdSecgsi/XrdSecProtocolgsi.cc | 33 +++++++++++++++++++++--------- src/XrdSecgsi/XrdSecProtocolgsi.hh | 5 +++-- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index b0420b07613..4219e538507 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -308,6 +308,7 @@ XrdSecProtocolgsi::XrdSecProtocolgsi(int opts, const char *hname, // usage of DNS (processed on XrdSecProtocolgsiInit). // Default is to fallback to DNS lookups in limited // cases for backward compatibility. + expectedHost = NULL; if (TrustDNS) { if (!hname || !XrdNetAddrInfo::isHostName(hname)) { Entity.host = strdup(endPoint.Name("")); @@ -337,6 +338,7 @@ XrdSecProtocolgsi::XrdSecProtocolgsi(int opts, const char *hname, // We have been told via environment variable to not trust DNS; use the exact // hostname provided by the user. Entity.host = strdup(hname); + expectedHost = strdup(hname); } epAddr = endPoint; Entity.addrInfo = &epAddr; @@ -1031,6 +1033,7 @@ void XrdSecProtocolgsi::Delete() SafeDelete(sessionKsig); // RSA key to sign SafeDelete(sessionKver); // RSA key to verify SafeDelete(proxyChain); // Chain with delegated proxies + SafeDelete(expectedHost); delete this; } @@ -3118,10 +3121,20 @@ int XrdSecProtocolgsi::ClientDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, } // // Verify server identity - // First, check the DN. On failure, we will iterate through - // the alternate names. - if (!ServerCertNameOK(hs->Chain->End()->Subject(), emsg) && + // First, check the DN against the client-provided hostname. + // On failure, we will iterate through the alternate names in the cert. + // If that fails, we will do a reverse DNS lookup of the IP address. + if (!ServerCertNameOK(hs->Chain->End()->Subject(), Entity.host, emsg) && !hs->Chain->End()->MatchesSAN(Entity.host)) { + if ((expectedHost == NULL) && TrustDNS && Entity.addrInfo) { + char canonname[256]; + if (Entity.addrInfo->Format(canonname, 256, XrdNetAddrInfo::fmtName, XrdNetAddrInfo::noPort) > 0) { + expectedHost = strdup(canonname); + if (!ServerCertNameOK(hs->Chain->End()->Subject(), Entity.host, emsg)) { + return -1; + } + } + } return -1; } // @@ -5259,7 +5272,7 @@ XrdSecgsiVOMS_t XrdSecProtocolgsi::LoadVOMSFun(const char *plugin, //_____________________________________________________________________________ -bool XrdSecProtocolgsi::ServerCertNameOK(const char *subject, XrdOucString &emsg) +bool XrdSecProtocolgsi::ServerCertNameOK(const char *subject, const char *hname, XrdOucString &emsg) { // Check that the server certificate subject name is consistent with the // expectations defined by the static SrvAllowedNames @@ -5277,12 +5290,12 @@ bool XrdSecProtocolgsi::ServerCertNameOK(const char *subject, XrdOucString &emsg if (cnidx != STR_NPOS) srvcn.assign(srvsubj, cnidx + 3); // Always check if the server CN is in the standard form "[*/][/*]" - if (Entity.host) { + if (hname) { size_t ih = srvcn.find("/"); if (ih != std::string::npos) { srvcn.erasefromstart(ih + 1); } - allowed = XrdCryptoX509::MatchHostnames(srvcn.c_str(), Entity.host); + allowed = XrdCryptoX509::MatchHostnames(srvcn.c_str(), hname); // Update the error msg, if the case if (!allowed) { @@ -5290,7 +5303,7 @@ bool XrdSecProtocolgsi::ServerCertNameOK(const char *subject, XrdOucString &emsg emsg = "server certificate CN '"; emsg += srvcn; emsg += "' does not match the expected format(s):"; } - String defcn("[*/]"); defcn += Entity.host; defcn += "[/*]"; + String defcn("[*/]"); defcn += hname; defcn += "[/*]"; emsg += " '"; emsg += defcn; emsg += "' (default)"; } } @@ -5299,12 +5312,12 @@ bool XrdSecProtocolgsi::ServerCertNameOK(const char *subject, XrdOucString &emsg if (SrvAllowedNames.length() > 0) { // The SrvAllowedNames string contains the allowed formats separated by a '|'. // The specifications can contain the or placeholders which - // are replaced by Entity.host; they can also contain the '*' wildcard, in + // are replaced by hname; they can also contain the '*' wildcard, in // which case XrdOucString::matches is used. A '-' before the specification // will deny the matching CN's; the last matching wins. String allowedfmts(SrvAllowedNames); - allowedfmts.replace("", (const char *) Entity.host); - allowedfmts.replace("", (const char *) Entity.host); + allowedfmts.replace("", hname); + allowedfmts.replace("", hname); int from = 0; String fmt; while ((from = allowedfmts.tokenize(fmt, from, '|')) != -1) { diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.hh b/src/XrdSecgsi/XrdSecProtocolgsi.hh index 060eff0a027..73a277f7a76 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.hh +++ b/src/XrdSecgsi/XrdSecProtocolgsi.hh @@ -390,7 +390,8 @@ private: XrdCryptoRSA *sessionKsig; // RSA key to sign XrdCryptoRSA *sessionKver; // RSA key to verify X509Chain *proxyChain; // Chain with the delegated proxy on servers - bool srvMode; // TRUE if server mode + bool srvMode; // TRUE if server mode + char *expectedHost; // Expected hostname if TrustDNS is enabled. // Temporary Handshake local info gsiHSVars *hs; @@ -426,7 +427,7 @@ private: static bool VerifyCA(int opt, X509Chain *cca, XrdCryptoFactory *cf); static int VerifyCRL(XrdCryptoX509Crl *crl, XrdCryptoX509 *xca, XrdOucString crldir, XrdCryptoFactory *CF, int hashalg); - bool ServerCertNameOK(const char *subject, String &e); + bool ServerCertNameOK(const char *subject, const char *hname, String &e); static XrdSutCacheEntry *GetSrvCertEnt(XrdSutCERef &gcref, XrdCryptoFactory *cf, time_t timestamp, String &cal); From 5762f137b6bf84379eb1a88e6d1cc868133f9952 Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Wed, 17 Oct 2018 09:14:16 -0500 Subject: [PATCH 2/2] Simplify hostname generation logic. --- src/XrdSecgsi/XrdSecProtocolgsi.cc | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index 4219e538507..64eee10aa27 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -3127,12 +3127,10 @@ int XrdSecProtocolgsi::ClientDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, if (!ServerCertNameOK(hs->Chain->End()->Subject(), Entity.host, emsg) && !hs->Chain->End()->MatchesSAN(Entity.host)) { if ((expectedHost == NULL) && TrustDNS && Entity.addrInfo) { - char canonname[256]; - if (Entity.addrInfo->Format(canonname, 256, XrdNetAddrInfo::fmtName, XrdNetAddrInfo::noPort) > 0) { - expectedHost = strdup(canonname); - if (!ServerCertNameOK(hs->Chain->End()->Subject(), Entity.host, emsg)) { + const char *name = Entity.addrInfo->Name(); + expectedHost = strdup(name); + if ((name == NULL) || !ServerCertNameOK(hs->Chain->End()->Subject(), expectedHost, emsg)) { return -1; - } } } return -1;