| Message ID | 20220127130741.31940-1-jsegitz@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libselinux: Prevent cached context giving wrong results | expand |
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 --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; } }
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(-)