| Message ID | 20161115230723.20043-3-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 11/15/2016 06:07 PM, Nicolas Iooss wrote: > When hll/pp loads a policy file which has been modified so that the > nprim field of one of its non-empty symbol table was changed to zero, it > crashes with a segmentation fault. A quick analysis leads to > "p->sym_val_to_name[i] = (char **)alloc(p->symtab[i].nprim, sizeof(char > *));" in policydb_index_others(), which is not executed when > p->symtab[i].nprim is zero even though there are items in > p->symtab[i].table. > > Detect such an oddity in the policy file early to exit with a clean > error message. I'll apply this but I'd like to know where the segmentation fault occurred. The index functions already check whether the value exceeds the nprim value, and therefore shouldn't try to set the _val_to_name[] entry. > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> > --- > libsepol/src/policydb.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > index b112fd5465b5..d1019e42de16 100644 > --- a/libsepol/src/policydb.c > +++ b/libsepol/src/policydb.c > @@ -3510,6 +3510,10 @@ static int avrule_decl_read(policydb_t * p, avrule_decl_t * decl, > return -1; > nprim = le32_to_cpu(buf[0]); > nel = le32_to_cpu(buf[1]); > + if (nel && !nprim) { > + ERR(fp->handle, "unexpected items in decl symbol table with no symbol"); > + return -1; > + } > for (j = 0; j < nel; j++) { > if (read_f[i] (p, decl->symtab[i].table, fp)) { > return -1; > @@ -3881,6 +3885,10 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose) > goto bad; > nprim = le32_to_cpu(buf[0]); > nel = le32_to_cpu(buf[1]); > + if (nel && !nprim) { > + ERR(fp->handle, "unexpected items in symbol table with no symbol"); > + goto bad; > + } > for (j = 0; j < nel; j++) { > if (read_f[i] (p, p->symtab[i].table, fp)) > goto bad; >
On 11/15/2016 06:07 PM, Nicolas Iooss wrote: > When hll/pp loads a policy file which has been modified so that the > nprim field of one of its non-empty symbol table was changed to zero, it > crashes with a segmentation fault. A quick analysis leads to > "p->sym_val_to_name[i] = (char **)alloc(p->symtab[i].nprim, sizeof(char > *));" in policydb_index_others(), which is not executed when > p->symtab[i].nprim is zero even though there are items in > p->symtab[i].table. > > Detect such an oddity in the policy file early to exit with a clean > error message. > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> Thanks, applied all three. > --- > libsepol/src/policydb.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > index b112fd5465b5..d1019e42de16 100644 > --- a/libsepol/src/policydb.c > +++ b/libsepol/src/policydb.c > @@ -3510,6 +3510,10 @@ static int avrule_decl_read(policydb_t * p, avrule_decl_t * decl, > return -1; > nprim = le32_to_cpu(buf[0]); > nel = le32_to_cpu(buf[1]); > + if (nel && !nprim) { > + ERR(fp->handle, "unexpected items in decl symbol table with no symbol"); > + return -1; > + } > for (j = 0; j < nel; j++) { > if (read_f[i] (p, decl->symtab[i].table, fp)) { > return -1; > @@ -3881,6 +3885,10 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose) > goto bad; > nprim = le32_to_cpu(buf[0]); > nel = le32_to_cpu(buf[1]); > + if (nel && !nprim) { > + ERR(fp->handle, "unexpected items in symbol table with no symbol"); > + goto bad; > + } > for (j = 0; j < nel; j++) { > if (read_f[i] (p, p->symtab[i].table, fp)) > goto bad; >
On Wed, Nov 16, 2016 at 3:12 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 11/15/2016 06:07 PM, Nicolas Iooss wrote: > > When hll/pp loads a policy file which has been modified so that the > > nprim field of one of its non-empty symbol table was changed to zero, it > > crashes with a segmentation fault. A quick analysis leads to > > "p->sym_val_to_name[i] = (char **)alloc(p->symtab[i].nprim, sizeof(char > > *));" in policydb_index_others(), which is not executed when > > p->symtab[i].nprim is zero even though there are items in > > p->symtab[i].table. > > > > Detect such an oddity in the policy file early to exit with a clean > > error message. > > I'll apply this but I'd like to know where the segmentation fault > occurred. The index functions already check whether the value exceeds > the nprim value, and therefore shouldn't try to set the _val_to_name[] > entry. Thanks. The segfault occurs in required_scopes_to_cil() in module_to_cil.c. Here is a gdb session. % gdb-run ./DESTDIR/usr/libexec/selinux/hll/pp afl-out/pp/crashes.2016-11-15-16:49:07/id:000000,sig:11,* Reading symbols from ./DESTDIR/usr/libexec/selinux/hll/pp...done. Starting program: ./DESTDIR/usr/libexec/selinux/hll/pp afl-out/pp/crashes.2016-11-15-16:49:07/id:000000,sig:11,src:000000,op:flip1,pos:109 Program received signal SIGSEGV, Segmentation fault. required_scopes_to_cil (decl_stack=0x6030b0, block=0x606780, pdb=0x6032e0, indent=0) at module_to_cil.c:3476 3476 key = pdb->sym_val_to_name[sym][i]; => 0x00007ffff7b6c708 <block_to_cil+1224>: 4a 8b 04 e8 mov (%rax,%r13,8),%rax (gdb) bt #0 required_scopes_to_cil (decl_stack=0x6030b0, block=0x606780, pdb=0x6032e0, indent=0) at module_to_cil.c:3476 #1 block_to_cil (pdb=0x6032e0, block=0x606780, stack=0x6030b0, indent=0) at module_to_cil.c:3622 #2 0x00007ffff7b6e0c6 in blocks_to_cil (pdb=0x6032e0) at module_to_cil.c:3760 #3 sepol_module_policydb_to_cil (fp=fp@entry=0x7ffff7b405e0 <_IO_2_1_stdout_>, pdb=0x6032e0, linked=linked@entry=0) at module_to_cil.c:4047 #4 0x00007ffff7b6e3c6 in sepol_module_package_to_cil (fp=fp@entry=0x7ffff7b405e0 <_IO_2_1_stdout_>, mod_pkg=0x603280) at module_to_cil.c:4076 #5 0x0000000000400e76 in main (argc=<optimized out>, argv=<optimized out>) at pp.c:150 (gdb) list 3471 map = decl->required.scope[sym]; 3472 ebitmap_for_each_bit(&map, node, i) { 3473 if (!ebitmap_get_bit(&map, i)) { 3474 continue; 3475 } 3476 key = pdb->sym_val_to_name[sym][i]; 3477 3478 scope_datum = hashtab_search(pdb->scope[sym].table, key); 3479 for (j = 0; j < scope_datum->decl_ids_len; j++) { 3480 if (scope_datum->decl_ids[j] == decl->decl_id) { Nicolas
On 11/16/2016 10:29 AM, Nicolas Iooss wrote: > On Wed, Nov 16, 2016 at 3:12 PM, Stephen Smalley <sds@tycho.nsa.gov > <mailto:sds@tycho.nsa.gov>> wrote: > > On 11/15/2016 06:07 PM, Nicolas Iooss wrote: > > When hll/pp loads a policy file which has been modified so that the > > nprim field of one of its non-empty symbol table was changed to zero, it > > crashes with a segmentation fault. A quick analysis leads to > > "p->sym_val_to_name[i] = (char **)alloc(p->symtab[i].nprim, sizeof(char > > *));" in policydb_index_others(), which is not executed when > > p->symtab[i].nprim is zero even though there are items in > > p->symtab[i].table. > > > > Detect such an oddity in the policy file early to exit with a clean > > error message. > > I'll apply this but I'd like to know where the segmentation fault > occurred. The index functions already check whether the value exceeds > the nprim value, and therefore shouldn't try to set the _val_to_name[] > entry. > > > Thanks. The segfault occurs in required_scopes_to_cil() > in module_to_cil.c. Here is a gdb session. > > % gdb-run ./DESTDIR/usr/libexec/selinux/hll/pp > afl-out/pp/crashes.2016-11-15-16:49:07/id:000000,sig:11,* > > Reading symbols from ./DESTDIR/usr/libexec/selinux/hll/pp...done. > Starting program: ./DESTDIR/usr/libexec/selinux/hll/pp > afl-out/pp/crashes.2016-11-15-16:49:07/id:000000,sig:11,src:000000,op:flip1,pos:109 > > Program received signal SIGSEGV, Segmentation fault. > required_scopes_to_cil (decl_stack=0x6030b0, block=0x606780, > pdb=0x6032e0, indent=0) at module_to_cil.c:3476 > 3476 key = pdb->sym_val_to_name[sym][i]; > => 0x00007ffff7b6c708 <block_to_cil+1224>: 4a 8b 04 e8 mov > (%rax,%r13,8),%rax > (gdb) bt > #0 required_scopes_to_cil (decl_stack=0x6030b0, block=0x606780, > pdb=0x6032e0, indent=0) at module_to_cil.c:3476 > #1 block_to_cil (pdb=0x6032e0, block=0x606780, stack=0x6030b0, > indent=0) at module_to_cil.c:3622 > #2 0x00007ffff7b6e0c6 in blocks_to_cil (pdb=0x6032e0) at > module_to_cil.c:3760 > #3 sepol_module_policydb_to_cil (fp=fp@entry=0x7ffff7b405e0 > <_IO_2_1_stdout_>, pdb=0x6032e0, linked=linked@entry=0) at > module_to_cil.c:4047 > #4 0x00007ffff7b6e3c6 in sepol_module_package_to_cil > (fp=fp@entry=0x7ffff7b405e0 <_IO_2_1_stdout_>, mod_pkg=0x603280) at > module_to_cil.c:4076 > #5 0x0000000000400e76 in main (argc=<optimized out>, argv=<optimized > out>) at pp.c:150 > (gdb) list > 3471 map = decl->required.scope[sym]; > 3472 ebitmap_for_each_bit(&map, node, i) { > 3473 if (!ebitmap_get_bit(&map, i)) { > 3474 continue; > 3475 } > 3476 key = pdb->sym_val_to_name[sym][i]; > 3477 > 3478 scope_datum = > hashtab_search(pdb->scope[sym].table, key); > 3479 for (j = 0; j < > scope_datum->decl_ids_len; j++) { > 3480 if (scope_datum->decl_ids[j] == > decl->decl_id) { So, don't we need to also check that i < pdb->symtab[sym].nprim before line 3476?
On Wed, Nov 16, 2016 at 4:51 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 11/16/2016 10:29 AM, Nicolas Iooss wrote: > > On Wed, Nov 16, 2016 at 3:12 PM, Stephen Smalley <sds@tycho.nsa.gov > > <mailto:sds@tycho.nsa.gov>> wrote: > > > > On 11/15/2016 06:07 PM, Nicolas Iooss wrote: > > > When hll/pp loads a policy file which has been modified so that the > > > nprim field of one of its non-empty symbol table was changed to > zero, it > > > crashes with a segmentation fault. A quick analysis leads to > > > "p->sym_val_to_name[i] = (char **)alloc(p->symtab[i].nprim, > sizeof(char > > > *));" in policydb_index_others(), which is not executed when > > > p->symtab[i].nprim is zero even though there are items in > > > p->symtab[i].table. > > > > > > Detect such an oddity in the policy file early to exit with a clean > > > error message. > > > > I'll apply this but I'd like to know where the segmentation fault > > occurred. The index functions already check whether the value > exceeds > > the nprim value, and therefore shouldn't try to set the > _val_to_name[] > > entry. > > > > > > Thanks. The segfault occurs in required_scopes_to_cil() > > in module_to_cil.c. Here is a gdb session. > > > > % gdb-run ./DESTDIR/usr/libexec/selinux/hll/pp > > afl-out/pp/crashes.2016-11-15-16:49:07/id:000000,sig:11,* > > > > Reading symbols from ./DESTDIR/usr/libexec/selinux/hll/pp...done. > > Starting program: ./DESTDIR/usr/libexec/selinux/hll/pp > > afl-out/pp/crashes.2016-11-15-16:49:07/id:000000,sig:11,src: > 000000,op:flip1,pos:109 > > > > Program received signal SIGSEGV, Segmentation fault. > > required_scopes_to_cil (decl_stack=0x6030b0, block=0x606780, > > pdb=0x6032e0, indent=0) at module_to_cil.c:3476 > > 3476 key = pdb->sym_val_to_name[sym][i]; > > => 0x00007ffff7b6c708 <block_to_cil+1224>: 4a 8b 04 e8 mov > > (%rax,%r13,8),%rax > > (gdb) bt > > #0 required_scopes_to_cil (decl_stack=0x6030b0, block=0x606780, > > pdb=0x6032e0, indent=0) at module_to_cil.c:3476 > > #1 block_to_cil (pdb=0x6032e0, block=0x606780, stack=0x6030b0, > > indent=0) at module_to_cil.c:3622 > > #2 0x00007ffff7b6e0c6 in blocks_to_cil (pdb=0x6032e0) at > > module_to_cil.c:3760 > > #3 sepol_module_policydb_to_cil (fp=fp@entry=0x7ffff7b405e0 > > <_IO_2_1_stdout_>, pdb=0x6032e0, linked=linked@entry=0) at > > module_to_cil.c:4047 > > #4 0x00007ffff7b6e3c6 in sepol_module_package_to_cil > > (fp=fp@entry=0x7ffff7b405e0 <_IO_2_1_stdout_>, mod_pkg=0x603280) at > > module_to_cil.c:4076 > > #5 0x0000000000400e76 in main (argc=<optimized out>, argv=<optimized > > out>) at pp.c:150 > > (gdb) list > > 3471 map = decl->required.scope[sym]; > > 3472 ebitmap_for_each_bit(&map, node, i) { > > 3473 if (!ebitmap_get_bit(&map, i)) { > > 3474 continue; > > 3475 } > > 3476 key = pdb->sym_val_to_name[sym][i]; > > 3477 > > 3478 scope_datum = > > hashtab_search(pdb->scope[sym].table, key); > > 3479 for (j = 0; j < > > scope_datum->decl_ids_len; j++) { > > 3480 if (scope_datum->decl_ids[j] == > > decl->decl_id) { > > So, don't we need to also check that i < pdb->symtab[sym].nprim before > line 3476? > As i is an iterator on decl->required.scope[sym] bitmap, it makes more sense to compare the bitmap high bit value with pdb->symtab[sym].nprim when it is loaded in policydb.c. More precisely, from an API perspective, it makes sense for sepol_module_policydb_to_cil() to expect a well-formed policy database as input. I see this issue as the fact that policydb_read() can load an invalid policy file and still returns a successful return value. Right now I have no time to investigate/write a patch, so I am sending to the mailing list the files I am using to reproduce the issue (basic.pp is the file I gave to the fuzzer and the other attachment is the invalid policy which made pp crash before the patch I sent yesterday). Nicolas
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c index b112fd5465b5..d1019e42de16 100644 --- a/libsepol/src/policydb.c +++ b/libsepol/src/policydb.c @@ -3510,6 +3510,10 @@ static int avrule_decl_read(policydb_t * p, avrule_decl_t * decl, return -1; nprim = le32_to_cpu(buf[0]); nel = le32_to_cpu(buf[1]); + if (nel && !nprim) { + ERR(fp->handle, "unexpected items in decl symbol table with no symbol"); + return -1; + } for (j = 0; j < nel; j++) { if (read_f[i] (p, decl->symtab[i].table, fp)) { return -1; @@ -3881,6 +3885,10 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose) goto bad; nprim = le32_to_cpu(buf[0]); nel = le32_to_cpu(buf[1]); + if (nel && !nprim) { + ERR(fp->handle, "unexpected items in symbol table with no symbol"); + goto bad; + } for (j = 0; j < nel; j++) { if (read_f[i] (p, p->symtab[i].table, fp)) goto bad;
When hll/pp loads a policy file which has been modified so that the nprim field of one of its non-empty symbol table was changed to zero, it crashes with a segmentation fault. A quick analysis leads to "p->sym_val_to_name[i] = (char **)alloc(p->symtab[i].nprim, sizeof(char *));" in policydb_index_others(), which is not executed when p->symtab[i].nprim is zero even though there are items in p->symtab[i].table. Detect such an oddity in the policy file early to exit with a clean error message. Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> --- libsepol/src/policydb.c | 8 ++++++++ 1 file changed, 8 insertions(+)