[PATCH net-next] scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.

Subsystems: networking [general], networking [sockets], the rest

STALE5080d

6 messages, 3 authors, 2012-09-07 · open the first message on its own page

[PATCH net-next] scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.

From: Eric W. Biederman <hidden>
Date: 2012-09-07 04:20:12

Passing uids and gids on NETLINK_CB from a process in one user
namespace to a process in another user namespace can result in the
wrong uid or gid being presented to userspace.  Avoid that problem by
passing kuids and kgids instead.

- define struct scm_creds for use in scm_cookie and netlink_skb_parms
  that holds uid and gid information in kuid_t and kgid_t.

- Modify scm_set_cred to fill out scm_creds by heand instead of using
  cred_to_ucred to fill out struct ucred.  This conversion ensures
  userspace does not get incorrect uid or gid values to look at.

- Modify scm_recv to convert from struct scm_creds to struct ucred
  before copying credential values to userspace.

- Modify __scm_send to populate struct scm_creds on in the scm_cookie,
  instead of just copying struct ucred from userspace.

- Modify netlink_sendmsg to copy scm_creds instead of struct ucred
  into the NETLINK_CB.

Signed-off-by: "Eric W. Biederman" <redacted>
---
 include/linux/netlink.h  |    3 ++-
 include/net/scm.h        |   23 +++++++++++++++++++----
 net/core/scm.c           |   17 +++++++++++------
 net/netlink/af_netlink.c |    2 +-
 4 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index c9fdde2..df73cf4 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -153,6 +153,7 @@ struct nlattr {
 
 #include <linux/capability.h>
 #include <linux/skbuff.h>
+#include <net/scm.h>
 
 struct net;
 
@@ -162,7 +163,7 @@ static inline struct nlmsghdr *nlmsg_hdr(const struct sk_buff *skb)
 }
 
 struct netlink_skb_parms {
-	struct ucred		creds;		/* Skb credentials	*/
+	struct scm_creds	creds;		/* Skb credentials	*/
 	__u32			pid;
 	__u32			dst_group;
 	struct sock		*ssk;
diff --git a/include/net/scm.h b/include/net/scm.h
index 079d788..000b6f7 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -12,6 +12,12 @@
  */
 #define SCM_MAX_FD	253
 
+struct scm_creds {
+	u32	pid;
+	kuid_t	uid;
+	kgid_t	gid;
+};
+
 struct scm_fp_list {
 	short			count;
 	short			max;
@@ -22,7 +28,7 @@ struct scm_cookie {
 	struct pid		*pid;		/* Skb credentials */
 	const struct cred	*cred;
 	struct scm_fp_list	*fp;		/* Passed files		*/
-	struct ucred		creds;		/* Skb credentials	*/
+	struct scm_creds	creds;		/* Skb credentials	*/
 #ifdef CONFIG_SECURITY_NETWORK
 	u32			secid;		/* Passed security ID 	*/
 #endif
@@ -49,7 +55,9 @@ static __inline__ void scm_set_cred(struct scm_cookie *scm,
 {
 	scm->pid  = get_pid(pid);
 	scm->cred = cred ? get_cred(cred) : NULL;
-	cred_to_ucred(pid, cred, &scm->creds);
+	scm->creds.pid = pid_vnr(pid);
+	scm->creds.uid = cred ? cred->euid : INVALID_UID;
+	scm->creds.gid = cred ? cred->egid : INVALID_GID;
 }
 
 static __inline__ void scm_destroy_cred(struct scm_cookie *scm)
@@ -110,8 +118,15 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg,
 		return;
 	}
 
-	if (test_bit(SOCK_PASSCRED, &sock->flags))
-		put_cmsg(msg, SOL_SOCKET, SCM_CREDENTIALS, sizeof(scm->creds), &scm->creds);
+	if (test_bit(SOCK_PASSCRED, &sock->flags)) {
+		struct user_namespace *current_ns = current_user_ns();
+		struct ucred ucreds = {
+			.pid = scm->creds.pid,
+			.uid = from_kuid_munged(current_ns, scm->creds.uid),
+			.gid = from_kgid_munged(current_ns, scm->creds.gid),
+		};
+		put_cmsg(msg, SOL_SOCKET, SCM_CREDENTIALS, sizeof(ucreds), &ucreds);
+	}
 
 	scm_destroy_cred(scm);
 
diff --git a/net/core/scm.c b/net/core/scm.c
index 5472ae7..64b41d8 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -155,19 +155,21 @@ int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *p)
 			break;
 		case SCM_CREDENTIALS:
 		{
+			struct ucred creds;
 			kuid_t uid;
 			kgid_t gid;
 			if (cmsg->cmsg_len != CMSG_LEN(sizeof(struct ucred)))
 				goto error;
-			memcpy(&p->creds, CMSG_DATA(cmsg), sizeof(struct ucred));
-			err = scm_check_creds(&p->creds);
+			memcpy(&creds, CMSG_DATA(cmsg), sizeof(struct ucred));
+			err = scm_check_creds(&creds);
 			if (err)
 				goto error;
 
-			if (!p->pid || pid_vnr(p->pid) != p->creds.pid) {
+			p->creds.pid = creds.pid;
+			if (!p->pid || pid_vnr(p->pid) != creds.pid) {
 				struct pid *pid;
 				err = -ESRCH;
-				pid = find_get_pid(p->creds.pid);
+				pid = find_get_pid(creds.pid);
 				if (!pid)
 					goto error;
 				put_pid(p->pid);
@@ -175,11 +177,14 @@ int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *p)
 			}
 
 			err = -EINVAL;
-			uid = make_kuid(current_user_ns(), p->creds.uid);
-			gid = make_kgid(current_user_ns(), p->creds.gid);
+			uid = make_kuid(current_user_ns(), creds.uid);
+			gid = make_kgid(current_user_ns(), creds.gid);
 			if (!uid_valid(uid) || !gid_valid(gid))
 				goto error;
 
+			p->creds.uid = uid;
+			p->creds.gid = gid;
+
 			if (!p->cred ||
 			    !uid_eq(p->cred->euid, uid) ||
 			    !gid_eq(p->cred->egid, gid)) {
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7cb7867..6473267 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1398,7 +1398,7 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock,
 
 	NETLINK_CB(skb).pid	= nlk->pid;
 	NETLINK_CB(skb).dst_group = dst_group;
-	memcpy(NETLINK_CREDS(skb), &siocb->scm->creds, sizeof(struct ucred));
+	NETLINK_CB(skb).creds	= siocb->scm->creds;
 
 	err = -EFAULT;
 	if (memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len)) {
-- 
1.7.5.4

Re: [PATCH net-next] scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.

From: Eric Dumazet <hidden>
Date: 2012-09-07 15:07:32

On Thu, 2012-09-06 at 21:20 -0700, Eric W. Biederman wrote:
Passing uids and gids on NETLINK_CB from a process in one user
namespace to a process in another user namespace can result in the
wrong uid or gid being presented to userspace.  Avoid that problem by
passing kuids and kgids instead.

- define struct scm_creds for use in scm_cookie and netlink_skb_parms
  that holds uid and gid information in kuid_t and kgid_t.

- Modify scm_set_cred to fill out scm_creds by heand instead of using
  cred_to_ucred to fill out struct ucred.  This conversion ensures
  userspace does not get incorrect uid or gid values to look at.

- Modify scm_recv to convert from struct scm_creds to struct ucred
  before copying credential values to userspace.

- Modify __scm_send to populate struct scm_creds on in the scm_cookie,
  instead of just copying struct ucred from userspace.

- Modify netlink_sendmsg to copy scm_creds instead of struct ucred
  into the NETLINK_CB.

Signed-off-by: "Eric W. Biederman" <redacted>
quoted hunk
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7cb7867..6473267 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1398,7 +1398,7 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock,
 
 	NETLINK_CB(skb).pid	= nlk->pid;
 	NETLINK_CB(skb).dst_group = dst_group;
-	memcpy(NETLINK_CREDS(skb), &siocb->scm->creds, sizeof(struct ucred));
+	NETLINK_CB(skb).creds	= siocb->scm->creds;
 
 	err = -EFAULT;
 	if (memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len)) {
Seems fine to me, but I am not sure why you kept NETLINK_CREDS()
defined/used once.

Re: [PATCH net-next] scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.

From: David Miller <davem@davemloft.net>
Date: 2012-09-07 18:41:54

From: Eric Dumazet <redacted>
Date: Fri, 07 Sep 2012 17:07:26 +0200
Seems fine to me, but I am not sure why you kept NETLINK_CREDS()
defined/used once.
There are two remaining users, kernel/audit.c and scsi netlink.

Re: [PATCH net-next] scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.

From: David Miller <davem@davemloft.net>
Date: 2012-09-07 18:42:52

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 06 Sep 2012 21:20:01 -0700
Passing uids and gids on NETLINK_CB from a process in one user
namespace to a process in another user namespace can result in the
wrong uid or gid being presented to userspace.  Avoid that problem by
passing kuids and kgids instead.

- define struct scm_creds for use in scm_cookie and netlink_skb_parms
  that holds uid and gid information in kuid_t and kgid_t.

- Modify scm_set_cred to fill out scm_creds by heand instead of using
  cred_to_ucred to fill out struct ucred.  This conversion ensures
  userspace does not get incorrect uid or gid values to look at.

- Modify scm_recv to convert from struct scm_creds to struct ucred
  before copying credential values to userspace.

- Modify __scm_send to populate struct scm_creds on in the scm_cookie,
  instead of just copying struct ucred from userspace.

- Modify netlink_sendmsg to copy scm_creds instead of struct ucred
  into the NETLINK_CB.

Signed-off-by: "Eric W. Biederman" <redacted>
Applied, thanks Eric.

Re: [PATCH net-next] scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.

From: Eric W. Biederman <hidden>
Date: 2012-09-07 21:25:21

David Miller [off-list ref] writes:
From: Eric Dumazet <redacted>
Date: Fri, 07 Sep 2012 17:07:26 +0200
quoted
Seems fine to me, but I am not sure why you kept NETLINK_CREDS()
defined/used once.
There are two remaining users, kernel/audit.c and scsi netlink.
Good point.  I have a change to kernel/audit.c that removes the
usage.    I believe the scsi netlink usage is just plain bogus,
and it wants the netlink port identifier instead.

Would anyone mind if we changed the name of the field in netlink from
pid to portid?  It seems to cause a lot of confusion.  We can't change
the user space fields of course but...

Right now because we can now use current using NETLINK_CREDS in the
kernel is unnecessary.

Eric

Re: [PATCH net-next] scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.

From: David Miller <davem@davemloft.net>
Date: 2012-09-07 21:32:05

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Fri, 07 Sep 2012 14:25:09 -0700
Would anyone mind if we changed the name of the field in netlink from
pid to portid?  It seems to cause a lot of confusion.  We can't change
the user space fields of course but...
No objections.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help