| Message ID | 20170104220229.26497-1-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Jan 4, 2017 at 2:02 PM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > When sepol_polcap_getname() is called with a negative capnum, it > dereferences polcap_names[capnum] which produces a segmentation fault > most of the time. > > For information, here is a gdb session when hll/pp loads a policy module > which has been mutated by American Fuzzy Lop: > > Program received signal SIGSEGV, Segmentation fault. > sepol_polcap_getname (capnum=capnum@entry=-4259840) at polcaps.c:34 > 34 return polcap_names[capnum]; > => 0x00007ffff7a8da07 <sepol_polcap_getname+135>: 48 8b 04 f8 mov > (%rax,%rdi,8),%rax > > (gdb) bt > #0 sepol_polcap_getname (capnum=capnum@entry=-4259840) at > polcaps.c:34 > #1 0x00007ffff7a7c440 in polcaps_to_cil (pdb=0x6042e0) at > module_to_cil.c:2492 > #2 sepol_module_policydb_to_cil (fp=fp@entry=0x7ffff79c75e0 > <_IO_2_1_stdout_>, pdb=0x6042e0, linked=linked@entry=0) at > module_to_cil.c:4039 > #3 0x00007ffff7a7e695 in sepol_module_package_to_cil > (fp=fp@entry=0x7ffff79c75e0 <_IO_2_1_stdout_>, mod_pkg=0x604280) at > module_to_cil.c:4087 > #4 0x0000000000401acc in main (argc=<optimized out>, > argv=<optimized out>) at pp.c:150 > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> > --- > libsepol/include/sepol/policydb/polcaps.h | 2 +- > libsepol/src/polcaps.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libsepol/include/sepol/policydb/polcaps.h b/libsepol/include/sepol/policydb/polcaps.h > index c9e40f62048d..623818ab10f5 100644 > --- a/libsepol/include/sepol/policydb/polcaps.h > +++ b/libsepol/include/sepol/policydb/polcaps.h > @@ -19,7 +19,7 @@ enum { > extern int sepol_polcap_getnum(const char *name); > > /* Convert a capability number to name. */ > -extern const char *sepol_polcap_getname(int capnum); > +extern const char *sepol_polcap_getname(unsigned int capnum); unsigned is definitely a way to fix this, but now I see that sepol_polcap_getnum() wouldn't be able to use the full range of unsigned as its return of < 0 indicates error. Perhaps use the enum types in both cases? However, based on compiler flags, IIRC, enums may be treated as signed or unsigned int. It's really likely never a problem, but just something to mention. > > #ifdef __cplusplus > } > diff --git a/libsepol/src/polcaps.c b/libsepol/src/polcaps.c > index 3924cb83f29c..248d2f525185 100644 > --- a/libsepol/src/polcaps.c > +++ b/libsepol/src/polcaps.c > @@ -26,7 +26,7 @@ int sepol_polcap_getnum(const char *name) > return -1; > } > > -const char *sepol_polcap_getname(int capnum) > +const char *sepol_polcap_getname(unsigned int capnum) > { > if (capnum > POLICYDB_CAPABILITY_MAX) > return NULL; There is a possibility that this enum and the strings could get out of sync and then we would also want to bound this value (sizeof(polcap_names)/sizeof(polcap_names[0]))) -1 We would be able to drop the -1 if the NULL on polcap_names can go away... We could also detect this mismatch at compile time: typedef int WRONG_POLCAP_SIZE [((sizeof(polcap_names)/sizeof(polcap_names[0]))) -1 == POLICYDB_CAPABILITY_MAX) - 1]; -1 size arrays are illegal in C, so we want the array to be 0 length on correct size, -1 on bad size. That above code snippet might not be 100% but that should be close. > -- > 2.11.0 > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
On Thu, Jan 5, 2017 at 12:04 AM, William Roberts <bill.c.roberts@gmail.com> wrote: > On Wed, Jan 4, 2017 at 2:02 PM, Nicolas Iooss <nicolas.iooss@m4x.org> > wrote: > > When sepol_polcap_getname() is called with a negative capnum, it > > dereferences polcap_names[capnum] which produces a segmentation fault > > most of the time. > > > > For information, here is a gdb session when hll/pp loads a policy module > > which has been mutated by American Fuzzy Lop: > > > > Program received signal SIGSEGV, Segmentation fault. > > sepol_polcap_getname (capnum=capnum@entry=-4259840) at polcaps.c:34 > > 34 return polcap_names[capnum]; > > => 0x00007ffff7a8da07 <sepol_polcap_getname+135>: 48 8b 04 f8 mov > > (%rax,%rdi,8),%rax > > > > (gdb) bt > > #0 sepol_polcap_getname (capnum=capnum@entry=-4259840) at > > polcaps.c:34 > > #1 0x00007ffff7a7c440 in polcaps_to_cil (pdb=0x6042e0) at > > module_to_cil.c:2492 > > #2 sepol_module_policydb_to_cil (fp=fp@entry=0x7ffff79c75e0 > > <_IO_2_1_stdout_>, pdb=0x6042e0, linked=linked@entry=0) at > > module_to_cil.c:4039 > > #3 0x00007ffff7a7e695 in sepol_module_package_to_cil > > (fp=fp@entry=0x7ffff79c75e0 <_IO_2_1_stdout_>, mod_pkg=0x604280) at > > module_to_cil.c:4087 > > #4 0x0000000000401acc in main (argc=<optimized out>, > > argv=<optimized out>) at pp.c:150 > > > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> > > --- > > libsepol/include/sepol/policydb/polcaps.h | 2 +- > > libsepol/src/polcaps.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libsepol/include/sepol/policydb/polcaps.h > b/libsepol/include/sepol/policydb/polcaps.h > > index c9e40f62048d..623818ab10f5 100644 > > --- a/libsepol/include/sepol/policydb/polcaps.h > > +++ b/libsepol/include/sepol/policydb/polcaps.h > > @@ -19,7 +19,7 @@ enum { > > extern int sepol_polcap_getnum(const char *name); > > > > /* Convert a capability number to name. */ > > -extern const char *sepol_polcap_getname(int capnum); > > +extern const char *sepol_polcap_getname(unsigned int capnum); > > unsigned is definitely a way to fix this, but now I see that > sepol_polcap_getnum() wouldn't be able > to use the full range of unsigned as its return of < 0 indicates error. > > Perhaps use the enum types in both cases? However, based on compiler > flags, IIRC, enums may be treated as signed or unsigned int. > > It's really likely never a problem, but just something to > mention. > I prefer not to use enum in both sepol_polcap_getname() and sepol_polcap_getnum(). For the first function because it is always called while iterating the policycaps bitmap of a policy so the meaning of the parameter is really "bit position in a bitmap" instead of "the compiler-generated index of the item in the enum" and for the second one because every caller expects the result to be -1 or positive. By the way it looks funny that "git grep -A5 sepol_polcap_getname" shows three callers with three different ways to compare the result (capnum < 0, capnum == -1 or capnum == SEPOL_ERR). > > > #ifdef __cplusplus > > } > > diff --git a/libsepol/src/polcaps.c b/libsepol/src/polcaps.c > > index 3924cb83f29c..248d2f525185 100644 > > --- a/libsepol/src/polcaps.c > > +++ b/libsepol/src/polcaps.c > > @@ -26,7 +26,7 @@ int sepol_polcap_getnum(const char *name) > > return -1; > > } > > > > -const char *sepol_polcap_getname(int capnum) > > +const char *sepol_polcap_getname(unsigned int capnum) > > { > > if (capnum > POLICYDB_CAPABILITY_MAX) > > return NULL; > > There is a possibility that this enum and the strings could get out of > sync and then we would also want > to bound this value (sizeof(polcap_names)/sizeof(polcap_names[0]))) -1 > We would be able to drop the -1 if the NULL on polcap_names can go away... > > We could also detect this mismatch at compile time: > typedef int WRONG_POLCAP_SIZE > [((sizeof(polcap_names)/sizeof(polcap_names[0]))) -1 == > POLICYDB_CAPABILITY_MAX) - 1]; > -1 size arrays are illegal in C, so we want the array to be 0 length > on correct size, -1 on bad size. That above code snippet might not be > 100% but that should be close. I will wait to hear what the project developers think of introducing such a construction as there are several ways to achieve the same result: using a global typedef with a conditionally-sized array, using gcc's __attribute__((error(message))) construction like the kernel does [1][2], using ((void)sizeof(char[1 - 2 * condition])) like the kernel also does [3], or even using static_assert(expr, message) introduced by C11 standard. Thanks for your comments, Nicolas [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/compiler-gcc.h?h=v4.9#n186 [2] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/compiler.h?h=v4.9#n496 [3] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/compiler.h?h=v4.9#n489
On Wed, 2017-01-04 at 23:02 +0100, Nicolas Iooss wrote: > When sepol_polcap_getname() is called with a negative capnum, it > dereferences polcap_names[capnum] which produces a segmentation fault > most of the time. > > For information, here is a gdb session when hll/pp loads a policy > module > which has been mutated by American Fuzzy Lop: > > Program received signal SIGSEGV, Segmentation fault. > sepol_polcap_getname (capnum=capnum@entry=-4259840) at > polcaps.c:34 > 34 return polcap_names[capnum]; > => 0x00007ffff7a8da07 <sepol_polcap_getname+135>: 48 8b 04 f8 > mov > (%rax,%rdi,8),%rax > > (gdb) bt > #0 sepol_polcap_getname (capnum=capnum@entry=-4259840) at > polcaps.c:34 > #1 0x00007ffff7a7c440 in polcaps_to_cil (pdb=0x6042e0) at > module_to_cil.c:2492 > #2 sepol_module_policydb_to_cil (fp=fp@entry=0x7ffff79c75e0 > <_IO_2_1_stdout_>, pdb=0x6042e0, linked=linked@entry=0) at > module_to_cil.c:4039 > #3 0x00007ffff7a7e695 in sepol_module_package_to_cil > (fp=fp@entry=0x7ffff79c75e0 <_IO_2_1_stdout_>, mod_pkg=0x604280) > at > module_to_cil.c:4087 > #4 0x0000000000401acc in main (argc=<optimized out>, > argv=<optimized out>) at pp.c:150 > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> Thanks, applied. > --- > libsepol/include/sepol/policydb/polcaps.h | 2 +- > libsepol/src/polcaps.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libsepol/include/sepol/policydb/polcaps.h > b/libsepol/include/sepol/policydb/polcaps.h > index c9e40f62048d..623818ab10f5 100644 > --- a/libsepol/include/sepol/policydb/polcaps.h > +++ b/libsepol/include/sepol/policydb/polcaps.h > @@ -19,7 +19,7 @@ enum { > extern int sepol_polcap_getnum(const char *name); > > /* Convert a capability number to name. */ > -extern const char *sepol_polcap_getname(int capnum); > +extern const char *sepol_polcap_getname(unsigned int capnum); > > #ifdef __cplusplus > } > diff --git a/libsepol/src/polcaps.c b/libsepol/src/polcaps.c > index 3924cb83f29c..248d2f525185 100644 > --- a/libsepol/src/polcaps.c > +++ b/libsepol/src/polcaps.c > @@ -26,7 +26,7 @@ int sepol_polcap_getnum(const char *name) > return -1; > } > > -const char *sepol_polcap_getname(int capnum) > +const char *sepol_polcap_getname(unsigned int capnum) > { > if (capnum > POLICYDB_CAPABILITY_MAX) > return NULL;
diff --git a/libsepol/include/sepol/policydb/polcaps.h b/libsepol/include/sepol/policydb/polcaps.h index c9e40f62048d..623818ab10f5 100644 --- a/libsepol/include/sepol/policydb/polcaps.h +++ b/libsepol/include/sepol/policydb/polcaps.h @@ -19,7 +19,7 @@ enum { extern int sepol_polcap_getnum(const char *name); /* Convert a capability number to name. */ -extern const char *sepol_polcap_getname(int capnum); +extern const char *sepol_polcap_getname(unsigned int capnum); #ifdef __cplusplus } diff --git a/libsepol/src/polcaps.c b/libsepol/src/polcaps.c index 3924cb83f29c..248d2f525185 100644 --- a/libsepol/src/polcaps.c +++ b/libsepol/src/polcaps.c @@ -26,7 +26,7 @@ int sepol_polcap_getnum(const char *name) return -1; } -const char *sepol_polcap_getname(int capnum) +const char *sepol_polcap_getname(unsigned int capnum) { if (capnum > POLICYDB_CAPABILITY_MAX) return NULL;
When sepol_polcap_getname() is called with a negative capnum, it dereferences polcap_names[capnum] which produces a segmentation fault most of the time. For information, here is a gdb session when hll/pp loads a policy module which has been mutated by American Fuzzy Lop: Program received signal SIGSEGV, Segmentation fault. sepol_polcap_getname (capnum=capnum@entry=-4259840) at polcaps.c:34 34 return polcap_names[capnum]; => 0x00007ffff7a8da07 <sepol_polcap_getname+135>: 48 8b 04 f8 mov (%rax,%rdi,8),%rax (gdb) bt #0 sepol_polcap_getname (capnum=capnum@entry=-4259840) at polcaps.c:34 #1 0x00007ffff7a7c440 in polcaps_to_cil (pdb=0x6042e0) at module_to_cil.c:2492 #2 sepol_module_policydb_to_cil (fp=fp@entry=0x7ffff79c75e0 <_IO_2_1_stdout_>, pdb=0x6042e0, linked=linked@entry=0) at module_to_cil.c:4039 #3 0x00007ffff7a7e695 in sepol_module_package_to_cil (fp=fp@entry=0x7ffff79c75e0 <_IO_2_1_stdout_>, mod_pkg=0x604280) at module_to_cil.c:4087 #4 0x0000000000401acc in main (argc=<optimized out>, argv=<optimized out>) at pp.c:150 Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> --- libsepol/include/sepol/policydb/polcaps.h | 2 +- libsepol/src/polcaps.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)