From 41ca4872672e6b077b168280ad61e05eb76e7086 Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Thu, 7 Jun 2018 09:34:11 -0500 Subject: [PATCH 1/5] Expand the hostname if necessary. Use `getaddrinfo` to determine whether the user-provided hostname is a complete, valid hostname. If it isn't, then ask `getaddrinfo` for a canonical name and use that. --- src/XrdSecgsi/XrdSecProtocolgsi.cc | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index 74050336b9a..8af55402fac 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -35,6 +35,8 @@ #include #include #include +#include +#include #include #include #include @@ -303,7 +305,32 @@ XrdSecProtocolgsi::XrdSecProtocolgsi(int opts, const char *hname, if (!hname || !XrdNetAddrInfo::isHostName(hname)) { Entity.host = strdup(endPoint.Name("")); } else { - Entity.host = strdup(hname); + // At this point, hname still may possibly be a non-qualified domain name. + // We append a '.' to the name, which prevents getaddrinfo from doing any + // appending of search domains (i.e., expanding "www" to "wwww.unl.edu"). + // If getaddrinfo succeeds, then we know this was a valid FQDN and we use that. + // If it doesn't succeed, then we do a full lookup. + struct addrinfo hints; + struct addrinfo *results; + std::string hname_with_dot(hname); + hname_with_dot += "."; + memset(&hints, '\0', sizeof(struct addrinfo)); + hints.ai_family = AF_UNSPEC; + int retval = getaddrinfo(hname_with_dot.c_str(), NULL, &hints, &results); + if (retval == 0) { + freeaddrinfo(results); + // We have a valid hostname; proceed. + Entity.host = strdup(hname); + } else { + hints.ai_flags = AI_CANONNAME; + int retval = getaddrinfo(hname, NULL, &hints, &results); + if (retval == 0 && results && results->ai_canonname) { + Entity.host = strdup(results->ai_canonname); + freeaddrinfo(results); + } else { // Lookups aren't working; trust user has done something reasonable. + Entity.host = strdup(hname); + } + } } epAddr = endPoint; Entity.addrInfo = &epAddr; From 5e5867390ef557b97aa9b54a4fd98a08b78c7f8d Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Thu, 7 Jun 2018 10:20:22 -0500 Subject: [PATCH 2/5] Allow XrdSecGSITrustDNS setting to disable use of all DNS lookups. By setting XrdSecGSITrustDNS=0, one can disable all DNS lookups in the client for matching a server certificate to the current connection. This is the most safe setting but has fairly significant backward compatibility implications if this is set. The default is to trust DNS for a few limited cases. --- src/XrdSecgsi/XrdSecProtocolgsi.cc | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index 8af55402fac..b802822c192 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -302,8 +302,15 @@ XrdSecProtocolgsi::XrdSecProtocolgsi(int opts, const char *hname, // As of time of testing (June 2018), EOS will redirect to an IP address to handle // metadata commands and rely on the reverse DNS lookup for GSI security to function. // Hence, this fallback likely needs to be kept for some time. + // + // We allow an environment variable to override all usage of DNS; default is to fallback + // to DNS lookups in limited cases for backward compatibility. + const char *trust_dns = getenv("XrdSecGSITrustDNS"); + if (trust_dns == NULL || !strcmp(trust_dns, "1")) { if (!hname || !XrdNetAddrInfo::isHostName(hname)) { Entity.host = strdup(endPoint.Name("")); + } else if (hname && (hname[0] != '\0') && (hname[strlen(hname)-1] == '.')) { + Entity.host = strdup(hname); } else { // At this point, hname still may possibly be a non-qualified domain name. // We append a '.' to the name, which prevents getaddrinfo from doing any @@ -332,8 +339,13 @@ XrdSecProtocolgsi::XrdSecProtocolgsi(int opts, const char *hname, } } } - epAddr = endPoint; - Entity.addrInfo = &epAddr; + } else { + // We have been told via environment variable to not trust DNS; use the exact + // hostname provided by the user. + Entity.host = strdup(hname); + } + epAddr = endPoint; + Entity.addrInfo = &epAddr; // Init session variables sessionCF = 0; From 2831c4e394e25d9df96bd40de8b048ce4ea0a584 Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Thu, 7 Jun 2018 20:24:51 -0500 Subject: [PATCH 3/5] Simplify logic for utilizing DNS. Rely more on XrdNetAddr routines where at all possible. We now call a hostname non-qualified if it contains no '.' characters. While the previous algorithm potentially handled more side cases, it had the strong downside of always relying on DNS security. Since that's precisely what we want to avoid, we only consider the case where the user specifies `foo` and wants the search name to expand it to `foo.example.com`. --- src/XrdSecgsi/XrdSecProtocolgsi.cc | 35 ++++++++++++------------------ 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index b802822c192..f7ec44938e6 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -43,6 +43,7 @@ #include "XrdVersion.hh" +#include "XrdNet/XrdNetAddr.hh" #include "XrdSys/XrdSysHeaders.hh" #include "XrdSys/XrdSysLogger.hh" #include "XrdSys/XrdSysError.hh" @@ -309,33 +310,25 @@ XrdSecProtocolgsi::XrdSecProtocolgsi(int opts, const char *hname, if (trust_dns == NULL || !strcmp(trust_dns, "1")) { if (!hname || !XrdNetAddrInfo::isHostName(hname)) { Entity.host = strdup(endPoint.Name("")); - } else if (hname && (hname[0] != '\0') && (hname[strlen(hname)-1] == '.')) { - Entity.host = strdup(hname); } else { // At this point, hname still may possibly be a non-qualified domain name. - // We append a '.' to the name, which prevents getaddrinfo from doing any - // appending of search domains (i.e., expanding "www" to "wwww.unl.edu"). - // If getaddrinfo succeeds, then we know this was a valid FQDN and we use that. - // If it doesn't succeed, then we do a full lookup. - struct addrinfo hints; - struct addrinfo *results; - std::string hname_with_dot(hname); - hname_with_dot += "."; - memset(&hints, '\0', sizeof(struct addrinfo)); - hints.ai_family = AF_UNSPEC; - int retval = getaddrinfo(hname_with_dot.c_str(), NULL, &hints, &results); - if (retval == 0) { - freeaddrinfo(results); + // If there is a '.' character, then we assume it is a qualified domain name -- + // otherwise, we use DNS. + // + // NOTE: We can definitively test whether this is a qualified domain name by + // simply appending a '.' to `hname` and performing a lookup. However, this + // causes DNS to be used by every lookup - meaning we rely on the security + // of DNS for all cases; we want to avoid this. + if (strchr(hname, '.')) { // We have a valid hostname; proceed. Entity.host = strdup(hname); } else { - hints.ai_flags = AI_CANONNAME; - int retval = getaddrinfo(hname, NULL, &hints, &results); - if (retval == 0 && results && results->ai_canonname) { - Entity.host = strdup(results->ai_canonname); - freeaddrinfo(results); - } else { // Lookups aren't working; trust user has done something reasonable. + XrdNetAddr xrd_addr; + char canonname[256]; + if (!xrd_addr.Set(hname) || (xrd_addr.Format(canonname, 256, XrdNetAddrInfo::fmtName, XrdNetAddrInfo::noPort) <= 0)) { Entity.host = strdup(hname); + } else { + Entity.host = strdup(canonname); } } } From ef677245919768aef64e9bd1766b83f3f96c7717 Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Thu, 7 Jun 2018 20:29:14 -0500 Subject: [PATCH 4/5] Remove unnecessary includes. --- src/XrdSecgsi/XrdSecProtocolgsi.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index f7ec44938e6..f96e6b2695c 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -35,8 +35,6 @@ #include #include #include -#include -#include #include #include #include From 6d714efedc89346629bd1fe4a546ac7953269225 Mon Sep 17 00:00:00 2001 From: Gerardo Ganis Date: Fri, 8 Jun 2018 15:01:43 +0200 Subject: [PATCH 5/5] secgsi: improve control of new option 'Trust DNS' For consistency the variable should be called XrdSecGSITRUSTDNS and, server side, the new option should be controlled by switch -trustdns:[0|1] (default 1) . The switch and the env are processed in XrdSecProtocolgsiInit() . Signed-off-by: Brian Bockelman --- src/XrdSecgsi/XrdSecProtocolgsi.cc | 24 ++++++++++++++++++++---- src/XrdSecgsi/XrdSecProtocolgsi.hh | 5 ++++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index f96e6b2695c..58cf250c071 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -163,6 +163,7 @@ XrdSecgsiAuthz_t XrdSecProtocolgsi::VOMSFun = 0; int XrdSecProtocolgsi::VOMSCertFmt = -1; int XrdSecProtocolgsi::MonInfoOpt = 0; bool XrdSecProtocolgsi::HashCompatibility = 1; +bool XrdSecProtocolgsi::TrustDNS = true; // // Crypto related info int XrdSecProtocolgsi::ncrypt = 0; // Number of factories @@ -302,10 +303,11 @@ XrdSecProtocolgsi::XrdSecProtocolgsi(int opts, const char *hname, // metadata commands and rely on the reverse DNS lookup for GSI security to function. // Hence, this fallback likely needs to be kept for some time. // - // We allow an environment variable to override all usage of DNS; default is to fallback - // to DNS lookups in limited cases for backward compatibility. - const char *trust_dns = getenv("XrdSecGSITrustDNS"); - if (trust_dns == NULL || !strcmp(trust_dns, "1")) { + // We provide servers a switch and clients an environment variable to override all + // usage of DNS (processed on XrdSecProtocolgsiInit). + // Default is to fallback to DNS lookups in limited + // cases for backward compatibility. + if (TrustDNS) { if (!hname || !XrdNetAddrInfo::isHostName(hname)) { Entity.host = strdup(endPoint.Name("")); } else { @@ -2280,6 +2282,11 @@ void gsiOptions::Print(XrdOucTrace *t) POPTS(t, " Crypto modules: "<< (clist ? clist : XrdSecProtocolgsi::DefCrypto)); POPTS(t, " Ciphers: "<< (cipher ? cipher : XrdSecProtocolgsi::DefCipher)); POPTS(t, " MDigests: "<< (md ? md : XrdSecProtocolgsi::DefMD)); + if (trustdns) { + POPTS(t, " Trusting DNS for hostname checking"); + } else { + POPTS(t, " Untrusting DNS for hostname checking"); + } POPTS(t, "*** ------------------------------------------------------------ ***"); } @@ -2453,6 +2460,10 @@ char *XrdSecProtocolgsiInit(const char mode, if (cenv) opts.hashcomp = 0; + // DNS trusting control + if ((cenv = getenv("XrdSecGSITRUSTDNS"))) + opts.trustdns = (!strcmp(cenv, "0")) ? false : true; + // // Setup the object with the chosen options rc = XrdSecProtocolgsi::Init(opts,erp); @@ -2519,6 +2530,7 @@ char *XrdSecProtocolgsiInit(const char mode, // [-vomsfun:] // [-vomsfunparms:] // [-defaulthash] + // [-trustdns:<0|1>] // int debug = -1; String clist = ""; @@ -2548,6 +2560,7 @@ char *XrdSecProtocolgsiInit(const char mode, int vomsat = 1; int moninfo = 0; int hashcomp = 1; + int trustdns = 1; char *op = 0; while (inParms.GetLine()) { while ((op = inParms.GetToken())) { @@ -2611,6 +2624,8 @@ char *XrdSecProtocolgsiInit(const char mode, moninfo = atoi(op+9); } else if (!strcmp(op, "-defaulthash")) { hashcomp = 0; + } else if (!strncmp(op, "-trustdns:",10)) { + trustdns = atoi(op+10); } else { PRINT("ignoring unknown switch: "< 0) opts.clist = (char *)clist.c_str(); if (certdir.length() > 0) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.hh b/src/XrdSecgsi/XrdSecProtocolgsi.hh index 05b9ce46e6c..de78af26179 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.hh +++ b/src/XrdSecgsi/XrdSecProtocolgsi.hh @@ -200,6 +200,8 @@ public: int moninfo; // [s] 0 do not look for; 1 use DN as default int hashcomp; // [cs] 1 send hash names with both algorithms; 0 send only the default [1] + bool trustdns; // [cs] 'true' if DNS is trusted [true] + gsiOptions() { debug = -1; mode = 's'; clist = 0; certdir = 0; crldir = 0; crlext = 0; cert = 0; key = 0; cipher = 0; md = 0; ca = 1 ; crl = 1; crlrefresh = 86400; @@ -208,7 +210,7 @@ public: gmapfun = 0; gmapfunparms = 0; authzfun = 0; authzfunparms = 0; authzto = -1; ogmap = 1; dlgpxy = 0; sigpxy = 1; srvnames = 0; exppxy = 0; authzpxy = 0; - vomsat = 1; vomsfun = 0; vomsfunparms = 0; moninfo = 0; hashcomp = 1; } + vomsat = 1; vomsfun = 0; vomsfunparms = 0; moninfo = 0; hashcomp = 1; trustdns = true; } virtual ~gsiOptions() { } // Cleanup inside XrdSecProtocolgsiInit void Print(XrdOucTrace *t); // Print summary of gsi option status }; @@ -341,6 +343,7 @@ private: static int VOMSCertFmt; static int MonInfoOpt; static bool HashCompatibility; + static bool TrustDNS; // // Crypto related info static int ncrypt; // Number of factories