| Message ID | 20221024091354.2253669-1-tweek@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f56a72ac9e86 |
| Headers | show |
| Series | libselinux: ignore invalid class name lookup | expand |
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
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 >
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 > >
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 > > >
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 > > > >
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 >
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 > >
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)
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(+)