| Message ID | 20160929055012.10460-1-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 09/29/2016 01:50 AM, Nicolas Iooss wrote: > When compiling a CIL policy with more than 32 items in a class (e.g. in > (class capability (chown ...)) with many items), > cil_classorder_to_policydb() overflows perm_value_to_cil[class_index] > array. As this array is allocated on the heap through > calloc(PERMS_PER_CLASS+1, sizeof(...)), this makes secilc crash with the > following message: > > *** Error in `/usr/bin/secilc': double free or corruption (!prev): 0x000000000062be80 *** > ======= Backtrace: ========= > /usr/lib/libc.so.6(+0x70c4b)[0x7ffff76a7c4b] > /usr/lib/libc.so.6(+0x76fe6)[0x7ffff76adfe6] > /usr/lib/libc.so.6(+0x777de)[0x7ffff76ae7de] > /lib/libsepol.so.1(+0x14fbda)[0x7ffff7b24bda] > /lib/libsepol.so.1(+0x152db8)[0x7ffff7b27db8] > /lib/libsepol.so.1(cil_build_policydb+0x63)[0x7ffff7af8723] > /usr/bin/secilc[0x40273b] > /usr/lib/libc.so.6(__libc_start_main+0xf1)[0x7ffff7657291] > /usr/bin/secilc[0x402f7a] > > Fix this by detecting the overflow before adding new permissions to a > class. > > This bug has been found by fuzzing secilc with american fuzzy lop. > I think I'd rather see these bounds checks done earlier in cil_build_ast.c in cil_gen_class, cil_gen_classcommon. And maybe in cil_resolve_classcommon as well for the class + classcommon < 32 check. That way we can avoid alot of unnecessary processing and provide an error message that includes file/line number information. This check shouldn't be necessary in cil_binary if we perform the validation earlier. > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> > --- > v2: check that sepol_common->permissions.nprim would not make > sepol_class->permissions.nprim overflow when added to it. > > libsepol/cil/src/cil_binary.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c > index cc73648ad1b7..f83dc2ddd1ee 100644 > --- a/libsepol/cil/src/cil_binary.c > +++ b/libsepol/cil/src/cil_binary.c > @@ -332,6 +332,12 @@ int cil_classorder_to_policydb(policydb_t *pdb, const struct cil_db *db, struct > goto exit; > } > } > + if (sepol_common->permissions.nprim > PERMS_PER_CLASS || > + sepol_class->permissions.nprim + sepol_common->permissions.nprim > PERMS_PER_CLASS) { > + cil_log(CIL_ERR, "Too many permissions in class '%s'\n", cil_class->datum.fqn); > + rc = SEPOL_ERR; > + goto exit; > + } > sepol_class->comdatum = sepol_common; > sepol_class->comkey = cil_strdup(key); > sepol_class->permissions.nprim += sepol_common->permissions.nprim; > @@ -344,9 +350,15 @@ int cil_classorder_to_policydb(policydb_t *pdb, const struct cil_db *db, struct > > for (curr = NODE(cil_class)->cl_head; curr; curr = curr->next) { > struct cil_perm *cil_perm = curr->data; > - perm_datum_t *sepol_perm = cil_malloc(sizeof(*sepol_perm)); > - memset(sepol_perm, 0, sizeof(perm_datum_t)); > + perm_datum_t *sepol_perm; > > + if (sepol_class->permissions.nprim + 1 > PERMS_PER_CLASS) { > + cil_log(CIL_ERR, "Too many permissions in class '%s'\n", cil_class->datum.fqn); > + rc = SEPOL_ERR; > + goto exit; > + } > + sepol_perm = cil_malloc(sizeof(*sepol_perm)); > + memset(sepol_perm, 0, sizeof(perm_datum_t)); > key = cil_strdup(cil_perm->datum.fqn); > rc = hashtab_insert(sepol_class->permissions.table, key, sepol_perm); > if (rc != SEPOL_OK) { >
diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c index cc73648ad1b7..f83dc2ddd1ee 100644 --- a/libsepol/cil/src/cil_binary.c +++ b/libsepol/cil/src/cil_binary.c @@ -332,6 +332,12 @@ int cil_classorder_to_policydb(policydb_t *pdb, const struct cil_db *db, struct goto exit; } } + if (sepol_common->permissions.nprim > PERMS_PER_CLASS || + sepol_class->permissions.nprim + sepol_common->permissions.nprim > PERMS_PER_CLASS) { + cil_log(CIL_ERR, "Too many permissions in class '%s'\n", cil_class->datum.fqn); + rc = SEPOL_ERR; + goto exit; + } sepol_class->comdatum = sepol_common; sepol_class->comkey = cil_strdup(key); sepol_class->permissions.nprim += sepol_common->permissions.nprim; @@ -344,9 +350,15 @@ int cil_classorder_to_policydb(policydb_t *pdb, const struct cil_db *db, struct for (curr = NODE(cil_class)->cl_head; curr; curr = curr->next) { struct cil_perm *cil_perm = curr->data; - perm_datum_t *sepol_perm = cil_malloc(sizeof(*sepol_perm)); - memset(sepol_perm, 0, sizeof(perm_datum_t)); + perm_datum_t *sepol_perm; + if (sepol_class->permissions.nprim + 1 > PERMS_PER_CLASS) { + cil_log(CIL_ERR, "Too many permissions in class '%s'\n", cil_class->datum.fqn); + rc = SEPOL_ERR; + goto exit; + } + sepol_perm = cil_malloc(sizeof(*sepol_perm)); + memset(sepol_perm, 0, sizeof(perm_datum_t)); key = cil_strdup(cil_perm->datum.fqn); rc = hashtab_insert(sepol_class->permissions.table, key, sepol_perm); if (rc != SEPOL_OK) {
When compiling a CIL policy with more than 32 items in a class (e.g. in (class capability (chown ...)) with many items), cil_classorder_to_policydb() overflows perm_value_to_cil[class_index] array. As this array is allocated on the heap through calloc(PERMS_PER_CLASS+1, sizeof(...)), this makes secilc crash with the following message: *** Error in `/usr/bin/secilc': double free or corruption (!prev): 0x000000000062be80 *** ======= Backtrace: ========= /usr/lib/libc.so.6(+0x70c4b)[0x7ffff76a7c4b] /usr/lib/libc.so.6(+0x76fe6)[0x7ffff76adfe6] /usr/lib/libc.so.6(+0x777de)[0x7ffff76ae7de] /lib/libsepol.so.1(+0x14fbda)[0x7ffff7b24bda] /lib/libsepol.so.1(+0x152db8)[0x7ffff7b27db8] /lib/libsepol.so.1(cil_build_policydb+0x63)[0x7ffff7af8723] /usr/bin/secilc[0x40273b] /usr/lib/libc.so.6(__libc_start_main+0xf1)[0x7ffff7657291] /usr/bin/secilc[0x402f7a] Fix this by detecting the overflow before adding new permissions to a class. This bug has been found by fuzzing secilc with american fuzzy lop. Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> --- v2: check that sepol_common->permissions.nprim would not make sepol_class->permissions.nprim overflow when added to it. libsepol/cil/src/cil_binary.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)