From: Kees Cook <keescook@chromium.org>
To: Casey Schaufler <casey@schaufler-ca.com>
Cc: casey.schaufler@intel.com, jmorris@namei.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
john.johansen@canonical.com, penguin-kernel@i-love.sakura.ne.jp,
paul@paul-moore.com, sds@tycho.nsa.gov
Subject: Re: [PATCH v7 26/28] NET: Add SO_PEERCONTEXT for multiple LSMs
Date: Thu, 8 Aug 2019 15:21:26 -0700 [thread overview]
Message-ID: <201908081500.992E5330@keescook> (raw)
In-Reply-To: <20190807194410.9762-27-casey@schaufler-ca.com>
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
>
--
Kees Cook
next prev parent reply other threads:[~2019-08-08 22:21 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-07 19:43 [PATCH v7 00/28] LSM: Module stacking for AppArmor Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 01/28] LSM: Infrastructure management of the superblock Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 02/28] LSM: Infrastructure management of the sock security Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 03/28] LSM: Infrastructure management of the key blob Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 04/28] LSM: Create and manage the lsmblob data structure Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 05/28] LSM: Use lsmblob in security_audit_rule_match Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 06/28] LSM: Use lsmblob in security_kernel_act_as Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 07/28] net: Prepare UDS for security module stacking Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 08/28] LSM: Use lsmblob in security_secctx_to_secid Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 09/28] LSM: Use lsmblob in security_secid_to_secctx Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 10/28] LSM: Use lsmblob in security_ipc_getsecid Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 11/28] LSM: Use lsmblob in security_task_getsecid Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 12/28] LSM: Use lsmblob in security_inode_getsecid Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 13/28] LSM: Use lsmblob in security_cred_getsecid Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 14/28] IMA: Change internal interfaces to use lsmblobs Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 15/28] LSM: Specify which LSM to display Casey Schaufler
2019-08-08 21:39 ` Kees Cook
2019-08-08 23:38 ` Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 16/28] LSM: Ensure the correct LSM context releaser Casey Schaufler
2019-08-07 19:43 ` [PATCH v7 17/28] LSM: Use lsmcontext in security_secid_to_secctx Casey Schaufler
2019-08-07 19:44 ` [PATCH v7 18/28] LSM: Use lsmcontext in security_dentry_init_security Casey Schaufler
2019-08-07 19:44 ` [PATCH v7 19/28] LSM: Use lsmcontext in security_inode_getsecctx Casey Schaufler
2019-08-07 19:44 ` [PATCH v7 20/28] LSM: security_secid_to_secctx in netlink netfilter Casey Schaufler
2019-08-07 19:44 ` [PATCH v7 21/28] NET: Store LSM netlabel data in a lsmblob Casey Schaufler
2019-08-07 19:44 ` [PATCH v7 22/28] SELinux: Verify LSM display sanity in binder Casey Schaufler
2019-08-08 21:55 ` Kees Cook
2019-08-09 0:56 ` Casey Schaufler
2019-08-07 19:44 ` [PATCH v7 23/28] Audit: Add subj_LSM fields when necessary Casey Schaufler
2019-08-07 19:44 ` [PATCH v7 24/28] Audit: Include object data for all security modules Casey Schaufler
2019-08-07 19:44 ` [PATCH v7 25/28] LSM: Provide an user space interface for the default display Casey Schaufler
2019-08-07 19:44 ` [PATCH v7 26/28] NET: Add SO_PEERCONTEXT for multiple LSMs Casey Schaufler
2019-08-08 22:21 ` Kees Cook [this message]
2019-08-09 0:18 ` Casey Schaufler
2019-08-07 19:44 ` [PATCH v7 27/28] LSM: Add /proc attr entry for full LSM context Casey Schaufler
2019-08-08 22:22 ` Kees Cook
2019-08-09 0:23 ` Casey Schaufler
2019-08-07 19:44 ` [PATCH v7 28/28] AppArmor: Remove the exclusive flag Casey Schaufler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201908081500.992E5330@keescook \
--to=keescook@chromium.org \
--cc=casey.schaufler@intel.com \
--cc=casey@schaufler-ca.com \
--cc=jmorris@namei.org \
--cc=john.johansen@canonical.com \
--cc=linux-security-module@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=sds@tycho.nsa.gov \
--cc=selinux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).