Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Casey Schaufler <casey@schaufler-ca.com>
To: Kees Cook <keescook@chromium.org>
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 17:18:34 -0700
Message-ID: <9cfae450-6878-4666-1328-934ea07a8e56@schaufler-ca.com> (raw)
In-Reply-To: <201908081500.992E5330@keescook>

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
>>


  reply index

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
2019-08-09  0:18     ` Casey Schaufler [this message]
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 publically 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=9cfae450-6878-4666-1328-934ea07a8e56@schaufler-ca.com \
    --to=casey@schaufler-ca.com \
    --cc=casey.schaufler@intel.com \
    --cc=jmorris@namei.org \
    --cc=john.johansen@canonical.com \
    --cc=keescook@chromium.org \
    --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

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org linux-security-module@archiver.kernel.org
	public-inbox-index linux-security-module


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/ public-inbox