* [PATCH v18 22/23] LSM: Add /proc attr entry for full LSM context
[not found] <20200709001234.9719-1-casey@schaufler-ca.com>
@ 2020-07-09 0:12 ` Casey Schaufler
2020-07-09 1:30 ` Jann Horn
0 siblings, 1 reply; 3+ messages in thread
From: Casey Schaufler @ 2020-07-09 0:12 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds, linux-api
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.
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 | 63 ++++++++++++++++++++++++++++
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
9 files changed, 119 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 e6d72a010606..ceaba9d4792a 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 2b729d8c94b4..3469a387d6b0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -754,6 +754,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;
+}
+
/*
* The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
* can be accessed with:
@@ -2109,6 +2145,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;
@@ -2136,6 +2176,29 @@ 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);
+ 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v18 22/23] LSM: Add /proc attr entry for full LSM context
2020-07-09 0:12 ` [PATCH v18 22/23] LSM: Add /proc attr entry for full LSM context Casey Schaufler
@ 2020-07-09 1:30 ` Jann Horn
2020-07-24 1:08 ` Casey Schaufler
0 siblings, 1 reply; 3+ messages in thread
From: Jann Horn @ 2020-07-09 1:30 UTC (permalink / raw)
To: Casey Schaufler
Cc: Casey Schaufler, James Morris, linux-security-module,
SElinux list, Kees Cook, John Johansen, Tetsuo Handa, Paul Moore,
Stephen Smalley, Linux API
On Thu, Jul 9, 2020 at 2:42 AM Casey Schaufler <casey@schaufler-ca.com> 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.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-api@vger.kernel.org
[...]
> diff --git a/security/security.c b/security/security.c
[...]
> +/**
> + * 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;
Please use size_t to represent object sizes, instead of implicitly
truncating them and assuming that that doesn't wrap. Using "int" here
not only makes it harder to statically reason about this code, it
actually can also make the generated code worse:
$ cat numtrunc.c
#include <stddef.h>
size_t my_strlen(char *p);
void *my_alloc(size_t len);
void *blah_trunc(char *p) {
int len = my_strlen(p) + 1;
return my_alloc(len);
}
void *blah_notrunc(char *p) {
size_t len = my_strlen(p) + 1;
return my_alloc(len);
}
$ gcc -O2 -c -o numtrunc.o numtrunc.c
$ objdump -d numtrunc.o
[...]
0000000000000000 <blah_trunc>:
0: 48 83 ec 08 sub $0x8,%rsp
4: e8 00 00 00 00 callq 9 <blah_trunc+0x9>
9: 48 83 c4 08 add $0x8,%rsp
d: 8d 78 01 lea 0x1(%rax),%edi
10: 48 63 ff movslq %edi,%rdi <<<<<<<<unnecessary instruction
13: e9 00 00 00 00 jmpq 18 <blah_trunc+0x18>
[...]
0000000000000020 <blah_notrunc>:
20: 48 83 ec 08 sub $0x8,%rsp
24: e8 00 00 00 00 callq 29 <blah_notrunc+0x9>
29: 48 83 c4 08 add $0x8,%rsp
2d: 48 8d 78 01 lea 0x1(%rax),%rdi
31: e9 00 00 00 00 jmpq 36 <blah_notrunc+0x16>
$
This is because GCC documents
(https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html) that
for integer conversions where the value does not fit into the signed
target type, "the value is reduced modulo 2^N to be within range of
the type"; so the compiler has to assume that you are actually
intentionally trying to truncate the more significant bits from the
length, and therefore may have to insert extra code to ensure that
this truncation happens.
> + llen = strlen(lsm) + 1;
> + newlen = strnlen(new, newlen) + 1;
This strnlen() call seems dodgy. If an LSM can return a string that
already contains null bytes, shouldn't that be considered a bug, given
that it can't be displayed properly? Would it be more appropriate to
have a WARN_ON(memchr(new, '\0', newlen)) check here and bail out if
that happens?
> + 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:
> @@ -2109,6 +2145,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;
>
[...]
> 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;
> + }
This means that if SELinux refuses to give the caller PROCESS__GETATTR
access to the target process, the entire "context" file will refuse to
show anything, even if e.g. an AppArmor label would be visible through
the LSM-specific attribute directory, right? That seems awkward. Can
you maybe omit context elements for which the access check failed
instead, or embed an extra flag byte to signal for each element
whether the lookup failed, or something along those lines?
If this is an intentional design limitation, it should probably be
documented in the commit message or so.
> + rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
> + cp, rc);
> + if (rc < 0) {
> + kfree(final);
> + return rc;
> + }
Isn't there a memory leak here? `cp` points to memory that was
allocated by hp->hook.getprocattr(), and you're not freeing it after
append_ctx(). (And append_ctx() also doesn't free it.)
> + }
> + 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;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v18 22/23] LSM: Add /proc attr entry for full LSM context
2020-07-09 1:30 ` Jann Horn
@ 2020-07-24 1:08 ` Casey Schaufler
0 siblings, 0 replies; 3+ messages in thread
From: Casey Schaufler @ 2020-07-24 1:08 UTC (permalink / raw)
To: Jann Horn
Cc: Casey Schaufler, James Morris, linux-security-module,
SElinux list, Kees Cook, John Johansen, Tetsuo Handa, Paul Moore,
Stephen Smalley, Linux API, Casey Schaufler
On 7/8/2020 6:30 PM, Jann Horn wrote:
> On Thu, Jul 9, 2020 at 2:42 AM Casey Schaufler <casey@schaufler-ca.com> 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.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Cc: linux-api@vger.kernel.org
> [...]
>> diff --git a/security/security.c b/security/security.c
> [...]
>> +/**
>> + * 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;
> Please use size_t to represent object sizes
OK.
> , instead of implicitly
> truncating them and assuming that that doesn't wrap. Using "int" here
> not only makes it harder to statically reason about this code, it
> actually can also make the generated code worse:
>
>
> $ cat numtrunc.c
> #include <stddef.h>
>
> size_t my_strlen(char *p);
> void *my_alloc(size_t len);
>
> void *blah_trunc(char *p) {
> int len = my_strlen(p) + 1;
> return my_alloc(len);
> }
>
> void *blah_notrunc(char *p) {
> size_t len = my_strlen(p) + 1;
> return my_alloc(len);
> }
> $ gcc -O2 -c -o numtrunc.o numtrunc.c
> $ objdump -d numtrunc.o
> [...]
> 0000000000000000 <blah_trunc>:
> 0: 48 83 ec 08 sub $0x8,%rsp
> 4: e8 00 00 00 00 callq 9 <blah_trunc+0x9>
> 9: 48 83 c4 08 add $0x8,%rsp
> d: 8d 78 01 lea 0x1(%rax),%edi
> 10: 48 63 ff movslq %edi,%rdi <<<<<<<<unnecessary instruction
> 13: e9 00 00 00 00 jmpq 18 <blah_trunc+0x18>
> [...]
> 0000000000000020 <blah_notrunc>:
> 20: 48 83 ec 08 sub $0x8,%rsp
> 24: e8 00 00 00 00 callq 29 <blah_notrunc+0x9>
> 29: 48 83 c4 08 add $0x8,%rsp
> 2d: 48 8d 78 01 lea 0x1(%rax),%rdi
> 31: e9 00 00 00 00 jmpq 36 <blah_notrunc+0x16>
> $
>
> This is because GCC documents
> (https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html) that
> for integer conversions where the value does not fit into the signed
> target type, "the value is reduced modulo 2^N to be within range of
> the type"; so the compiler has to assume that you are actually
> intentionally trying to truncate the more significant bits from the
> length, and therefore may have to insert extra code to ensure that
> this truncation happens.
>
>
>> + llen = strlen(lsm) + 1;
>> + newlen = strnlen(new, newlen) + 1;
> This strnlen() call seems dodgy. If an LSM can return a string that
> already contains null bytes, shouldn't that be considered a bug, given
> that it can't be displayed properly? Would it be more appropriate to
> have a WARN_ON(memchr(new, '\0', newlen)) check here and bail out if
> that happens?
Whether or not a security module should include a trailing nul has
been a matter of some discussion. Alas, the discussion has not reached
conscensus. The strnlen() is here to allow modules their own convention.
>
>> + 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:
>> @@ -2109,6 +2145,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;
>>
> [...]
>> 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;
>> + }
> This means that if SELinux refuses to give the caller PROCESS__GETATTR
> access to the target process, the entire "context" file will refuse to
> show anything, even if e.g. an AppArmor label would be visible through
> the LSM-specific attribute directory, right?
That is correct.
> That seems awkward.
Sure is.
> Can
> you maybe omit context elements for which the access check failed
> instead, or embed an extra flag byte to signal for each element
> whether the lookup failed, or something along those lines?
The SELinux team seems convinced that if their check fails, the
whole thing must fail.
> If this is an intentional design limitation, it should probably be
> documented in the commit message or so.
Point.
>
>> + rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
>> + cp, rc);
>> + if (rc < 0) {
>> + kfree(final);
>> + return rc;
>> + }
> Isn't there a memory leak here?
Why yes, there is.
> `cp` points to memory that was
> allocated by hp->hook.getprocattr(), and you're not freeing it after
> append_ctx(). (And append_ctx() also doesn't free it.)
>
>> + }
>> + 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;
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-24 1:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200709001234.9719-1-casey@schaufler-ca.com>
2020-07-09 0:12 ` [PATCH v18 22/23] LSM: Add /proc attr entry for full LSM context Casey Schaufler
2020-07-09 1:30 ` Jann Horn
2020-07-24 1:08 ` Casey Schaufler
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).