On Wed, Mar 17, 2021 at 3:45 AM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > 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 I am an idiot. The code makes a lot more sense now. You only want to count an escaped character as one character. > 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? > I think that it is an invalid path, so maybe I need to add some sort of verification to the path at some point. I finally found the source of this code in refpolicy/support/fc_sort.py where the function compute_diffdata() is very similar. That function would increment str_len in this case, so to be consistent with that fc->str_len should also be incremented. Jim > 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 > > > >
