libsepol/src/conditional.c:cond_evaluate_expr()

libsepol/src/conditional.c:cond_evaluate_expr()

[Serge E. Hallyn]

Hi,

just doing a quick update static analysis scan of the nsa/selinux-usr
sources.  Most of the output in libsepol seems like the same false
positives it used to get, but I'm worried about cond_evaluate_expr.

It is supposed to return -1 on error, but as is, if the second arg
(expr) is passed in NULL, the value returned is truly undefined -
it is just taken off the stack.

Patch appended, in case this is deemed to really be a danger.

thanks,
-serge

Index: libsepol/src/conditional.c
===================================================================
--- libsepol.orig/src/conditional.c	2005-10-13 12:06:06.000000000 -0500
+++ libsepol/src/conditional.c	2006-03-06 15:40:16.000000000 -0600
@@ -191,6 +191,8 @@ int cond_evaluate_expr(policydb_t *p, co
 	int s[COND_EXPR_MAXDEPTH];
 	int sp = -1;
 
+	s[0] = -1;
+
 	for (cur = expr; cur != NULL; cur = cur->next) {
 		switch (cur->expr_type) {
 		case COND_BOOL:

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: libsepol/src/link.c:copy_avrule_list( )

[Ivan Gyurdiev]

That reminds me...
This is a bug I sent to Joshua.

==10331== Invalid read of size 4
==10331==    at 0x4B3D12C: copy_avrule_list (link.c:876)
==10331==    by 0x4B3E061: copy_avrule_decl (link.c:1173)
==10331==    by 0x4B3E356: copy_avrule_block (link.c:1220)
==10331==    by 0x4B3E8FE: copy_module (link.c:1345)
==10331==    by 0x4B4073C: link_modules (link.c:1977)
==10331==    by 0x4B42F8D: sepol_link_packages (module.c:242)
==10331==    by 0x4C91D21: semanage_link_sandbox (in 
/lib64/libsemanage.so.1)
==10331==    by 0x4C86BC1: semanage_direct_commit (in 
/lib64/libsemanage.so.1)
==10331==    by 0x4C893A9: semanage_commit (in /lib64/libsemanage.so.1)
==10331==    by 0x401ACB: (within /usr/sbin/semodule)
==10331==    by 0x4DBE023: __libc_start_main (in /lib64/libc-2.3.90.so)
==10331==  Address 0xEC11728 is 0 bytes after a block of size 8 alloc'd
==10331==    at 0x4A1DA18: calloc (vg_replace_malloc.c:279)
==10331==    by 0x4B3A8D7: permission_copy_callback (link.c:157)
==10331==    by 0x4B37FAA: hashtab_map (hashtab.c:236)
==10331==    by 0x4B3AB96: class_copy_callback (link.c:189)
==10331==    by 0x4B37FAA: hashtab_map (hashtab.c:236)
==10331==    by 0x4B3D9C8: copy_identifiers (link.c:1057)
==10331==    by 0x4B3DC79: copy_module_identifiers (link.c:1102)
==10331==    by 0x4B3E84E: copy_module (link.c:1332)
==10331==    by 0x4B4073C: link_modules (link.c:1977)
==10331==    by 0x4B42F8D: sepol_link_packages (module.c:242)
==10331==    by 0x4C91D21: semanage_link_sandbox (in 
/lib64/libsemanage.so.1)
==10331==    by 0x4C86BC1: semanage_direct_commit (in 
/lib64/libsemanage.so.1)


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: libsepol/src/link.c:copy_avrule_list( )

[Stephen Smalley]

On Mon, 2006-03-06 at 20:57 -0500, Ivan Gyurdiev wrote:
> That reminds me...
> This is a bug I sent to Joshua.
> 
> ==10331== Invalid read of size 4
> ==10331==    at 0x4B3D12C: copy_avrule_list (link.c:876)
> ==10331==    by 0x4B3E061: copy_avrule_decl (link.c:1173)
> ==10331==    by 0x4B3E356: copy_avrule_block (link.c:1220)
> ==10331==    by 0x4B3E8FE: copy_module (link.c:1345)
> ==10331==    by 0x4B4073C: link_modules (link.c:1977)
> ==10331==    by 0x4B42F8D: sepol_link_packages (module.c:242)
> ==10331==    by 0x4C91D21: semanage_link_sandbox (in 
> /lib64/libsemanage.so.1)
> ==10331==    by 0x4C86BC1: semanage_direct_commit (in 
> /lib64/libsemanage.so.1)
> ==10331==    by 0x4C893A9: semanage_commit (in /lib64/libsemanage.so.1)
> ==10331==    by 0x401ACB: (within /usr/sbin/semodule)
> ==10331==    by 0x4DBE023: __libc_start_main (in /lib64/libc-2.3.90.so)
> ==10331==  Address 0xEC11728 is 0 bytes after a block of size 8 alloc'd
> ==10331==    at 0x4A1DA18: calloc (vg_replace_malloc.c:279)
> ==10331==    by 0x4B3A8D7: permission_copy_callback (link.c:157)
> ==10331==    by 0x4B37FAA: hashtab_map (hashtab.c:236)
> ==10331==    by 0x4B3AB96: class_copy_callback (link.c:189)
> ==10331==    by 0x4B37FAA: hashtab_map (hashtab.c:236)
> ==10331==    by 0x4B3D9C8: copy_identifiers (link.c:1057)
> ==10331==    by 0x4B3DC79: copy_module_identifiers (link.c:1102)
> ==10331==    by 0x4B3E84E: copy_module (link.c:1332)
> ==10331==    by 0x4B4073C: link_modules (link.c:1977)
> ==10331==    by 0x4B42F8D: sepol_link_packages (module.c:242)
> ==10331==    by 0x4C91D21: semanage_link_sandbox (in 
> /lib64/libsemanage.so.1)
> ==10331==    by 0x4C86BC1: semanage_direct_commit (in 
> /lib64/libsemanage.so.1)

What is your test case (i.e. what policy)?
The (i < 32) looks suspicious to me there; should it be (i <
module->perm_map_len[cur_perm->class - 1]), e.g.:

Index: libsepol/src/link.c
===================================================================
RCS file: /nfshome/pal/CVS/selinux-usr/libsepol/src/link.c,v
retrieving revision 1.16
diff -u -p -r1.16 link.c
--- libsepol/src/link.c	1 Feb 2006 13:59:19 -0000	1.16
+++ libsepol/src/link.c	8 Mar 2006 13:41:33 -0000
@@ -835,7 +835,7 @@ static int (*merge_callback_f[SYM_NUM]) 
 static int copy_avrule_list(avrule_t *list, avrule_t **dst,
                             policy_module_t *module, link_state_t *state)
 {
-        int i;
+        unsigned int i;
         avrule_t *cur, *new_rule = NULL, *tail;
         class_perm_node_t *cur_perm, *new_perm, *tail_perm = NULL;
 
@@ -870,7 +870,7 @@ static int copy_avrule_list(avrule_t *li
                         assert(new_perm->class);
 
                         if (new_rule->specified & (AVRULE_AV | AVRULE_NEVERALLOW)) {
-                                for (i = 0; i < 32; i++) {
+                                for (i = 0; i < module->perm_map_len[cur_perm->class - 1]; i++) {
                                         if (!(cur_perm->data & (1U << i)))
                                                 continue;
 					new_perm->data |=

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: libsepol/src/link.c:copy_avrule_list( )

[Ivan Gyurdiev]

> What is your test case (i.e. what policy)?
>   
Fedora strict. I don't remember this happening under targeted.


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: libsepol/src/link.c:copy_avrule_list( )

[Stephen Smalley]

On Wed, 2006-03-08 at 09:05 -0500, Ivan Gyurdiev wrote:
> > What is your test case (i.e. what policy)?
> >   
> Fedora strict. I don't remember this happening under targeted.

Ok, reproduced it, and it seems to be resolved by the patch I posted.
Joshua, do you concur with the patch?

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: libsepol/src/link.c:copy_avrule_list( )

[Joshua Brindle]

Stephen Smalley wrote:
> On Wed, 2006-03-08 at 09:05 -0500, Ivan Gyurdiev wrote:
>>> What is your test case (i.e. what policy)?
>>>   
>> Fedora strict. I don't remember this happening under targeted.
> 
> Ok, reproduced it, and it seems to be resolved by the patch I posted.
> Joshua, do you concur with the patch?
> 

Yes, that does look right. I hadn't had time to look at that bug but it 
looks pretty straightforward.

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: libsepol/src/conditional.c:cond_evaluate_expr()

[Stephen Smalley]

On Mon, 2006-03-06 at 15:41 -0600, Serge E. Hallyn wrote:
> Hi,
> 
> just doing a quick update static analysis scan of the nsa/selinux-usr
> sources.  Most of the output in libsepol seems like the same false
> positives it used to get, but I'm worried about cond_evaluate_expr.
> 
> It is supposed to return -1 on error, but as is, if the second arg
> (expr) is passed in NULL, the value returned is truly undefined -
> it is just taken off the stack.
> 
> Patch appended, in case this is deemed to really be a danger.

Looks sane, applied.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.