[PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context

[PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context

[Casey Schaufler]

Add an entry /proc/.../attr/context which displays the full
process security "context" in compound format:
        lsm1\0value\0lsm2\0value\0...
This entry is not writable.

A security module may decide that its policy does not allow
this information to be displayed. In this case none of the
information will be displayed.

Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-api@vger.kernel.org
---
 Documentation/security/lsm.rst       | 28 +++++++++++
 fs/proc/base.c                       |  1 +
 include/linux/lsm_hooks.h            |  6 +++
 security/apparmor/include/procattr.h |  2 +-
 security/apparmor/lsm.c              |  8 +++-
 security/apparmor/procattr.c         | 22 +++++----
 security/security.c                  | 70 ++++++++++++++++++++++++++++
 security/selinux/hooks.c             |  2 +-
 security/smack/smack_lsm.c           |  2 +-
 9 files changed, 126 insertions(+), 15 deletions(-)

diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
index 6a2a2e973080..fd4c87358d54 100644
--- a/Documentation/security/lsm.rst
+++ b/Documentation/security/lsm.rst
@@ -129,3 +129,31 @@ to identify it as the first security module to be registered.
 The capabilities security module does not use the general security
 blobs, unlike other modules. The reasons are historical and are
 based on overhead, complexity and performance concerns.
+
+LSM External Interfaces
+=======================
+
+The LSM infrastructure does not generally provide external interfaces.
+The individual security modules provide what external interfaces they
+require.
+
+The file ``/sys/kernel/security/lsm`` provides a comma
+separated list of the active security modules.
+
+The file ``/proc/pid/attr/display`` contains the name of the security
+module for which the ``/proc/pid/attr/current`` interface will
+apply. This interface can be written to.
+
+The infrastructure does provide an interface for the special
+case where multiple security modules provide a process context.
+This is provided in compound context format.
+
+-  `lsm\0value\0lsm\0value\0`
+
+The `lsm` and `value` fields are nul terminated bytestrings.
+Each field may contain whitespace or non-printable characters.
+The nul bytes are included in the size of a compound context.
+The context ``Bell\0Secret\0Biba\0Loose\0`` has a size of 23.
+
+The file ``/proc/pid/attr/context`` provides the security
+context of the identified process.
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 40471a12ced2..ba8b0316e999 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2795,6 +2795,7 @@ static const struct pid_entry attr_dir_stuff[] = {
 	ATTR(NULL, "keycreate",		0666),
 	ATTR(NULL, "sockcreate",	0666),
 	ATTR(NULL, "display",		0666),
+	ATTR(NULL, "context",		0444),
 #ifdef CONFIG_SECURITY_SMACK
 	DIR("smack",			0555,
 	    proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 6908fa03cf31..5be04dacc17a 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1359,6 +1359,12 @@
  *	@pages contains the number of pages.
  *	Return 0 if permission is granted.
  *
+ * @getprocattr:
+ *	Provide the named process attribute for display in special files in
+ *	the /proc/.../attr directory.  Attribute naming and the data displayed
+ *	is at the discretion of the security modules.  The exception is the
+ *	"context" attribute, which will contain the security context of the
+ *	task as a nul terminated text string without trailing whitespace.
  * @ismaclabel:
  *	Check if the extended attribute specified by @name
  *	represents a MAC label. Returns 1 if name is a MAC
diff --git a/security/apparmor/include/procattr.h b/security/apparmor/include/procattr.h
index 31689437e0e1..03dbfdb2f2c0 100644
--- a/security/apparmor/include/procattr.h
+++ b/security/apparmor/include/procattr.h
@@ -11,7 +11,7 @@
 #ifndef __AA_PROCATTR_H
 #define __AA_PROCATTR_H
 
-int aa_getprocattr(struct aa_label *label, char **string);
+int aa_getprocattr(struct aa_label *label, char **string, bool newline);
 int aa_setprocattr_changehat(char *args, size_t size, int flags);
 
 #endif /* __AA_PROCATTR_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 31a6f11890f1..7ce570b0f491 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -593,6 +593,7 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
 	const struct cred *cred = get_task_cred(task);
 	struct aa_task_ctx *ctx = task_ctx(current);
 	struct aa_label *label = NULL;
+	bool newline = true;
 
 	if (strcmp(name, "current") == 0)
 		label = aa_get_newest_label(cred_label(cred));
@@ -600,11 +601,14 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
 		label = aa_get_newest_label(ctx->previous);
 	else if (strcmp(name, "exec") == 0 && ctx->onexec)
 		label = aa_get_newest_label(ctx->onexec);
-	else
+	else if (strcmp(name, "context") == 0) {
+		label = aa_get_newest_label(cred_label(cred));
+		newline = false;
+	} else
 		error = -EINVAL;
 
 	if (label)
-		error = aa_getprocattr(label, value);
+		error = aa_getprocattr(label, value, newline);
 
 	aa_put_label(label);
 	put_cred(cred);
diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c
index c929bf4a3df1..be3b083d9b74 100644
--- a/security/apparmor/procattr.c
+++ b/security/apparmor/procattr.c
@@ -20,6 +20,7 @@
  * aa_getprocattr - Return the profile information for @profile
  * @profile: the profile to print profile info about  (NOT NULL)
  * @string: Returns - string containing the profile info (NOT NULL)
+ * @newline: Should a newline be added to @string.
  *
  * Returns: length of @string on success else error on failure
  *
@@ -30,20 +31,21 @@
  *
  * Returns: size of string placed in @string else error code on failure
  */
-int aa_getprocattr(struct aa_label *label, char **string)
+int aa_getprocattr(struct aa_label *label, char **string, bool newline)
 {
 	struct aa_ns *ns = labels_ns(label);
 	struct aa_ns *current_ns = aa_get_current_ns();
+	int flags = FLAG_VIEW_SUBNS | FLAG_HIDDEN_UNCONFINED;
 	int len;
 
 	if (!aa_ns_visible(current_ns, ns, true)) {
 		aa_put_ns(current_ns);
 		return -EACCES;
 	}
+	if (newline)
+		flags |= FLAG_SHOW_MODE;
 
-	len = aa_label_snxprint(NULL, 0, current_ns, label,
-				FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
-				FLAG_HIDDEN_UNCONFINED);
+	len = aa_label_snxprint(NULL, 0, current_ns, label, flags);
 	AA_BUG(len < 0);
 
 	*string = kmalloc(len + 2, GFP_KERNEL);
@@ -52,19 +54,19 @@ int aa_getprocattr(struct aa_label *label, char **string)
 		return -ENOMEM;
 	}
 
-	len = aa_label_snxprint(*string, len + 2, current_ns, label,
-				FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
-				FLAG_HIDDEN_UNCONFINED);
+	len = aa_label_snxprint(*string, len + 2, current_ns, label, flags);
 	if (len < 0) {
 		aa_put_ns(current_ns);
 		return len;
 	}
 
-	(*string)[len] = '\n';
-	(*string)[len + 1] = 0;
+	if (newline) {
+		(*string)[len] = '\n';
+		(*string)[++len] = 0;
+	}
 
 	aa_put_ns(current_ns);
-	return len + 1;
+	return len;
 }
 
 /**
diff --git a/security/security.c b/security/security.c
index d35e578fa45b..bce6be720401 100644
--- a/security/security.c
+++ b/security/security.c
@@ -754,6 +754,48 @@ 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;
+	size_t llen;
+
+	llen = strlen(lsm) + 1;
+	/*
+	 * A security module may or may not provide a trailing nul on
+	 * when returning a security context. There is no definition
+	 * of which it should be, and there are modules that do it
+	 * each way.
+	 */
+	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;
+}
+
 /*
  * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
  * can be accessed with:
@@ -2124,6 +2166,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
 				char **value)
 {
 	struct security_hook_list *hp;
+	char *final = NULL;
+	char *cp;
+	int rc = 0;
+	int finallen = 0;
 	int display = lsm_task_display(current);
 	int slot = 0;
 
@@ -2151,6 +2197,30 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
 		return -ENOMEM;
 	}
 
+	if (!strcmp(name, "context")) {
+		hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
+				     list) {
+			rc = hp->hook.getprocattr(p, "context", &cp);
+			if (rc == -EINVAL)
+				continue;
+			if (rc < 0) {
+				kfree(final);
+				return rc;
+			}
+			rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
+					cp, rc);
+			kfree(cp);
+			if (rc < 0) {
+				kfree(final);
+				return rc;
+			}
+		}
+		if (final == NULL)
+			return -EINVAL;
+		*value = final;
+		return finallen;
+	}
+
 	hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
 		if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
 			continue;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c13c207c5da1..43d5c09b9a9e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6288,7 +6288,7 @@ static int selinux_getprocattr(struct task_struct *p,
 			goto bad;
 	}
 
-	if (!strcmp(name, "current"))
+	if (!strcmp(name, "current") || !strcmp(name, "context"))
 		sid = __tsec->sid;
 	else if (!strcmp(name, "prev"))
 		sid = __tsec->osid;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 6f0cdb40addc..d7bb6442f192 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3463,7 +3463,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
 	char *cp;
 	int slen;
 
-	if (strcmp(name, "current") != 0)
+	if (strcmp(name, "current") != 0 && strcmp(name, "context") != 0)
 		return -EINVAL;
 
 	cp = kstrdup(skp->smk_known, GFP_KERNEL);
-- 
2.24.1

Re: [PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context

[John Johansen]

On 7/24/20 1:32 PM, Casey Schaufler wrote:
> Add an entry /proc/.../attr/context which displays the full
> process security "context" in compound format:
>         lsm1\0value\0lsm2\0value\0...
> This entry is not writable.
> 
> A security module may decide that its policy does not allow
> this information to be displayed. In this case none of the
> information will be displayed.
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-api@vger.kernel.org
> ---
>  Documentation/security/lsm.rst       | 28 +++++++++++
>  fs/proc/base.c                       |  1 +
>  include/linux/lsm_hooks.h            |  6 +++
>  security/apparmor/include/procattr.h |  2 +-
>  security/apparmor/lsm.c              |  8 +++-
>  security/apparmor/procattr.c         | 22 +++++----
>  security/security.c                  | 70 ++++++++++++++++++++++++++++
>  security/selinux/hooks.c             |  2 +-
>  security/smack/smack_lsm.c           |  2 +-
>  9 files changed, 126 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
> index 6a2a2e973080..fd4c87358d54 100644
> --- a/Documentation/security/lsm.rst
> +++ b/Documentation/security/lsm.rst
> @@ -129,3 +129,31 @@ to identify it as the first security module to be registered.
>  The capabilities security module does not use the general security
>  blobs, unlike other modules. The reasons are historical and are
>  based on overhead, complexity and performance concerns.
> +
> +LSM External Interfaces
> +=======================
> +
> +The LSM infrastructure does not generally provide external interfaces.
> +The individual security modules provide what external interfaces they
> +require.
> +
> +The file ``/sys/kernel/security/lsm`` provides a comma
> +separated list of the active security modules.
> +
> +The file ``/proc/pid/attr/display`` contains the name of the security
> +module for which the ``/proc/pid/attr/current`` interface will
> +apply. This interface can be written to.
> +
> +The infrastructure does provide an interface for the special
> +case where multiple security modules provide a process context.
> +This is provided in compound context format.
> +
> +-  `lsm\0value\0lsm\0value\0`
> +
> +The `lsm` and `value` fields are nul terminated bytestrings.
> +Each field may contain whitespace or non-printable characters.
> +The nul bytes are included in the size of a compound context.
> +The context ``Bell\0Secret\0Biba\0Loose\0`` has a size of 23.
> +
> +The file ``/proc/pid/attr/context`` provides the security
> +context of the identified process.
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 40471a12ced2..ba8b0316e999 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2795,6 +2795,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>  	ATTR(NULL, "keycreate",		0666),
>  	ATTR(NULL, "sockcreate",	0666),
>  	ATTR(NULL, "display",		0666),
> +	ATTR(NULL, "context",		0444),
>  #ifdef CONFIG_SECURITY_SMACK
>  	DIR("smack",			0555,
>  	    proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 6908fa03cf31..5be04dacc17a 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1359,6 +1359,12 @@
>   *	@pages contains the number of pages.
>   *	Return 0 if permission is granted.
>   *
> + * @getprocattr:
> + *	Provide the named process attribute for display in special files in
> + *	the /proc/.../attr directory.  Attribute naming and the data displayed
> + *	is at the discretion of the security modules.  The exception is the
> + *	"context" attribute, which will contain the security context of the
> + *	task as a nul terminated text string without trailing whitespace.
>   * @ismaclabel:
>   *	Check if the extended attribute specified by @name
>   *	represents a MAC label. Returns 1 if name is a MAC
> diff --git a/security/apparmor/include/procattr.h b/security/apparmor/include/procattr.h
> index 31689437e0e1..03dbfdb2f2c0 100644
> --- a/security/apparmor/include/procattr.h
> +++ b/security/apparmor/include/procattr.h
> @@ -11,7 +11,7 @@
>  #ifndef __AA_PROCATTR_H
>  #define __AA_PROCATTR_H
>  
> -int aa_getprocattr(struct aa_label *label, char **string);
> +int aa_getprocattr(struct aa_label *label, char **string, bool newline);
>  int aa_setprocattr_changehat(char *args, size_t size, int flags);
>  
>  #endif /* __AA_PROCATTR_H */
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 31a6f11890f1..7ce570b0f491 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -593,6 +593,7 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>  	const struct cred *cred = get_task_cred(task);
>  	struct aa_task_ctx *ctx = task_ctx(current);
>  	struct aa_label *label = NULL;
> +	bool newline = true;
>  
>  	if (strcmp(name, "current") == 0)
>  		label = aa_get_newest_label(cred_label(cred));
> @@ -600,11 +601,14 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>  		label = aa_get_newest_label(ctx->previous);
>  	else if (strcmp(name, "exec") == 0 && ctx->onexec)
>  		label = aa_get_newest_label(ctx->onexec);
> -	else
> +	else if (strcmp(name, "context") == 0) {
> +		label = aa_get_newest_label(cred_label(cred));
> +		newline = false;
> +	} else
>  		error = -EINVAL;
>  
>  	if (label)
> -		error = aa_getprocattr(label, value);
> +		error = aa_getprocattr(label, value, newline);
>  
>  	aa_put_label(label);
>  	put_cred(cred);
> diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c
> index c929bf4a3df1..be3b083d9b74 100644
> --- a/security/apparmor/procattr.c
> +++ b/security/apparmor/procattr.c
> @@ -20,6 +20,7 @@
>   * aa_getprocattr - Return the profile information for @profile
>   * @profile: the profile to print profile info about  (NOT NULL)
>   * @string: Returns - string containing the profile info (NOT NULL)
> + * @newline: Should a newline be added to @string.
>   *
>   * Returns: length of @string on success else error on failure
>   *
> @@ -30,20 +31,21 @@
>   *
>   * Returns: size of string placed in @string else error code on failure
>   */
> -int aa_getprocattr(struct aa_label *label, char **string)
> +int aa_getprocattr(struct aa_label *label, char **string, bool newline)
>  {
>  	struct aa_ns *ns = labels_ns(label);
>  	struct aa_ns *current_ns = aa_get_current_ns();
> +	int flags = FLAG_VIEW_SUBNS | FLAG_HIDDEN_UNCONFINED;
>  	int len;
>  
>  	if (!aa_ns_visible(current_ns, ns, true)) {
>  		aa_put_ns(current_ns);
>  		return -EACCES;
>  	}
> +	if (newline)
> +		flags |= FLAG_SHOW_MODE;
>  
> -	len = aa_label_snxprint(NULL, 0, current_ns, label,
> -				FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
> -				FLAG_HIDDEN_UNCONFINED);
> +	len = aa_label_snxprint(NULL, 0, current_ns, label, flags);
>  	AA_BUG(len < 0);
>  
>  	*string = kmalloc(len + 2, GFP_KERNEL);
> @@ -52,19 +54,19 @@ int aa_getprocattr(struct aa_label *label, char **string)
>  		return -ENOMEM;
>  	}
>  
> -	len = aa_label_snxprint(*string, len + 2, current_ns, label,
> -				FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
> -				FLAG_HIDDEN_UNCONFINED);
> +	len = aa_label_snxprint(*string, len + 2, current_ns, label, flags);
>  	if (len < 0) {
>  		aa_put_ns(current_ns);
>  		return len;
>  	}
>  
> -	(*string)[len] = '\n';
> -	(*string)[len + 1] = 0;
> +	if (newline) {
> +		(*string)[len] = '\n';
> +		(*string)[++len] = 0;
> +	}
>  
>  	aa_put_ns(current_ns);
> -	return len + 1;
> +	return len;
>  }
>  
>  /**
> diff --git a/security/security.c b/security/security.c
> index d35e578fa45b..bce6be720401 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -754,6 +754,48 @@ 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;
> +	size_t llen;
> +
> +	llen = strlen(lsm) + 1;
> +	/*
> +	 * A security module may or may not provide a trailing nul on
> +	 * when returning a security context. There is no definition
> +	 * of which it should be, and there are modules that do it
> +	 * each way.
> +	 */
> +	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);

