| Message ID | 1250703037-3640-1-git-send-email-jlayton@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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).
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.
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.
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; }
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(-)