[PATCH] libselinux: ignore invalid class name lookup

[PATCH] libselinux: ignore invalid class name lookup

[Thiébaud Weksteen]

selinux_check_access relies on string_to_security_class to resolve the
class index from its char* argument. There is no input validation done
on the string provided. It is possible to supply an argument containing
trailing backslashes (i.e., "sock_file//////") so that the paths built
in discover_class get truncated. The processing will then reference the
same permission file multiple time (e.g., perms/watch_reads will be
truncated to perms/watch). This will leak the memory allocated when
strdup'ing the permission name. The discover_class_cache will end up in
an invalid state (but not corrupted).

Ensure that the class provided does not contain any path separator.

Signed-off-by: Thiébaud Weksteen <tweek@google.com>
---
 libselinux/src/stringrep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
index 2fe69f43..592410e5 100644
--- a/libselinux/src/stringrep.c
+++ b/libselinux/src/stringrep.c
@@ -63,6 +63,9 @@ static struct discover_class_node * discover_class(const char *s)
 		return NULL;
 	}
 
+	if (strchr(s, '/') != NULL)
+		return NULL;
+
 	/* allocate a node */
 	node = malloc(sizeof(struct discover_class_node));
 	if (node == NULL)
-- 
2.38.0.135.g90850a2211-goog

Re: [PATCH] libselinux: ignore invalid class name lookup

[Thiébaud Weksteen]

An alternative to this patch is to implement stricter input validation
on the class name. I could not find any explicit restriction on the
characters of a class. Empirically, it seems that [A-Za-z0-9_] would
be sufficient to cover the refpolicy and Android classes. A regex
matching would have a performance impact here, this is why the strchr
solution was sent. Let me know if you’d prefer to explore the regex
alternative.

Thanks

Re: [PATCH] libselinux: ignore invalid class name lookup

[James Carter]

On Mon, Oct 24, 2022 at 5:14 AM Thiébaud Weksteen <tweek@google.com> wrote:
>
> selinux_check_access relies on string_to_security_class to resolve the
> class index from its char* argument. There is no input validation done
> on the string provided. It is possible to supply an argument containing
> trailing backslashes (i.e., "sock_file//////") so that the paths built

I am having trouble reproducing this. Using backslashes causes an
error when looking up the "%s/class/%s/index" path.
Using forward slashes just works. Valgrind does not report any memory
leaks in either case and I don't see the same permission file being
referenced multiple times.

I don't think that we need the regex solution.

Thanks,
Jim


> in discover_class get truncated. The processing will then reference the
> same permission file multiple time (e.g., perms/watch_reads will be
> truncated to perms/watch). This will leak the memory allocated when
> strdup'ing the permission name. The discover_class_cache will end up in
> an invalid state (but not corrupted).
>
> Ensure that the class provided does not contain any path separator.
>
> Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> ---
>  libselinux/src/stringrep.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
> index 2fe69f43..592410e5 100644
> --- a/libselinux/src/stringrep.c
> +++ b/libselinux/src/stringrep.c
> @@ -63,6 +63,9 @@ static struct discover_class_node * discover_class(const char *s)
>                 return NULL;
>         }
>
> +       if (strchr(s, '/') != NULL)
> +               return NULL;
> +
>         /* allocate a node */
>         node = malloc(sizeof(struct discover_class_node));
>         if (node == NULL)
> --
> 2.38.0.135.g90850a2211-goog
>

Re: [PATCH] libselinux: ignore invalid class name lookup

[Christian Göttsche]

On Fri, 4 Nov 2022 at 22:03, James Carter <jwcart2@gmail.com> wrote:
>
> On Mon, Oct 24, 2022 at 5:14 AM Thiébaud Weksteen <tweek@google.com> wrote:
> >
> > selinux_check_access relies on string_to_security_class to resolve the
> > class index from its char* argument. There is no input validation done
> > on the string provided. It is possible to supply an argument containing
> > trailing backslashes (i.e., "sock_file//////") so that the paths built

Two other interfaces might also be affected by path traversing inputs:

- getseuser(3) via the username parameter
- bool_open(), called by security_get_boolean_pending(3),
security_get_boolean_active(3) and security_set_boolean(3) , via the
name parameter

>
> I am having trouble reproducing this. Using backslashes causes an
> error when looking up the "%s/class/%s/index" path.
> Using forward slashes just works. Valgrind does not report any memory
> leaks in either case and I don't see the same permission file being
> referenced multiple times.
>
> I don't think that we need the regex solution.
>
> Thanks,
> Jim
>
>
> > in discover_class get truncated. The processing will then reference the
> > same permission file multiple time (e.g., perms/watch_reads will be
> > truncated to perms/watch). This will leak the memory allocated when
> > strdup'ing the permission name. The discover_class_cache will end up in
> > an invalid state (but not corrupted).
> >
> > Ensure that the class provided does not contain any path separator.
> >
> > Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> > ---
> >  libselinux/src/stringrep.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
> > index 2fe69f43..592410e5 100644
> > --- a/libselinux/src/stringrep.c
> > +++ b/libselinux/src/stringrep.c
> > @@ -63,6 +63,9 @@ static struct discover_class_node * discover_class(const char *s)
> >                 return NULL;
> >         }
> >
> > +       if (strchr(s, '/') != NULL)
> > +               return NULL;
> > +
> >         /* allocate a node */
> >         node = malloc(sizeof(struct discover_class_node));
> >         if (node == NULL)
> > --
> > 2.38.0.135.g90850a2211-goog
> >

