All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] cifs: session matching and authentication fixes and cleanups
@ 2010-06-20 21:10 Jeff Layton
       [not found] ` <1277068251-16344-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2010-06-20 21:10 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

This patchset does some cleanup and fixes a number of bugs related to
the handling of the TCP_Server_Info->secType. It also adds a bit of code
to allow the userspace mount helper to use the krb5 credcache for the
real user doing the mount instead of the one given by the uid= option.

This is a more incremental change from the previous patchsets that I
proposed for this. In most of those, I moved the secType field to the
cifsSesInfo struct. While I still think that's a better approach, it's
very difficult to do for a couple of reasons:

- the NTLMSSP auth code is broken, so we can't unconditionally set the
  extended security bit in the session setup code yet.

- there is apparent need to allow for multiple sec= mount options.
  Unfortunately, the rules for how that's supposed to work are not at
  all clear to me. I decided it was safer to leave the existing code
  that sets the secType as untouched as possible.

I see these patches as 2.6.36 material. They are also required for the
multisession mount patchset. They're also in the cifs-2.6.36 branch of
my kernel.org git tree as well if it's easier to pull from there.

Comments and suggestions welcome.

Jeff Layton (6):
  cifs: have cifs_convert_address set port
  cifs: move address comparison into separate function
  cifs: match secType when searching for existing tcp session
  cifs: clean up cifs_find_smb_ses
  cifs: remove unused cifsUidInfo struct
  cifs: add separate cred_uid field to sesInfo

 fs/cifs/cifs_spnego.c |    3 +
 fs/cifs/cifsglob.h    |   24 +------
 fs/cifs/cifsproto.h   |    3 +-
 fs/cifs/connect.c     |  161 ++++++++++++++++++++++++++++++++++---------------
 fs/cifs/dns_resolve.c |    2 +-
 fs/cifs/netmisc.c     |    5 +-
 6 files changed, 125 insertions(+), 73 deletions(-)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/6] cifs: have cifs_convert_address set port
       [not found] ` <1277068251-16344-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2010-06-20 21:10   ` Jeff Layton
  2010-06-20 21:10   ` [PATCH 2/6] cifs: move address comparison into separate function Jeff Layton
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2010-06-20 21:10 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Currently we set this in cifs_find_tcp_session, but that's more of a
side effect than anything. Set it in cifs_convert_address instead as
that's a better fit.

This also allows us to skip passing in the port as a separate parm to
cifs_find_tcp_session.

Also, pass in a struct sockaddr * rather than void * to make it
clearer how this function should be called.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsproto.h   |    3 ++-
 fs/cifs/connect.c     |   12 +++++-------
 fs/cifs/dns_resolve.c |    2 +-
 fs/cifs/netmisc.c     |    5 ++++-
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index fb6318b..fe26f09 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -86,7 +86,8 @@ extern unsigned int smbCalcSize(struct smb_hdr *ptr);
 extern unsigned int smbCalcSize_LE(struct smb_hdr *ptr);
 extern int decode_negTokenInit(unsigned char *security_blob, int length,
 			struct TCP_Server_Info *server);
