[SEPOL] Fix overflow bugs on athlon 64

[SEPOL] Fix overflow bugs on athlon 64

[Ivan Gyurdiev]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

Fix overflow bugs of two kinds, visible on my athlon64:

- find perm should not abuse the integer (4 byte) return value to put an 
8-byte char* in it.
- Use %ul to print size_t, %u will overflow.

Also, someone should take a look into this:

==31626== Syscall param write(buf) points to uninitialised byte(s)
==31626==    at 0x37E2EBBC60: __write_nocancel (in /lib64/libc-2.3.90.so)
==31626==    by 0x37E2E66812: _IO_file_write@@GLIBC_2.2.5 (in 
/lib64/libc-2.3.90.so)
==31626==    by 0x37E2E66725: _IO_do_write@@GLIBC_2.2.5 (in 
/lib64/libc-2.3.90.so)
==31626==    by 0x37E2E67B48: _IO_file_xsputn@@GLIBC_2.2.5 (in 
/lib64/libc-2.3.90.so)
==31626==    by 0x37E2E5DB59: fwrite (in /lib64/libc-2.3.90.so)
==31626==    by 0x4A913D0: put_entry (private.h:69)
==31626==    by 0x4A93088: class_write (write.c:816)
==31626==    by 0x4A747D4: hashtab_map (hashtab.c:236)
==31626==    by 0x4A950EE: policydb_write (write.c:1483)
==31626==    by 0x4A81713: sepol_module_package_write (module.c:573)
==31626==    by 0x4BBD39F: semanage_write_module (in 
/lib64/libsemanage.so.1)
==31626==    by 0x4BBD76C: semanage_direct_commit (in 
/lib64/libsemanage.so.1)

One suspicious thing is passing the 8-byte size_t values len/len2 into 
cpu_to_le32, but I couldn't completely track this down, so I leave it to 
someone else for now :)