if @new doesn't have a newline appended at its end this will read 1 byte
passed the end of the @new buffer. Nor will the result have a trailing
\0 as expected unless we get lucky.


> +	kfree(*ctx);
> +	*ctx = final;
> +	*ctxlen = *ctxlen + llen + newlen;
> +	return 0;
> +}
> +
>  /*
>   * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
>   * can be accessed with:
> @@ -2124,6 +2166,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>  				char **value)
>  {
>  	struct security_hook_list *hp;
> +	char *final = NULL;
> +	char *cp;
> +	int rc = 0;
> +	int finallen = 0;

these are only used by context so they could be moved under its if, this
is really just a style comment and I'll leave it up to you

>  	int display = lsm_task_display(current);
>  	int slot = 0;
>  
> @@ -2151,6 +2197,30 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>  		return -ENOMEM;
>  	}
>  
> +	if (!strcmp(name, "context")) {
> +		hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
> +				     list) {
> +			rc = hp->hook.getprocattr(p, "context", &cp);
> +			if (rc == -EINVAL)
> +				continue;
> +			if (rc < 0) {
> +				kfree(final);
> +				return rc;
> +			}
> +			rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
> +					cp, rc);
> +			kfree(cp);
> +			if (rc < 0) {
> +				kfree(final);
> +				return rc;
> +			}
> +		}
> +		if (final == NULL)
> +			return -EINVAL;
> +		*value = final;
> +		return finallen;
> +	}
> +
>  	hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>  		if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>  			continue;
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c13c207c5da1..43d5c09b9a9e 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6288,7 +6288,7 @@ static int selinux_getprocattr(struct task_struct *p,
>  			goto bad;
>  	}
>  
> -	if (!strcmp(name, "current"))
> +	if (!strcmp(name, "current") || !strcmp(name, "context"))
>  		sid = __tsec->sid;
>  	else if (!strcmp(name, "prev"))
>  		sid = __tsec->osid;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 6f0cdb40addc..d7bb6442f192 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3463,7 +3463,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>  	char *cp;
>  	int slen;
>  
> -	if (strcmp(name, "current") != 0)
> +	if (strcmp(name, "current") != 0 && strcmp(name, "context") != 0)
>  		return -EINVAL;
>  
>  	cp = kstrdup(skp->smk_known, GFP_KERNEL);
> 

Re: [PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context

[Casey Schaufler]

On 7/30/2020 3:03 AM, John Johansen wrote:
> On 7/24/20 1:32 PM, Casey Schaufler wrote:
>> Add an entry /proc/.../attr/context which displays the full
>> process security "context" in compound format:
>>         lsm1\0value\0lsm2\0value\0...
>> This entry is not writable.
>>
>> A security module may decide that its policy does not allow
>> this information to be displayed. In this case none of the
>> information will be displayed.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Cc: linux-api@vger.kernel.org
>> ---
>>  Documentation/security/lsm.rst       | 28 +++++++++++
>>  fs/proc/base.c                       |  1 +
>>  include/linux/lsm_hooks.h            |  6 +++
>>  security/apparmor/include/procattr.h |  2 +-
>>  security/apparmor/lsm.c              |  8 +++-
>>  security/apparmor/procattr.c         | 22 +++++----
>>  security/security.c                  | 70 ++++++++++++++++++++++++++++
>>  security/selinux/hooks.c             |  2 +-
>>  security/smack/smack_lsm.c           |  2 +-
>>  9 files changed, 126 insertions(+), 15 deletions(-)

<snip>

>>  
>>  /**
>> diff --git a/security/security.c b/security/security.c
>> index d35e578fa45b..bce6be720401 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -754,6 +754,48 @@ 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;
>> +	size_t llen;
>> +
>> +	llen = strlen(lsm) + 1;
>> +	/*
>> +	 * A security module may or may not provide a trailing nul on
>> +	 * when returning a security context. There is no definition
>> +	 * of which it should be, and there are modules that do it
>> +	 * each way.
>> +	 */
>> +	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);
> if @new doesn't have a newline appended at its end this will read 1 byte
> passed the end of the @new buffer. Nor will the result have a trailing
> \0 as expected unless we get lucky.

