[PATCH] cifs.upcall: make using ip address conditional on new option
Q (Igor Mammedov)
niallain at gmail.com
Thu Aug 20 14:33:02 MDT 2009
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 at 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 at 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
>
>
More information about the samba-technical
mailing list