[PATCH] libsepol: bool_copy_callback set state on creation

[PATCH] libsepol: bool_copy_callback set state on creation

[Thomas Hurd]

Boolean states are only written on a declaration.
If a module is turned off which includes a tunable declaration that
is required in another module, the state is never set. This patch
sets the state when the booldatum is created so that an uninitialized
memory read does not occur in cond_write_bool and write garbage to
the link binary.  This can cause a failure in cond_read_bool when
running semodule_expand.

Signed-off-by: Thomas Hurd <thurd@tresys.com>
---
 libsepol/src/link.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libsepol/src/link.c b/libsepol/src/link.c
index f98a8d2..f211164 100644
--- a/libsepol/src/link.c
+++ b/libsepol/src/link.c
@@ -630,6 +630,7 @@ static int bool_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
 		state->base->p_bools.nprim++;
 		base_bool = new_bool;
 		base_bool->flags = booldatum->flags;
+		base_bool->state = booldatum->state;
 	} else if ((booldatum->flags & COND_BOOL_FLAGS_TUNABLE) !=
 		   (base_bool->flags & COND_BOOL_FLAGS_TUNABLE)) {
 			/* A mismatch between boolean/tunable declaration
-- 
2.1.0

Re: [PATCH] libsepol: bool_copy_callback set state on creation

[Stephen Smalley]

On 03/19/2015 01:03 PM, Thomas Hurd wrote:
> Boolean states are only written on a declaration.
> If a module is turned off which includes a tunable declaration that
> is required in another module, the state is never set. This patch
> sets the state when the booldatum is created so that an uninitialized
> memory read does not occur in cond_write_bool and write garbage to
> the link binary.  This can cause a failure in cond_read_bool when
> running semodule_expand.
> 
> Signed-off-by: Thomas Hurd <thurd@tresys.com>
> ---
>  libsepol/src/link.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libsepol/src/link.c b/libsepol/src/link.c
> index f98a8d2..f211164 100644
> --- a/libsepol/src/link.c
> +++ b/libsepol/src/link.c
> @@ -630,6 +630,7 @@ static int bool_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
>  		state->base->p_bools.nprim++;
>  		base_bool = new_bool;
>  		base_bool->flags = booldatum->flags;
> +		base_bool->state = booldatum->state;
>  	} else if ((booldatum->flags & COND_BOOL_FLAGS_TUNABLE) !=
>  		   (base_bool->flags & COND_BOOL_FLAGS_TUNABLE)) {
>  			/* A mismatch between boolean/tunable declaration
> 

Hmm...commit 3df79fc5ebf08a35aaa095b2ee3fd24b3ece6ae5 (libsepol: fix
boolean state smashing) removed the setting of the state here, replacing
it with conditional setting iff it is a decl further down.

Re: [PATCH] libsepol: bool_copy_callback set state on creation

[Thomas Hurd]

On 03/19/2015 02:44 PM, Stephen Smalley wrote:
> On 03/19/2015 01:03 PM, Thomas Hurd wrote:
>> Boolean states are only written on a declaration.
>> If a module is turned off which includes a tunable declaration that
>> is required in another module, the state is never set. This patch
>> sets the state when the booldatum is created so that an uninitialized
>> memory read does not occur in cond_write_bool and write garbage to
>> the link binary.  This can cause a failure in cond_read_bool when
>> running semodule_expand.
>>
>> Signed-off-by: Thomas Hurd <thurd@tresys.com>
>> ---
>>   libsepol/src/link.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/libsepol/src/link.c b/libsepol/src/link.c
>> index f98a8d2..f211164 100644
>> --- a/libsepol/src/link.c
>> +++ b/libsepol/src/link.c
>> @@ -630,6 +630,7 @@ static int bool_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
>>   		state->base->p_bools.nprim++;
>>   		base_bool = new_bool;
>>   		base_bool->flags = booldatum->flags;
>> +		base_bool->state = booldatum->state;
>>   	} else if ((booldatum->flags & COND_BOOL_FLAGS_TUNABLE) !=
>>   		   (base_bool->flags & COND_BOOL_FLAGS_TUNABLE)) {
>>   			/* A mismatch between boolean/tunable declaration
>>
> Hmm...commit 3df79fc5ebf08a35aaa095b2ee3fd24b3ece6ae5 (libsepol: fix
> boolean state smashing) removed the setting of the state here, replacing
> it with conditional setting iff it is a decl further down.
>

I think the change to set the base_bool->state when the scope == SCOPE_DECL
solves the problem of a boolean getting the required state and not the
declared state.
If a bool is encountered in a require block before it's declared, the state
should be overwritten with the declared state. If a bool is required but not
declared, the state is never set.

Alternatively, we could change the malloc of new_bool to a calloc, and 
it should
solve the uninitialized memory issue.

To reproduce the problem I ran into, I updated modules.conf to turn off 
vmware, xen,
qemu, and virt modules.  semodule_expand failed in bool_isvalid on the 
state of virt_use_nfs.

     ==31226== Syscall param write(buf) points to uninitialised byte(s)
     ==31226==    at 0x53EB940: __write_nocancel (in 
/usr/lib64/libc-2.20.so)
     ==31226==    by 0x5373D2C: _IO_file_write@@GLIBC_2.2.5 (in 
/usr/lib64/libc-2.20.so)
     ==31226==    by 0x53751D8: _IO_do_write@@GLIBC_2.2.5 (in 
/usr/lib64/libc-2.20.so)
     ==31226==    by 0x53743EC: _IO_file_xsputn@@GLIBC_2.2.5 (in 
/usr/lib64/libc-2.20.so)
     ==31226==    by 0x5369E68: fwrite (in /usr/lib64/libc-2.20.so)
     ==31226==    by 0x4E65B06: put_entry (services.c:1665)
     ==31226==    by 0x4E3F57A: cond_write_bool (write.c:622)
     ==31226==    by 0x4E48488: hashtab_map (hashtab.c:235)
     ==31226==    by 0x4E4340F: policydb_write (write.c:2032)
     ==31226==    by 0x4E4BB75: sepol_module_package_write (module.c:959)
     ==31226==    by 0x401334: main (semodule_link.c:162)
     ==31226==  Address 0x4023706 is not stack'd, malloc'd or (recently) 
free'd
     ==31226==  Uninitialised value was created by a heap allocation
     ==31226==    at 0x4C29BCF: malloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
     ==31226==    by 0x4E74420: bool_copy_callback (link.c:618)
     ==31226==    by 0x4E48488: hashtab_map (hashtab.c:235)
     ==31226==    by 0x4E776F8: copy_identifiers (link.c:1622)
     ==31226==    by 0x4E7A6AD: link_modules (link.c:2605)
     ==31226==    by 0x4E48F96: sepol_link_packages (module.c:322)
     ==31226==    by 0x401269: main (semodule_link.c:145)