[PATCH 3/6] libsepol: resource leaks in policydb.c

[PATCH 3/6] libsepol: resource leaks in policydb.c

[serue]

This patch fixes 2 resource leaks found by Coverity in policydb.c.

	1. in avrule_read(), the 'cur' element has been malloc'ed
	but not yet added to the avrule->perms list at one point
	whene we jump to bad:

	2. 'key' is not freed in one case in scope_read().

The second point makes me wonder - is scope_destroy() fully
implemented?  It takes 'key' as an argument, but does nothing with
it.  Same with its third arg.  Of course, they both have attribute
unused...

Hmm, I see now my formatting is not matching what's in the original
file...  I'll fix them by hand as I'm mailing them, and try to get
it right next time.

thanks,
-serge

Index: src/policydb.c
===================================================================
--- src.orig/policydb.c
+++ src/policydb.c
@@ -2027,8 +2027,10 @@ static avrule_t *avrule_read(policydb_t 
                 class_perm_node_init(cur);
 
                 buf = next_entry(fp, sizeof(uint32_t) * 2);
-                if (!buf)
+                if (!buf) {
+                        free(cur);
                         goto bad;
+       }
 
                 cur->class = le32_to_cpu(buf[0]);
                 cur->data = le32_to_cpu(buf[1]);
@@ -2363,7 +2365,8 @@ static int scope_read(policydb_t * p, ha
 
         if (strcmp(key, "object_r") == 0 && h == p->p_roles_scope.table) {
                 /* object_r was already added to this table in roles_init() */
-                scope_destroy(key, scope, NULL);
+                free(key);
+                scope_destroy(NULL, scope, NULL);
         }
         else {
                 if (hashtab_insert(h, key, scope)) {

--
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: [PATCH 3/6] libsepol: resource leaks in policydb.c

[Stephen Smalley]

On Mon, 2005-08-22 at 11:02 -0500, serue@us.ibm.com wrote:
> This patch fixes 2 resource leaks found by Coverity in policydb.c.
> 
> 	1. in avrule_read(), the 'cur' element has been malloc'ed
> 	but not yet added to the avrule->perms list at one point
> 	whene we jump to bad:
> 
> 	2. 'key' is not freed in one case in scope_read().
> 
> The second point makes me wonder - is scope_destroy() fully
> implemented?  It takes 'key' as an argument, but does nothing with
> it.  Same with its third arg.  Of course, they both have attribute
> unused...

I'm merging this patch at present, but I think you are correct about
scope_destroy being ambiguous in its usage - either it shouldn't take
the key as an argument at all, or it should free it.  Given that
scope_destroy is also being used in a hashtab_map call by
policydb_destroy to free the scope, I'd think we would want it to free
the key.  Joshua?

-- 
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: [PATCH 3/6] libsepol: resource leaks in policydb.c

[Joshua Brindle]

Stephen Smalley wrote:

>On Mon, 2005-08-22 at 11:02 -0500, serue@us.ibm.com wrote:
>  
>
>>This patch fixes 2 resource leaks found by Coverity in policydb.c.
>>
>>	1. in avrule_read(), the 'cur' element has been malloc'ed
>>	but not yet added to the avrule->perms list at one point
>>	whene we jump to bad:
>>
>>	2. 'key' is not freed in one case in scope_read().
>>
>>The second point makes me wonder - is scope_destroy() fully
>>implemented?  It takes 'key' as an argument, but does nothing with
>>it.  Same with its third arg.  Of course, they both have attribute
>>unused...
>>    
>>
>
>I'm merging this patch at present, but I think you are correct about
>scope_destroy being ambiguous in its usage - either it shouldn't take
>the key as an argument at all, or it should free it.  Given that
>scope_destroy is also being used in a hashtab_map call by
>policydb_destroy to free the scope, I'd think we would want it to free
>the key.  Joshua?
>
>  
>
You are right, I'm currently testing this patch but I think it's correct (this is before Serge's patch).

Index: policydb.c
===================================================================
RCS file: /cvsroot/selinux/nsa/selinux-usr/libsepol/src/policydb.c,v
retrieving revision 1.19
diff -u -r1.19 policydb.c
--- policydb.c  18 Aug 2005 20:42:25 -0000      1.19
+++ policydb.c  23 Aug 2005 15:04:09 -0000
@@ -970,8 +970,9 @@
        }
 }
 
-int scope_destroy(hashtab_key_t key __attribute__ ((unused)), 
hashtab_datum_t datum, void *p __attribute__ ((unused)))
+int scope_destroy(hashtab_key_t key, hashtab_datum_t datum, void *p 
__attribute__ ((unused)))
 {
+       free(key);
         scope_datum_t *cur = (scope_datum_t *) datum;
         if (cur != NULL) {
                 free (cur->decl_ids);
@@ -2374,8 +2375,7 @@
        return 0;
         
  cleanup:
-        free(key);
-       scope_destroy(NULL, scope, NULL);
+       scope_destroy(key, scope, NULL);
        return -1;
 }
 
@@ -2518,26 +2518,26 @@
         if (p->policy_type == POLICY_MOD) {
                 /* Get the module name and version */
                 if ((buf = next_entry(fp, sizeof(uint32_t))) == NULL) {
-                        return -1;
+                        goto bad;
                 }
                 len = le32_to_cpu(buf[0]);
                 if ((buf = next_entry(fp, len)) == NULL) {
-                        return -1;
+                        goto bad;
                 }
                 if ((p->name = malloc(len + 1)) == NULL) {
-                        return -1;
+                        goto bad;
                 }
                 memcpy(p->name, buf, len);
                 p->name[len] = '\0';
                 if ((buf = next_entry(fp, sizeof(uint32_t))) == NULL) {
-                        return -1;
+                        goto bad;
                 }
                 len = le32_to_cpu(buf[0]);
                 if ((buf = next_entry(fp, len)) == NULL) {
-                        return -1;
+                        goto bad;
                 }
                 if ((p->version = malloc(len + 1)) == NULL) {
-                        return -1;
+                        goto bad;
                 }
                 memcpy(p->version, buf, len);
                 p->version[len] = '\0';
@@ -2574,7 +2574,7 @@
                 free(p->global);
                 p->global = NULL;
                 if (avrule_block_read(p, &p->global, info->sym_num, fp) 
== -1) {
-                        return -1;
+                        goto bad;
                 }
                 for (i = 0; i < info->sym_num; i++) {
                         if ((buf = next_entry(fp, sizeof(uint32_t))) == 
NULL) {

--
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: [PATCH 3/6] libsepol: resource leaks in policydb.c

[Stephen Smalley]

On Tue, 2005-08-23 at 13:20 -0400, Joshua Brindle wrote:
> Stephen Smalley wrote:
> 
> >On Mon, 2005-08-22 at 11:02 -0500, serue@us.ibm.com wrote:
> >>The second point makes me wonder - is scope_destroy() fully
> >>implemented?  It takes 'key' as an argument, but does nothing with
> >>it.  Same with its third arg.  Of course, they both have attribute
> >>unused...
> >>    
> >>
> >
> >I'm merging this patch at present, but I think you are correct about
> >scope_destroy being ambiguous in its usage - either it shouldn't take
> >the key as an argument at all, or it should free it.  Given that
> >scope_destroy is also being used in a hashtab_map call by
> >policydb_destroy to free the scope, I'd think we would want it to free
> >the key.  Joshua?
> >
> >  
> >
> You are right, I'm currently testing this patch but I think it's
> correct (this is before Serge's patch).

Yielded a double free in /sbin/init, load_policy, and checkpolicy due to
pointer aliasing in symtab_insert.  Patch below should fix.

Index: libsepol/src/util.c
===================================================================
RCS file: /nfshome/pal/CVS/selinux-usr/libsepol/src/util.c,v
retrieving revision 1.3
diff -u -p -r1.3 util.c
--- libsepol/src/util.c	2 Aug 2005 18:32:15 -0000	1.3
+++ libsepol/src/util.c	25 Aug 2005 16:40:14 -0000
@@ -186,13 +186,18 @@ int symtab_insert(policydb_t *pol, uint3
          * create it */
         scope_datum = (scope_datum_t *) hashtab_search(pol->scope[sym].table, key);
         if (scope_datum == NULL) {
+		hashtab_key_t key2 = strdup((char*)key);
+		if (!key2)
+			return -ENOMEM;
                 if ((scope_datum = malloc(sizeof(*scope_datum))) == NULL) {
+			free(key2);
                         return -ENOMEM;
                 }
                 scope_datum->scope = scope;
                 scope_datum->decl_ids = NULL;
                 scope_datum->decl_ids_len = 0;
-                if ((rc = hashtab_insert(pol->scope[sym].table, key, scope_datum)) != 0) {
+                if ((rc = hashtab_insert(pol->scope[sym].table, key2, scope_datum)) != 0) {	
+			free(key2);
 			free(scope_datum);
                         return rc;
                 }


-- 
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.