| Message ID | 20210314201651.474432-1-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] libsepol/cil: fix out-of-bound read of a file context pattern ending with "\" | expand |
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 >
On Mon, Mar 15, 2021 at 10:02 PM James Carter <jwcart2@gmail.com> wrote: > > 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? 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
On Mon, Mar 15, 2021 at 5:34 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > On Mon, Mar 15, 2021 at 10:02 PM James Carter <jwcart2@gmail.com> wrote: > > > > 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? > > 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 >
On Tue, Mar 16, 2021 at 2:34 PM James Carter <jwcart2@gmail.com> wrote: > > On Mon, Mar 15, 2021 at 5:34 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > > > On Mon, Mar 15, 2021 at 10:02 PM James Carter <jwcart2@gmail.com> wrote: > > > > > > 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? > > > > 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 > >
On Wed, Mar 17, 2021 at 3:45 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > On Tue, Mar 16, 2021 at 2:34 PM James Carter <jwcart2@gmail.com> wrote: > > > > On Mon, Mar 15, 2021 at 5:34 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > > > > > On Mon, Mar 15, 2021 at 10:02 PM James Carter <jwcart2@gmail.com> wrote: > > > > > > > > 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? > > > > > > 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 > > > >
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++; + if (path[c] == '\0') { + if (!fc->meta) { + fc->stem_len++; + } + return; + } /* FALLTHRU */ default: if (!fc->meta) {
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(+)