[PATCH] Avoid PTR lookups when possible

[PATCH] Avoid PTR lookups when possible

[Simo Sorce]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

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 ...).

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[-- Attachment #2: 0001-Avoid-forced-reverse-resolution-for-server-name.patch --]
[-- Type: text/x-patch, Size: 5483 bytes --]

>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",
 		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 ]
 .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.
 .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);
+		}
+	}
 
 	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

Re: [PATCH] Avoid PTR lookups when possible

[Jeff Layton]

On Tue, 02 Apr 2013 09:45:51 -0400
Simo Sorce <simo@redhat.com> wrote:

> 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",
>  		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 ]
>  .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.
>  .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);
> +		}
> +	}
>  
>  	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;
>  

Ok, so with this we expect that users with "marginal" setups will
break. At that point they'll need to hunt down the '-N' switch to gssd
and turn it on (or fix their name resolution).

What I had in mind was a slightly more gentle transition:

Add a switch that can be on (new behavior), off (old behavior) or
default. The default will start at "off" and then in a few releases,
switch to "on".

In the meantime, if the switch is not set explicitly, throw a warning
to syslog when the daemon starts that explains what the default is, and
in what release it will change to the new behavior.

Then when the merge window for that release comes, you can reverse the
default and remove the syslog warning. Eventually (in a year or two?)
we could consider deprecating the new option altogether.

I do agree that it's a pain, but it at least gives users some warning
and recourse for this sort of behavior change. It may also forstall some
pleas for help on the mailing lists.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] Avoid PTR lookups when possible

[Jim Rees]

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.

What happens if it's a partially qualified domain name?

Wouldn't it be better to use something like inet_pton?

I agree that insisting on correct PTR records is a bad idea, but I don't
understand your threat model. It shouldn't be possible for an attacker to do
anything bad by redirecting the client to a spoof server. If it is possible,
we've got bigger problems. How do you think that would work exactly?

Re: [PATCH] Avoid PTR lookups when possible

[Simo Sorce]

On Tue, 2013-04-02 at 11:00 -0400, Jim Rees wrote:
> 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.
> 
> What happens if it's a partially qualified domain name?

you use the "-N off" switch and rely on reverse resolution I guess.
(about to send patch that fixes -N as Jeff asked).

> Wouldn't it be better to use something like inet_pton?

I'll look.

> I agree that insisting on correct PTR records is a bad idea, but I don't
> understand your threat model. It shouldn't be possible for an attacker to do
> anything bad by redirecting the client to a spoof server. If it is possible,
> we've got bigger problems. How do you think that would work exactly?

It's a MITM setting.

Suppose you have 2 servers:

secure.server.name where you store confidential documents
public.server.name where everyone have access for reading (and you can
write)

If you mount -t nfs secure.server.name/export and rely on PTR records
and an attacker can spoof your PTR request to return public.server.name
you will end up acquiring a ticket for public.server.name from the KDC.
Then the attacker will redirect your mount to public.server.name all the
while you think you have mounted secure.server.name

Now anything you copy there goes to a public server instead of your
secure server.

Now imagine your secure server is a backup server which gets mounted by
a cron job every night for backups of sensitive information.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [PATCH] Avoid PTR lookups when possible

[Jim Rees]

Simo Sorce wrote:

  It's a MITM setting.
  
  Suppose you have 2 servers:
  
  secure.server.name where you store confidential documents
  public.server.name where everyone have access for reading (and you can
  write)
  
  If you mount -t nfs secure.server.name/export and rely on PTR records
  and an attacker can spoof your PTR request to return public.server.name
  you will end up acquiring a ticket for public.server.name from the KDC.
  Then the attacker will redirect your mount to public.server.name all the
  while you think you have mounted secure.server.name
  
  Now anything you copy there goes to a public server instead of your
  secure server.
  
  Now imagine your secure server is a backup server which gets mounted by
  a cron job every night for backups of sensitive information.

I guess what you're saying is that the client might not know the full name
of the secure server, and could be tricked into thinking it's
public.server.name. That's not what I'd call mitm; kerberos is pretty much
immune to mitm. It's not really a dns mitm either, it's a spoofed reply.

