diff mbox series

libselinux: Prevent cached context giving wrong results

Message ID 20220127130741.31940-1-jsegitz@suse.de (mailing list archive)
State New
Headers show
Series libselinux: Prevent cached context giving wrong results | expand

Commit Message

Johannes Segitz Jan. 27, 2022, 1:07 p.m. UTC
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(-)

Comments

Nicolas Iooss May 29, 2022, 6:22 p.m. UTC | #1
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
>
diff mbox series

Patch

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