Hi Steve,
what is the status of this issue in your mind?
upstream nfs-utils is still vulnerable to CVE-2013-1923, so probably a few
distros think that have addressed it by upgrading, but haven't.
Can we get a 1.2.9 that really fixes this issue?
Thanks,
NeilBrown
On Thu, 2 May 2013 13:13:32 +1000 NeilBrown <neilb@xxxxxxx> wrote:
>
> Coming to the party a bit late....
>
> On Fri, 19 Apr 2013 10:16:38 -0400 Steve Dickson <steved@xxxxxxxxxx> wrote:
>
> > From: Simo Sorce <simo@xxxxxxxxxx>
> >
> > A NFS client should be able to work properly even if the DNS Reverse
> > record for the server is not set. This means a DNS lookup should not be
> > done on server names at are passed to GSSAPI. This patch changes the default
> > behavior to no longer do those types of lookups
>
> OK, this is confusing, When you do a "DNS lookup .. on server names",
> that is a Forward lookup, not a Reverse lookup.
>
> >
> > This change default behavior could negatively impact some current
> > environments, so the -D option is also being added that will re-enable
> > the DNS reverse looks on server names, which are passed to GSSAPI.
>
> Which current environments? Mine? Maybe the man page update will tell me.
> I'll look.
>
>
> >
> > Signed-off-by: Simo Sorce <simo@xxxxxxxxxx>
> > Signed-off-by: Steve Dickson <steved@xxxxxxxxxx>
> > ---
> > utils/gssd/gss_util.h | 2 ++
> > utils/gssd/gssd.c | 7 +++++--
> > utils/gssd/gssd.man | 8 +++++++-
> > utils/gssd/gssd_proc.c | 31 +++++++++++++++++++++++++++----
> > 4 files changed, 41 insertions(+), 7 deletions(-)
> >
> > diff --git a/utils/gssd/gss_util.h b/utils/gssd/gss_util.h
> > index aa9f778..c81fc5a 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_dns;
> > +
> > #endif /* _GSS_UTIL_H_ */
> > diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
> > index 07b1e52..8ee478b 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] [-D]\n",
> > progname);
> > exit(1);
> > }
> > @@ -102,7 +102,7 @@ main(int argc, char *argv[])
> > char *progname;
> >
> > memset(ccachesearch, 0, sizeof(ccachesearch));
> > - while ((opt = getopt(argc, argv, "fvrlmnMp:k:d:t:R:")) != -1) {
> > + while ((opt = getopt(argc, argv, "DfvrlmnMp:k:d:t:R:")) != -1) {
> > switch (opt) {
> > case 'f':
> > fg = 1;
> > @@ -150,6 +150,9 @@ main(int argc, char *argv[])
> > errx(1, "Encryption type limits not supported by Kerberos libraries.");
> > #endif
> > break;
> > + case 'D':
> > + avoid_dns = 0;
> > + break;
> > default:
> > usage(argv[0]);
> > break;
> > diff --git a/utils/gssd/gssd.man b/utils/gssd/gssd.man
> > index 79d9bf9..1df75c5 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 [ \-DfMnlvr ]
> > .RB [ \-k
> > .IR keytab ]
> > .RB [ \-p
> > @@ -195,6 +195,12 @@ option when starting
> > .BR rpc.gssd .
> > .SH OPTIONS
> > .TP
> > +.B -D
> > +DNS Reverse lookups are not used for determining the
> > +server names pass to GSSAPI. This option will reverses that and forces
> > +the use of DNS Reverse resolution of the server's IP address to
> > +retrieve the server name to use in GSAPI authentication.
> > +.TP
>
> Nope. "This option will reverses that" doesn't give me a lot of confidence
> either.
> May I try?
>
>
> The server name passed to GSSAPI (whatever that is) for authorisation (or is
> it authentication) is normally the name exactly as requested. e.g. for NFS
> it is the server name in the "servername:/path" mount request. Only if this
> servername appears to be an IP address (IPv4 or IPv6) will a reverse DNS lookup
> will be performed to get the canoncial server name.
> If this -D option is present, a reverse DNS lookup will *always* be used,
> even if the server name looks like a canonical name. So it is needed if partially
> qualified, or non canonical names are regularly used.
> Using -D can introduce a security vulnerability, so it is recommended that
> -D not be used, and that canonical names always be used when requesting
> services.
>
> Ahh.. now I know if I might need -D... Assuming the code is correct..
>
>
> > .B -f
> > Runs
> > .B rpc.gssd
> > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > index d6f07e6..e4ab253 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,9 @@ struct pollfd * pollarray;
> >
> > unsigned long pollsize; /* the size of pollaray (in pollfd's) */
> >
> > +/* Avoid DNS reverse lookups on server names */
> > +int avoid_dns = 1;
> > +
> > /*
> > * convert a presentation address string to a sockaddr_storage struct. Returns
> > * true on success or false on failure.
> > @@ -165,12 +169,31 @@ 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];
> > + unsigned char buf[sizeof(struct in6_addr)];
> > + int servername = 0;
> > +
> > + if (avoid_dns) {
> > + /*
> > + * Determine if this is a server name, or an IP address.
> > + * If it is an IP address, do the DNS lookup otherwise
> > + * skip the DNS lookup.
> > + */
> > + servername = 0;
> > + if (strchr(name, '.') && inet_pton(AF_INET, name, buf) == 1)
> > + servername = 1; /* IPv4 */
> > + else if (strchr(name, ':') && inet_pton(AF_INET6, name, buf) == 1)
> > + servername = 1; /* or IPv6 */
> > +
> > + if (servername) {
> > + return strdup(name);
> > + }
> > + }
>
> Uh oh. Look at that code. Now look at me. Now back at the code.
> Now look at this code that was prepared earlier (in Take 2).
>
> + if (avoid_dns) {
> + /*
> + * Determine if this is a server name, or an IP address.
> + * If it is an IP address, do the DNS lookup
> + */
> + if (strchr(name, '.') && inet_pton(AF_INET, name, buf) == 1)
> + do_dns_lookup = 1; /* IPv4 */
> + else if (strchr(name, ':') && inet_pton(AF_INET6, name, buf) == 1)
> + do_dns_lookup = 1; /* or IPv6 */
> +
> + if (!do_dns_lookup) {
> + return strdup(name);
> + }
> + }
>
> So we changed "do_dns_lookup" to "servername", set it to zero one more time
> (just to be sure), and the final condition has been REVERSED.
> So if I use an IP address, that is the string passed back to GSSAPI.
> If I used a fully qualified name (like the man page suggests), then it goes to
> do a reverse DNS lookup for me :-(
>
> And was there a reason for dropping the test on "non-qualified host name"
> that was in the original? That seemed like a good idea to me.
>
> Is inet_pton so expensive that we need to do the 'strchr' first to avoid the
> extra work?
>
> Are we going to get a 1-2-9 once this has been fixed properly ?
>
> Possible patch below. Note that I also needed
>
> diff --git a/utils/statd/simu.c b/utils/statd/simu.c
> index f1d0bf8..b57e504 100644
> --- a/utils/statd/simu.c
> +++ b/utils/statd/simu.c
> @@ -7,6 +7,7 @@
> #ifdef HAVE_CONFIG_H
> #include <config.h>
> #endif
> +#include <stdlib.h>
>
> #include <netdb.h>
> #include <arpa/inet.h>
>
>
> to get a clean compile.
>
> Or mostly clean. On openSUSE I also get:
>
> gssd-context_lucid.o: In function `serialize_krb5_ctx':
> /home/git/nfs-utils/utils/gssd/context_lucid.c:269: undefined reference to `gss_krb5_export_lucid_sec_context'
> /home/git/nfs-utils/utils/gssd/context_lucid.c:305: undefined reference to `gss_krb5_free_lucid_sec_context'
> gssd-krb5_util.o: In function `limit_krb5_enctypes':
> /home/git/nfs-utils/utils/gssd/krb5_util.c:1441: undefined reference to `gss_krb5_set_allowable_enctypes'
> /home/git/nfs-utils/utils/gssd/krb5_util.c:1444: undefined reference to `gss_krb5_set_allowable_enctypes'
> collect2: error: ld returned 1 exit status
>
>
> any idea what is causing that"?
>
>
> Subject: Fix recent fix to Avoid DNS reverse resolution in gssd.
>
> The final version for this fix that was committed inverted the test
> so makes no change in the important cases.
> The documentation didn't really help a naive user know when the new -D flag
> should be used.
> And the code (once fixed) avoided DNS resolution on non-qualified names too,
> which probably isn't a good idea.
>
> This patch fixes all three issues.
>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
>
>
> diff --git a/utils/gssd/gssd.man b/utils/gssd/gssd.man
> index 1df75c5..ac13fd4 100644
> --- a/utils/gssd/gssd.man
> +++ b/utils/gssd/gssd.man
> @@ -195,11 +195,28 @@ option when starting
> .BR rpc.gssd .
> .SH OPTIONS
> .TP
> -.B -D
> -DNS Reverse lookups are not used for determining the
> -server names pass to GSSAPI. This option will reverses that and forces
> -the use of DNS Reverse resolution of the server's IP address to
> -retrieve the server name to use in GSAPI authentication.
> +.B \-D
> +The server name passed to GSSAPI for authentication is normally the
> +name exactly as requested. e.g. for NFS
> +it is the server name in the "servername:/path" mount request. Only if this
> +servername appears to be an IP address (IPv4 or IPv6) or an
> +unqualified name (no dots) will a reverse DNS lookup
> +will be performed to get the canoncial server name.
> +
> +If
> +.B \-D
> +is present, a reverse DNS lookup will
> +.I always
> +be used, even if the server name looks like a canonical name. So it
> +is needed if partially qualified, or non canonical names are regularly
> +used.
> +
> +Using
> +.B \-D
> +can introduce a security vulnerability, so it is recommended that
> +.B \-D
> +not be used, and that canonical names always be used when requesting
> +services.
> .TP
> .B -f
> Runs
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index af1844c..d381664 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -176,7 +176,6 @@ get_servername(const char *name, const struct sockaddr *sa, const char *addr)
> char *hostname;
> char hbuf[NI_MAXHOST];
> unsigned char buf[sizeof(struct in6_addr)];
> - int servername = 0;
>
> if (avoid_dns) {
> /*
> @@ -184,15 +183,18 @@ get_servername(const char *name, const struct sockaddr *sa, const char *addr)
> * If it is an IP address, do the DNS lookup otherwise
> * skip the DNS lookup.
> */
> - servername = 0;
> - if (strchr(name, '.') && inet_pton(AF_INET, name, buf) == 1)
> - servername = 1; /* IPv4 */
> - else if (strchr(name, ':') && inet_pton(AF_INET6, name, buf) == 1)
> - servername = 1; /* or IPv6 */
> -
> - if (servername) {
> + int is_fqdn = 1;
> + if (strchr(name, '.') == NULL)
> + is_fqdn = 0; /* local name */
> + else if (inet_pton(AF_INET, name, buf) == 1)
> + is_fqdn = 0; /* IPv4 address */
> + else if (inet_pton(AF_INET6, name, buf) == 1)
> + is_fqdn = 0; /* IPv6 addrss */
> +
> + if (is_fqdn) {
> return strdup(name);
> }
> + /* Sorry, cannot avoid dns after all */
> }
>
> switch (sa->sa_family) {
Attachment:
signature.asc
Description: PGP signature