Re: [PATCH] libselinux: ignore invalid class name lookup

[Thiébaud Weksteen]

On Sat, Nov 5, 2022 at 8:21 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> On Fri, 4 Nov 2022 at 22:03, James Carter <jwcart2@gmail.com> wrote:
> >
> > On Mon, Oct 24, 2022 at 5:14 AM Thiébaud Weksteen <tweek@google.com> wrote:
> > >
> > > selinux_check_access relies on string_to_security_class to resolve the
> > > class index from its char* argument. There is no input validation done
> > > on the string provided. It is possible to supply an argument containing
> > > trailing backslashes (i.e., "sock_file//////") so that the paths built
>
> Two other interfaces might also be affected by path traversing inputs:
>
> - getseuser(3) via the username parameter
> - bool_open(), called by security_get_boolean_pending(3),
> security_get_boolean_active(3) and security_set_boolean(3) , via the
> name parameter
>
> >
> > I am having trouble reproducing this. Using backslashes causes an
> > error when looking up the "%s/class/%s/index" path.
> > Using forward slashes just works. Valgrind does not report any memory
> > leaks in either case and I don't see the same permission file being
> > referenced multiple times.

Apologies, it should read "forward slashes" and not "backward
slashes". The exact argument is "sock_file" followed by 4051 forward
slashes. With that length, the index file and perms directory will
open successfully (both paths are 4089 bytes long, assuming
/sys/fs/selinux for selinuxfs). When reading the watch_with_perm
entry, the "_with_perm" will be dropped and the "watch" permission
will be read instead (the "_" character is the 4096th byte). On
Android, PATH_MAX is 4096.

This was found by our fuzzer for the selinux_check_access function [1].

Thanks

[1] https://cs.android.com/android/platform/superproject/+/master:external/selinux/libselinux/fuzzers/selinux_check_access_fuzzer.cpp

> >
> > I don't think that we need the regex solution.
> >
> > Thanks,
> > Jim
> >
> >
> > > in discover_class get truncated. The processing will then reference the
> > > same permission file multiple time (e.g., perms/watch_reads will be
> > > truncated to perms/watch). This will leak the memory allocated when
> > > strdup'ing the permission name. The discover_class_cache will end up in
> > > an invalid state (but not corrupted).
> > >
> > > Ensure that the class provided does not contain any path separator.
> > >
> > > Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> > > ---
> > >  libselinux/src/stringrep.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
> > > index 2fe69f43..592410e5 100644
> > > --- a/libselinux/src/stringrep.c
> > > +++ b/libselinux/src/stringrep.c
> > > @@ -63,6 +63,9 @@ static struct discover_class_node * discover_class(const char *s)
> > >                 return NULL;
> > >         }
> > >
> > > +       if (strchr(s, '/') != NULL)
> > > +               return NULL;
> > > +
> > >         /* allocate a node */
> > >         node = malloc(sizeof(struct discover_class_node));
> > >         if (node == NULL)
> > > --
> > > 2.38.0.135.g90850a2211-goog
> > >

Re: [PATCH] libselinux: ignore invalid class name lookup

[James Carter]

On Mon, Nov 7, 2022 at 10:56 PM Thiébaud Weksteen <tweek@google.com> wrote:
>
> On Sat, Nov 5, 2022 at 8:21 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > On Fri, 4 Nov 2022 at 22:03, James Carter <jwcart2@gmail.com> wrote:
> > >
> > > On Mon, Oct 24, 2022 at 5:14 AM Thiébaud Weksteen <tweek@google.com> wrote:
> > > >
> > > > selinux_check_access relies on string_to_security_class to resolve the
> > > > class index from its char* argument. There is no input validation done
> > > > on the string provided. It is possible to supply an argument containing
> > > > trailing backslashes (i.e., "sock_file//////") so that the paths built
> >
> > Two other interfaces might also be affected by path traversing inputs:
> >
> > - getseuser(3) via the username parameter
> > - bool_open(), called by security_get_boolean_pending(3),
> > security_get_boolean_active(3) and security_set_boolean(3) , via the
> > name parameter
> >
> > >
> > > I am having trouble reproducing this. Using backslashes causes an
> > > error when looking up the "%s/class/%s/index" path.
> > > Using forward slashes just works. Valgrind does not report any memory
> > > leaks in either case and I don't see the same permission file being
> > > referenced multiple times.
>
> Apologies, it should read "forward slashes" and not "backward
> slashes". The exact argument is "sock_file" followed by 4051 forward
> slashes. With that length, the index file and perms directory will
> open successfully (both paths are 4089 bytes long, assuming
> /sys/fs/selinux for selinuxfs). When reading the watch_with_perm
> entry, the "_with_perm" will be dropped and the "watch" permission
> will be read instead (the "_" character is the 4096th byte). On
> Android, PATH_MAX is 4096.
>
> This was found by our fuzzer for the selinux_check_access function [1].
>
> Thanks
>
> [1] https://cs.android.com/android/platform/superproject/+/master:external/selinux/libselinux/fuzzers/selinux_check_access_fuzzer.cpp
>

