[PATCH] cifs.upcall: make using ip address conditional on new option

[Jeff Layton-2]

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

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Wed, 19 Aug 2009 13:30:37 -0400
Jeff Layton <jlayton@...> 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@...>
> ---
>  client/cifs.upcall.c |   62 ++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 40 insertions(+), 22 deletions(-)
>

Pushed to samba master branch (along with a corresponding manpage update).

--
Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[simo-7]

On Wed, 2009-08-26 at 06:29 -0400, Jeff Layton wrote:

> On Wed, 19 Aug 2009 13:30:37 -0400
> Jeff Layton <jlayton@...> 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@...>
> > ---
> >  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.

--
Simo Sorce
Samba Team GPL Compliance Officer <simo@...>
Principal Software Engineer at Red Hat, Inc. <simo@...>

_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Wed, 26 Aug 2009 08:02:59 -0400
simo <idra@...> 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@...> 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@...>
> > > ---
> > >  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.

--
Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client