On Tue, Mar 16, 2021 at 2:34 PM James Carter <jwcart2@xxxxxxxxx> wrote: > > 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. The matched character is a backslash ("\"), not a slash ("/"). I understand that this implementation of "c++; without fc->str_len++; nor fc->stem_len++;" is there in order to count sequences such as "\(", "\.", "\["... as a single character. More precisely, when for example the two-character sequence "\." is encountered in a path (which happens often, as it is the way to escape dots in file context patterns): * c is increased twice ("c++;" in present twice in the while loop), in order to go to the character next to the sequence ; * fc->str_len is increased once (this sequence counts as a single non-special character) ; * if fc->meta is false (i.e. if no meta character such as ".", "(", "["... has been encountered yet), fc->stem_len is increased once (this sequence counts as a single non-special character in the "stem" of the path) The code I added in my patch made fc->stem_len increase when a path ends with "\" (the character after the backslash character is a NUL string terminator), before exiting cil_post_fc_fill_data. Now I am wondering whether fc->str_len should also be increased, in order to "count the backslash". In fact, finishing a path pattern with an incomplete escape sequence is weird and I do not precisely know the semantic of the length counters in such a case. What do you think? Nicolas > > 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 > >
