| Message ID | 4981B058.6000805@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 29 Jan 2009 19:04:16 +0530 Suresh Jayaraman <sjayaraman@suse.de> wrote: > Jeff Layton wrote: > > On Tue, 27 Jan 2009 18:29:00 +0530 > > >> (using IP) > >> #kinit Administrator > >> #mount -t cifs -o //164.99.99.182/Winshare /mnt/cifs -o > >> user=Administrator,sec=krb5i > >> > >> fails. > >> > >> I enabled CifsFYI o/p and the only difference I see is: > >> > >> (with hostname) > >> fs/cifs/cifs_spnego.c: key description = > >> ver=0x2;host=myserver;ip4=164.99.99.182;sec=mskrb5;uid=0x0;user=Administrator > >> (with IP) > >> fs/cifs/cifs_spnego.c: key description = > >> ver=0x2;host=164.99.99.182;ip4=164.99.99.182;sec=mskrb5;uid=0x0;user=Administrator > >> > >> * note "host=" parameter ^^^ it contains value of IP, when we use IP to > >> mount. May be this is the problem, passing down ip as "host=" down to > >> request_key() ? > >> > >> In CIFS_SessSetup > >> spnego_key = cifs_get_spnego_key(ses); > >> > >> fails and returns error -126 > > > > The upcall program needs some way to know what cifs or host principal to look > > for. When it just has an IP address to go on, then it often doesn't have a > > way to know. To fix this, we'll need to fix cifs.upcall to be able to make > > better guesses as to the hostname when we try to get the SPNEGO key. > > > > Patches welcome. > > > > Something like this.. > (quick hack just to ensure I get your hint correct - > untested as my setup is goofed up) > > > > Index: source/client/cifs.upcall.c > =================================================================== > --- source/client/cifs.upcall.c.orig > +++ source/client/cifs.upcall.c > @@ -27,6 +27,7 @@ create dns_resolver * * /usr/local/sbin/ > #include "includes.h" > #include <keyutils.h> > > + > #include "cifs_spnego.h" > > const char *CIFSSPNEGO_VERSION = "1.2"; > @@ -113,6 +114,59 @@ decode_key_description(const char *desc, > *hostname = SMB_XMALLOC_ARRAY(char, len); > strlcpy(*hostname, tkn + 5, len); > retval |= DKD_HAVE_HOSTNAME; > + /* May be it's an IP address? > + * request_key() might fail if we pass it down, > + * so attempt to lookup the hostname. > + */ > + if (is_ipaddress(*hostname)) { > + struct sockaddr_storage ss; > + struct sockaddr *addr; > + socklen_t length; > + char *hp = *hostname; > + char host[MAX_DNS_NAME_LENGTH]; > + > + addr = (struct sockaddr *)&ss; > + > + /* IPv4 */ > + if (is_ipaddress_v4(*hostname)) { > + struct sockaddr_in addr4; > + length = sizeof(addr4); > + > + if (inet_pton(AF_INET, hp, &addr4.sin_addr) <= 0) { > + syslog(LOG_WARNING, "cifs.upcall: error converting hostname to IP address"); > + return -1; > + } > + addr4.sin_family = AF_INET; > + addr4.sin_port = 0; > + if (!getnameinfo((struct sockaddr *)&addr4, > + length, host, > + sizeof(host), NULL, 0, 0)) > + syslog(LOG_WARNING, "hostname is %s", host); > + else > + syslog(LOG_WARNING, "getnameinfo() failed"); > + } else { /* IPv6 */ > + struct sockaddr_in6 addr6; > + length = sizeof(addr6); > + > + if (inet_pton(AF_INET6, hp, &addr6.sin6_addr) <= 0) { > + syslog(LOG_WARNING, "cifs.upcall: error converting hostname to IP address"); > + return -1; > + } > + addr6.sin6_family = AF_INET6; > + addr6.sin6_port = 0; > + if (!getnameinfo((struct sockaddr *)&addr6, > + length, host, > + sizeof(host), NULL, 0, 0)) > + syslog(LOG_WARNING, "hostname is %s", host); > + else > + syslog(LOG_WARNING, "getnameinfo() failed"); > + } > + > + } else > + syslog(LOG_WARNING, "cifs.upcall: is_ipaddress() block skipped"); > + > + /* BB: do we need to err if we don't get hostname if IP > + * address and krb5 if used? */ > } else if (strncmp(tkn, "ipv4=", 5) == 0) { > /* BB: do we need it if we have hostname already? */ > } else if (strncmp(tkn, "ipv6=", 5) == 0) { > > Looks like a good start... There's probably some code that can be consolidated there. You're doing basically the same thing in the ipv4 and ipv6 cases. It would be better to just set the variables appropriately and do the getnameinfo() call in just 1 spot -- maybe break it out into a separate function. Also, we're sending the ipv4/ipv6 addrs in the upcall as well, so we probably either need to drop those parms from the upcall string, or have your new code use those fields (not sure which would be better). You'll also want to do a forward lookup with the name you get from the reverse lookup and verify that it matches the original IP addr. Simo outlined the attack vector as such (snippet from IRC conversation): 09:06 < simo> jlayton, this is the attack vector 09:06 < simo> you know someone uses the IP address to connect to ip 1.2.3.4 09:06 < simo> you know that rev 1.2.3.4 -> old.name.address 09:07 < simo> at this point you know that you can be old.name to make the user think he is connected to super.secure.server instaed (which is the one at 1.2.3.4) 09:08 < simo> jlayton, now from a lower level server you can get docs that should go only to super.secret 09:08 < simo> jlayton, if DNS is tructed this is not much of a problem 09:08 < simo> but if you add spoofind DNS 09:08 < simo> then it is a piece of cake 09:08 < simo> but in general using reverse address is bad enough 09:09 < simo> so a switch to toggle it off is really necessary 09:09 < jlayton> then again, with spoofed DNS you're still screwed anyway 09:09 < simo> jlayton, no 09:09 < simo> well yes and no 09:09 < jlayton> if you can spoof forward and reverse then you have the same prob 09:10 < simo> but that's the reason why you must be able to say: no, but no thanks to cifs.upcall if it wants to use DNS 09:10 < jlayton> right 09:10 < jlayton> which makes sense 09:10 < simo> jlayton, yes it is a problem if you do rev-> forward 09:10 < simo> spoofing is not a problem if you do name lookups only (and not rev. lookups) because the KDC will give you the ticket based on the name ...in general we should probably make this new code an option (or give an option to disable it). This scheme implicitly trusts hostname reverse lookups and that may not be suitable in all situations. There's probably also some other verification/canonicalization we could do with the server IP address and hostname as well. -- Jeff Layton <jlayton@redhat.com>
Index: source/client/cifs.upcall.c =================================================================== --- source/client/cifs.upcall.c.orig +++ source/client/cifs.upcall.c @@ -27,6 +27,7 @@ create dns_resolver * * /usr/local/sbin/ #include "includes.h" #include <keyutils.h> + #include "cifs_spnego.h" const char *CIFSSPNEGO_VERSION = "1.2"; @@ -113,6 +114,59 @@ decode_key_description(const char *desc, *hostname = SMB_XMALLOC_ARRAY(char, len); strlcpy(*hostname, tkn + 5, len); retval |= DKD_HAVE_HOSTNAME; + /* May be it's an IP address? + * request_key() might fail if we pass it down, + * so attempt to lookup the hostname. + */ + if (is_ipaddress(*hostname)) { + struct sockaddr_storage ss; + struct sockaddr *addr; + socklen_t length; + char *hp = *hostname; + char host[MAX_DNS_NAME_LENGTH]; + + addr = (struct sockaddr *)&ss; + + /* IPv4 */ + if (is_ipaddress_v4(*hostname)) { + struct sockaddr_in addr4; + length = sizeof(addr4); + + if (inet_pton(AF_INET, hp, &addr4.sin_addr) <= 0) { + syslog(LOG_WARNING, "cifs.upcall: error converting hostname to IP address"); + return -1; + } + addr4.sin_family = AF_INET; + addr4.sin_port = 0; + if (!getnameinfo((struct sockaddr *)&addr4, + length, host, + sizeof(host), NULL, 0, 0)) + syslog(LOG_WARNING, "hostname is %s", host); + else + syslog(LOG_WARNING, "getnameinfo() failed"); + } else { /* IPv6 */ + struct sockaddr_in6 addr6; + length = sizeof(addr6); + + if (inet_pton(AF_INET6, hp, &addr6.sin6_addr) <= 0) { + syslog(LOG_WARNING, "cifs.upcall: error converting hostname to IP address"); + return -1; + } + addr6.sin6_family = AF_INET6; + addr6.sin6_port = 0; + if (!getnameinfo((struct sockaddr *)&addr6, + length, host, + sizeof(host), NULL, 0, 0)) + syslog(LOG_WARNING, "hostname is %s", host); + else + syslog(LOG_WARNING, "getnameinfo() failed"); + } + + } else + syslog(LOG_WARNING, "cifs.upcall: is_ipaddress() block skipped"); + + /* BB: do we need to err if we don't get hostname if IP + * address and krb5 if used? */ } else if (strncmp(tkn, "ipv4=", 5) == 0) { /* BB: do we need it if we have hostname already? */ } else if (strncmp(tkn, "ipv6=", 5) == 0) {