I would be in favor of requiring kerberized nfs clients to know the full
name of the server they're trying to connect to. Kerberized nfs clients
should not be relying on dns for any part of their security. But I guess we
haven't been doing that, and to make that a requirement now would be a
compatibility problem.

Re: [PATCH] Avoid PTR lookups when possible

[Simo Sorce]

On Tue, 2013-04-02 at 12:46 -0400, Jim Rees wrote:
> Simo Sorce wrote:
> 
>   It's a MITM setting.
>   
>   Suppose you have 2 servers:
>   
>   secure.server.name where you store confidential documents
>   public.server.name where everyone have access for reading (and you can
>   write)
>   
>   If you mount -t nfs secure.server.name/export and rely on PTR records
>   and an attacker can spoof your PTR request to return public.server.name
>   you will end up acquiring a ticket for public.server.name from the KDC.
>   Then the attacker will redirect your mount to public.server.name all the
>   while you think you have mounted secure.server.name
>   
>   Now anything you copy there goes to a public server instead of your
>   secure server.
>   
>   Now imagine your secure server is a backup server which gets mounted by
>   a cron job every night for backups of sensitive information.
> 
> I guess what you're saying is that the client might not know the full name
> of the secure server, and could be tricked into thinking it's
> public.server.name. That's not what I'd call mitm; kerberos is pretty much
> immune to mitm. It's not really a dns mitm either, it's a spoofed reply.

No this is not what I am saying.
The client uses the fully qualified name at the mount command but due to
the use of a PTR lookup it is fooled into acquiring a ticket for the
wrong server which will then happily allow you to connect.

Currently:
The mount command tells the kernel both the server name
'secure.server.name' and it's IP address 1.2.3.4 however rpc.gssd
currently completely ignored the server name passed in by the mount
command (see the 'dummy' variable) and instead uses the ip address to do
a reverse lookup.

PTR lookup of 1.2.3.4 --- MITM ---> public.server.com

Then it uses the retrieved name (public..) to construct the GSSAPI name
it wants to connect to, the result is that the name
nfs@public.server.name is constructed instead of the requested
nfs@secure.server.com that should be requested.

So the client does ask for the right server but can be easily tricked
into connecting to another one.

Kerberos itself *is* correctly identifying the server, it's rpc.gssd
that opened it'sself to being fooled before involving Kerberos.

> I would be in favor of requiring kerberized nfs clients to know the full
> name of the server they're trying to connect to.

Yes, this would be my preference too.

>  Kerberized nfs clients
> should not be relying on dns for any part of their security. But I guess we
> haven't been doing that, and to make that a requirement now would be a
> compatibility problem.

Yes which is why I am adding a switch, I strongly suggest distribution
to use -N on in newer releases and allow -N off only for existing setups
for compatibility reasons.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [PATCH] Avoid PTR lookups when possible

[Jim Rees]

Simo Sorce wrote:

  No this is not what I am saying.
  The client uses the fully qualified name at the mount command but due to
  the use of a PTR lookup it is fooled into acquiring a ticket for the
  wrong server which will then happily allow you to connect.
  
  Currently:
  The mount command tells the kernel both the server name
  'secure.server.name' and it's IP address 1.2.3.4 however rpc.gssd
  currently completely ignored the server name passed in by the mount
  command (see the 'dummy' variable) and instead uses the ip address to do
  a reverse lookup.
  
  PTR lookup of 1.2.3.4 --- MITM ---> public.server.com
  
  Then it uses the retrieved name (public..) to construct the GSSAPI name
  it wants to connect to, the result is that the name
  nfs@public.server.name is constructed instead of the requested
  nfs@secure.server.com that should be requested.
  
  So the client does ask for the right server but can be easily tricked
  into connecting to another one.
  
  Kerberos itself *is* correctly identifying the server, it's rpc.gssd
  that opened it'sself to being fooled before involving Kerberos.

Ok, I think I understand now. I suggest adding some of that text into the
commit log. And stop using the term "mitm". A mitm attack is used to
convince both ends of a connection that they are talking to each other. DNS
is not a mutually authenticated exchange.

This discussion is making me nervous. It makes me think no one has thought
through the use of kerberos to authenticate nfs connections on linux.

