[linux-cifs-client] Unable to mount CIFS with kerberos security
diff mbox

Message ID 4981B058.6000805@suse.de
State New, archived
Headers show

Commit Message

Suresh Jayaraman Jan. 29, 2009, 1:34 p.m. UTC
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)

Comments

Jeff Layton Jan. 29, 2009, 2:16 p.m. UTC | #1
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>

Patch
diff mbox

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) {