Patchwork cifs.upcall: make using ip address conditional on new option

login
register
mail settings
Submitter Jeff Layton
Date Aug. 19, 2009, 5:30 p.m.
Message ID <1250703037-3640-1-git-send-email-jlayton@redhat.com>
Download mbox | patch
Permalink /patch/31663/
State New
Headers show

Comments

Jeff Layton - Aug. 19, 2009, 5:30 p.m.
Igor Mammedov pointed out that reverse resolving an IP address to get
the hostname portion of a principal could open a possible attack
vector. If an attacker were to gain control of DNS, then he could
redirect the mount to a server of his choosing, and fix the reverse
resolution to point to a hostname of his choosing (one where he has
the key for the corresponding cifs/ or host/ principal).

That said, we often trust DNS for other reasons and it can be useful
to do so. Make the code that allows trusting DNS to be enabled by
adding --trust-dns to the cifs.upcall invocation.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 client/cifs.upcall.c |   62 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 40 insertions(+), 22 deletions(-)
Igor Mammedov - Aug. 20, 2009, 8:39 p.m.
I'm ok with this path, as far as it's not a default behavior and
requires an explicit
option to enable an insecure behavior (and documented in man pages).

Probably logging appropriate warning in syslog each time it is used wouldn't
hurt either. Lets scare people to the death in the hope that successors will fix
dns/AD settings. ;)

PS:
A year ago, I sent to the list patches that allowed to use krb5 && DFS with
screwed up DNS settings and were vulnerable to the same attack vector.
http://lists.samba.org/archive/linux-cifs-client/2008-August/003348.html
The conclusion of a long discussion was something like this, use ntlm/ntlmv2
if it isn't possible to use kerberos in a secure way.

Jeff,
You may be interested in opinion of other participants of the
mentioned discussion.