Re: [PATCH] Avoid PTR lookups when possible

[Simo Sorce]

On Tue, 2013-04-02 at 14:39 -0400, Jim Rees wrote:
> Simo Sorce wrote:
> 
>   No this is not what I am saying.
>   The client uses the fully qualified name at the mount command but due to
>   the use of a PTR lookup it is fooled into acquiring a ticket for the
>   wrong server which will then happily allow you to connect.
>   
>   Currently:
>   The mount command tells the kernel both the server name
>   'secure.server.name' and it's IP address 1.2.3.4 however rpc.gssd
>   currently completely ignored the server name passed in by the mount
>   command (see the 'dummy' variable) and instead uses the ip address to do
>   a reverse lookup.
>   
>   PTR lookup of 1.2.3.4 --- MITM ---> public.server.com
>   
>   Then it uses the retrieved name (public..) to construct the GSSAPI name
>   it wants to connect to, the result is that the name
>   nfs@public.server.name is constructed instead of the requested
>   nfs@secure.server.com that should be requested.
>   
>   So the client does ask for the right server but can be easily tricked
>   into connecting to another one.
>   
>   Kerberos itself *is* correctly identifying the server, it's rpc.gssd
>   that opened it'sself to being fooled before involving Kerberos.
> 
> Ok, I think I understand now. I suggest adding some of that text into the
> commit log.

To be honest this is a well known problem in the kerberos community, I
will see if I can find a public text that references this problem and
add a link to it in the commit log ?

It's a bit of a long explanation otherwise ... maybe I can point at this
thread in the archives ?

>  And stop using the term "mitm". A mitm attack is used to
> convince both ends of a connection that they are talking to each other. DNS
> is not a mutually authenticated exchange.

Well it is still a sort of Man in the Middle, as you also have to
redirect communications (nfsv4 uses TCP) for it to be effective, it is
just not exploiting a crypto issue.

> This discussion is making me nervous. It makes me think no one has thought
> through the use of kerberos to authenticate nfs connections on linux.

I think this is just an oversight, this 'bug' has been present since the
original patches have been merged in.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [PATCH] Avoid PTR lookups when possible

[Jim Rees]

Simo Sorce wrote:

  >  And stop using the term "mitm". A mitm attack is used to
  > convince both ends of a connection that they are talking to each other. DNS
  > is not a mutually authenticated exchange.
  
  Well it is still a sort of Man in the Middle, as you also have to
  redirect communications (nfsv4 uses TCP) for it to be effective, it is
  just not exploiting a crypto issue.

Now you've lost me again. I thought we were discussing dns. What does nfs
have to do with it?

Re: [PATCH] Avoid PTR lookups when possible

[Simo Sorce]

On Tue, 2013-04-02 at 14:53 -0400, Jim Rees wrote:
> Simo Sorce wrote:
> 
>   >  And stop using the term "mitm". A mitm attack is used to
>   > convince both ends of a connection that they are talking to each other. DNS
>   > is not a mutually authenticated exchange.
>   
>   Well it is still a sort of Man in the Middle, as you also have to
>   redirect communications (nfsv4 uses TCP) for it to be effective, it is
>   just not exploiting a crypto issue.
> 
> Now you've lost me again. I thought we were discussing dns. What does nfs
> have to do with it?

It's complicated, but if you re-read the scenario I wrote and think how
the rpcgss communication happens you should see it.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [PATCH] Avoid PTR lookups when possible

[J. Bruce Fields]