[-- Attachment #2: libsepol.fix_overflow.diff --]
[-- Type: text/x-patch, Size: 5698 bytes --]

diff -Naurp --exclude-from excludes old/libsepol/src/link.c new/libsepol/src/link.c
--- old/libsepol/src/link.c	2005-10-18 08:08:39.000000000 -0600
+++ new/libsepol/src/link.c	2006-01-28 17:07:41.000000000 -0700
@@ -1358,16 +1358,24 @@ static int copy_module(link_state_t *sta
 /***** functions that check requirements and enable blocks in a module ******/
 
 /* borrowed from checkpolicy.c */
-static int find_perm(hashtab_key_t key, hashtab_datum_t datum, void *p)
-{
-	unsigned int *valuep;
-	perm_datum_t *perdatum;
-
-	valuep = (unsigned int *) p;
-	perdatum = (perm_datum_t *) datum;
 
-	if (*valuep == perdatum->value)
-		return (int) key;
+struct find_perm_arg {
+	unsigned int valuep;
+	hashtab_key_t key;
+};
+
+static int find_perm(
+	hashtab_key_t key, 
+	hashtab_datum_t datum, 
+	void *varg) {
+
+	struct find_perm_arg* arg = varg;
+
+	perm_datum_t* perdatum = (perm_datum_t *) datum;
+	if (arg->valuep == perdatum->value) {
+		arg->key = key;
+		return 1;
+	}
 
 	return 0;
 }
@@ -1424,8 +1432,12 @@ static int is_decl_requires_met(link_sta
         }
         /* check that all classes and permissions have been satisfied */
         for (i = 0; i < decl->required.class_perms_len; i++) {
+
                 bitmap = decl->required.class_perms_map + i;
 		ebitmap_for_each_bit(bitmap, node, j) {
+
+                        struct find_perm_arg fparg;
+
                         class_datum_t *cladatum;
                         uint32_t perm_value = j + 1;
                         if (!ebitmap_node_get_bit(node, j)) {
@@ -1433,10 +1445,15 @@ static int is_decl_requires_met(link_sta
                         }
                         id = pol->p_class_val_to_name[i];
                         cladatum = pol->class_val_to_struct[i];
-                        perm_id = (char *) hashtab_map(cladatum->permissions.table, find_perm, &perm_value);
-                        if (perm_id == NULL && cladatum->comdatum != NULL) {
-                                perm_id = (char *) hashtab_map(cladatum->comdatum->permissions.table, find_perm, &perm_value);
-                        }
+		
+			fparg.valuep = perm_value;
+			fparg.key = NULL;
+			
+                        hashtab_map(cladatum->permissions.table, find_perm, &fparg);
+                        if (fparg.key == NULL && cladatum->comdatum != NULL)
+                               hashtab_map(cladatum->comdatum->permissions.table, find_perm, &fparg);
+                        perm_id = fparg.key;
+
                         assert(perm_id != NULL);
                         if (!is_perm_enabled(id, perm_id, state->base)) {
                                 if (req != NULL) {
@@ -1523,14 +1540,21 @@ static int verify_module_requirements(li
                 module_global->enabled = 0;
                 if (!is_decl_requires_met(state, module_global, &req)) {
                         if (req.symbol_type == SYM_CLASSES) {
+	
+                                struct find_perm_arg fparg;
+
                                 class_datum_t *cladatum;
                                 cladatum = p->class_val_to_struct[req.symbol_value - 1];
-                                char *perm_id = (char *)hashtab_map(cladatum->permissions.table, find_perm, &req.perm_value);
+
+                                fparg.valuep = req.perm_value;
+                                fparg.key = NULL;
+                                hashtab_map(cladatum->permissions.table, find_perm, &fparg);
+
                                 ERR(state->handle,
                                             "Module %s's global requirements were not met: class %s, permission %s",
                                             mod_name,
                                             p->p_class_val_to_name[req.symbol_value - 1],
-                                            perm_id);
+                                            fparg.key);
                                 return -1;
                         }
                         else {
diff -Naurp --exclude-from excludes old/libsepol/src/module.c new/libsepol/src/module.c
--- old/libsepol/src/module.c	2005-11-15 06:06:55.000000000 -0700
+++ new/libsepol/src/module.c	2006-01-28 16:50:02.000000000 -0700
@@ -274,7 +274,8 @@ static int module_package_read_offsets(s
 	for (i = 0; i < mod->num_sections; i++) {
 		(*offsets)[i] = le32_to_cpu(buf[i]);
 		if (i && (*offsets)[i] < (*offsets)[i - 1]) {
-			ERR(file->handle, "offsets are not increasing (at %u, offset %u->%u)", i, (*offsets)[i-1], (*offsets)[i]);
+			ERR(file->handle, "offsets are not increasing (at %u, "
+				"offset %lu -> %lu)", i, (*offsets)[i-1], (*offsets)[i]);
 			return -1;
 		}
 	}
@@ -304,14 +305,16 @@ int sepol_module_package_read(sepol_modu
 	for (i = 0; i < mod->num_sections; i++ ) {
 	
 		if (policy_file_seek(file, offsets[i])) {
-			ERR(file->handle, "error seeking to offset %u for module package section %u", offsets[i], i);
+			ERR(file->handle, "error seeking to offset %lu for "
+				"module package section %u", offsets[i], i);
 			goto cleanup;
 		}
 
 		len = offsets[i + 1] - offsets[i];
 
 		if (len < sizeof(uint32_t)) {
-			ERR(file->handle, "module package section %u has too small length %u", i, len);
+			ERR(file->handle, "module package section %u "
+				"has too small length %lu", i, len);
 			goto cleanup;
 		}
 
@@ -397,7 +400,8 @@ int sepol_module_package_info(struct sep
 	for (i = 0; i < mod->num_sections; i++ ) {
 	
 		if (policy_file_seek(file, offsets[i])) {
-			ERR(file->handle, "error seeking to offset %u for module package section %u", offsets[i], i);
+			ERR(file->handle, "error seeking to offset "
+				"%lu for module package section %u", offsets[i], i);
 			goto cleanup;
 		}
 

Re: [SEPOL] Fix overflow bugs on athlon 64

[Stephen Smalley]

On Sat, 2006-01-28 at 17:37 -0700, Ivan Gyurdiev wrote:
> Fix overflow bugs of two kinds, visible on my athlon64:
> 
> - find perm should not abuse the integer (4 byte) return value to put an 
> 8-byte char* in it.

Sigh.  This one was fixed in checkpolicy a long time ago (2003/05/09),
not sure how it crept back into libsepol.

> - Use %ul to print size_t, %u will overflow.

No good, as that then complains on x86:
module.c:278: warning: format ‘%lu’ expects type ‘long unsigned int’,
but argument 5 has type ‘size_t’
...

Have to double-check, but I think that %z is what we have to use here
for portability.

> Also, someone should take a look into this:
> 
> ==31626== Syscall param write(buf) points to uninitialised byte(s)
> ==31626==    at 0x37E2EBBC60: __write_nocancel (in /lib64/libc-2.3.90.so)
> ==31626==    by 0x37E2E66812: _IO_file_write@@GLIBC_2.2.5 (in 
> /lib64/libc-2.3.90.so)
> ==31626==    by 0x37E2E66725: _IO_do_write@@GLIBC_2.2.5 (in 
> /lib64/libc-2.3.90.so)
> ==31626==    by 0x37E2E67B48: _IO_file_xsputn@@GLIBC_2.2.5 (in 
> /lib64/libc-2.3.90.so)
> ==31626==    by 0x37E2E5DB59: fwrite (in /lib64/libc-2.3.90.so)
> ==31626==    by 0x4A913D0: put_entry (private.h:69)
> ==31626==    by 0x4A93088: class_write (write.c:816)
> ==31626==    by 0x4A747D4: hashtab_map (hashtab.c:236)
> ==31626==    by 0x4A950EE: policydb_write (write.c:1483)
> ==31626==    by 0x4A81713: sepol_module_package_write (module.c:573)
> ==31626==    by 0x4BBD39F: semanage_write_module (in 
> /lib64/libsemanage.so.1)
> ==31626==    by 0x4BBD76C: semanage_direct_commit (in 
> /lib64/libsemanage.so.1)
> 
> One suspicious thing is passing the 8-byte size_t values len/len2 into 
> cpu_to_le32, but I couldn't completely track this down, so I leave it to 
> someone else for now :)
> 
-- 
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: [SEPOL] Fix overflow bugs on athlon 64

[Stephen Smalley]

On Mon, 2006-01-30 at 14:41 -0500, Stephen Smalley wrote:
> On Sat, 2006-01-28 at 17:37 -0700, Ivan Gyurdiev wrote:
> > Fix overflow bugs of two kinds, visible on my athlon64:
> > 
> > - find perm should not abuse the integer (4 byte) return value to put an 
> > 8-byte char* in it.
> 
> Sigh.  This one was fixed in checkpolicy a long time ago (2003/05/09),
> not sure how it crept back into libsepol.
> 
> > - Use %ul to print size_t, %u will overflow.
> 
> No good, as that then complains on x86:
> module.c:278: warning: format ‘%lu’ expects type ‘long unsigned int’,
> but argument 5 has type ‘size_t’
> ...
> 
> Have to double-check, but I think that %z is what we have to use here
> for portability.

%zu was it.  Merged with that change as of libsepol 1.11.10.

> > Also, someone should take a look into this:
> > 
> > ==31626== Syscall param write(buf) points to uninitialised byte(s)
> > ==31626==    at 0x37E2EBBC60: __write_nocancel (in /lib64/libc-2.3.90.so)
> > ==31626==    by 0x37E2E66812: _IO_file_write@@GLIBC_2.2.5 (in 
> > /lib64/libc-2.3.90.so)
> > ==31626==    by 0x37E2E66725: _IO_do_write@@GLIBC_2.2.5 (in 
> > /lib64/libc-2.3.90.so)
> > ==31626==    by 0x37E2E67B48: _IO_file_xsputn@@GLIBC_2.2.5 (in 
> > /lib64/libc-2.3.90.so)
> > ==31626==    by 0x37E2E5DB59: fwrite (in /lib64/libc-2.3.90.so)
> > ==31626==    by 0x4A913D0: put_entry (private.h:69)
> > ==31626==    by 0x4A93088: class_write (write.c:816)
> > ==31626==    by 0x4A747D4: hashtab_map (hashtab.c:236)
> > ==31626==    by 0x4A950EE: policydb_write (write.c:1483)
> > ==31626==    by 0x4A81713: sepol_module_package_write (module.c:573)
> > ==31626==    by 0x4BBD39F: semanage_write_module (in 
> > /lib64/libsemanage.so.1)
> > ==31626==    by 0x4BBD76C: semanage_direct_commit (in 
> > /lib64/libsemanage.so.1)
> > 
> > One suspicious thing is passing the 8-byte size_t values len/len2 into 
> > cpu_to_le32, but I couldn't completely track this down, so I leave it to 
> > someone else for now :)

Any other occurrences beyond the one from class_write?
 
-- 
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: [SEPOL] Fix overflow bugs on athlon 64

[Ivan Gyurdiev]

>>> Also, someone should take a look into this:
>>>
>>> ==31626== Syscall param write(buf) points to uninitialised byte(s)
>>> ==31626==    at 0x37E2EBBC60: __write_nocancel (in /lib64/libc-2.3.90.so)
>>> ==31626==    by 0x37E2E66812: _IO_file_write@@GLIBC_2.2.5 (in 
>>> /lib64/libc-2.3.90.so)
>>> ==31626==    by 0x37E2E66725: _IO_do_write@@GLIBC_2.2.5 (in 
>>> /lib64/libc-2.3.90.so)
>>> ==31626==    by 0x37E2E67B48: _IO_file_xsputn@@GLIBC_2.2.5 (in 
>>> /lib64/libc-2.3.90.so)
>>> ==31626==    by 0x37E2E5DB59: fwrite (in /lib64/libc-2.3.90.so)
>>> ==31626==    by 0x4A913D0: put_entry (private.h:69)
>>> ==31626==    by 0x4A93088: class_write (write.c:816)
>>> ==31626==    by 0x4A747D4: hashtab_map (hashtab.c:236)
>>> ==31626==    by 0x4A950EE: policydb_write (write.c:1483)
>>> ==31626==    by 0x4A81713: sepol_module_package_write (module.c:573)
>>> ==31626==    by 0x4BBD39F: semanage_write_module (in 
>>> /lib64/libsemanage.so.1)
>>> ==31626==    by 0x4BBD76C: semanage_direct_commit (in 
>>> /lib64/libsemanage.so.1)
>>>
>>> One suspicious thing is passing the 8-byte size_t values len/len2 into 
>>> cpu_to_le32, but I couldn't completely track this down, so I leave it to 
>>> someone else for now :)
>>>       
>
> Any other occurrences beyond the one from class_write?
>   
No, that's the only trace I got from valgrind /usr/sbin/semanage -B.

It'd be useful to be able to valgrind over some of the python code (like 
pywrap-test.py),
but that seems to be a hopeless task. I tried installing a suppressions 
file for python, which did not work for me..


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