scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.
Subsystems (3)
PATCH 1/1 scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.
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.
--- 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(-)
include/linux/netlink.h +2 -1
@@ -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;
include/net/scm.h +19 -4
@@ -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);
net/core/scm.c +11 -6
@@ -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)) {
net/netlink/af_netlink.c +1 -1
@@ -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)) {
Signature · 1 line
1.7.5.4
Quoted 29 lines · Eric W. Biederman
Passing uids and gids on
NETLINK_CBfrom 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_credsfor use inscm_cookieandnetlink_skb_parmsthat holds uid and gid information inkuid_tandkgid_t.- Modify
scm_set_credto fill outscm_credsby heand instead of usingcred_to_ucredto fill out struct ucred. This conversion ensures userspace does not get incorrect uid or gid values to look at.- Modify
scm_recvto convert from structscm_credsto struct ucred before copying credential values to userspace.- Modify
__scm_sendto populate structscm_credson in thescm_cookie, instead of just copying struct ucred from userspace.- Modify
netlink_sendmsgto copyscm_credsinstead of struct ucred into theNETLINK_CB.Signed-off-by "Eric W. Biederman" <[email protected]>net/netlink/af_netlink.c +0
-0
net/netlink/af_netlink.c static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock, · line 1404
Quoted 8 lines
net/netlink/af_netlink.c +1
-1@@ -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.
David Miller
maintainerFrom: Eric Dumazet <[email protected]>
Date: Fri, 07 Sep 2012 17:07:26 +0200
Quoted 2 lines
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.
From: [email protected] (Eric W. Biederman)
Date: Thu, 06 Sep 2012 21:20:01 -0700
Quoted 16 lines
Passing uids and gids on
NETLINK_CBfrom 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_credsfor use inscm_cookieandnetlink_skb_parmsthat holds uid and gid information inkuid_tandkgid_t.- Modify
scm_set_credto fill outscm_credsby heand instead of usingcred_to_ucredto fill out struct ucred. This conversion ensures userspace does not get incorrect uid or gid values to look at.- Modify
scm_recvto convert from structscm_credsto struct ucred before copying credential values to userspace.
Quoted 7 lines
- Modify
__scm_sendto populate structscm_credson in thescm_cookie, instead of just copying struct ucred from userspace.- Modify
netlink_sendmsgto copyscm_credsinstead of struct ucred into theNETLINK_CB.Signed-off-by "Eric W. Biederman" <[email protected]>
Applied, thanks Eric.
Quoted 7 lines · David Miller
From: Eric Dumazet <[email protected]>
Date: Fri, 07 Sep 2012 17:07:26 +0200Quoted 2 lines
Seems fine to me, but I am not sure why you kept
NETLINK_CREDS()defined/used once.There are two remaining users,
kernel/audit.cand 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
David Miller
maintainerFrom: [email protected] (Eric W. Biederman)
Date: Fri, 07 Sep 2012 14:25:09 -0700
Quoted 3 lines
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.