On Tue, Apr 02, 2013 at 02:48:39PM -0400, Simo Sorce wrote:
> On Tue, 2013-04-02 at 14:39 -0400, Jim Rees wrote:
> > Simo Sorce wrote:
> > 
> >   No this is not what I am saying.
> >   The client uses the fully qualified name at the mount command but due to
> >   the use of a PTR lookup it is fooled into acquiring a ticket for the
> >   wrong server which will then happily allow you to connect.
> >   
> >   Currently:
> >   The mount command tells the kernel both the server name
> >   'secure.server.name' and it's IP address 1.2.3.4 however rpc.gssd
> >   currently completely ignored the server name passed in by the mount
> >   command (see the 'dummy' variable) and instead uses the ip address to do
> >   a reverse lookup.
> >   
> >   PTR lookup of 1.2.3.4 --- MITM ---> public.server.com
> >   
> >   Then it uses the retrieved name (public..) to construct the GSSAPI name
> >   it wants to connect to, the result is that the name
> >   nfs@public.server.name is constructed instead of the requested
> >   nfs@secure.server.com that should be requested.
> >   
> >   So the client does ask for the right server but can be easily tricked
> >   into connecting to another one.
> >   
> >   Kerberos itself *is* correctly identifying the server, it's rpc.gssd
> >   that opened it'sself to being fooled before involving Kerberos.
> > 
> > Ok, I think I understand now. I suggest adding some of that text into the
> > commit log.
> 
> To be honest this is a well known problem in the kerberos community, I
> will see if I can find a public text that references this problem and
> add a link to it in the commit log ?
> 
> It's a bit of a long explanation otherwise ... maybe I can point at this
> thread in the archives ?
> 
> >  And stop using the term "mitm". A mitm attack is used to
> > convince both ends of a connection that they are talking to each other. DNS
> > is not a mutually authenticated exchange.
> 
> Well it is still a sort of Man in the Middle, as you also have to
> redirect communications (nfsv4 uses TCP) for it to be effective, it is
> just not exploiting a crypto issue.

Like Jim I don't understand how that fits the definition of a "man in
the middle" attack.

I also wonder whether "GSSAPI name" is specialized jargon.  And I don't
understand what you mean by "tries to avoid" reverse lookups--does it
use them or not?  How about:

	With this option, gssd expects the name given on the mount
	command line to be exactly the name that the server
	authenticates as.  This option is strongly recommended.

	Without this option, gssd will instead do a reverse lookup on
	the server's IP address to decide what name it should use.  This
	is vulnerable to attacks on DNS, and is fragile in cases where
	reverse DNS (PTR) records are not set up correctly.

--b.

Re: [PATCH] Avoid PTR lookups when possible

[Jim Rees]

J. Bruce Fields wrote:

  Like Jim I don't understand how that fits the definition of a "man in
  the middle" attack.

If Bruce is having trouble too, I think the explanation needs to be
improved. But I think the latest patch set drops the talk about a
vulnerability, and I'm fine with that.

Re: [PATCH] Avoid PTR lookups when possible

[Simo Sorce]

On Wed, 2013-04-03 at 10:40 -0400, Jim Rees wrote:
> J. Bruce Fields wrote:
> 
>   Like Jim I don't understand how that fits the definition of a "man in
>   the middle" attack.
> 
> If Bruce is having trouble too, I think the explanation needs to be
> improved. But I think the latest patch set drops the talk about a
> vulnerability, and I'm fine with that.

Yeah I figured that just concentrating on the functional aspect of being
able to work on networks with missing PTR records is sufficient to
explain why this patchset is good :-)

However let me give a try at explaining in a different way using the
same example I gave before but from a different angle.

- Assume user Alice has very important documents that are backed up
daily to a server named secure.server.name where Alice can write to and
nobody can read from.
- Assume an attacker Eve, that is in the condition of intercepting and
modifying all network traffic from Alice.
- Assume there exist another server called public.server.name where
Alice can write to and Eve can read from.

Eve wants to fool Alice in mounting /export from public.server.name
instead or secure.server.name so she can copy Alice's documents away.


