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

[Nicolas Iooss <nicolas.iooss@xxxxxxx>]

On Mon, Mar 15, 2021 at 10:02 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> On Sun, Mar 14, 2021 at 4:23 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> 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@xxxxxxx>
> > ---
> >  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?

Which line? "break;" ? In case you and/or other people are confused
about the code in cil_post_fc_fill_data, this "break;" exits the
switch(path[c]) block but still executes the lines right after
("fc->str_len++;" and "c++;"):

while (path[c] != '\0') {
    switch (path[c]) {
    case '.':
    /* ... */
    case '{':
        fc->meta = 1;
        break;
    case '\\':
        c++;
        /* FALLTHRU */
    default:
// This code is executed for every character before a special one
// (while "meta" is false)
// and "\c" counts as a single character, for c being anything.
        if (!fc->meta) {
            fc->stem_len++;
        }
        break;
    }
// These lines are executed for every character.
// "str_len" counts the number of unescaped characters
// ("\c" counts as a single character)
    fc->str_len++;
    c++;
}

In my opinion, the code looks correct, but this could be verified with
a new unit test which could computes str_len and stem_len for some
strings.

Cheers,
Nicolas