Re: [PATCH 1/6] libsepol/cil: fix out-of-bound read of a file context pattern ending with "\"

[James Carter <jwcart2@gmail.com>]

On Sun, Mar 14, 2021 at 4:23 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> OSS-Fuzz found a Heap-buffer-overflow in the CIL compiler when trying to
> compile the following policy:
>
>     (sid SID)
>     (sidorder(SID))
>     (filecon "\" any ())
>     (filecon "" any ())
>
> When cil_post_fc_fill_data() processes "\", it goes beyond the NUL
> terminator of the string. Fix this by returning when '\0' is read after
> a backslash.
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28484
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsepol/cil/src/cil_post.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> index a55df1ea5bb0..5f9cf4efd242 100644
> --- a/libsepol/cil/src/cil_post.c
> +++ b/libsepol/cil/src/cil_post.c
> @@ -179,6 +179,12 @@ void cil_post_fc_fill_data(struct fc_data *fc, char *path)
>                         break;
>                 case '\\':
>                         c++;

The patch below is fine, but I can't figure out the reason for the
line above. I guess it means that fc->str_len++ will be skipped, but
if that is the purpose, it is not very clear. Does anyone know if this
is correct?

Jim


> +                       if (path[c] == '\0') {
> +                               if (!fc->meta) {
> +                                       fc->stem_len++;
> +                               }
> +                               return;
> +                       }
>                         /* FALLTHRU */
>                 default:
>                         if (!fc->meta) {
> --
> 2.30.2
>