From: Simo Sorce <[email protected]>
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
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.
Signed-off-by: Simo Sorce <[email protected]>
Signed-off-by: Steve Dickson <[email protected]>
---
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
.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);
+ }
+ }
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
On 19/04/13 10:16, Steve Dickson wrote:
> From: Simo Sorce <[email protected]>
>
> 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
>
> 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.
>
> Signed-off-by: Simo Sorce <[email protected]>
> Signed-off-by: Steve Dickson <[email protected]>
Committed...
steved.
> ---
> 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
> .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);
> + }
> + }
>
> 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;
>
>
Hey Neil,
On 27/05/13 19:11, NeilBrown wrote:
> 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.
Hmmm... I thought commit f9f5450f, which is in 1.2.8, resolved
that issue... What am I'm missing?
>
> Can we get a 1.2.9 that really fixes this issue?
Sure... I'm all for it!
steved.
On Tue, 28 May 2013 14:40:58 -0400 Steve Dickson <[email protected]> wrote:
>
>
> On 01/05/13 23:13, NeilBrown wrote:
> > 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 <[email protected]>
> Committed....
>
Thanks :-)
NeilBrown
On 02/05/13 02:53, NeilBrown wrote:
> On Thu, 2 May 2013 13:13:32 +1000 NeilBrown <[email protected]> wrote:
>
>
>> Possible patch below.
>
> BTW if you end up applying that without requiring a second revision, please
> add
>
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -67,7 +67,6 @@
> #include <errno.h>
> #include <gssapi/gssapi.h>
> #include <netdb.h>
> -#include <ctype.h>
>
> #include "gssd.h"
> #include "err_util.h"
>
>
> original patch added this include, but doesn't need it.
I'll remove it when I apply your other patch that
fixes is_subdirectory...
thanks!
steved.
Coming to the party a bit late....
On Fri, 19 Apr 2013 10:16:38 -0400 Steve Dickson <[email protected]> wrote:
> From: Simo Sorce <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Steve Dickson <[email protected]>
> ---
> 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 <[email protected]>
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) {
On Thu, 2 May 2013 13:13:32 +1000 NeilBrown <[email protected]> wrote:
> Possible patch below.
BTW if you end up applying that without requiring a second revision, please
add
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -67,7 +67,6 @@
#include <errno.h>
#include <gssapi/gssapi.h>
#include <netdb.h>
-#include <ctype.h>
#include "gssd.h"
#include "err_util.h"
original patch added this include, but doesn't need it.
NeilBrown
On 27/05/13 19:11, NeilBrown wrote:
> 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?
Ok... I did miss your analysis and patch at the bottom of
http://marc.info/?l=linux-nfs&m=136746443205479&w=2
Sorry about that... I'm on it...
steved.
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 <[email protected]> wrote:
>
> Coming to the party a bit late....
>
> On Fri, 19 Apr 2013 10:16:38 -0400 Steve Dickson <[email protected]> wrote:
>
> > From: Simo Sorce <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Steve Dickson <[email protected]>
> > ---
> > 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 <[email protected]>
>
>
> 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) {
I really disliked the man page wording in the original patch. Yours is much
better.
Note that it's possible to have a fqdn with no dots in it, but I expect this
will never happen.
On 02/05/13 02:53, NeilBrown wrote:
> On Thu, 2 May 2013 13:13:32 +1000 NeilBrown <[email protected]> wrote:
>
>
>> Possible patch below.
>
> BTW if you end up applying that without requiring a second revision, please
> add
>
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -67,7 +67,6 @@
> #include <errno.h>
> #include <gssapi/gssapi.h>
> #include <netdb.h>
> -#include <ctype.h>
>
> #include "gssd.h"
> #include "err_util.h"
>
>
> original patch added this include, but doesn't need it.
Fixed...
steved.
>
> NeilBrown
>
On Thu, 2013-05-02 at 01:56 -0400, Jim Rees wrote:
> I really disliked the man page wording in the original patch. Yours is much
> better.
I agree the man page wording is much better +1
> Note that it's possible to have a fqdn with no dots in it, but I expect this
> will never happen.
A FQDN with no dots in it is a top level domain name, it is extremely
unlikely you will have a NFS server that uses a TLD as name although
theoretically possible :-)
Simo.
--
Simo Sorce * Red Hat, Inc * New York
On 01/05/13 23:13, NeilBrown wrote:
> 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 <[email protected]>
Committed....
steved.
>
>
> 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) {