Re: [Patch 2/2] libsemanage: remember and retrieve dontaudit settings

[Christopher Pardy <cpardy@redhat.com>]

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

On 07/02/2009 08:46 AM, Stephen Smalley wrote:
> On Wed, 2009-07-01 at 22:13 -0400, Christopher Pardy wrote:
>    
>> On 07/01/2009 10:08 PM, Christopher Pardy wrote:
>>      
>>> This is a heavily modified version of the patch I recently submitted.
>>> It provides 3 new functions: in libsepol sepol_get_disable_dontaudit;
>>> in libsemanage semanage_get_disable_dontaudit; in libselinux
>>> is_dontaudit_disabled. It also fixes issues with the previous patch.
>>>
>>> The justification for this patch is the same as the one I posted
>>> earlier. Simply, there is currently no way to know if dontaudit rules
>>> are  enabled. Additionally once don't audit rules are turned they turn
>>> themselves off after policy rebuild (is that the desired
>>> functionality?) This patch provides  a way to check on both the
>>> current and pending state of the dontaudit rules and it maintains this
>>> state between policy rebuilds.
>>>
>>> Signed-off-by Christopher Pardy<cpardy@redhat.com>
>>>        
>> This patch implements the functions in libsemanage and libselinux.
>>
>> diff -urN selinux.orig2/libselinux/include/selinux/selinux.h
>> selinux/libselinux/include/selinux/selinux.h
>>      
>
> diff with -p (or just git diff) is nicer in that it shows function names
> too.
>
>    
Thank you.
>> --- selinux.orig2/libselinux/include/selinux/selinux.h    2009-07-01
>> 21:15:17.009238289 -0400
>> +++ selinux/libselinux/include/selinux/selinux.h    2009-07-01
>> 21:44:57.264509874 -0400
>> @@ -8,6 +8,9 @@
>>    extern "C" {
>>    #endif
>>
>> +/* Return 1 if the dont audit rules have been turned off or 0 if not. */
>> +extern int is_dontaudit_disabled(void);
>>      
>
> I'm not sure why we'd push this out to libselinux and expose the file
> location to both libselinux and libsemanage.  What programs would use
> this that couldn't just link against libsemanage?
>
>    
It's not that a program would use this that couldn't link against 
libsemanage the functionality just seemed closer to that of the 
functions in libselinux, I've been doing alot of work on fedora stuff It 
seems to me  that 90% of the code in libsemanage is handle dependent 
functions. libselinux seems to be more of a global setting kind of deal. 
so it made sense to put it here. Let me know if this isn't the case
>> diff -urN selinux.orig2/libselinux/src/dontaudit.c
>> selinux/libselinux/src/dontaudit.c
>> --- selinux.orig2/libselinux/src/dontaudit.c    1969-12-31
>> 19:00:00.000000000 -0500
>> +++ selinux/libselinux/src/dontaudit.c    2009-07-01 21:48:48.635521208
>> -0400
>> @@ -0,0 +1,21 @@
>> +#include<unistd.h>
>> +#include<selinux/selinux.h>
>> +#include "selinux_internal.h"
>> +#include<stdlib.h>
>> +#include<limits.h>
>> +#include<stdarg.h>
>> +#include<stdio.h>
>> +#include<string.h>
>> +
>> +int is_dontaudit_disabled(void)
>> +{
>> +    char path[PATH_MAX];
>> +    snprintf(path,PATH_MAX,"%s/disable_dontaudit",selinux_policy_root());
>> +
>> +    if (access(path,F_OK) == 0)
>> +        return 1;
>> +    else
>> +        return 0;
>> +}
>> +
>> +hidden_def(is_dontaudit_disabled)
>>      
>
> We don't need a hidden def unless libselinux internally calls the
> function as well.
>    
Thank you I'll be resubmitting this patch shortly
>    
>> diff -urN selinux.orig2/libselinux/src/selinux_internal.h
>> selinux/libselinux/src/selinux_internal.h
>> --- selinux.orig2/libselinux/src/selinux_internal.h    2009-07-01
>> 21:15:17.074235819 -0400
>> +++ selinux/libselinux/src/selinux_internal.h    2009-07-01
>> 21:44:57.272486689 -0400
>> @@ -24,6 +24,7 @@
>>        hidden_proto(security_compute_create_raw)
>>        hidden_proto(security_compute_member_raw)
>>        hidden_proto(security_compute_relabel_raw)
>> +    hidden_proto(is_dontaudit_disabled)
>>      
>
> Ditto.
>
>    
>> diff -urN selinux.orig2/libsemanage/include/semanage/handle.h
>> selinux/libsemanage/include/semanage/handle.h
>> --- selinux.orig2/libsemanage/include/semanage/handle.h    2009-07-01
>> 21:15:17.224235939 -0400
>> +++ selinux/libsemanage/include/semanage/handle.h    2009-07-01
>> 21:44:57.274484577 -0400
>> @@ -69,6 +69,9 @@
>>     * 1 for yes, 0 for no (default) */
>>    void semanage_set_create_store(semanage_handle_t * handle, int
>> create_store);
>>
>> +/*Get whether or not to dontaudits will be disabled upon commit */
>> +int semanage_get_disable_dontaudit(semanage_handle_t * handle);
>>      
>
> As before, I don't know why we'd export this transient information
> outside of the library, vs. only exporting the persistent dontaudit
> setting.
>    
See explaination from previous patch.
>    
>> diff -urN selinux.orig2/libsemanage/src/handle.c
>> selinux/libsemanage/src/handle.c
>> --- selinux.orig2/libsemanage/src/handle.c    2009-07-01
>> 21:15:17.288238017 -0400
>> +++ selinux/libsemanage/src/handle.c    2009-07-01 21:55:04.525487189 -0400
>>      
> <snip>
>    
>> @@ -264,11 +276,22 @@
>>        assert(sh != NULL&&  sh->funcs != NULL&&  sh->funcs->commit != NULL);
>>        if (!sh->is_in_transaction) {
>>            ERR(sh,
>> -            "Will not commit because caller does not have a tranaction
>> lock yet.");
>> +            "Will not commit because caller does not have a transaction
>> lock yet.");
>>            return -1;
>>        }
>>        retval = sh->funcs->commit(sh);
>>        sh->is_in_transaction = 0;
>>        sh->modules_modified = 0;
>> +    if (retval == 0){
>> +        char path[PATH_MAX];
>> +
>> snprintf(path,PATH_MAX,"%s/disable_dontaudit",selinux_policy_root());
>> +        if(semanage_get_disable_dontaudit(sh) == 1){
>> +            FILE *touch;
>> +            touch = fopen(path,"w");
>> +            fclose(touch);
>> +        }else{
>> +            remove(path);
>> +        }
>> +    }
>>      
>
> This doesn't make sense to me - we check whether we've already set
> disable dontaudit and use that to decide whether to create the file?
> But the existence of the file is what would have triggered setting
> disable dontaudit in the first place.  Round and round we go...
>    
When we create the handle we set it's default property to the system 
default. When we commit a handle we set the system default property to 
the handles property. In between it is fully possible to that we have 
called a set_disable_dontaudit to change the value in the handle. If you 
would rather I checked if the two were different first I can.
> Also, I think it makes more sense to keep all of this private to
> libsemanage and to keep this file in the module store, as Joshua already
> said.
>    
Noted I'll move the file into the module folder.
>    
>>        return retval;
>>    }
>>      
>
>    


[-- Attachment #2: Type: text/html, Size: 8340 bytes --]