On Thu, Jan 5, 2017 at 12:04 AM, William Roberts <bill.c.roberts@xxxxxxxxx> wrote:
unsigned is definitely a way to fix this, but now I see thatOn Wed, Jan 4, 2017 at 2:02 PM, Nicolas Iooss <nicolas.iooss@xxxxxxx> 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@xxxxxxx>
> ---
> 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);
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
_______________________________________________ Selinux mailing list Selinux@xxxxxxxxxxxxx To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx. To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
