| Message ID | 515AF6DB.4000708@RedHat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Apr 2, 2013, at 8:18 AM, Steve Dickson <SteveD@redhat.com> wrote: > > > On 02/04/13 09:45, Simo Sorce wrote: >> The attached patch adds a new command line switch to rpc.gssd to avoid >> PTR resolution when possible. >> >> The current code *depends* on PTR resolution for GSSAPI authentication >> and this is *bad*. >> It imposes an annoying, and unnecessary, constraint on the correctness >> of DNS resolution, which prevents mounts from working in networks where >> the PTR record cannot be easily controlled (for example networks where >> the forward name is reasonable while the PTR is set to some artificial >> name based on the IP address or so that is not the canonical name or >> where no PTR exist at all). >> >> Depending on PTR resolution for GSSAPI is also very bad practice because >> it opens up DNS spoofing attacks where an attacker can try to redirect a >> user to the wrong server fooling mutual authentication, and induce a >> user to trust improper data or disclose (by copying on the impostor >> server) data that should be confidential. >> >> Ideally people will always use the -N switch, however I added an actual >> switch to avoid breaking existing configuration where someone may >> intentionally or inadvertently be using multiple A names instead of >> using CNAMEs but relying on PTR records to find the "canonical name". >> >> It would be probably nice to switch the behavior on by default in >> future. >> >> Jeff in CC as he also implemented a similar switch for cifs.upcall >> (there it is called -t|--trust-dns, but in rpc.gsssd -t is already >> taken ...). >> > A nits.. >>>>> Please do not post patches as attachments. In-lining them in >>>>> the email makes it easier to review... > > From 53dade1e5d22eee06a08ace2684257292514cb49 Mon Sep 17 00:00:00 2001 > From: Simo Sorce <simo@redhat.com> > Date: Mon, 1 Apr 2013 21:00:55 -0400 > Subject: [PATCH] Avoid forced reverse resolution for server name > > A NFS client should be able to work properly even if the PTR record for the > server is not set. there is no excuse to forcefully prevent that from working > when it can. > So, use some heuristics to see if the server is a FQDN or an IP address. > If it is an IP address or a shortname (no '.' in name) then do a PTR lookup, > otherwise avoid it. > > This is enabled by the new -N flag which is off by default for now, ideally we > will turn this on by default after a transition period. > --- > utils/gssd/gss_util.h | 2 ++ > utils/gssd/gssd.c | 4 +++- > utils/gssd/gssd.man | 7 ++++++- > utils/gssd/gssd_proc.c | 31 +++++++++++++++++++++++++++---- > 4 files changed, 38 insertions(+), 6 deletions(-) > > diff --git a/utils/gssd/gss_util.h b/utils/gssd/gss_util.h > index aa9f77806075f9ab67a7763a75a010369ba2d1b9..663fb0998bede6144118f890b9311ee8687176e3 100644 > --- a/utils/gssd/gss_util.h > +++ b/utils/gssd/gss_util.h > @@ -52,4 +52,6 @@ int gssd_check_mechs(void); > gss_krb5_set_allowable_enctypes(min, cred, num, types) > #endif > > +extern int avoid_ptr; > + > #endif /* _GSS_UTIL_H_ */ > diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c > index 0be251781bacaa562270f773341126bc95ca6b45..2a98e94812a662f0fd868fce8dc3419f65648a4d 100644 > --- a/utils/gssd/gssd.c > +++ b/utils/gssd/gssd.c > @@ -85,7 +85,7 @@ sig_hup(int signal) > static void > usage(char *progname) > { > - fprintf(stderr, "usage: %s [-f] [-l] [-M] [-n] [-v] [-r] [-p pipefsdir] [-k keytab] [-d ccachedir] [-t timeout] [-R preferred realm]\n", > + fprintf(stderr, "usage: %s [-f] [-l] [-M] [-n] [-v] [-r] [-p pipefsdir] [-k keytab] [-d ccachedir] [-t timeout] [-R preferred realm] [-N]\n", >>>>> Could please move the [-N] in front of [-n]... At lease that that point in the list things >>>>> are in alphabetical order > > progname); > exit(1); > } > @@ -150,6 +150,8 @@ main(int argc, char *argv[]) > errx(1, "Encryption type limits not supported by Kerberos libraries."); > #endif > break; > + case 'N': > + avoid_ptr = 1; > default: > usage(argv[0]); > break; > diff --git a/utils/gssd/gssd.man b/utils/gssd/gssd.man > index 79d9bf91ac6b976c57d167e60d07f828a3ff5b1f..09253b43ba6502202c17bbc84bd6bcf9dec85e9b 100644 > --- a/utils/gssd/gssd.man > +++ b/utils/gssd/gssd.man > @@ -8,7 +8,7 @@ > rpc.gssd \- RPCSEC_GSS daemon > .SH SYNOPSIS > .B rpc.gssd > -.RB [ \-fMnlvr ] > +.RB [ \-fMnlvrN ] >>>>>> Same as above.... > > .RB [ \-k > .IR keytab ] > .RB [ \-p > @@ -266,6 +266,11 @@ new kernel contexts to be negotiated after > seconds, which allows changing Kerberos tickets and identities frequently. > The default is no explicit timeout, which means the kernel context will live > the lifetime of the Kerberos service ticket used in its creation. > +.TP > +.B -N > +Tries to avoid PTR lookups for resolving the server name, if the name used at > +mount looks like a Fully Qualified Domain Name. This is import to avoid GSSAPI > +issues when PTR records are set and to help avoid some kind of MITM attack. >>>>> I'm sorry this does not make much sense. Can we use more meaningful terms >>>>> like DNS lookups or DNS PTR lookups... something not so condensed >>>>> >>>>>> Also, could you please talk about who will be needing this new flag and how will >>>>>> they know they needed? > > .SH SEE ALSO > .BR rpc.svcgssd (8), > .BR kerberos (1), > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c > index ea01e92e4565670b97dea1a936d2f0dbdc7c4610..0a584d2084d626ee98a8ad6ff56da95464b882fe 100644 > --- a/utils/gssd/gssd_proc.c > +++ b/utils/gssd/gssd_proc.c > @@ -67,6 +67,7 @@ > #include <errno.h> > #include <gssapi/gssapi.h> > #include <netdb.h> > +#include <ctype.h> > > #include "gssd.h" > #include "err_util.h" > @@ -107,6 +108,8 @@ struct pollfd * pollarray; > > unsigned long pollsize; /* the size of pollaray (in pollfd's) */ > > +int avoid_ptr = 0; > + > /* > * convert a presentation address string to a sockaddr_storage struct. Returns > * true on success or false on failure. > @@ -165,12 +168,32 @@ addrstr_to_sockaddr(struct sockaddr *sa, const char *node, const char *port) > * convert a sockaddr to a hostname > */ > static char * > -sockaddr_to_hostname(const struct sockaddr *sa, const char *addr) > +get_servername(const char *name, const struct sockaddr *sa, const char *addr) > { > socklen_t addrlen; > int err; > char *hostname; > char hbuf[NI_MAXHOST]; > + const char *p; > + int do_ptr_lookup = 0; > + > + if (avoid_ptr) { > + /* try to determine if this is FQDN, or an IP address with > + * basic heuristics, if they fail try a PTR lookup */ > + p = strrchr(name, '.'); > + if (!p || strchr(name, ':')) > + do_ptr_lookup = 1; /* non-FQDN, or IPv6 */ > + else { > + for (++p; *p; p++) > + if (!isdigit(*p)) > + break; > + if (!*p) > + do_ptr_lookup = 1; /* IPv4 */ > + } > + if (!do_ptr_lookup) { > + return strdup(name); > + } > + } > Is this "reinventing the wheel"? ;-) Don't we already have a why of determining a FQDN? > Maybe not... Yes, we do. Have a look in support/export/hostname.c for tools and helpers for dealing with IP addresses properly. > > > steved. > > > switch (sa->sa_family) { > case AF_INET: > @@ -208,7 +231,7 @@ read_service_info(char *info_file_name, char **servicename, char **servername, > struct sockaddr *addr) { > #define INFOBUFLEN 256 > char buf[INFOBUFLEN + 1]; > - static char dummy[128]; > + static char server[128]; > int nbytes; > static char service[128]; > static char address[128]; > @@ -236,7 +259,7 @@ read_service_info(char *info_file_name, char **servicename, char **servername, > "service: %127s %15s version %15s\n" > "address: %127s\n" > "protocol: %15s\n", > - dummy, > + server, > service, program, version, > address, > protoname); > @@ -258,7 +281,7 @@ read_service_info(char *info_file_name, char **servicename, char **servername, > if (!addrstr_to_sockaddr(addr, address, port)) > goto fail; > > - *servername = sockaddr_to_hostname(addr, address); > + *servername = get_servername(server, addr, address); > if (*servername == NULL) > goto fail; > > -- > 1.8.1.4 > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-04-02 at 08:51 -0700, Chuck Lever wrote: > > Is this "reinventing the wheel"? ;-) Don't we already have a why of > determining a FQDN? > > Maybe not... > > Yes, we do. Have a look in support/export/hostname.c for tools and > helpers for dealing with IP addresses properly. > The only function I see that may fit the bill is host_addrinfo() which calls getaddrinfo() which I avoided because gssapi will alraedy do that. Maybe it is ok to only check by negatives ? IE check if the string is an ipv4 or an ipv6 address and if not just pass the string in as is ? Simo.
On Apr 2, 2013, at 9:29 AM, Simo Sorce <simo@redhat.com> wrote: > On Tue, 2013-04-02 at 08:51 -0700, Chuck Lever wrote: >>> Is this "reinventing the wheel"? ;-) Don't we already have a why of >> determining a FQDN? >>> Maybe not... >> >> Yes, we do. Have a look in support/export/hostname.c for tools and >> helpers for dealing with IP addresses properly. >> > The only function I see that may fit the bill is host_addrinfo() which > calls getaddrinfo() which I avoided because gssapi will alraedy do that. That looks like the right way to do it. It may be reasonable to assume our resolver caches results, so a second call may not be much of a latency hit. But AI_CANONNAME may rely on the target host having a PTR record. > Maybe it is ok to only check by negatives ? > IE check if the string is an ipv4 or an ipv6 address and if not just > pass the string in as is ? IIRC, the pton() function fails if your string does not contain a presentation address. But that allows unqualified domain labels, doesn't it? -- Chuck Lever chuck[dot]lever[at]oracle[dot]com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-04-02 at 13:21 -0700, Chuck Lever wrote: > On Apr 2, 2013, at 9:29 AM, Simo Sorce <simo@redhat.com> wrote: > > > On Tue, 2013-04-02 at 08:51 -0700, Chuck Lever wrote: > >>> Is this "reinventing the wheel"? ;-) Don't we already have a why of > >> determining a FQDN? > >>> Maybe not... > >> > >> Yes, we do. Have a look in support/export/hostname.c for tools and > >> helpers for dealing with IP addresses properly. > >> > > The only function I see that may fit the bill is host_addrinfo() which > > calls getaddrinfo() which I avoided because gssapi will alraedy do that. > > That looks like the right way to do it. It may be reasonable to assume our resolver caches results, so a second call may not be much of a latency hit. > > But AI_CANONNAME may rely on the target host having a PTR record. No, I already filed bugs against glibc in the past where it happened by mistake that a PTR request was done with some flag combinations. Some distributions have patches to fix that as getaddrinfo should exclusively deal with CNAME -> A name "canonicalization" and nothing else. Using a PTR request is a bug. However I already addressed the fact I cannot use any function in that file from gssd_proc.c so I simply used inet_pton directly, works fine and according to comments in host_pton it is also required even if getaddrinfo was used, and sufficient for my purposes. > > Maybe it is ok to only check by negatives ? > > IE check if the string is an ipv4 or an ipv6 address and if not just > > pass the string in as is ? > > IIRC, the pton() function fails if your string does not contain a presentation address. This is fine. > But that allows unqualified domain labels, doesn't it? GSSAPI will call getaddrinfo() anyway so if a shortname is used it will be canonicalized as long as the local resolver can resolve it into a fqdn. If it can't though luck, either use a fqdn on the mount command or do not enable this feature (which is why it is off by default and selectable in the last patchset). Simo.
diff --git a/utils/gssd/gss_util.h b/utils/gssd/gss_util.h index aa9f77806075f9ab67a7763a75a010369ba2d1b9..663fb0998bede6144118f890b9311ee8687176e3 100644 --- a/utils/gssd/gss_util.h +++ b/utils/gssd/gss_util.h @@ -52,4 +52,6 @@ int gssd_check_mechs(void); gss_krb5_set_allowable_enctypes(min, cred, num, types) #endif +extern int avoid_ptr; + #endif /* _GSS_UTIL_H_ */ diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c index 0be251781bacaa562270f773341126bc95ca6b45..2a98e94812a662f0fd868fce8dc3419f65648a4d 100644 --- a/utils/gssd/gssd.c +++ b/utils/gssd/gssd.c @@ -85,7 +85,7 @@ sig_hup(int signal) static void usage(char *progname) { - fprintf(stderr, "usage: %s [-f] [-l] [-M] [-n] [-v] [-r] [-p pipefsdir] [-k keytab] [-d ccachedir] [-t timeout] [-R preferred realm]\n", + fprintf(stderr, "usage: %s [-f] [-l] [-M] [-n] [-v] [-r] [-p pipefsdir] [-k keytab] [-d ccachedir] [-t timeout] [-R preferred realm] [-N]\n", >>>> Could please move the [-N] in front of [-n]... At lease that that point in the list things >>>> are in alphabetical order progname); exit(1); } @@ -150,6 +150,8 @@ main(int argc, char *argv[]) errx(1, "Encryption type limits not supported by Kerberos libraries."); #endif break; + case 'N': + avoid_ptr = 1; default: usage(argv[0]); break; diff --git a/utils/gssd/gssd.man b/utils/gssd/gssd.man index 79d9bf91ac6b976c57d167e60d07f828a3ff5b1f..09253b43ba6502202c17bbc84bd6bcf9dec85e9b 100644 --- a/utils/gssd/gssd.man +++ b/utils/gssd/gssd.man @@ -8,7 +8,7 @@ rpc.gssd \- RPCSEC_GSS daemon .SH SYNOPSIS .B rpc.gssd -.RB [ \-fMnlvr ] +.RB [ \-fMnlvrN ] >>>>> Same as above.... .RB [ \-k .IR keytab ] .RB [ \-p @@ -266,6 +266,11 @@ new kernel contexts to be negotiated after seconds, which allows changing Kerberos tickets and identities frequently. The default is no explicit timeout, which means the kernel context will live the lifetime of the Kerberos service ticket used in its creation. +.TP +.B -N +Tries to avoid PTR lookups for resolving the server name, if the name used at +mount looks like a Fully Qualified Domain Name. This is import to avoid GSSAPI +issues when PTR records are set and to help avoid some kind of MITM attack. >>>> I'm sorry this does not make much sense. Can we use more meaningful terms >>>> like DNS lookups or DNS PTR lookups... something not so condensed >>>> >>>>> Also, could you please talk about who will be needing this new flag and how will >>>>> they know they needed? .SH SEE ALSO .BR rpc.svcgssd (8), .BR kerberos (1), diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c index ea01e92e4565670b97dea1a936d2f0dbdc7c4610..0a584d2084d626ee98a8ad6ff56da95464b882fe 100644 --- a/utils/gssd/gssd_proc.c +++ b/utils/gssd/gssd_proc.c @@ -67,6 +67,7 @@ #include <errno.h> #include <gssapi/gssapi.h> #include <netdb.h> +#include <ctype.h> #include "gssd.h" #include "err_util.h" @@ -107,6 +108,8 @@ struct pollfd * pollarray; unsigned long pollsize; /* the size of pollaray (in pollfd's) */ +int avoid_ptr = 0; + /* * convert a presentation address string to a sockaddr_storage struct. Returns * true on success or false on failure. @@ -165,12 +168,32 @@ addrstr_to_sockaddr(struct sockaddr *sa, const char *node, const char *port) * convert a sockaddr to a hostname */ static char * -sockaddr_to_hostname(const struct sockaddr *sa, const char *addr) +get_servername(const char *name, const struct sockaddr *sa, const char *addr) { socklen_t addrlen; int err; char *hostname; char hbuf[NI_MAXHOST]; + const char *p; + int do_ptr_lookup = 0; + + if (avoid_ptr) { + /* try to determine if this is FQDN, or an IP address with + * basic heuristics, if they fail try a PTR lookup */ + p = strrchr(name, '.'); + if (!p || strchr(name, ':')) + do_ptr_lookup = 1; /* non-FQDN, or IPv6 */ + else { + for (++p; *p; p++) + if (!isdigit(*p)) + break; + if (!*p) + do_ptr_lookup = 1; /* IPv4 */ + } + if (!do_ptr_lookup) { + return strdup(name); + } + } Is this "reinventing the wheel"? ;-) Don't we already have a why of determining a FQDN? Maybe not...