* [PATCH] libselinux: Prevent cached context giving wrong results
@ 2022-01-27 13:07 Johannes Segitz
2022-05-29 18:22 ` Nicolas Iooss
0 siblings, 1 reply; 2+ messages in thread
From: Johannes Segitz @ 2022-01-27 13:07 UTC (permalink / raw)
To: selinux; +Cc: Johannes Segitz
The procattr cache doesn't work properly in all cases. This fixes the issue at
the cost of not using the cache as soon as a pid is specified.
In most use cases this will never occur, but it's still a small security issue,
since this incorrect information might lead to incorrect access decisions.
Signed-off-by: Johannes Segitz <jsegitz@suse.de>
---
libselinux/src/procattr.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
index 142fbf3a..4ca8337a 100644
--- a/libselinux/src/procattr.c
+++ b/libselinux/src/procattr.c
@@ -148,7 +148,7 @@ static int getprocattrcon_raw(char ** context,
return -1;
}
- if (prev_context && prev_context != UNSET) {
+ if (pid == 0 && prev_context && prev_context != UNSET) {
*context = strdup(prev_context);
if (!(*context)) {
return -1;
@@ -242,9 +242,9 @@ static int setprocattrcon_raw(const char * context,
return -1;
}
- if (!context && !*prev_context)
+ if (pid == 0 && !context && !*prev_context)
return 0;
- if (context && *prev_context && *prev_context != UNSET
+ if (pid == 0 && context && *prev_context && *prev_context != UNSET
&& !strcmp(context, *prev_context))
return 0;
@@ -272,9 +272,11 @@ out:
free(context2);
return -1;
} else {
- if (*prev_context != UNSET)
- free(*prev_context);
- *prev_context = context2;
+ if (pid == 0) {
+ if (*prev_context != UNSET)
+ free(*prev_context);
+ *prev_context = context2;
+ }
return 0;
}
}
--
2.31.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] libselinux: Prevent cached context giving wrong results
2022-01-27 13:07 [PATCH] libselinux: Prevent cached context giving wrong results Johannes Segitz
@ 2022-05-29 18:22 ` Nicolas Iooss
0 siblings, 0 replies; 2+ messages in thread
From: Nicolas Iooss @ 2022-05-29 18:22 UTC (permalink / raw)
To: Johannes Segitz; +Cc: SElinux list
Hello,
Sorry for not answering sooner. I was busy in the past few months, and
it seems the other libselinux maintainers too.
I had some difficulty understanding what your patch was about, as I
missed its context (from
https://lore.kernel.org/selinux/20220121084012.GS7643@suse.com/ ) and
the commit message did not explain what the issue actually was. While
trying to reverse-engineer it, I found (again) the bug which is fixed.
Instead of adding "pid == 0" to some functions, I decided to
rework/simplify libselinux/src/procattr.c to fix this bug. The result
of this work was sent on the mailing list:
https://patchwork.kernel.org/project/selinux/patch/20220529180111.408899-1-nicolas.iooss@m4x.org/
. I personally prefer this simplification, but if anyone thinks this
makes it more difficult to maintain libselinux (for example), I am
open to discussion.
Best regards,
Nicolas
On Thu, Jan 27, 2022 at 2:07 PM Johannes Segitz <jsegitz@suse.de> wrote:
>
> The procattr cache doesn't work properly in all cases. This fixes the issue at
> the cost of not using the cache as soon as a pid is specified.
>
> In most use cases this will never occur, but it's still a small security issue,
> since this incorrect information might lead to incorrect access decisions.
>
> Signed-off-by: Johannes Segitz <jsegitz@suse.de>
> ---
> libselinux/src/procattr.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
> index 142fbf3a..4ca8337a 100644
> --- a/libselinux/src/procattr.c
> +++ b/libselinux/src/procattr.c
> @@ -148,7 +148,7 @@ static int getprocattrcon_raw(char ** context,
> return -1;
> }
>
> - if (prev_context && prev_context != UNSET) {
> + if (pid == 0 && prev_context && prev_context != UNSET) {
> *context = strdup(prev_context);
> if (!(*context)) {
> return -1;
> @@ -242,9 +242,9 @@ static int setprocattrcon_raw(const char * context,
> return -1;
> }
>
> - if (!context && !*prev_context)
> + if (pid == 0 && !context && !*prev_context)
> return 0;
> - if (context && *prev_context && *prev_context != UNSET
> + if (pid == 0 && context && *prev_context && *prev_context != UNSET
> && !strcmp(context, *prev_context))
> return 0;
>
> @@ -272,9 +272,11 @@ out:
> free(context2);
> return -1;
> } else {
> - if (*prev_context != UNSET)
> - free(*prev_context);
> - *prev_context = context2;
> + if (pid == 0) {
> + if (*prev_context != UNSET)
> + free(*prev_context);
> + *prev_context = context2;
> + }
> return 0;
> }
> }
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-05-29 18:22 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-27 13:07 [PATCH] libselinux: Prevent cached context giving wrong results Johannes Segitz
2022-05-29 18:22 ` Nicolas Iooss
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.