Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs

From: Stephen Smalley <sds@tycho.nsa.gov>
To: Casey Schaufler <casey@schaufler-ca.com>,
	casey.schaufler@intel.com, jmorris@namei.org,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org
Cc: keescook@chromium.org, john.johansen@canonical.com,
	penguin-kernel@i-love.sakura.ne.jp, paul@paul-moore.com
Subject: Re: [PATCH v12 23/25] NET: Add SO_PEERCONTEXT for multiple LSMs
Date: Wed, 18 Dec 2019 13:28:54 -0500	[thread overview]
Message-ID: <56b96440-a304-42b6-1515-1ad2659b2581@tycho.nsa.gov> (raw)
In-Reply-To: <20191216223621.5127-24-casey@schaufler-ca.com>

On 12/16/19 5:36 PM, 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
> usable.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org

Requires ack by netdev and linux-api.  A couple of comments below.

> ---

> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 2bf82e1cf347..2ae10e7f81a7 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -880,8 +880,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.

This is misleading; it suggests that the caller is providing an 
allocated buffer into which the security module copies its data. Instead 
it is just a pointer to a pointer that is then set by the security 
module to a buffer the module allocates.

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 536db4dbfcbb..b72bb90b1903 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -181,7 +181,7 @@ struct lsmblob {
>   #define LSMBLOB_NEEDED		-2	/* Slot requested on initialization */
>   #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	-5	/* A compound "display" */

I'm puzzled by the removal of LSMBLOB_FIRST by this patch; it suggests 
it was never needed in the first place by the patch that introduced it. 
But more below.

> diff --git a/security/security.c b/security/security.c
> index d0b57a7c3b31..1afe245f3246 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -723,6 +723,42 @@ static void __init lsm_early_task(struct task_struct *task)
>   		panic("%s: Early task alloc failed.\n", __func__);
>   }
>   
> +/**
> + * append_ctx - append a lsm/context pair to a compound context
> + * @ctx: the existing compound context
> + * @ctxlen: size of the old context, including terminating nul byte
> + * @lsm: new lsm name, nul terminated
> + * @new: new context, possibly nul terminated
> + * @newlen: maximum size of @new
> + *
> + * replace @ctx with a new compound context, appending @newlsm and @new
> + * to @ctx. On exit the new data replaces the old, which is freed.
> + * @ctxlen is set to the new size, which includes a trailing nul byte.
> + *
> + * Returns 0 on success, -ENOMEM if no memory is available.
> + */
> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new,
> +		      int newlen)
> +{
> +	char *final;
> +	int llen;
> +
> +	llen = strlen(lsm) + 1;
> +	newlen = strnlen(new, newlen) + 1;
> +
> +	final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
> +	if (final == NULL)
> +		return -ENOMEM;
> +	if (*ctxlen)
> +		memcpy(final, *ctx, *ctxlen);
> +	memcpy(final + *ctxlen, lsm, llen);
> +	memcpy(final + *ctxlen + llen, new, newlen);
> +	kfree(*ctx);
> +	*ctx = final;
> +	*ctxlen = *ctxlen + llen + newlen;
> +	return 0;
> +}

You should likely take some precautions against integer overflows in the 
above code?

> +
>   /*
>    * Hook list operation macros.
>    *
> @@ -2164,8 +2200,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);
>   	}

Is this a bug fix that should be folded into the earlier patch that 
introduced it?

> @@ -2196,7 +2232,7 @@ int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
>   	 */
>   	if (display == LSMBLOB_DISPLAY)
>   		display = lsm_task_display(current);
> -	else if (display == LSMBLOB_FIRST)
> +	else if (display == 0)
>   		display = LSMBLOB_INVALID;
>   	else if (display < 0) {
>   		WARN_ONCE(true,

Why is it necessary to re-map display 0 in this manner? Previously if 
display 0 was specified, it would require it to match the lsmid->slot 
value.  Won't it match anyway?

> @@ -2246,6 +2282,15 @@ void security_release_secctx(struct lsmcontext *cp)
>   	struct security_hook_list *hp;
>   	bool found = false;
>   
> +	if (cp->slot == LSMBLOB_INVALID)
> +		return;
> +
> +	if (cp->slot == LSMBLOB_COMPOUND) {
> +		kfree(cp->context);
> +		found = true;
> +		goto clear_out;
> +	}
> +

If you re-order your pr_warn() below with your memset() to address the 
earlier comment, you'll end up trying to print the freed memory.  Not a 
problem if you just drop the pr_warn() altogether.