@new will never have a newline at the end. The trailing nul comes
from the allocation being done with kzalloc(). This function has to
be considered in the context of its caller.

>
>
>> +	kfree(*ctx);
>> +	*ctx = final;
>> +	*ctxlen = *ctxlen + llen + newlen;
>> +	return 0;
>> +}
>> +
>>  /*
>>   * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
>>   * can be accessed with:
>> @@ -2124,6 +2166,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>>  				char **value)
>>  {
>>  	struct security_hook_list *hp;
>> +	char *final = NULL;
>> +	char *cp;
>> +	int rc = 0;
>> +	int finallen = 0;
> these are only used by context so they could be moved under its if, this
> is really just a style comment and I'll leave it up to you

Old coding habits die hard. Unless there's value to gain, I'll leave it
as is.

>
>>  	int display = lsm_task_display(current);
>>  	int slot = 0;
>>  
>> @@ -2151,6 +2197,30 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>>  		return -ENOMEM;
>>  	}
>>  
>> +	if (!strcmp(name, "context")) {
>> +		hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
>> +				     list) {
>> +			rc = hp->hook.getprocattr(p, "context", &cp);
>> +			if (rc == -EINVAL)
>> +				continue;
>> +			if (rc < 0) {
>> +				kfree(final);
>> +				return rc;
>> +			}
>> +			rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
>> +					cp, rc);
>> +			kfree(cp);
>> +			if (rc < 0) {
>> +				kfree(final);
>> +				return rc;
>> +			}
>> +		}
>> +		if (final == NULL)
>> +			return -EINVAL;
>> +		*value = final;
>> +		return finallen;
>> +	}
>> +
>>  	hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>>  		if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>>  			continue;
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index c13c207c5da1..43d5c09b9a9e 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -6288,7 +6288,7 @@ static int selinux_getprocattr(struct task_struct *p,
>>  			goto bad;
>>  	}
>>  
>> -	if (!strcmp(name, "current"))
>> +	if (!strcmp(name, "current") || !strcmp(name, "context"))
>>  		sid = __tsec->sid;
>>  	else if (!strcmp(name, "prev"))
>>  		sid = __tsec->osid;
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 6f0cdb40addc..d7bb6442f192 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -3463,7 +3463,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>>  	char *cp;
>>  	int slen;
>>  
>> -	if (strcmp(name, "current") != 0)
>> +	if (strcmp(name, "current") != 0 && strcmp(name, "context") != 0)
>>  		return -EINVAL;
>>  
>>  	cp = kstrdup(skp->smk_known, GFP_KERNEL);
>>

Re: [PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context

[John Johansen]

On 7/30/20 1:44 PM, Casey Schaufler wrote:
> On 7/30/2020 3:03 AM, John Johansen wrote:
>> On 7/24/20 1:32 PM, Casey Schaufler wrote:
>>> Add an entry /proc/.../attr/context which displays the full
>>> process security "context" in compound format:
>>>         lsm1\0value\0lsm2\0value\0...
>>> This entry is not writable.
>>>
>>> A security module may decide that its policy does not allow
>>> this information to be displayed. In this case none of the
>>> information will be displayed.
>>>
>>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>> Cc: linux-api@vger.kernel.org
>>> ---
>>>  Documentation/security/lsm.rst       | 28 +++++++++++
>>>  fs/proc/base.c                       |  1 +
>>>  include/linux/lsm_hooks.h            |  6 +++
>>>  security/apparmor/include/procattr.h |  2 +-
>>>  security/apparmor/lsm.c              |  8 +++-
>>>  security/apparmor/procattr.c         | 22 +++++----
>>>  security/security.c                  | 70 ++++++++++++++++++++++++++++
>>>  security/selinux/hooks.c             |  2 +-
>>>  security/smack/smack_lsm.c           |  2 +-
>>>  9 files changed, 126 insertions(+), 15 deletions(-)
> 
> <snip>
> 
>>>  
>>>  /**
>>> diff --git a/security/security.c b/security/security.c
>>> index d35e578fa45b..bce6be720401 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -754,6 +754,48 @@ 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;
>>> +	size_t llen;
>>> +
>>> +	llen = strlen(lsm) + 1;
>>> +	/*
>>> +	 * A security module may or may not provide a trailing nul on
>>> +	 * when returning a security context. There is no definition
>>> +	 * of which it should be, and there are modules that do it
>>> +	 * each way.
>>> +	 */
>>> +	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);
>> if @new doesn't have a newline appended at its end this will read 1 byte
>> passed the end of the @new buffer. Nor will the result have a trailing
>> \0 as expected unless we get lucky.
> 
> @new will never have a newline at the end. The trailing nul comes
> from the allocation being done with kzalloc(). This function has to
> be considered in the context of its caller.
> 

