| Message ID | 20190807194410.9762-27-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
On Wed, Aug 07, 2019 at 12:44:08PM -0700, Casey Schaufler wrote: > The getsockopt SO_PEERSEC provides the LSM based security > information for a single module, but for reasons of backward > compatibility cannot include the information for multiple > modules. A new option SO_PEERCONTEXT is added to report the > security "context" of multiple modules using a "compound" format > > lsm1\0value\0lsm2\0value\0 > > This is expected to be used by system services, including dbus-daemon. > The exact format of a compound context has been the subject of > considerable debate. This format was suggested by Simon McVittie, > a dbus maintainer with a significant stake in the format being > uasable. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > [...] > diff --git a/security/security.c b/security/security.c > index 2f4a430a1126..0ea7ee27e331 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2117,8 +2117,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value, > hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { > if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) > continue; > - if (lsm == NULL && *display != LSMBLOB_INVALID && > - *display != hp->lsmid->slot) > + if (lsm == NULL && display != NULL && > + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot) > continue; > return hp->hook.setprocattr(name, value, size); > } > @@ -2342,17 +2342,91 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) > EXPORT_SYMBOL(security_sock_rcv_skb); > > int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, > - int __user *optlen, unsigned len) > + int __user *optlen, unsigned len, > + int display) > { > - int display = lsm_task_display(current); > struct security_hook_list *hp; > + char *final = NULL; > + char *cp; > + char *tp; > + int rc = 0; > + unsigned finallen = 0; > + unsigned llen; > + unsigned clen = 0; > + unsigned tlen; Please move the case-specific variables into the case scope, like (and expand type names): case LSMBLOB_COMPOUND: { unsigned int clen ...; > + > + switch (display) { > + case LSMBLOB_DISPLAY: > + rc = -ENOPROTOOPT; > + display = lsm_task_display(current); > + hlist_for_each_entry(hp, > + &security_hook_heads.socket_getpeersec_stream, > + list) > + if (display == LSMBLOB_INVALID || > + display == hp->lsmid->slot) { > + rc = hp->hook.socket_getpeersec_stream(sock, > + &final, &finallen, len); > + break; > + } > + break; > + case LSMBLOB_COMPOUND: > + /* > + * A compound context, in the form [lsm\0value\0]... > + */ > + hlist_for_each_entry(hp, > + &security_hook_heads.socket_getpeersec_stream, > + list) { > + rc = hp->hook.socket_getpeersec_stream(sock, &cp, &clen, > + len); Is passing "len" here useful at all? It's kind of a lie, but nothing else wouldn't also be a lie. :) > + if (rc == -EINVAL || rc == -ENOPROTOOPT) { > + rc = 0; > + continue; > + } > + if (rc) { > + kfree(final); > + return rc; > + } > + /* > + * Don't propogate trailing nul bytes. typo: propagate > + */ > + clen = strnlen(cp, clen) + 1; > + llen = strlen(hp->lsmid->lsm) + 1; > + tlen = llen + clen; > + if (final) > + tlen += finallen; You can drop the "if (final)" since finallen is initialized to 0. > + tp = kzalloc(tlen, GFP_KERNEL); I'm not a huge fan of "c", "l", and "t" prefixes -- can you just make these a little more self-documenting? cp and clen could be value and value_len. llen could be lsm_name_len. tp and tlen could be tuple and tuple_len. (And maybe final and finallen could be compound and compound_len?) > + if (tp == NULL) { > + kfree(cp); > + kfree(final); > + return -ENOMEM; > + } > + if (final) > + memcpy(tp, final, finallen); > + memcpy(tp + finallen, hp->lsmid->lsm, llen); > + memcpy(tp + finallen + llen, cp, clen); > + kfree(cp); > + if (final) > + kfree(final); Just kfree(final) is safe here -- kfree(NULL) is valid. > + final = tp; > + finallen = tlen; > + } > + if (final == NULL) > + return -EINVAL; > + break; > + default: > + return -EINVAL; > + } > > - hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream, > - list) > - if (display == LSMBLOB_INVALID || display == hp->lsmid->slot) > - return hp->hook.socket_getpeersec_stream(sock, optval, > - optlen, len); > - return -ENOPROTOOPT; > + if (finallen > len) > + rc = -ERANGE; > + else if (copy_to_user(optval, final, finallen)) > + rc = -EFAULT; > + > + if (put_user(finallen, optlen)) > + rc = -EFAULT; > + > + kfree(final); > + return rc; > } Otherwise, looks good. Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > > int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index fcad2e3432d2..5e7d61754798 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4923,10 +4923,8 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) > return err; > } > > -static int selinux_socket_getpeersec_stream(struct socket *sock, > - char __user *optval, > - int __user *optlen, > - unsigned int len) > +static int selinux_socket_getpeersec_stream(struct socket *sock, char **optval, > + int *optlen, unsigned int len) > { > int err = 0; > char *scontext; > @@ -4946,18 +4944,12 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, > if (err) > return err; > > - if (scontext_len > len) { > + if (scontext_len > len) > err = -ERANGE; > - goto out_len; > - } > - > - if (copy_to_user(optval, scontext, scontext_len)) > - err = -EFAULT; > + else > + *optval = scontext; > > -out_len: > - if (put_user(scontext_len, optlen)) > - err = -EFAULT; > - kfree(scontext); > + *optlen = scontext_len; > return err; > } > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index 7a30b8692b1e..40c75205a914 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -3919,28 +3919,29 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) > * > * returns zero on success, an error code otherwise > */ > -static int smack_socket_getpeersec_stream(struct socket *sock, > - char __user *optval, > - int __user *optlen, unsigned len) > +static int smack_socket_getpeersec_stream(struct socket *sock, char **optval, > + int *optlen, unsigned len) > { > - struct socket_smack *ssp; > - char *rcp = ""; > - int slen = 1; > + struct socket_smack *ssp = smack_sock(sock->sk); > + char *rcp; > + int slen; > int rc = 0; > > - ssp = smack_sock(sock->sk); > - if (ssp->smk_packet != NULL) { > - rcp = ssp->smk_packet->smk_known; > - slen = strlen(rcp) + 1; > + if (ssp->smk_packet == NULL) { > + *optlen = 0; > + return -EINVAL; > } > > + rcp = ssp->smk_packet->smk_known; > + slen = strlen(rcp) + 1; > if (slen > len) > rc = -ERANGE; > - else if (copy_to_user(optval, rcp, slen) != 0) > - rc = -EFAULT; > - > - if (put_user(slen, optlen) != 0) > - rc = -EFAULT; > + else { > + *optval = kstrdup(rcp, GFP_KERNEL); > + if (*optval == NULL) > + rc = -ENOMEM; > + } > + *optlen = slen; > > return rc; > } > -- > 2.20.1 >
On 8/8/2019 3:21 PM, Kees Cook wrote: > On Wed, Aug 07, 2019 at 12:44:08PM -0700, Casey Schaufler wrote: >> The getsockopt SO_PEERSEC provides the LSM based security >> information for a single module, but for reasons of backward >> compatibility cannot include the information for multiple >> modules. A new option SO_PEERCONTEXT is added to report the >> security "context" of multiple modules using a "compound" format >> >> lsm1\0value\0lsm2\0value\0 >> >> This is expected to be used by system services, including dbus-daemon. >> The exact format of a compound context has been the subject of >> considerable debate. This format was suggested by Simon McVittie, >> a dbus maintainer with a significant stake in the format being >> uasable. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> [...] >> diff --git a/security/security.c b/security/security.c >> index 2f4a430a1126..0ea7ee27e331 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -2117,8 +2117,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value, >> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { >> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) >> continue; >> - if (lsm == NULL && *display != LSMBLOB_INVALID && >> - *display != hp->lsmid->slot) >> + if (lsm == NULL && display != NULL && >> + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot) >> continue; >> return hp->hook.setprocattr(name, value, size); >> } >> @@ -2342,17 +2342,91 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) >> EXPORT_SYMBOL(security_sock_rcv_skb); >> >> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, >> - int __user *optlen, unsigned len) >> + int __user *optlen, unsigned len, >> + int display) >> { >> - int display = lsm_task_display(current); >> struct security_hook_list *hp; >> + char *final = NULL; >> + char *cp; >> + char *tp; >> + int rc = 0; >> + unsigned finallen = 0; >> + unsigned llen; >> + unsigned clen = 0; >> + unsigned tlen; > Please move the case-specific variables into the case scope, like (and > expand type names): > > case LSMBLOB_COMPOUND: { > unsigned int clen ...; I've never been a fan of that style. I'll probably make the special cases into functions. > >> + >> + switch (display) { >> + case LSMBLOB_DISPLAY: >> + rc = -ENOPROTOOPT; >> + display = lsm_task_display(current); >> + hlist_for_each_entry(hp, >> + &security_hook_heads.socket_getpeersec_stream, >> + list) >> + if (display == LSMBLOB_INVALID || >> + display == hp->lsmid->slot) { >> + rc = hp->hook.socket_getpeersec_stream(sock, >> + &final, &finallen, len); >> + break; >> + } >> + break; >> + case LSMBLOB_COMPOUND: >> + /* >> + * A compound context, in the form [lsm\0value\0]... >> + */ >> + hlist_for_each_entry(hp, >> + &security_hook_heads.socket_getpeersec_stream, >> + list) { >> + rc = hp->hook.socket_getpeersec_stream(sock, &cp, &clen, >> + len); > Is passing "len" here useful at all? It's kind of a lie, but nothing > else wouldn't also be a lie. :) I could be reducing the value of len for each component gathered. It requires yet another temporary length variable, and exceeding the len passed has to get checked in here anyway. I may be able to make this more sensible. > >> + if (rc == -EINVAL || rc == -ENOPROTOOPT) { >> + rc = 0; >> + continue; >> + } >> + if (rc) { >> + kfree(final); >> + return rc; >> + } >> + /* >> + * Don't propogate trailing nul bytes. > typo: propagate Not a typo, a spelling error. I'm real good at spelling errors. > >> + */ >> + clen = strnlen(cp, clen) + 1; >> + llen = strlen(hp->lsmid->lsm) + 1; >> + tlen = llen + clen; >> + if (final) >> + tlen += finallen; > You can drop the "if (final)" since finallen is initialized to 0. Right you are. > >> + tp = kzalloc(tlen, GFP_KERNEL); > I'm not a huge fan of "c", "l", and "t" prefixes -- can you just make > these a little more self-documenting? cp and clen could be value and > value_len. llen could be lsm_name_len. tp and tlen could be tuple and > tuple_len. (And maybe final and finallen could be compound and > compound_len?) You didn't spend much time using TTY33s, did you? Or 80 column punch cards where the last 8 are reserved for collation numbers, I'd bet. Kids. OK, I'll see what I can do since I will be fiddling about anyway. > >> + if (tp == NULL) { >> + kfree(cp); >> + kfree(final); >> + return -ENOMEM; >> + } >> + if (final) >> + memcpy(tp, final, finallen); >> + memcpy(tp + finallen, hp->lsmid->lsm, llen); >> + memcpy(tp + finallen + llen, cp, clen); >> + kfree(cp); >> + if (final) >> + kfree(final); > Just kfree(final) is safe here -- kfree(NULL) is valid. Just so. At some point there was more in the if condition. > >> + final = tp; >> + finallen = tlen; >> + } >> + if (final == NULL) >> + return -EINVAL; >> + break; >> + default: >> + return -EINVAL; >> + } >> >> - hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream, >> - list) >> - if (display == LSMBLOB_INVALID || display == hp->lsmid->slot) >> - return hp->hook.socket_getpeersec_stream(sock, optval, >> - optlen, len); >> - return -ENOPROTOOPT; >> + if (finallen > len) >> + rc = -ERANGE; >> + else if (copy_to_user(optval, final, finallen)) >> + rc = -EFAULT; >> + >> + if (put_user(finallen, optlen)) >> + rc = -EFAULT; >> + >> + kfree(final); >> + return rc; >> } > Otherwise, looks good. > > Reviewed-by: Kees Cook <keescook@chromium.org> > > -Kees > >> >> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index fcad2e3432d2..5e7d61754798 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -4923,10 +4923,8 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) >> return err; >> } >> >> -static int selinux_socket_getpeersec_stream(struct socket *sock, >> - char __user *optval, >> - int __user *optlen, >> - unsigned int len) >> +static int selinux_socket_getpeersec_stream(struct socket *sock, char **optval, >> + int *optlen, unsigned int len) >> { >> int err = 0; >> char *scontext; >> @@ -4946,18 +4944,12 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, >> if (err) >> return err; >> >> - if (scontext_len > len) { >> + if (scontext_len > len) >> err = -ERANGE; >> - goto out_len; >> - } >> - >> - if (copy_to_user(optval, scontext, scontext_len)) >> - err = -EFAULT; >> + else >> + *optval = scontext; >> >> -out_len: >> - if (put_user(scontext_len, optlen)) >> - err = -EFAULT; >> - kfree(scontext); >> + *optlen = scontext_len; >> return err; >> } >> >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >> index 7a30b8692b1e..40c75205a914 100644 >> --- a/security/smack/smack_lsm.c >> +++ b/security/smack/smack_lsm.c >> @@ -3919,28 +3919,29 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) >> * >> * returns zero on success, an error code otherwise >> */ >> -static int smack_socket_getpeersec_stream(struct socket *sock, >> - char __user *optval, >> - int __user *optlen, unsigned len) >> +static int smack_socket_getpeersec_stream(struct socket *sock, char **optval, >> + int *optlen, unsigned len) >> { >> - struct socket_smack *ssp; >> - char *rcp = ""; >> - int slen = 1; >> + struct socket_smack *ssp = smack_sock(sock->sk); >> + char *rcp; >> + int slen; >> int rc = 0; >> >> - ssp = smack_sock(sock->sk); >> - if (ssp->smk_packet != NULL) { >> - rcp = ssp->smk_packet->smk_known; >> - slen = strlen(rcp) + 1; >> + if (ssp->smk_packet == NULL) { >> + *optlen = 0; >> + return -EINVAL; >> } >> >> + rcp = ssp->smk_packet->smk_known; >> + slen = strlen(rcp) + 1; >> if (slen > len) >> rc = -ERANGE; >> - else if (copy_to_user(optval, rcp, slen) != 0) >> - rc = -EFAULT; >> - >> - if (put_user(slen, optlen) != 0) >> - rc = -EFAULT; >> + else { >> + *optval = kstrdup(rcp, GFP_KERNEL); >> + if (*optval == NULL) >> + rc = -ENOMEM; >> + } >> + *optlen = slen; >> >> return rc; >> } >> -- >> 2.20.1 >>
diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h index 976e89b116e5..019e5fa8bcda 100644 --- a/arch/alpha/include/uapi/asm/socket.h +++ b/arch/alpha/include/uapi/asm/socket.h @@ -121,6 +121,7 @@ #define SO_RCVTIMEO_NEW 66 #define SO_SNDTIMEO_NEW 67 +#define SO_PEERCONTEXT 68 #if !defined(__KERNEL__) diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h index d41765cfbc6e..df8d984d76ed 100644 --- a/arch/mips/include/uapi/asm/socket.h +++ b/arch/mips/include/uapi/asm/socket.h @@ -132,6 +132,7 @@ #define SO_RCVTIMEO_NEW 66 #define SO_SNDTIMEO_NEW 67 +#define SO_PEERCONTEXT 68 #if !defined(__KERNEL__) diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h index 66c5dd245ac7..9ae358309f46 100644 --- a/arch/parisc/include/uapi/asm/socket.h +++ b/arch/parisc/include/uapi/asm/socket.h @@ -113,6 +113,7 @@ #define SO_RCVTIMEO_NEW 0x4040 #define SO_SNDTIMEO_NEW 0x4041 +#define SO_PEERCONTEXT 0x4042 #if !defined(__KERNEL__) diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h index 9265a9eece15..e8a53ef65210 100644 --- a/arch/sparc/include/uapi/asm/socket.h +++ b/arch/sparc/include/uapi/asm/socket.h @@ -114,6 +114,7 @@ #define SO_RCVTIMEO_NEW 0x0044 #define SO_SNDTIMEO_NEW 0x0045 +#define SO_PEERCONTEXT 0x0046 #if !defined(__KERNEL__) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 33e5ab4af9f8..b0f788bf82b6 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -864,8 +864,8 @@ * SO_GETPEERSEC. For tcp sockets this can be meaningful if the * socket is associated with an ipsec SA. * @sock is the local socket. - * @optval userspace memory where the security state is to be copied. - * @optlen userspace int where the module should copy the actual length + * @optval memory where the security state is to be copied. + * @optlen int where the module should copy the actual length * of the security state. * @len as input is the maximum length to copy to userspace provided * by the caller. @@ -1697,9 +1697,8 @@ union security_list_options { int (*socket_setsockopt)(struct socket *sock, int level, int optname); int (*socket_shutdown)(struct socket *sock, int how); int (*socket_sock_rcv_skb)(struct sock *sk, struct sk_buff *skb); - int (*socket_getpeersec_stream)(struct socket *sock, - char __user *optval, - int __user *optlen, unsigned len); + int (*socket_getpeersec_stream)(struct socket *sock, char **optval, + int *optlen, unsigned len); int (*socket_getpeersec_dgram)(struct socket *sock, struct sk_buff *skb, u32 *secid); int (*sk_alloc_security)(struct sock *sk, int family, gfp_t priority); diff --git a/include/linux/security.h b/include/linux/security.h index 12f6d5fcbf6a..0665a27a2891 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -130,6 +130,7 @@ struct lsmblob { #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */ #define LSMBLOB_DISPLAY -4 /* Use the "display" slot */ #define LSMBLOB_FIRST -5 /* Use the default "display" slot */ +#define LSMBLOB_COMPOUND -6 /* A compound "display" */ /** * lsmblob_init - initialize an lsmblob structure. @@ -1324,7 +1325,8 @@ int security_socket_setsockopt(struct socket *sock, int level, int optname); int security_socket_shutdown(struct socket *sock, int how); int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb); int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len); + int __user *optlen, unsigned len, + int display); int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, struct lsmblob *blob); int security_sk_alloc(struct sock *sk, int family, gfp_t priority); @@ -1458,8 +1460,10 @@ static inline int security_sock_rcv_skb(struct sock *sk, return 0; } -static inline int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len) +static inline int security_socket_getpeersec_stream(struct socket *sock, + char __user *optval, + int __user *optlen, + unsigned len, int display) { return -ENOPROTOOPT; } diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h index 8c1391c89171..b38d080c2802 100644 --- a/include/uapi/asm-generic/socket.h +++ b/include/uapi/asm-generic/socket.h @@ -116,6 +116,7 @@ #define SO_RCVTIMEO_NEW 66 #define SO_SNDTIMEO_NEW 67 +#define SO_PEERCONTEXT 68 #if !defined(__KERNEL__) diff --git a/net/core/sock.c b/net/core/sock.c index 782343bb925b..b0955a34167c 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1412,7 +1412,12 @@ int sock_getsockopt(struct socket *sock, int level, int optname, break; case SO_PEERSEC: - return security_socket_getpeersec_stream(sock, optval, optlen, len); + return security_socket_getpeersec_stream(sock, optval, optlen, + len, LSMBLOB_DISPLAY); + + case SO_PEERCONTEXT: + return security_socket_getpeersec_stream(sock, optval, optlen, + len, LSMBLOB_COMPOUND); case SO_MARK: v.val = sk->sk_mark; diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index ec2e39aa9a84..5d25959610f9 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -1037,10 +1037,8 @@ static struct aa_label *sk_peer_label(struct sock *sk) * * Note: for tcp only valid if using ipsec or cipso on lan */ -static int apparmor_socket_getpeersec_stream(struct socket *sock, - char __user *optval, - int __user *optlen, - unsigned int len) +static int apparmor_socket_getpeersec_stream(struct socket *sock, char **optval, + int *optlen, unsigned int len) { char *name; int slen, error = 0; @@ -1060,17 +1058,11 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock, if (slen < 0) { error = -ENOMEM; } else { - if (slen > len) { + if (slen > len) error = -ERANGE; - } else if (copy_to_user(optval, name, slen)) { - error = -EFAULT; - goto out; - } - if (put_user(slen, optlen)) - error = -EFAULT; -out: - kfree(name); - + else + *optval = name; + *optlen = slen; } done: diff --git a/security/security.c b/security/security.c index 2f4a430a1126..0ea7ee27e331 100644 --- a/security/security.c +++ b/security/security.c @@ -2117,8 +2117,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value, hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) continue; - if (lsm == NULL && *display != LSMBLOB_INVALID && - *display != hp->lsmid->slot) + if (lsm == NULL && display != NULL && + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot) continue; return hp->hook.setprocattr(name, value, size); } @@ -2342,17 +2342,91 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) EXPORT_SYMBOL(security_sock_rcv_skb); int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len) + int __user *optlen, unsigned len, + int display) { - int display = lsm_task_display(current); struct security_hook_list *hp; + char *final = NULL; + char *cp; + char *tp; + int rc = 0; + unsigned finallen = 0; + unsigned llen; + unsigned clen = 0; + unsigned tlen; + + switch (display) { + case LSMBLOB_DISPLAY: + rc = -ENOPROTOOPT; + display = lsm_task_display(current); + hlist_for_each_entry(hp, + &security_hook_heads.socket_getpeersec_stream, + list) + if (display == LSMBLOB_INVALID || + display == hp->lsmid->slot) { + rc = hp->hook.socket_getpeersec_stream(sock, + &final, &finallen, len); + break; + } + break; + case LSMBLOB_COMPOUND: + /* + * A compound context, in the form [lsm\0value\0]... + */ + hlist_for_each_entry(hp, + &security_hook_heads.socket_getpeersec_stream, + list) { + rc = hp->hook.socket_getpeersec_stream(sock, &cp, &clen, + len); + if (rc == -EINVAL || rc == -ENOPROTOOPT) { + rc = 0; + continue; + } + if (rc) { + kfree(final); + return rc; + } + /* + * Don't propogate trailing nul bytes. + */ + clen = strnlen(cp, clen) + 1; + llen = strlen(hp->lsmid->lsm) + 1; + tlen = llen + clen; + if (final) + tlen += finallen; + tp = kzalloc(tlen, GFP_KERNEL); + if (tp == NULL) { + kfree(cp); + kfree(final); + return -ENOMEM; + } + if (final) + memcpy(tp, final, finallen); + memcpy(tp + finallen, hp->lsmid->lsm, llen); + memcpy(tp + finallen + llen, cp, clen); + kfree(cp); + if (final) + kfree(final); + final = tp; + finallen = tlen; + } + if (final == NULL) + return -EINVAL; + break; + default: + return -EINVAL; + } - hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream, - list) - if (display == LSMBLOB_INVALID || display == hp->lsmid->slot) - return hp->hook.socket_getpeersec_stream(sock, optval, - optlen, len); - return -ENOPROTOOPT; + if (finallen > len) + rc = -ERANGE; + else if (copy_to_user(optval, final, finallen)) + rc = -EFAULT; + + if (put_user(finallen, optlen)) + rc = -EFAULT; + + kfree(final); + return rc; } int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index fcad2e3432d2..5e7d61754798 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4923,10 +4923,8 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) return err; } -static int selinux_socket_getpeersec_stream(struct socket *sock, - char __user *optval, - int __user *optlen, - unsigned int len) +static int selinux_socket_getpeersec_stream(struct socket *sock, char **optval, + int *optlen, unsigned int len) { int err = 0; char *scontext; @@ -4946,18 +4944,12 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, if (err) return err; - if (scontext_len > len) { + if (scontext_len > len) err = -ERANGE; - goto out_len; - } - - if (copy_to_user(optval, scontext, scontext_len)) - err = -EFAULT; + else + *optval = scontext; -out_len: - if (put_user(scontext_len, optlen)) - err = -EFAULT; - kfree(scontext); + *optlen = scontext_len; return err; } diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 7a30b8692b1e..40c75205a914 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3919,28 +3919,29 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) * * returns zero on success, an error code otherwise */ -static int smack_socket_getpeersec_stream(struct socket *sock, - char __user *optval, - int __user *optlen, unsigned len) +static int smack_socket_getpeersec_stream(struct socket *sock, char **optval, + int *optlen, unsigned len) { - struct socket_smack *ssp; - char *rcp = ""; - int slen = 1; + struct socket_smack *ssp = smack_sock(sock->sk); + char *rcp; + int slen; int rc = 0; - ssp = smack_sock(sock->sk); - if (ssp->smk_packet != NULL) { - rcp = ssp->smk_packet->smk_known; - slen = strlen(rcp) + 1; + if (ssp->smk_packet == NULL) { + *optlen = 0; + return -EINVAL; } + rcp = ssp->smk_packet->smk_known; + slen = strlen(rcp) + 1; if (slen > len) rc = -ERANGE; - else if (copy_to_user(optval, rcp, slen) != 0) - rc = -EFAULT; - - if (put_user(slen, optlen) != 0) - rc = -EFAULT; + else { + *optval = kstrdup(rcp, GFP_KERNEL); + if (*optval == NULL) + rc = -ENOMEM; + } + *optlen = slen; return rc; }
The getsockopt SO_PEERSEC provides the LSM based security information for a single module, but for reasons of backward compatibility cannot include the information for multiple modules. A new option SO_PEERCONTEXT is added to report the security "context" of multiple modules using a "compound" format lsm1\0value\0lsm2\0value\0 This is expected to be used by system services, including dbus-daemon. The exact format of a compound context has been the subject of considerable debate. This format was suggested by Simon McVittie, a dbus maintainer with a significant stake in the format being uasable. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- arch/alpha/include/uapi/asm/socket.h | 1 + arch/mips/include/uapi/asm/socket.h | 1 + arch/parisc/include/uapi/asm/socket.h | 1 + arch/sparc/include/uapi/asm/socket.h | 1 + include/linux/lsm_hooks.h | 9 ++- include/linux/security.h | 10 ++- include/uapi/asm-generic/socket.h | 1 + net/core/sock.c | 7 +- security/apparmor/lsm.c | 20 ++---- security/security.c | 94 ++++++++++++++++++++++++--- security/selinux/hooks.c | 20 ++---- security/smack/smack_lsm.c | 31 ++++----- 12 files changed, 134 insertions(+), 62 deletions(-)