Re: Debugging sepolgen-ifgen?

[Daniel J Walsh <dwalsh@redhat.com>]

On 08/25/2014 03:18 PM, Steve Lawrence wrote:
> On 08/05/2014 09:09 AM, Stephen Smalley wrote:
>> On 08/04/2014 05:44 PM, Daniel J Walsh wrote:
>>> On 08/04/2014 01:07 PM, Stephen Smalley wrote:
>>>> On 08/02/2014 03:19 PM, Sven Vermeulen wrote:
>>>>> Hi all
>>>>>
>>>>> I've noticed that on my system, for some interfaces, the results in
>>>>> /var/lib/sepolgen/interface_info are missing file-specific feedback.
>>>>>
>>>>> For instance, consider the kernel_rw_kernel_sysctl() interface, which is
>>>>> coded as follows:
>>>>>
>>>>> interface(`kernel_rw_kernel_sysctl',`
>>>>>         gen_require(`
>>>>>                 type proc_t, sysctl_t, sysctl_kernel_t;
>>>>>         ')
>>>>>
>>>>>         rw_files_pattern($1, { proc_t sysctl_t sysctl_kernel_t }, sysctl_kernel_t)
>>>>>
>>>>>         list_dirs_pattern($1, { proc_t sysctl_t }, sysctl_kernel_t)
>>>>> ')
>>>>>
>>>>> In the interface_info file, I only find the following metadata about this
>>>>> interface:
>>>>>
>>>>> [InterfaceVector kernel_rw_kernel_sysctl $1:source ]
>>>>> $1,sysctl_t,dir,getattr,open,search
>>>>> $1,sysctl_kernel_t,dir,getattr,open,search
>>>>> $1,proc_t,dir,getattr,open,search
>>>>>
>>>>> Shouldn't this at least contain something like this?
>>>>>
>>>>> $1,sysctl_kernel_t,file,write,getattr,lock,open,ioctl,append 
>>>>>
>>>>> Although not critical, it does result in audit2allow -R to not use
>>>>> refpolicy-style interfaces when possible...
>>>>>
>>>>> How can I debug this? I know the file is generated by sepolgen-ifgen, but
>>>>> rerunning doesn't add in any file-related metadata and I'm totally oblivious
>>>>> on how the parsing is done...
>>>> Not sure about that beyond the -d -v options.
>>>> However, this appears to be a regression; despite encountering some syntax errors during parsing,
>>>> sepolgen-ifgen from 21030423 generates a more accurate vector:
>>>>
>>>> [InterfaceVector kernel_rw_kernel_sysctl $1:source ]
>>>> $1,sysctl_t,dir,getattr,open,search
>>>> $1,sysctl_kernel_t,file,write,getattr,read,lock,open,ioctl,append
>>>> $1,sysctl_kernel_t,dir,search,read,lock,ioctl,getattr,open
>>>> $1,proc_t,dir,getattr,open,search
>>>>
>>>> while sepolgen-ifgen from 20131030_4 generates the reduced set you have above.
>>>>
>>>> Seems to have been broken by:
>>>>
>>>> commit 17cc87e56b0241688c119f774f103622b002e0ae
>>>> Author: Dan Walsh <dwalsh@redhat.com>
>>>> Date:   Wed Oct 9 17:01:35 2013 -0400
>>>>
>>>>     sepolgen did not work with filename transitions.
>>>>     
>>>>     This patch adds support for it.
>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Selinux mailing list
>>>> Selinux@tycho.nsa.gov
>>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>>>>
>>>>
>>> I don't see anything obviously wrong with that patch?
>> If I revert that one patch, it works.  I don't know offhand what the
>> underlying issue is, but I'd guess you are introducing an ambiguity into
>> the grammar.  I do notice that the definitions of IDENTIFIER and
>> FILENAME do not match the ones in checkpolicy policy_scan.l; I do not
>> know why that is.  I also notice that whereas bison reports no warnings
>> on the checkpolicy policy_parse.y grammar, sepolgen-ifgen -d reports 669
>> shift/reduce conflicts before your patch and 671 shift/reduce conflicts
>> afterward; neither seems very good...
> Looking at the patch that seems to have caused the problem, it makes the
> following change:
>
> @@ -461,6 +469,7 @@ def p_interface_call_param(p):
>  def p_interface_call_param_list(p):
>      '''interface_call_param_list : interface_call_param
>                                   | interface_call_param_list COMMA
> interface_call_param
> +                                 | interface_call_param_list COMMA
> interface_call_param COMMA interface_call_param_list
>      '''
>      if len(p) == 2:
>          p[0] = [p[1]]
>
> For some reason, it adds a new pattern to the interface_call_param_list
> pattern, which is "list, param, list". To me, this doesn't seem
> necessary. The first two patterns should cover all combinations of
> interface call parameters. And reverting this hunk gives the same
> InterfaceVector as with the patch reverted. So it seems like the right fix.
>
> Dan, any idea why this hunk was added?
>
> Thanks,
> - Steve
If you remove it and the command succeeds I am happy.  Most of these
changes were made to just stop the
application from blowing up.  If we get better accuracy on matches and
the code does not break, It is a win win.