-extern int cifs_convert_address(char *src, void *dst);
+extern int cifs_convert_address(struct sockaddr *dst, char *src,
+				unsigned short int port);
 extern int map_smb_to_linux_error(struct smb_hdr *smb, int logErr);
 extern void header_assemble(struct smb_hdr *, char /* command */ ,
 			    const struct cifsTconInfo *, int /* length of
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 2208f06..640c7d5 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1381,7 +1381,7 @@ cifs_parse_mount_options(char *options, const char *devname,
 }
 
 static struct TCP_Server_Info *
-cifs_find_tcp_session(struct sockaddr_storage *addr, unsigned short int port)
+cifs_find_tcp_session(struct sockaddr_storage *addr)
 {
 	struct list_head *tmp;
 	struct TCP_Server_Info *server;
@@ -1405,7 +1405,6 @@ cifs_find_tcp_session(struct sockaddr_storage *addr, unsigned short int port)
 		case AF_INET:
 			if (addr4->sin_addr.s_addr ==
 			    server->addr.sockAddr.sin_addr.s_addr) {
-				addr4->sin_port = htons(port);
 				/* user overrode default port? */
 				if (addr4->sin_port) {
 					if (addr4->sin_port !=
@@ -1421,7 +1420,6 @@ cifs_find_tcp_session(struct sockaddr_storage *addr, unsigned short int port)
 			    &server->addr.sockAddr6.sin6_addr) &&
 			    (addr6->sin6_scope_id ==
 			    server->addr.sockAddr6.sin6_scope_id)) {
-				addr6->sin6_port = htons(port);
 				/* user overrode default port? */
 				if (addr6->sin6_port) {
 					if (addr6->sin6_port !=
@@ -1479,7 +1477,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	cFYI(1, "UNC: %s ip: %s", volume_info->UNC, volume_info->UNCip);
 
 	if (volume_info->UNCip && volume_info->UNC) {
-		rc = cifs_convert_address(volume_info->UNCip, &addr);
+		rc = cifs_convert_address((struct sockaddr *)&addr,
+					  volume_info->UNCip,
+					  volume_info->port);
 		if (!rc) {
 			/* we failed translating address */
 			rc = -EINVAL;
@@ -1499,7 +1499,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	}
 
 	/* see if we already have a matching tcp_ses */
-	tcp_ses = cifs_find_tcp_session(&addr, volume_info->port);
+	tcp_ses = cifs_find_tcp_session(&addr);
 	if (tcp_ses)
 		return tcp_ses;
 
@@ -1543,12 +1543,10 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 		cFYI(1, "attempting ipv6 connect");
 		/* BB should we allow ipv6 on port 139? */
 		/* other OS never observed in Wild doing 139 with v6 */
-		sin_server6->sin6_port = htons(volume_info->port);
 		memcpy(&tcp_ses->addr.sockAddr6, sin_server6,
 			sizeof(struct sockaddr_in6));
 		rc = ipv6_connect(tcp_ses);
 	} else {
-		sin_server->sin_port = htons(volume_info->port);
 		memcpy(&tcp_ses->addr.sockAddr, sin_server,
 			sizeof(struct sockaddr_in));
 		rc = ipv4_connect(tcp_ses);
diff --git a/fs/cifs/dns_resolve.c b/fs/cifs/dns_resolve.c
index 4db2c5e..b6f8e34 100644
--- a/fs/cifs/dns_resolve.c
+++ b/fs/cifs/dns_resolve.c
@@ -40,7 +40,7 @@ is_ip(char *name)
 {
 	struct sockaddr_storage ss;
 
-	return cifs_convert_address(name, &ss);
+	return cifs_convert_address((struct sockaddr *)&ss, name, 0);
 }
 
 static int
diff --git a/fs/cifs/netmisc.c b/fs/cifs/netmisc.c
index d35d528..33b7929 100644
--- a/fs/cifs/netmisc.c
+++ b/fs/cifs/netmisc.c
@@ -164,7 +164,8 @@ cifs_inet_pton(const int address_family, const char *cp, void *dst)
  * Returns 0 on failure.
  */
 int
-cifs_convert_address(char *src, void *dst)
+cifs_convert_address(struct sockaddr *dst, char *src,
+		     const unsigned short int port)
 {
 	int rc;
 	char *pct, *endp;
@@ -174,6 +175,7 @@ cifs_convert_address(char *src, void *dst)
 	/* IPv4 address */
 	if (cifs_inet_pton(AF_INET, src, &s4->sin_addr.s_addr)) {
 		s4->sin_family = AF_INET;
+		s4->sin_port = htons(port);
 		return 1;
 	}
 
@@ -192,6 +194,7 @@ cifs_convert_address(char *src, void *dst)
 		return rc;
 
 	s6->sin6_family = AF_INET6;
+	s6->sin6_port = htons(port);
 	if (pct) {
 		s6->sin6_scope_id = (u32) simple_strtoul(pct, &endp, 0);
 		if (!*pct || *endp)
-- 
1.6.6.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/6] cifs: move address comparison into separate function
       [not found] ` <1277068251-16344-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  2010-06-20 21:10   ` [PATCH 1/6] cifs: have cifs_convert_address set port Jeff Layton
@ 2010-06-20 21:10   ` Jeff Layton
  2010-06-20 21:10   ` [PATCH 3/6] cifs: match secType when searching for existing tcp session Jeff Layton
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2010-06-20 21:10 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Move the address comparator out of cifs_find_tcp_session and into a
separate function for cleanliness. Also change the argument to
that function to a "struct sockaddr" pointer. Passing pointers to
sockaddr_storage is a little odd since that struct is generally for
declaring static storage.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   73 ++++++++++++++++++++++++++---------------------------
 1 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 640c7d5..665ee37 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1380,18 +1380,44 @@ cifs_parse_mount_options(char *options, const char *devname,
 	return 0;
 }
 
+static bool
+match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
+{
+	struct sockaddr_in *addr4 = (struct sockaddr_in *)addr;
+	struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)addr;
+
+	switch (addr->sa_family) {
+	case AF_INET:
+		if (addr4->sin_addr.s_addr !=
+		    server->addr.sockAddr.sin_addr.s_addr)
+			return false;
+		if (addr4->sin_port &&
+		    addr4->sin_port != server->addr.sockAddr.sin_port)
+			return false;
+		break;
+	case AF_INET6:
+		if (!ipv6_addr_equal(&addr6->sin6_addr,
+				     &server->addr.sockAddr6.sin6_addr))
+			return false;
+		if (addr6->sin6_scope_id !=
+		    server->addr.sockAddr6.sin6_scope_id)
+			return false;
+		if (addr6->sin6_port &&
+		    addr6->sin6_port != server->addr.sockAddr6.sin6_port)
+			return false;
+		break;
+	}
+
+	return true;
+}
+
 static struct TCP_Server_Info *
-cifs_find_tcp_session(struct sockaddr_storage *addr)
+cifs_find_tcp_session(struct sockaddr *addr)
 {
-	struct list_head *tmp;
 	struct TCP_Server_Info *server;
-	struct sockaddr_in *addr4 = (struct sockaddr_in *) addr;
-	struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *) addr;
 
 	write_lock(&cifs_tcp_ses_lock);
-	list_for_each(tmp, &cifs_tcp_ses_list) {
-		server = list_entry(tmp, struct TCP_Server_Info,
-				    tcp_ses_list);
+	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
 		/*
 		 * the demux thread can exit on its own while still in CifsNew
 		 * so don't accept any sockets in that state. Since the
@@ -1401,35 +1427,8 @@ cifs_find_tcp_session(struct sockaddr_storage *addr)
 		if (server->tcpStatus == CifsNew)
 			continue;
 
-		switch (addr->ss_family) {
-		case AF_INET:
-			if (addr4->sin_addr.s_addr ==
-			    server->addr.sockAddr.sin_addr.s_addr) {
-				/* user overrode default port? */
-				if (addr4->sin_port) {
-					if (addr4->sin_port !=
-					    server->addr.sockAddr.sin_port)
-						continue;
-				}
-				break;
-			} else
-				continue;
-
-		case AF_INET6:
-			if (ipv6_addr_equal(&addr6->sin6_addr,
-			    &server->addr.sockAddr6.sin6_addr) &&
-			    (addr6->sin6_scope_id ==
-			    server->addr.sockAddr6.sin6_scope_id)) {
-				/* user overrode default port? */
-				if (addr6->sin6_port) {
-					if (addr6->sin6_port !=
-					   server->addr.sockAddr6.sin6_port)
-						continue;
-				}
-				break;
-			} else
-				continue;
-		}
+		if (!match_address(server, addr))
+			continue;
 
 		++server->srv_count;
 		write_unlock(&cifs_tcp_ses_lock);
@@ -1499,7 +1498,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	}
 
 	/* see if we already have a matching tcp_ses */
-	tcp_ses = cifs_find_tcp_session(&addr);
+	tcp_ses = cifs_find_tcp_session((struct sockaddr *)&addr);
 	if (tcp_ses)
 		return tcp_ses;
 
-- 
1.6.6.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/6] cifs: match secType when searching for existing tcp session
       [not found] ` <1277068251-16344-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  2010-06-20 21:10   ` [PATCH 1/6] cifs: have cifs_convert_address set port Jeff Layton
  2010-06-20 21:10   ` [PATCH 2/6] cifs: move address comparison into separate function Jeff Layton
@ 2010-06-20 21:10   ` Jeff Layton
  2010-06-20 21:10   ` [PATCH 4/6] cifs: clean up cifs_find_smb_ses Jeff Layton
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2010-06-20 21:10 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

The secType is a per-tcp session entity, but the current routine doesn't
verify that it is acceptible when attempting to match an existing TCP
session.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsglob.h |    3 +-
 fs/cifs/connect.c  |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index a88479c..1cb7c32 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -80,8 +80,7 @@ enum statusEnum {
 };
 
 enum securityEnum {
-	PLAINTXT = 0, 		/* Legacy with Plaintext passwords */
-	LANMAN,			/* Legacy LANMAN auth */
+	LANMAN = 0,			/* Legacy LANMAN auth */
 	NTLM,			/* Legacy NTLM012 auth with NTLM hash */
 	NTLMv2,			/* Legacy NTLM auth with NTLMv2 hash */
 	RawNTLMSSP,		/* NTLMSSP without SPNEGO, NTLMv2 hash */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 665ee37..6440b79 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1411,8 +1411,56 @@ match_address(struct TCP_Server_Info *server, struct sockaddr *addr)
 	return true;
 }
 
+static bool
+match_security(struct TCP_Server_Info *server, struct smb_vol *vol)
+{
+	unsigned int secFlags;
+
+	if (vol->secFlg & (~(CIFSSEC_MUST_SIGN | CIFSSEC_MUST_SEAL)))
+		secFlags = vol->secFlg;
+	else
+		secFlags = global_secflags | vol->secFlg;
+
+	switch (server->secType) {
+	case LANMAN:
+		if (!(secFlags & (CIFSSEC_MAY_LANMAN|CIFSSEC_MAY_PLNTXT)))
+			return false;
+		break;
+	case NTLMv2:
+		if (!(secFlags & CIFSSEC_MAY_NTLMV2))
+			return false;
+		break;
+	case NTLM:
+		if (!(secFlags & CIFSSEC_MAY_NTLM))
+			return false;
+		break;
+	case Kerberos:
+		if (!(secFlags & CIFSSEC_MAY_KRB5))
+			return false;
+		break;
+	case RawNTLMSSP:
+		if (!(secFlags & CIFSSEC_MAY_NTLMSSP))
+			return false;
+		break;
+	default:
+		/* shouldn't happen */
+		return false;
+	}
+
+	/* now check if signing mode is acceptible */
+	if ((secFlags & CIFSSEC_MAY_SIGN) == 0 &&
+	    (server->secMode & SECMODE_SIGN_REQUIRED))
+			return false;
+	else if (((secFlags & CIFSSEC_MUST_SIGN) == CIFSSEC_MUST_SIGN) &&
+		 (server->secMode &
+		  (SECMODE_SIGN_ENABLED|SECMODE_SIGN_REQUIRED)) == 0)
+			return false;
+
+	return true;
+}
+
 static struct TCP_Server_Info *
-cifs_find_tcp_session(struct sockaddr *addr)
+cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
 {
 	struct TCP_Server_Info *server;
 
@@ -1430,6 +1478,9 @@ cifs_find_tcp_session(struct sockaddr *addr)
 		if (!match_address(server, addr))
 			continue;
 
+		if (!match_security(server, vol))
+			continue;
+
 		++server->srv_count;
 		write_unlock(&cifs_tcp_ses_lock);
 		cFYI(1, "Existing tcp session with server found");
@@ -1498,7 +1549,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 	}
 
 	/* see if we already have a matching tcp_ses */
-	tcp_ses = cifs_find_tcp_session((struct sockaddr *)&addr);
+	tcp_ses = cifs_find_tcp_session((struct sockaddr *)&addr, volume_info);
 	if (tcp_ses)
 		return tcp_ses;
 
-- 
1.6.6.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/6] cifs: clean up cifs_find_smb_ses
       [not found] ` <1277068251-16344-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (2 preceding siblings ...)
  2010-06-20 21:10   ` [PATCH 3/6] cifs: match secType when searching for existing tcp session Jeff Layton
@ 2010-06-20 21:10   ` Jeff Layton
  2010-06-20 21:10   ` [PATCH 5/6] cifs: remove unused cifsUidInfo struct Jeff Layton
  2010-06-20 21:10   ` [PATCH 6/6] cifs: add separate cred_uid field to sesInfo Jeff Layton
  5 siblings, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2010-06-20 21:10 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Do a better job of matching sessions by authtype. Matching by username
for a Kerberos session is incorrect, and anonymous sessions need
special handling.

Also, in the case where we do match by username, we also need to match
by password. That ensures that someone else doesn't "borrow" an existing
session without needing to know the password.

Finally, passwords can be longer than 16 bytes. Bump MAX_PASSWORD_SIZE
to 128 to match the size that the userspace mount helper allows.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsglob.h |    2 +-
 fs/cifs/connect.c  |   26 ++++++++++++++++++--------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 1cb7c32..b24dc98 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -34,7 +34,7 @@
 #define MAX_SHARE_SIZE  64	/* used to be 20, this should still be enough */
 #define MAX_USERNAME_SIZE 32	/* 32 is to allow for 15 char names + null
 				   termination then *2 for unicode versions */
-#define MAX_PASSWORD_SIZE 16
+#define MAX_PASSWORD_SIZE 128
 
 #define CIFS_MIN_RCV_POOL 4
 
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 6440b79..58e0217 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1639,17 +1639,27 @@ out_err:
 }
 
 static struct cifsSesInfo *
-cifs_find_smb_ses(struct TCP_Server_Info *server, char *username)
+cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
 {
-	struct list_head *tmp;
 	struct cifsSesInfo *ses;
 
 	write_lock(&cifs_tcp_ses_lock);
-	list_for_each(tmp, &server->smb_ses_list) {
-		ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list);
-		if (strncmp(ses->userName, username, MAX_USERNAME_SIZE))
-			continue;
-
+	list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
+		switch (server->secType) {
+		case Kerberos:
+			if (vol->linux_uid != ses->linux_uid)
+				continue;
+			break;
+		default:
+			/* anything else takes username/password */
+			if (strncmp(ses->userName, vol->username,
+				    MAX_USERNAME_SIZE))
+				continue;
+			if (strlen(vol->username) != 0 &&
+			    strncmp(ses->password, vol->password,
+				    MAX_PASSWORD_SIZE))
+				continue;
+		}
 		++ses->ses_count;
 		write_unlock(&cifs_tcp_ses_lock);
 		return ses;
@@ -1691,7 +1701,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 
 	xid = GetXid();
 
-	ses = cifs_find_smb_ses(server, volume_info->username);
+	ses = cifs_find_smb_ses(server, volume_info);
 	if (ses) {
 		cFYI(1, "Existing smb sess found (status=%d)", ses->status);
 
-- 
1.6.6.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 5/6] cifs: remove unused cifsUidInfo struct
       [not found] ` <1277068251-16344-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (3 preceding siblings ...)
  2010-06-20 21:10   ` [PATCH 4/6] cifs: clean up cifs_find_smb_ses Jeff Layton
@ 2010-06-20 21:10   ` Jeff Layton
  2010-06-20 21:10   ` [PATCH 6/6] cifs: add separate cred_uid field to sesInfo Jeff Layton
  5 siblings, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2010-06-20 21:10 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsglob.h |   16 ----------------
 1 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index b24dc98..415703b 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -192,28 +192,12 @@ struct TCP_Server_Info {
 };
 
 /*
- * The following is our shortcut to user information.  We surface the uid,
- * and name. We always get the password on the fly in case it
- * has changed. We also hang a list of sessions owned by this user off here.
- */
-struct cifsUidInfo {
-	struct list_head userList;
-	struct list_head sessionList; /* SMB sessions for this user */
-	uid_t linux_uid;
-	char user[MAX_USERNAME_SIZE + 1];	/* ascii name of user */
-	/* BB may need ptr or callback for PAM or WinBind info */
-};
-
-/*
  * Session structure.  One of these for each uid session with a particular host
  */
 struct cifsSesInfo {
 	struct list_head smb_ses_list;
 	struct list_head tcon_list;
 	struct mutex session_mutex;
-#if 0
-	struct cifsUidInfo *uidInfo;	/* pointer to user info */
-#endif
 	struct TCP_Server_Info *server;	/* pointer to server info */
 	int ses_count;		/* reference counter */
 	enum statusEnum status;
-- 
1.6.6.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 6/6] cifs: add separate cred_uid field to sesInfo
       [not found] ` <1277068251-16344-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (4 preceding siblings ...)
  2010-06-20 21:10   ` [PATCH 5/6] cifs: remove unused cifsUidInfo struct Jeff Layton
@ 2010-06-20 21:10   ` Jeff Layton
       [not found]     ` <1277068251-16344-7-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  5 siblings, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2010-06-20 21:10 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Right now, there's no clear separation between the uid that owns the
credentials used to do the mount and the overriding owner of the files
on that mount.

Add a separate cred_uid field that is set to the real uid
of the mount user. Unlike the linux_uid, the uid= option does not
override this parameter. The parm is sent to cifs.upcall, which can then
preferentially use the creduid= parm instead of the uid= parm for
finding credentials.

This is not the only way to solve this. We could try to do all of this
in kernel instead by having a module parameter that affects what gets
passed in the uid= field of the upcall. That said, we have a lot more
flexibility to change things in userspace so I think it probably makes
sense to do it this way.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifs_spnego.c |    3 +++
 fs/cifs/cifsglob.h    |    3 ++-
 fs/cifs/connect.c     |    7 +++++--
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifs_spnego.c b/fs/cifs/cifs_spnego.c
index 379bd7d..6effccf 100644
--- a/fs/cifs/cifs_spnego.c
+++ b/fs/cifs/cifs_spnego.c
@@ -144,6 +144,9 @@ cifs_get_spnego_key(struct cifsSesInfo *sesInfo)
 	sprintf(dp, ";uid=0x%x", sesInfo->linux_uid);
 
 	dp = description + strlen(description);
+	sprintf(dp, ";creduid=0x%x", sesInfo->cred_uid);
+
+	dp = description + strlen(description);
 	sprintf(dp, ";user=%s", sesInfo->userName);
 
 	dp = description + strlen(description);
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 415703b..e15d7a5 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -209,7 +209,8 @@ struct cifsSesInfo {
 	char *serverNOS;	/* name of network operating system of server */
 	char *serverDomain;	/* security realm of server */
 	int Suid;		/* remote smb uid  */
-	uid_t linux_uid;        /* local Linux uid */
+	uid_t linux_uid;        /* overriding owner of files on the mount */
+	uid_t cred_uid;		/* owner of credentials */
 	int capabilities;
 	char serverName[SERVER_NAME_LEN_WITH_NULL * 2];	/* BB make bigger for
 				TCP names - will ipv6 and sctp addresses fit? */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 58e0217..920d94e 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -66,6 +66,7 @@ struct smb_vol {
 	char *iocharset;  /* local code page for mapping to and from Unicode */
 	char source_rfc1001_name[16]; /* netbios name of client */
 	char target_rfc1001_name[16]; /* netbios name of server for Win9x/ME */
+	uid_t cred_uid;
 	uid_t linux_uid;
 	gid_t linux_gid;
 	mode_t file_mode;
@@ -830,7 +831,8 @@ cifs_parse_mount_options(char *options, const char *devname,
 	/* null target name indicates to use *SMBSERVR default called name
 	   if we end up sending RFC1001 session initialize */
 	vol->target_rfc1001_name[0] = 0;
-	vol->linux_uid = current_uid();  /* use current_euid() instead? */
+	vol->cred_uid = current_uid();
+	vol->linux_uid = current_uid();
 	vol->linux_gid = current_gid();
 
 	/* default to only allowing write access to owner of the mount */
@@ -1647,7 +1649,7 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
 	list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
 		switch (server->secType) {
 		case Kerberos:
-			if (vol->linux_uid != ses->linux_uid)
+			if (vol->cred_uid != ses->cred_uid)
 				continue;
 			break;
 		default:
@@ -1764,6 +1766,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 		if (ses->domainName)
 			strcpy(ses->domainName, volume_info->domainname);
 	}
+	ses->cred_uid = volume_info->cred_uid;
 	ses->linux_uid = volume_info->linux_uid;
 	ses->overrideSecFlg = volume_info->secFlg;
 
-- 
1.6.6.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 6/6] cifs: add separate cred_uid field to sesInfo
       [not found]     ` <1277068251-16344-7-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2010-07-15 20:24       ` Steve French
       [not found]         ` <AANLkTik7vm4iYcRH2oGxuINyL2VxY_h9Y03sFhQWR19d-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Steve French @ 2010-07-15 20:24 UTC (permalink / raw)
  To: Jeff Layton; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

I merged the first 5 of this series, but wanted to understand what
behavior this changes first (it is probably ok).  With current
userspace code - what changes would a user see with this?

On Sun, Jun 20, 2010 at 4:10 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Right now, there's no clear separation between the uid that owns the
> credentials used to do the mount and the overriding owner of the files
> on that mount.
>
> Add a separate cred_uid field that is set to the real uid
> of the mount user. Unlike the linux_uid, the uid= option does not
> override this parameter. The parm is sent to cifs.upcall, which can then
> preferentially use the creduid= parm instead of the uid= parm for
> finding credentials.
>
> This is not the only way to solve this. We could try to do all of this
> in kernel instead by having a module parameter that affects what gets
> passed in the uid= field of the upcall. That said, we have a lot more
> flexibility to change things in userspace so I think it probably makes
> sense to do it this way.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifs_spnego.c |    3 +++
>  fs/cifs/cifsglob.h    |    3 ++-
>  fs/cifs/connect.c     |    7 +++++--
>  3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/fs/cifs/cifs_spnego.c b/fs/cifs/cifs_spnego.c
> index 379bd7d..6effccf 100644
> --- a/fs/cifs/cifs_spnego.c
> +++ b/fs/cifs/cifs_spnego.c
> @@ -144,6 +144,9 @@ cifs_get_spnego_key(struct cifsSesInfo *sesInfo)
>        sprintf(dp, ";uid=0x%x", sesInfo->linux_uid);
>
>        dp = description + strlen(description);
> +       sprintf(dp, ";creduid=0x%x", sesInfo->cred_uid);
> +
> +       dp = description + strlen(description);
>        sprintf(dp, ";user=%s", sesInfo->userName);
>
>        dp = description + strlen(description);
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 415703b..e15d7a5 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -209,7 +209,8 @@ struct cifsSesInfo {
>        char *serverNOS;        /* name of network operating system of server */
>        char *serverDomain;     /* security realm of server */
>        int Suid;               /* remote smb uid  */
> -       uid_t linux_uid;        /* local Linux uid */
> +       uid_t linux_uid;        /* overriding owner of files on the mount */
> +       uid_t cred_uid;         /* owner of credentials */
>        int capabilities;
>        char serverName[SERVER_NAME_LEN_WITH_NULL * 2]; /* BB make bigger for
>                                TCP names - will ipv6 and sctp addresses fit? */
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 58e0217..920d94e 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -66,6 +66,7 @@ struct smb_vol {
>        char *iocharset;  /* local code page for mapping to and from Unicode */
>        char source_rfc1001_name[16]; /* netbios name of client */
>        char target_rfc1001_name[16]; /* netbios name of server for Win9x/ME */
> +       uid_t cred_uid;
>        uid_t linux_uid;
>        gid_t linux_gid;
>        mode_t file_mode;
> @@ -830,7 +831,8 @@ cifs_parse_mount_options(char *options, const char *devname,
>        /* null target name indicates to use *SMBSERVR default called name
>           if we end up sending RFC1001 session initialize */
>        vol->target_rfc1001_name[0] = 0;
> -       vol->linux_uid = current_uid();  /* use current_euid() instead? */
> +       vol->cred_uid = current_uid();
> +       vol->linux_uid = current_uid();
>        vol->linux_gid = current_gid();
>
>        /* default to only allowing write access to owner of the mount */
> @@ -1647,7 +1649,7 @@ cifs_find_smb_ses(struct TCP_Server_Info *server, struct smb_vol *vol)
>        list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
>                switch (server->secType) {
>                case Kerberos:
> -                       if (vol->linux_uid != ses->linux_uid)
> +                       if (vol->cred_uid != ses->cred_uid)
>                                continue;
>                        break;
>                default:
> @@ -1764,6 +1766,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
>                if (ses->domainName)
>                        strcpy(ses->domainName, volume_info->domainname);
>        }
> +       ses->cred_uid = volume_info->cred_uid;
>        ses->linux_uid = volume_info->linux_uid;
>        ses->overrideSecFlg = volume_info->secFlg;
>
> --
> 1.6.6.1
>
>



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 6/6] cifs: add separate cred_uid field to sesInfo
       [not found]         ` <AANLkTik7vm4iYcRH2oGxuINyL2VxY_h9Y03sFhQWR19d-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-07-15 21:19           ` Jeff Layton
       [not found]             ` <20100715171936.4252a16d-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
  2010-07-23 18:41           ` Jeff Layton
  1 sibling, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2010-07-15 21:19 UTC (permalink / raw)
  To: Steve French; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Thu, 15 Jul 2010 15:24:46 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> I merged the first 5 of this series, but wanted to understand what
> behavior this changes first (it is probably ok).  With current
> userspace code - what changes would a user see with this?
> 

With this and the accompanying userspace patch, this makes it so that
the credentials cache used when mounting with sec=krb5 is unaffected by
the uid= option. The credcache will be determined using the real uid of
the user performing the mount. There will be a cifs.upcall option that
will make it use the legacy behavior for those that require it for some
reason. 

I consider the current situation a bad design decision on my part as
the ownership of files on the mount has no direct relationship to the
owner of the mount credentials. The mount credentials should always be
under the ownership of the user performing the mount. The existing
scheme allows someone to use the credcache of another user to perform
a mount.

I'll resend the userspace patch in another day or two when I get back
from vacation.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 6/6] cifs: add separate cred_uid field to sesInfo
       [not found]             ` <20100715171936.4252a16d-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
@ 2010-07-18 11:18               ` Jeff Layton
       [not found]                 ` <20100718071819.4264e8aa-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2010-07-18 11:18 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Steve French, linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Thu, 15 Jul 2010 17:19:36 -0400
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Thu, 15 Jul 2010 15:24:46 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > I merged the first 5 of this series, but wanted to understand what
> > behavior this changes first (it is probably ok).  With current
> > userspace code - what changes would a user see with this?
> > 
> 
> With this and the accompanying userspace patch, this makes it so that
> the credentials cache used when mounting with sec=krb5 is unaffected by
> the uid= option. The credcache will be determined using the real uid of
> the user performing the mount. There will be a cifs.upcall option that
> will make it use the legacy behavior for those that require it for some
> reason. 
> 
> I consider the current situation a bad design decision on my part as
> the ownership of files on the mount has no direct relationship to the
> owner of the mount credentials. The mount credentials should always be
> under the ownership of the user performing the mount. The existing
> scheme allows someone to use the credcache of another user to perform
> a mount.
> 
> I'll resend the userspace patch in another day or two when I get back
> from vacation.
> 

The corresponding userspace patch for this is below:

------------------------[snip]---------------------------

[PATCH] cifs.upcall: use "creduid=" parm by default when available

When I did the original krb5 implementation, I goofed and ended up making
it so that when someone specifies the "uid=" mount option that also affects
the owner of the krb5 credential cache and not just the ownership of the
mount. I'm proposing a patch for the kernel to attempt to fix this by
making the kernel send a "creduid=" parameter in the upcall which is
intended to be the user that should own the credentials cache.

That's not necessarily the same user that has "ownership" of the mount.
Usually the creduid= will be set to the real uid of the user doing the
mounting. When multisession mounts are introduced they will usually set
this to the fsuid that walks into the mount.

To ease the transition, this patch also adds a command line switch that
makes cifs.upcall use the "legacy" uid= parameter instead. Use that if you
want it to behave like it used to.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifs.upcall.8 |    9 ++++++++-
 cifs.upcall.c |   32 +++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/cifs.upcall.8 b/cifs.upcall.8
index 7fc1603..815dd04 100644
--- a/cifs.upcall.8
+++ b/cifs.upcall.8
@@ -22,7 +22,7 @@
 cifs.upcall \- Userspace upcall helper for Common Internet File System (CIFS)
 .SH "SYNOPSIS"
 .HP \w'\ 'u
-cifs\&.upcall [\-\-trust\-dns|\-t] [\-\-version|\-v] {keyid}
+cifs\&.upcall [\-\-trust\-dns|\-t] [\-\-version|\-v] [\-\-legacy\-uid|\-l] {keyid}
 .SH "DESCRIPTION"
 .PP
 This tool is part of the cifs-utils suite\&.
@@ -45,6 +45,13 @@ With krb5 upcalls, the name used as the host portion of the service principal de
 This is less secure than not trusting DNS\&. When using this option, it\'s possible that an attacker could get control of DNS and trick the client into mounting a different server altogether\&. It\'s preferable to instead add server principals to the KDC for every possible hostname, but this option exists for cases where that isn\'t possible\&. The default is to not trust reverse hostname lookups in this fashion\&.
 .RE
 .PP
+\-\-legacy\-uid|\-l
+.RS 4
+Traditionally, the kernel has sent only a single uid= parameter to the upcall for the SPNEGO upcall that\'s used to determine what user's credential cache to use. This parameter was generally determined by the uid= mount option, which also governs the ownership of files on the mount\&.
+.sp
+Newer kernels send a creduid= option as well, which contains what uid it thinks actually owns the credentials that it\'s looking for\&. At mount time, this is generally set to the real uid of the user doing the mount. For multisession mounts, it's set to the fsuid of the mount user. Set this option if you want cifs.upcall to use the older uid= parameter instead of the creduid= parameter\&.
+.RE
+.PP
 \-\-version|\-v
 .RS 4
 Print version number and exit\&.
diff --git a/cifs.upcall.c b/cifs.upcall.c
index d4376cc..1f4341d 100644
--- a/cifs.upcall.c
+++ b/cifs.upcall.c
@@ -389,6 +389,7 @@ 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_HAVE_CREDUID	0x40
 #define DKD_MUSTHAVE_SET (DKD_HAVE_HOSTNAME|DKD_HAVE_VERSION|DKD_HAVE_SEC)
 
 struct decoded_args {
@@ -396,6 +397,7 @@ struct decoded_args {
 	char *hostname;
 	char *ip;
 	uid_t uid;
+	uid_t creduid;
 	pid_t pid;
 	sectype_t sec;
 };
@@ -461,6 +463,16 @@ decode_key_description(const char *desc, struct decoded_args *arg)
 			} else {
 				retval |= DKD_HAVE_UID;
 			}
+		} else if (strncmp(tkn, "creduid=", 8) == 0) {
+			errno = 0;
+			arg->creduid = strtol(tkn + 8, NULL, 16);
+			if (errno != 0) {
+				syslog(LOG_ERR, "Invalid creduid format: %s",
+				       strerror(errno));
+				return 1;
+			} else {
+				retval |= DKD_HAVE_CREDUID;
+			}
 		} else if (strncmp(tkn, "ver=", 4) == 0) {	/* if version */
 			errno = 0;
 			arg->ver = strtol(tkn + 4, NULL, 16);
@@ -584,12 +596,13 @@ static int ip_to_fqdn(const char *addrstr, char *host, size_t hostlen)
 
 static void usage(void)
 {
-	syslog(LOG_INFO, "Usage: %s [-t] [-v] key_serial", prog);
-	fprintf(stderr, "Usage: %s [-t] [-v] key_serial\n", prog);
+	syslog(LOG_INFO, "Usage: %s [-t] [-v] [-l] key_serial", prog);
+	fprintf(stderr, "Usage: %s [-t] [-v] [-l] key_serial\n", prog);
 }
 
 const struct option long_options[] = {
 	{"trust-dns", 0, NULL, 't'},
+	{"legacy-uid", 0, NULL, 'l'},
 	{"version", 0, NULL, 'v'},
 	{NULL, 0, NULL, 0}
 };
@@ -603,7 +616,7 @@ int main(const int argc, char *const argv[])
 	size_t datalen;
 	unsigned int have;
 	long rc = 1;
-	int c, try_dns = 0;
+	int c, try_dns = 0, legacy_uid = 0;
 	char *buf, *princ = NULL, *ccname = NULL;
 	char hostbuf[NI_MAXHOST], *host;
 	struct decoded_args arg = { };
@@ -621,6 +634,9 @@ int main(const int argc, char *const argv[])
 		case 't':
 			try_dns++;
 			break;
+		case 'l':
+			legacy_uid++;
+			break;
 		case 'v':
 			printf("version: %s\n", VERSION);
 			goto out;
@@ -677,13 +693,19 @@ int main(const int argc, char *const argv[])
 		goto out;
 	}
 
-	if (have & DKD_HAVE_UID) {
+	if (!legacy_uid && (have & DKD_HAVE_CREDUID)) {
+		rc = setuid(arg.creduid);
+		if (rc == -1) {
+			syslog(LOG_ERR, "setuid: %s", strerror(errno));
+			goto out;
+		}
+		ccname = find_krb5_cc(CIFS_DEFAULT_KRB5_DIR, arg.creduid);
+	} else if (have & DKD_HAVE_UID) {
 		rc = setuid(arg.uid);
 		if (rc == -1) {
 			syslog(LOG_ERR, "setuid: %s", strerror(errno));
 			goto out;
 		}
-
 		ccname = find_krb5_cc(CIFS_DEFAULT_KRB5_DIR, arg.uid);
 	}
 
-- 
1.7.1.1


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 6/6] cifs: add separate cred_uid field to sesInfo
       [not found]         ` <AANLkTik7vm4iYcRH2oGxuINyL2VxY_h9Y03sFhQWR19d-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2010-07-15 21:19           ` Jeff Layton
@ 2010-07-23 18:41           ` Jeff Layton
  1 sibling, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2010-07-23 18:41 UTC (permalink / raw)
  To: Steve French; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Thu, 15 Jul 2010 15:24:46 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> I merged the first 5 of this series, but wanted to understand what
> behavior this changes first (it is probably ok).  With current
> userspace code - what changes would a user see with this?
> 

Hi Steve,
Haven't heard from you in a week or so on this. Any more progress on
merging these patches?

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 6/6] cifs: add separate cred_uid field to sesInfo
       [not found]                 ` <20100718071819.4264e8aa-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2010-07-23 20:51                   ` Jeff Layton
  0 siblings, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2010-07-23 20:51 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Steve French, linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Sun, 18 Jul 2010 07:18:19 -0400
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Thu, 15 Jul 2010 17:19:36 -0400
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > On Thu, 15 Jul 2010 15:24:46 -0500
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 
> > > I merged the first 5 of this series, but wanted to understand what
> > > behavior this changes first (it is probably ok).  With current
> > > userspace code - what changes would a user see with this?
> > > 
> > 
> > With this and the accompanying userspace patch, this makes it so that
> > the credentials cache used when mounting with sec=krb5 is unaffected by
> > the uid= option. The credcache will be determined using the real uid of
> > the user performing the mount. There will be a cifs.upcall option that
> > will make it use the legacy behavior for those that require it for some
> > reason. 
> > 
> > I consider the current situation a bad design decision on my part as
> > the ownership of files on the mount has no direct relationship to the
> > owner of the mount credentials. The mount credentials should always be
> > under the ownership of the user performing the mount. The existing
> > scheme allows someone to use the credcache of another user to perform
> > a mount.
> > 
> > I'll resend the userspace patch in another day or two when I get back
> > from vacation.
> > 
> 
> The corresponding userspace patch for this is below:
> 
> ------------------------[snip]---------------------------
> 
> [PATCH] cifs.upcall: use "creduid=" parm by default when available
> 
> When I did the original krb5 implementation, I goofed and ended up making
> it so that when someone specifies the "uid=" mount option that also affects
> the owner of the krb5 credential cache and not just the ownership of the
> mount. I'm proposing a patch for the kernel to attempt to fix this by
> making the kernel send a "creduid=" parameter in the upcall which is
> intended to be the user that should own the credentials cache.
> 
> That's not necessarily the same user that has "ownership" of the mount.
> Usually the creduid= will be set to the real uid of the user doing the
> mounting. When multisession mounts are introduced they will usually set
> this to the fsuid that walks into the mount.
> 
> To ease the transition, this patch also adds a command line switch that
> makes cifs.upcall use the "legacy" uid= parameter instead. Use that if you
> want it to behave like it used to.
> 
> Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> ---
>  cifs.upcall.8 |    9 ++++++++-
>  cifs.upcall.c |   32 +++++++++++++++++++++++++++-----
>  2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/cifs.upcall.8 b/cifs.upcall.8
> index 7fc1603..815dd04 100644
> --- a/cifs.upcall.8
> +++ b/cifs.upcall.8
> @@ -22,7 +22,7 @@
>  cifs.upcall \- Userspace upcall helper for Common Internet File System (CIFS)
>  .SH "SYNOPSIS"
>  .HP \w'\ 'u
> -cifs\&.upcall [\-\-trust\-dns|\-t] [\-\-version|\-v] {keyid}
> +cifs\&.upcall [\-\-trust\-dns|\-t] [\-\-version|\-v] [\-\-legacy\-uid|\-l] {keyid}
>  .SH "DESCRIPTION"
>  .PP
>  This tool is part of the cifs-utils suite\&.
> @@ -45,6 +45,13 @@ With krb5 upcalls, the name used as the host portion of the service principal de
>  This is less secure than not trusting DNS\&. When using this option, it\'s possible that an attacker could get control of DNS and trick the client into mounting a different server altogether\&. It\'s preferable to instead add server principals to the KDC for every possible hostname, but this option exists for cases where that isn\'t possible\&. The default is to not trust reverse hostname lookups in this fashion\&.
>  .RE
>  .PP
> +\-\-legacy\-uid|\-l
> +.RS 4
> +Traditionally, the kernel has sent only a single uid= parameter to the upcall for the SPNEGO upcall that\'s used to determine what user's credential cache to use. This parameter was generally determined by the uid= mount option, which also governs the ownership of files on the mount\&.
> +.sp
> +Newer kernels send a creduid= option as well, which contains what uid it thinks actually owns the credentials that it\'s looking for\&. At mount time, this is generally set to the real uid of the user doing the mount. For multisession mounts, it's set to the fsuid of the mount user. Set this option if you want cifs.upcall to use the older uid= parameter instead of the creduid= parameter\&.
> +.RE
> +.PP
>  \-\-version|\-v
>  .RS 4
>  Print version number and exit\&.
> diff --git a/cifs.upcall.c b/cifs.upcall.c
> index d4376cc..1f4341d 100644
> --- a/cifs.upcall.c
> +++ b/cifs.upcall.c
> @@ -389,6 +389,7 @@ 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_HAVE_CREDUID	0x40
>  #define DKD_MUSTHAVE_SET (DKD_HAVE_HOSTNAME|DKD_HAVE_VERSION|DKD_HAVE_SEC)
>  
>  struct decoded_args {
> @@ -396,6 +397,7 @@ struct decoded_args {
>  	char *hostname;
>  	char *ip;
>  	uid_t uid;
> +	uid_t creduid;
>  	pid_t pid;
>  	sectype_t sec;
>  };
> @@ -461,6 +463,16 @@ decode_key_description(const char *desc, struct decoded_args *arg)
>  			} else {
>  				retval |= DKD_HAVE_UID;
>  			}
> +		} else if (strncmp(tkn, "creduid=", 8) == 0) {
> +			errno = 0;
> +			arg->creduid = strtol(tkn + 8, NULL, 16);
> +			if (errno != 0) {
> +				syslog(LOG_ERR, "Invalid creduid format: %s",
> +				       strerror(errno));
> +				return 1;
> +			} else {
> +				retval |= DKD_HAVE_CREDUID;
> +			}
>  		} else if (strncmp(tkn, "ver=", 4) == 0) {	/* if version */
>  			errno = 0;
>  			arg->ver = strtol(tkn + 4, NULL, 16);
> @@ -584,12 +596,13 @@ static int ip_to_fqdn(const char *addrstr, char *host, size_t hostlen)
>  
>  static void usage(void)
>  {
> -	syslog(LOG_INFO, "Usage: %s [-t] [-v] key_serial", prog);
> -	fprintf(stderr, "Usage: %s [-t] [-v] key_serial\n", prog);
> +	syslog(LOG_INFO, "Usage: %s [-t] [-v] [-l] key_serial", prog);
> +	fprintf(stderr, "Usage: %s [-t] [-v] [-l] key_serial\n", prog);
>  }
>  
>  const struct option long_options[] = {
>  	{"trust-dns", 0, NULL, 't'},
> +	{"legacy-uid", 0, NULL, 'l'},
>  	{"version", 0, NULL, 'v'},
>  	{NULL, 0, NULL, 0}
>  };
> @@ -603,7 +616,7 @@ int main(const int argc, char *const argv[])
>  	size_t datalen;
>  	unsigned int have;
>  	long rc = 1;
> -	int c, try_dns = 0;
> +	int c, try_dns = 0, legacy_uid = 0;
>  	char *buf, *princ = NULL, *ccname = NULL;
>  	char hostbuf[NI_MAXHOST], *host;
>  	struct decoded_args arg = { };
> @@ -621,6 +634,9 @@ int main(const int argc, char *const argv[])
>  		case 't':
>  			try_dns++;
>  			break;
> +		case 'l':
> +			legacy_uid++;
> +			break;
>  		case 'v':
>  			printf("version: %s\n", VERSION);
>  			goto out;
> @@ -677,13 +693,19 @@ int main(const int argc, char *const argv[])
>  		goto out;
>  	}
>  
> -	if (have & DKD_HAVE_UID) {
> +	if (!legacy_uid && (have & DKD_HAVE_CREDUID)) {
> +		rc = setuid(arg.creduid);
> +		if (rc == -1) {
> +			syslog(LOG_ERR, "setuid: %s", strerror(errno));
> +			goto out;
> +		}
> +		ccname = find_krb5_cc(CIFS_DEFAULT_KRB5_DIR, arg.creduid);
> +	} else if (have & DKD_HAVE_UID) {
>  		rc = setuid(arg.uid);
>  		if (rc == -1) {
>  			syslog(LOG_ERR, "setuid: %s", strerror(errno));
>  			goto out;
>  		}
> -
>  		ccname = find_krb5_cc(CIFS_DEFAULT_KRB5_DIR, arg.uid);
>  	}
>  

Steve has queued the kernel patch up for 2.6.36, so I've gone ahead and
pushed this patch to the cifs-utils git repo. It should make the 4.6
release.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2010-07-23 20:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-20 21:10 [PATCH 0/6] cifs: session matching and authentication fixes and cleanups Jeff Layton
     [not found] ` <1277068251-16344-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2010-06-20 21:10   ` [PATCH 1/6] cifs: have cifs_convert_address set port Jeff Layton
2010-06-20 21:10   ` [PATCH 2/6] cifs: move address comparison into separate function Jeff Layton
2010-06-20 21:10   ` [PATCH 3/6] cifs: match secType when searching for existing tcp session Jeff Layton
2010-06-20 21:10   ` [PATCH 4/6] cifs: clean up cifs_find_smb_ses Jeff Layton
2010-06-20 21:10   ` [PATCH 5/6] cifs: remove unused cifsUidInfo struct Jeff Layton
2010-06-20 21:10   ` [PATCH 6/6] cifs: add separate cred_uid field to sesInfo Jeff Layton
     [not found]     ` <1277068251-16344-7-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2010-07-15 20:24       ` Steve French
     [not found]         ` <AANLkTik7vm4iYcRH2oGxuINyL2VxY_h9Y03sFhQWR19d-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-07-15 21:19           ` Jeff Layton
     [not found]             ` <20100715171936.4252a16d-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2010-07-18 11:18               ` Jeff Layton
     [not found]                 ` <20100718071819.4264e8aa-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2010-07-23 20:51                   ` Jeff Layton
2010-07-23 18:41           ` Jeff Layton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.