On Mon, Nov 7, 2022 at 10:56 PM Thiébaud Weksteen <tweek@xxxxxxxxxx> wrote: > > On Sat, Nov 5, 2022 at 8:21 AM Christian Göttsche > <cgzones@xxxxxxxxxxxxxx> wrote: > > > > On Fri, 4 Nov 2022 at 22:03, James Carter <jwcart2@xxxxxxxxx> wrote: > > > > > > On Mon, Oct 24, 2022 at 5:14 AM Thiébaud Weksteen <tweek@xxxxxxxxxx> 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@xxxxxxxxxx> > > > > --- > > > > 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 > > > >