On Wed, Aug 19, 2009 at 9:30 PM, Jeff Layton<jlayton@redhat.com> wrote:
> Igor Mammedov pointed out that reverse resolving an IP address to get
> the hostname portion of a principal could open a possible attack
> vector. If an attacker were to gain control of DNS, then he could
> redirect the mount to a server of his choosing, and fix the reverse
> resolution to point to a hostname of his choosing (one where he has
> the key for the corresponding cifs/ or host/ principal).
>
> That said, we often trust DNS for other reasons and it can be useful
> to do so. Make the code that allows trusting DNS to be enabled by
> adding --trust-dns to the cifs.upcall invocation.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  client/cifs.upcall.c |   62 ++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 40 insertions(+), 22 deletions(-)
>
> diff --git a/client/cifs.upcall.c b/client/cifs.upcall.c
> index f06d563..1645322 100644
> --- a/client/cifs.upcall.c
> +++ b/client/cifs.upcall.c
> @@ -154,9 +154,9 @@ handle_krb5_mech(const char *oid, const char *principal, DATA_BLOB *secblob,
>  #define DKD_HAVE_IP            0x8
>  #define DKD_HAVE_UID           0x10
>  #define DKD_HAVE_PID           0x20
> -#define DKD_MUSTHAVE_SET (DKD_HAVE_IP|DKD_HAVE_VERSION|DKD_HAVE_SEC)
> +#define DKD_MUSTHAVE_SET (DKD_HAVE_HOSTNAME|DKD_HAVE_VERSION|DKD_HAVE_SEC)
>
> -static struct decoded_args {
> +struct decoded_args {
>        int             ver;
>        char            *hostname;
>        char            *ip;
> @@ -354,11 +354,12 @@ ip_to_fqdn(const char *addrstr, char *host, size_t hostlen)
>  static void
>  usage(void)
>  {
> -       syslog(LOG_INFO, "Usage: %s [-v] key_serial", prog);
> -       fprintf(stderr, "Usage: %s [-v] key_serial\n", prog);
> +       syslog(LOG_INFO, "Usage: %s [-t] [-v] key_serial", prog);
> +       fprintf(stderr, "Usage: %s [-t] [-v] key_serial\n", prog);
>  }
>
>  const struct option long_options[] = {
> +       { "trust-dns",  0, NULL, 't' },
>        { "version",    0, NULL, 'v' },
>        { NULL,         0, NULL, 0 }
>  };
> @@ -372,19 +373,24 @@ int main(const int argc, char *const argv[])
>        size_t datalen;
>        unsigned int have;
>        long rc = 1;
> -       int c;
> -       char *buf, *princ, *ccname = NULL;
> -       char hostbuf[NI_MAXHOST];
> +       int c, try_dns = 0;
> +       char *buf, *princ = NULL, *ccname = NULL;
> +       char hostbuf[NI_MAXHOST], *host;
>        struct decoded_args arg = { };
>        const char *oid;
>
> +       hostbuf[0] = '\0';
> +
>        openlog(prog, 0, LOG_DAEMON);
>
> -       while ((c = getopt_long(argc, argv, "cv", long_options, NULL)) != -1) {
> +       while ((c = getopt_long(argc, argv, "ctv", long_options, NULL)) != -1) {
>                switch (c) {
>                case 'c':
>                        /* legacy option -- skip it */
>                        break;
> +               case 't':
> +                       try_dns++;
> +                       break;
>                case 'v':
>                        printf("version: %s\n", CIFSSPNEGO_VERSION);
>                        goto out;
> @@ -452,21 +458,18 @@ int main(const int argc, char *const argv[])
>        if (have & DKD_HAVE_PID)
>                ccname = get_krb5_ccname(arg.pid);
>
> -       if (have & DKD_HAVE_IP) {
> -               rc = ip_to_fqdn(arg.ip, hostbuf, sizeof(hostbuf));
> -               if (rc)
> -                       goto out;
> -       }
> +       host = arg.hostname;
>
>        // do mech specific authorization
>        switch (arg.sec) {
>        case MS_KRB5:
>        case KRB5:
> +retry_new_hostname:
>                /* for "cifs/" service name + terminating 0 */
> -               datalen = strnlen(hostbuf, sizeof(hostbuf)) + 5 + 1;
> +               datalen = strlen(host) + 5 + 1;
>                princ = SMB_XMALLOC_ARRAY(char, datalen);
>                if (!princ) {
> -                       rc = 1;
> +                       rc = -ENOMEM;
>                        break;
>                }
>
> @@ -480,21 +483,35 @@ int main(const int argc, char *const argv[])
>                 * getting a host/ principal if that doesn't work.
>                 */
>                strlcpy(princ, "cifs/", datalen);
> -               strlcpy(princ + 5, hostbuf, datalen - 5);
> +               strlcpy(princ + 5, host, datalen - 5);
>                rc = handle_krb5_mech(oid, princ, &secblob, &sess_key, ccname);
> -               if (rc) {
> -                       memcpy(princ, "host/", 5);
> -                       rc = handle_krb5_mech(oid, princ, &secblob, &sess_key,
> -                                               ccname);
> -               }
> +               if (!rc)
> +                       break;
> +
> +               memcpy(princ, "host/", 5);
> +               rc = handle_krb5_mech(oid, princ, &secblob, &sess_key, ccname);
> +               if (!rc)
> +                       break;
> +
> +               if (!try_dns || !(have & DKD_HAVE_IP))
> +                       break;
> +
> +               rc = ip_to_fqdn(arg.ip, hostbuf, sizeof(hostbuf));
> +               if (rc)
> +                       break;
> +
>                SAFE_FREE(princ);
> -               break;
> +               try_dns = 0;
> +               host = hostbuf;
> +               goto retry_new_hostname;
>        default:
>                syslog(LOG_ERR, "sectype: %d is not implemented", arg.sec);
>                rc = 1;
>                break;
>        }
>
> +       SAFE_FREE(princ);
> +
>        if (rc)
>                goto out;
>
> @@ -536,6 +553,7 @@ out:
>        data_blob_free(&sess_key);
>        SAFE_FREE(ccname);
>        SAFE_FREE(arg.hostname);
> +       SAFE_FREE(arg.ip);
>        SAFE_FREE(keydata);
>        return rc;
>  }
> --
> 1.6.2.5
>
>
Jeff Layton - Aug. 26, 2009, 10:29 a.m.
On Wed, 19 Aug 2009 13:30:37 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> Igor Mammedov pointed out that reverse resolving an IP address to get
> the hostname portion of a principal could open a possible attack
> vector. If an attacker were to gain control of DNS, then he could
> redirect the mount to a server of his choosing, and fix the reverse
> resolution to point to a hostname of his choosing (one where he has
> the key for the corresponding cifs/ or host/ principal).
> 
> That said, we often trust DNS for other reasons and it can be useful
> to do so. Make the code that allows trusting DNS to be enabled by
> adding --trust-dns to the cifs.upcall invocation.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  client/cifs.upcall.c |   62 ++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 40 insertions(+), 22 deletions(-)
> 

Pushed to samba master branch (along with a corresponding manpage update).
Simo Sorce - Aug. 26, 2009, 12:02 p.m.
On Wed, 2009-08-26 at 06:29 -0400, Jeff Layton wrote:
> On Wed, 19 Aug 2009 13:30:37 -0400
> Jeff Layton <jlayton@redhat.com> wrote:
> 
> > Igor Mammedov pointed out that reverse resolving an IP address to get
> > the hostname portion of a principal could open a possible attack
> > vector. If an attacker were to gain control of DNS, then he could
> > redirect the mount to a server of his choosing, and fix the reverse
> > resolution to point to a hostname of his choosing (one where he has
> > the key for the corresponding cifs/ or host/ principal).
> > 
> > That said, we often trust DNS for other reasons and it can be useful
> > to do so. Make the code that allows trusting DNS to be enabled by
> > adding --trust-dns to the cifs.upcall invocation.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  client/cifs.upcall.c |   62 ++++++++++++++++++++++++++++++++-----------------
> >  1 files changed, 40 insertions(+), 22 deletions(-)
> > 
> 
> Pushed to samba master branch (along with a corresponding manpage update).

We discussed this a few times in the past, I have no objections to the
patch, I am only wondering if the default shouldn't be reversed and make
only paranoid people disable it ?

Simo.
Jeff Layton - Aug. 26, 2009, 12:35 p.m.
On Wed, 26 Aug 2009 08:02:59 -0400
simo <idra@samba.org> wrote:

> On Wed, 2009-08-26 at 06:29 -0400, Jeff Layton wrote:
> > On Wed, 19 Aug 2009 13:30:37 -0400
> > Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > > Igor Mammedov pointed out that reverse resolving an IP address to get
> > > the hostname portion of a principal could open a possible attack
> > > vector. If an attacker were to gain control of DNS, then he could
> > > redirect the mount to a server of his choosing, and fix the reverse
> > > resolution to point to a hostname of his choosing (one where he has
> > > the key for the corresponding cifs/ or host/ principal).
> > > 
> > > That said, we often trust DNS for other reasons and it can be useful
> > > to do so. Make the code that allows trusting DNS to be enabled by
> > > adding --trust-dns to the cifs.upcall invocation.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  client/cifs.upcall.c |   62 ++++++++++++++++++++++++++++++++-----------------
> > >  1 files changed, 40 insertions(+), 22 deletions(-)
> > > 
> > 
> > Pushed to samba master branch (along with a corresponding manpage update).
> 
> We discussed this a few times in the past, I have no objections to the
> patch, I am only wondering if the default shouldn't be reversed and make
> only paranoid people disable it ?
> 

*shrug*

The attack vector is a little contrived, but it is valid. When in
doubt, it's probably best to make the safest option the default and
require a conscious step to lower security.

Patch

diff --git a/client/cifs.upcall.c b/client/cifs.upcall.c
index f06d563..1645322 100644
--- a/client/cifs.upcall.c
+++ b/client/cifs.upcall.c
@@ -154,9 +154,9 @@  handle_krb5_mech(const char *oid, const char *principal, DATA_BLOB *secblob,
 #define DKD_HAVE_IP		0x8
 #define DKD_HAVE_UID		0x10
 #define DKD_HAVE_PID		0x20
-#define DKD_MUSTHAVE_SET (DKD_HAVE_IP|DKD_HAVE_VERSION|DKD_HAVE_SEC)
+#define DKD_MUSTHAVE_SET (DKD_HAVE_HOSTNAME|DKD_HAVE_VERSION|DKD_HAVE_SEC)
 
-static struct decoded_args {
+struct decoded_args {
 	int		ver;
 	char		*hostname;
 	char		*ip;
@@ -354,11 +354,12 @@  ip_to_fqdn(const char *addrstr, char *host, size_t hostlen)
 static void
 usage(void)
 {
-	syslog(LOG_INFO, "Usage: %s [-v] key_serial", prog);
-	fprintf(stderr, "Usage: %s [-v] key_serial\n", prog);
+	syslog(LOG_INFO, "Usage: %s [-t] [-v] key_serial", prog);
+	fprintf(stderr, "Usage: %s [-t] [-v] key_serial\n", prog);
 }
 
 const struct option long_options[] = {
+	{ "trust-dns",	0, NULL, 't' },
 	{ "version",	0, NULL, 'v' },
 	{ NULL,		0, NULL, 0 }
 };
@@ -372,19 +373,24 @@  int main(const int argc, char *const argv[])
 	size_t datalen;
 	unsigned int have;
 	long rc = 1;
-	int c;
-	char *buf, *princ, *ccname = NULL;
-	char hostbuf[NI_MAXHOST];
+	int c, try_dns = 0;
+	char *buf, *princ = NULL, *ccname = NULL;
+	char hostbuf[NI_MAXHOST], *host;
 	struct decoded_args arg = { };
 	const char *oid;
 
+	hostbuf[0] = '\0';
+
 	openlog(prog, 0, LOG_DAEMON);
 
-	while ((c = getopt_long(argc, argv, "cv", long_options, NULL)) != -1) {
+	while ((c = getopt_long(argc, argv, "ctv", long_options, NULL)) != -1) {
 		switch (c) {
 		case 'c':
 			/* legacy option -- skip it */
 			break;
+		case 't':
+			try_dns++;
+			break;
 		case 'v':
 			printf("version: %s\n", CIFSSPNEGO_VERSION);
 			goto out;
@@ -452,21 +458,18 @@  int main(const int argc, char *const argv[])
 	if (have & DKD_HAVE_PID)
 		ccname = get_krb5_ccname(arg.pid);
 
-	if (have & DKD_HAVE_IP) {
-		rc = ip_to_fqdn(arg.ip, hostbuf, sizeof(hostbuf));
-		if (rc)
-			goto out;
-	}
+	host = arg.hostname;
 
 	// do mech specific authorization
 	switch (arg.sec) {
 	case MS_KRB5:
 	case KRB5:
+retry_new_hostname:
 		/* for "cifs/" service name + terminating 0 */
-		datalen = strnlen(hostbuf, sizeof(hostbuf)) + 5 + 1;
+		datalen = strlen(host) + 5 + 1;
 		princ = SMB_XMALLOC_ARRAY(char, datalen);
 		if (!princ) {
-			rc = 1;
+			rc = -ENOMEM;
 			break;
 		}
 
@@ -480,21 +483,35 @@  int main(const int argc, char *const argv[])
 		 * getting a host/ principal if that doesn't work.
 		 */
 		strlcpy(princ, "cifs/", datalen);
-		strlcpy(princ + 5, hostbuf, datalen - 5);
+		strlcpy(princ + 5, host, datalen - 5);
 		rc = handle_krb5_mech(oid, princ, &secblob, &sess_key, ccname);
-		if (rc) {
-			memcpy(princ, "host/", 5);
-			rc = handle_krb5_mech(oid, princ, &secblob, &sess_key,
-						ccname);
-		}
+		if (!rc)
+			break;
+
+		memcpy(princ, "host/", 5);
+		rc = handle_krb5_mech(oid, princ, &secblob, &sess_key, ccname);
+		if (!rc)
+			break;
+
+		if (!try_dns || !(have & DKD_HAVE_IP))
+			break;
+
+		rc = ip_to_fqdn(arg.ip, hostbuf, sizeof(hostbuf));
+		if (rc)
+			break;
+
 		SAFE_FREE(princ);
-		break;
+		try_dns = 0;
+		host = hostbuf;
+		goto retry_new_hostname;
 	default:
 		syslog(LOG_ERR, "sectype: %d is not implemented", arg.sec);
 		rc = 1;
 		break;
 	}
 
+	SAFE_FREE(princ);
+
 	if (rc)
 		goto out;
 
@@ -536,6 +553,7 @@  out:
 	data_blob_free(&sess_key);
 	SAFE_FREE(ccname);
 	SAFE_FREE(arg.hostname);
+	SAFE_FREE(arg.ip);
 	SAFE_FREE(keydata);
 	return rc;
 }