So Eve, manipulates traffic and when Alice's machine tries to talk to
secure.server.name, Eve redirects all the traffic to public.server.name
instead. (Can be done by a simple DNS poisoning attack that gives back
to Alice computer the public.server.name IP address, or by actual DNAT
on Alice's computer packets).
Now, normally, if Krb5 authentication is used the mount would fail,
because Alice would try to authenticate to public.server.com using a
ticket she obtained to talk to secure.server.com (remember that Alice
thinks she is mounting the secure.server.name share).

Here the reverse DNS resolution performed by Alice's computer's rpc.gssd
daemon is what allows Eve to successfully redirect Alice's mount
instead. Because at the time Alice's computer needs to grab a ticket for
the 'target' server, Eve fakes the DNS replies and tells Alice's
computer that the destination name for the secure.server.name's IP
address is 'public.server.name'.

So with current behavior Alice's computer asks the KDC for a ticket for
'public.server.name' instead of 'secure.server.name'.

Now Krb5 authentication does not fail anymore because Alice does have
the 'right' ticket for the server it is connecting to.

Alice authentication against the 'substituted' (by Eve, through packet
redirection) server therefore succeeds, and her computer is fooled to
think it really mounted secure.server.name when it really has mounted
public.server.name, the cron job for the backup starts and copies all
files to public.server.name, where Eve can find and access them.

Call it MITM, call it something else, it's an attack, and we can prevent
it.

HTH,
Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [PATCH] Avoid PTR lookups when possible

[J. Bruce Fields]

On Wed, Apr 03, 2013 at 12:03:38PM -0400, Simo Sorce wrote:
> On Wed, 2013-04-03 at 10:40 -0400, Jim Rees wrote:
> > J. Bruce Fields wrote:
> > 
> >   Like Jim I don't understand how that fits the definition of a "man in
> >   the middle" attack.
> > 
> > If Bruce is having trouble too, I think the explanation needs to be
> > improved. But I think the latest patch set drops the talk about a
> > vulnerability, and I'm fine with that.
> 
> Yeah I figured that just concentrating on the functional aspect of being
> able to work on networks with missing PTR records is sufficient to
> explain why this patchset is good :-)

Definitely, you're fine.  I was just nitpicking language, the problem's
clear enough:

