On Mon, Mar 15, 2021 at 5:34 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > 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++;"): > Sorry, I wasn't very clear. I am wondering what the "c++" is doing here because after the switch statement there is another "c++" (after "fc->str_length++"), so this skips the character after the "/". Why would one do that? My only thought is that maybe "/" is not supposed to count towards the string length and the author thought not counting the next character works just as well? Except, of course, it doesn't if there is no next character. Jim > 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 >