Thanks for the clarification.
Jim

> > >
> > > I don't think that we need the regex solution.
> > >
> > > Thanks,
> > > Jim
> > >
> > >
> > > > in discover_class get truncated. The processing will then reference the
> > > > same permission file multiple time (e.g., perms/watch_reads will be
> > > > truncated to perms/watch). This will leak the memory allocated when
> > > > strdup'ing the permission name. The discover_class_cache will end up in
> > > > an invalid state (but not corrupted).
> > > >
> > > > Ensure that the class provided does not contain any path separator.
> > > >
> > > > Signed-off-by: Thiébaud Weksteen <tweek@google.com>
> > > > ---
> > > >  libselinux/src/stringrep.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
> > > > index 2fe69f43..592410e5 100644
> > > > --- a/libselinux/src/stringrep.c
> > > > +++ b/libselinux/src/stringrep.c
> > > > @@ -63,6 +63,9 @@ static struct discover_class_node * discover_class(const char *s)
> > > >                 return NULL;
> > > >         }
> > > >
> > > > +       if (strchr(s, '/') != NULL)
> > > > +               return NULL;
> > > > +
> > > >         /* allocate a node */
> > > >         node = malloc(sizeof(struct discover_class_node));
> > > >         if (node == NULL)
> > > > --
> > > > 2.38.0.135.g90850a2211-goog
> > > >

Re: [PATCH] libselinux: ignore invalid class name lookup

[James Carter]

On Mon, Oct 24, 2022 at 5:14 AM Thiébaud Weksteen <tweek@google.com> wrote:
>
> selinux_check_access relies on string_to_security_class to resolve the
> class index from its char* argument. There is no input validation done
> on the string provided. It is possible to supply an argument containing
> trailing backslashes (i.e., "sock_file//////") so that the paths built
> in discover_class get truncated. The processing will then reference the
> same permission file multiple time (e.g., perms/watch_reads will be
> truncated to perms/watch). This will leak the memory allocated when
> strdup'ing the permission name. The discover_class_cache will end up in
> an invalid state (but not corrupted).
>
> Ensure that the class provided does not contain any path separator.
>
> Signed-off-by: Thiébaud Weksteen <tweek@google.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libselinux/src/stringrep.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
> index 2fe69f43..592410e5 100644
> --- a/libselinux/src/stringrep.c
> +++ b/libselinux/src/stringrep.c
> @@ -63,6 +63,9 @@ static struct discover_class_node * discover_class(const char *s)
>                 return NULL;
>         }
>
> +       if (strchr(s, '/') != NULL)
> +               return NULL;
> +
>         /* allocate a node */
>         node = malloc(sizeof(struct discover_class_node));
>         if (node == NULL)
> --
> 2.38.0.135.g90850a2211-goog
>

Re: [PATCH] libselinux: ignore invalid class name lookup

[James Carter]

On Tue, Nov 8, 2022 at 2:14 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Mon, Oct 24, 2022 at 5:14 AM Thiébaud Weksteen <tweek@google.com> wrote:
> >
> > selinux_check_access relies on string_to_security_class to resolve the
> > class index from its char* argument. There is no input validation done
> > on the string provided. It is possible to supply an argument containing
> > trailing backslashes (i.e., "sock_file//////") so that the paths built
> > in discover_class get truncated. The processing will then reference the
> > same permission file multiple time (e.g., perms/watch_reads will be
> > truncated to perms/watch). This will leak the memory allocated when
> > strdup'ing the permission name. The discover_class_cache will end up in
> > an invalid state (but not corrupted).
> >
> > Ensure that the class provided does not contain any path separator.
> >
> > Signed-off-by: Thiébaud Weksteen <tweek@google.com>
>
> Acked-by: James Carter <jwcart2@gmail.com>
>

Merged.
Thanks,
Jim

> > ---
> >  libselinux/src/stringrep.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
> > index 2fe69f43..592410e5 100644
> > --- a/libselinux/src/stringrep.c
> > +++ b/libselinux/src/stringrep.c
> > @@ -63,6 +63,9 @@ static struct discover_class_node * discover_class(const char *s)
> >                 return NULL;
> >         }
> >
> > +       if (strchr(s, '/') != NULL)
> > +               return NULL;
> > +
> >         /* allocate a node */
> >         node = malloc(sizeof(struct discover_class_node));
> >         if (node == NULL)
> > --
> > 2.38.0.135.g90850a2211-goog
> >