> However let me give a try at explaining in a different way using the
> same example I gave before but from a different angle.
> 
> - Assume user Alice has very important documents that are backed up
> daily to a server named secure.server.name where Alice can write to and
> nobody can read from.
> - Assume an attacker Eve, that is in the condition of intercepting and
> modifying all network traffic from Alice.
> - Assume there exist another server called public.server.name where
> Alice can write to and Eve can read from.
> 
> Eve wants to fool Alice in mounting /export from public.server.name
> instead or secure.server.name so she can copy Alice's documents away.
> 
> 
> So Eve, manipulates traffic and when Alice's machine tries to talk to
> secure.server.name, Eve redirects all the traffic to public.server.name
> instead. (Can be done by a simple DNS poisoning attack that gives back
> to Alice computer the public.server.name IP address, or by actual DNAT
> on Alice's computer packets).
> Now, normally, if Krb5 authentication is used the mount would fail,
> because Alice would try to authenticate to public.server.com using a
> ticket she obtained to talk to secure.server.com (remember that Alice
> thinks she is mounting the secure.server.name share).
> 
> Here the reverse DNS resolution performed by Alice's computer's rpc.gssd
> daemon is what allows Eve to successfully redirect Alice's mount
> instead. Because at the time Alice's computer needs to grab a ticket for
> the 'target' server, Eve fakes the DNS replies and tells Alice's
> computer that the destination name for the secure.server.name's IP
> address is 'public.server.name'.
> 
> So with current behavior Alice's computer asks the KDC for a ticket for
> 'public.server.name' instead of 'secure.server.name'.
> 
> Now Krb5 authentication does not fail anymore because Alice does have
> the 'right' ticket for the server it is connecting to.
> 
> Alice authentication against the 'substituted' (by Eve, through packet
> redirection) server therefore succeeds, and her computer is fooled to
> think it really mounted secure.server.name when it really has mounted
> public.server.name, the cron job for the backup starts and copies all
> files to public.server.name, where Eve can find and access them.

Argh, sorry Simo, I didn't mean to make you write all that out.

> Call it MITM, call it something else,

This was literally my only complaint.  The above doesn't sound to me
like what I'd normally call a "man in the middle" attack.

--b.

Re: [PATCH] Avoid PTR lookups when possible

[Simo Sorce]

On Wed, 2013-04-03 at 13:12 -0400, J. Bruce Fields wrote:
> On Wed, Apr 03, 2013 at 12:03:38PM -0400, Simo Sorce wrote:
> > On Wed, 2013-04-03 at 10:40 -0400, Jim Rees wrote:
> > > J. Bruce Fields wrote:
> > > 
> > >   Like Jim I don't understand how that fits the definition of a "man in
> > >   the middle" attack.
> > > 
> > > If Bruce is having trouble too, I think the explanation needs to be
> > > improved. But I think the latest patch set drops the talk about a
> > > vulnerability, and I'm fine with that.
> > 
> > Yeah I figured that just concentrating on the functional aspect of being
> > able to work on networks with missing PTR records is sufficient to
> > explain why this patchset is good :-)
> 
> Definitely, you're fine.  I was just nitpicking language, the problem's
> clear enough:
> 
> > However let me give a try at explaining in a different way using the
> > same example I gave before but from a different angle.
> > 
> > - Assume user Alice has very important documents that are backed up
> > daily to a server named secure.server.name where Alice can write to and
> > nobody can read from.
> > - Assume an attacker Eve, that is in the condition of intercepting and
> > modifying all network traffic from Alice.
> > - Assume there exist another server called public.server.name where
> > Alice can write to and Eve can read from.
> > 
> > Eve wants to fool Alice in mounting /export from public.server.name
> > instead or secure.server.name so she can copy Alice's documents away.
> > 
> > 
> > So Eve, manipulates traffic and when Alice's machine tries to talk to
> > secure.server.name, Eve redirects all the traffic to public.server.name
> > instead. (Can be done by a simple DNS poisoning attack that gives back
> > to Alice computer the public.server.name IP address, or by actual DNAT
> > on Alice's computer packets).
> > Now, normally, if Krb5 authentication is used the mount would fail,
> > because Alice would try to authenticate to public.server.com using a
> > ticket she obtained to talk to secure.server.com (remember that Alice
> > thinks she is mounting the secure.server.name share).
> > 
> > Here the reverse DNS resolution performed by Alice's computer's rpc.gssd
> > daemon is what allows Eve to successfully redirect Alice's mount
> > instead. Because at the time Alice's computer needs to grab a ticket for
> > the 'target' server, Eve fakes the DNS replies and tells Alice's
> > computer that the destination name for the secure.server.name's IP
> > address is 'public.server.name'.
> > 
> > So with current behavior Alice's computer asks the KDC for a ticket for
> > 'public.server.name' instead of 'secure.server.name'.
> > 
> > Now Krb5 authentication does not fail anymore because Alice does have
> > the 'right' ticket for the server it is connecting to.
> > 
> > Alice authentication against the 'substituted' (by Eve, through packet
> > redirection) server therefore succeeds, and her computer is fooled to
> > think it really mounted secure.server.name when it really has mounted
> > public.server.name, the cron job for the backup starts and copies all
> > files to public.server.name, where Eve can find and access them.
> 
> Argh, sorry Simo, I didn't mean to make you write all that out.

It's ok, I am planning to make it into a blog post, so next time someone
asks about this particular issue with PTR records and GSSAPI I can point
them there. So it is not worthless, I call it a draft :-)

> > Call it MITM, call it something else,
> 
> This was literally my only complaint.  The above doesn't sound to me
> like what I'd normally call a "man in the middle" attack.

Yup, ok.

So if anyone can ack the actual patches (or tell me if there is anything
else to change I'd love to move forward, I have other patches in the
pipeline :-)

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [PATCH] Avoid PTR lookups when possible

[J. Bruce Fields]

On Wed, Apr 03, 2013 at 02:12:57PM -0400, Simo Sorce wrote:
> Yup, ok.
> 
> So if anyone can ack the actual patches (or tell me if there is anything
> else to change I'd love to move forward, I have other patches in the
> pipeline :-)

Look fine to me; I didn't have any objection outside documentation.

I've lost track of which are the latest patches, honestly--hopefully
Steve's been keeping track, it's up to him to merge them.

--b.

Re: [PATCH] Avoid PTR lookups when possible

[Steve Dickson]

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...


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

Re: [PATCH] Avoid PTR lookups when possible

[Chuck Lever]

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

Re: [PATCH] Avoid PTR lookups when possible

[Simo Sorce]

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.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [PATCH] Avoid PTR lookups when possible

[Chuck Lever]

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

Re: [PATCH] Avoid PTR lookups when possible

[Simo Sorce]

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.

-- 
Simo Sorce * Red Hat, Inc * New York