ugh, sorry not trailing newline, I meant trailing \0. The problem isn't
the kzalloc, the target has the space. It is the source @new. It is
dangerous to assume that the @new buffer has a null byte after its
declared length. Which is potentially what we are doing if @new
doesn't have an embedded null byte. In that case strlen(new, newlen)
will then return newlen and we add 1 to it.

which means in the memcpy we are copying an extra byte beyond what
was declared to exist in @new.

>>
>>
>>> +	kfree(*ctx);
>>> +	*ctx = final;
>>> +	*ctxlen = *ctxlen + llen + newlen;
>>> +	return 0;
>>> +}
>>> +
>>>  /*
>>>   * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
>>>   * can be accessed with:
>>> @@ -2124,6 +2166,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>>>  				char **value)
>>>  {
>>>  	struct security_hook_list *hp;
>>> +	char *final = NULL;
>>> +	char *cp;
>>> +	int rc = 0;
>>> +	int finallen = 0;
>> these are only used by context so they could be moved under its if, this
>> is really just a style comment and I'll leave it up to you
> 
> Old coding habits die hard. Unless there's value to gain, I'll leave it
> as is.
> 
>>
>>>  	int display = lsm_task_display(current);
>>>  	int slot = 0;
>>>  
>>> @@ -2151,6 +2197,30 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>>>  		return -ENOMEM;
>>>  	}
>>>  
>>> +	if (!strcmp(name, "context")) {
>>> +		hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
>>> +				     list) {
>>> +			rc = hp->hook.getprocattr(p, "context", &cp);
>>> +			if (rc == -EINVAL)
>>> +				continue;
>>> +			if (rc < 0) {
>>> +				kfree(final);
>>> +				return rc;
>>> +			}
>>> +			rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
>>> +					cp, rc);
>>> +			kfree(cp);
>>> +			if (rc < 0) {
>>> +				kfree(final);
>>> +				return rc;
>>> +			}
>>> +		}
>>> +		if (final == NULL)
>>> +			return -EINVAL;
>>> +		*value = final;
>>> +		return finallen;
>>> +	}
>>> +
>>>  	hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>>>  		if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>>>  			continue;
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index c13c207c5da1..43d5c09b9a9e 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -6288,7 +6288,7 @@ static int selinux_getprocattr(struct task_struct *p,
>>>  			goto bad;
>>>  	}
>>>  
>>> -	if (!strcmp(name, "current"))
>>> +	if (!strcmp(name, "current") || !strcmp(name, "context"))
>>>  		sid = __tsec->sid;
>>>  	else if (!strcmp(name, "prev"))
>>>  		sid = __tsec->osid;
>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>> index 6f0cdb40addc..d7bb6442f192 100644
>>> --- a/security/smack/smack_lsm.c
>>> +++ b/security/smack/smack_lsm.c
>>> @@ -3463,7 +3463,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>>>  	char *cp;
>>>  	int slen;
>>>  
>>> -	if (strcmp(name, "current") != 0)
>>> +	if (strcmp(name, "current") != 0 && strcmp(name, "context") != 0)
>>>  		return -EINVAL;
>>>  
>>>  	cp = kstrdup(skp->smk_known, GFP_KERNEL);
>>>

Re: [PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context

[Casey Schaufler]

On 7/30/2020 1:57 PM, John Johansen wrote:
> On 7/30/20 1:44 PM, Casey Schaufler wrote:
>> On 7/30/2020 3:03 AM, John Johansen wrote:
>>> On 7/24/20 1:32 PM, Casey Schaufler wrote:
>>>> Add an entry /proc/.../attr/context which displays the full
>>>> process security "context" in compound format:
>>>>         lsm1\0value\0lsm2\0value\0...
>>>> This entry is not writable.
>>>>
>>>> A security module may decide that its policy does not allow
>>>> this information to be displayed. In this case none of the
>>>> information will be displayed.
>>>>
>>>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> Cc: linux-api@vger.kernel.org
>>>> ---
>>>>  Documentation/security/lsm.rst       | 28 +++++++++++
>>>>  fs/proc/base.c                       |  1 +
>>>>  include/linux/lsm_hooks.h            |  6 +++
>>>>  security/apparmor/include/procattr.h |  2 +-
>>>>  security/apparmor/lsm.c              |  8 +++-
>>>>  security/apparmor/procattr.c         | 22 +++++----
>>>>  security/security.c                  | 70 ++++++++++++++++++++++++++++
>>>>  security/selinux/hooks.c             |  2 +-
>>>>  security/smack/smack_lsm.c           |  2 +-
>>>>  9 files changed, 126 insertions(+), 15 deletions(-)
>> <snip>
>>
>>>>  
>>>>  /**
>>>> diff --git a/security/security.c b/security/security.c
>>>> index d35e578fa45b..bce6be720401 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -754,6 +754,48 @@ 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;
>>>> +	size_t llen;
>>>> +
>>>> +	llen = strlen(lsm) + 1;
>>>> +	/*
>>>> +	 * A security module may or may not provide a trailing nul on
>>>> +	 * when returning a security context. There is no definition
>>>> +	 * of which it should be, and there are modules that do it
>>>> +	 * each way.
>>>> +	 */
>>>> +	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);
>>> if @new doesn't have a newline appended at its end this will read 1 byte
>>> passed the end of the @new buffer. Nor will the result have a trailing
>>> \0 as expected unless we get lucky.
>> @new will never have a newline at the end. The trailing nul comes
>> from the allocation being done with kzalloc(). This function has to
>> be considered in the context of its caller.
>>
> ugh, sorry not trailing newline, I meant trailing \0. The problem isn't
> the kzalloc, the target has the space. It is the source @new. It is
> dangerous to assume that the @new buffer has a null byte after its
> declared length. Which is potentially what we are doing if @new
> doesn't have an embedded null byte. In that case strlen(new, newlen)
> will then return newlen and we add 1 to it.
>
> which means in the memcpy we are copying an extra byte beyond what
> was declared to exist in @new.

You're right. Good point. Fix coming.
??