[apparmor] [PATCH v1.1 05/11] parser: Allow change_profile rules to accept an exec mode modifier

[Tyler Hicks]

My mail client decided to sign and encrypt my previous reply. See what I
wrote below.

Tyler

On 05/31/2016 09:46 AM, Tyler Hicks wrote:
> On 05/31/2016 05:08 AM, John Johansen wrote:
>> On 05/28/2016 09:42 AM, Tyler Hicks wrote:
>>> https://launchpad.net/bugs/1584069
>>>
>>> This patch allows policy authors to specify how exec transitions should
>>> be handled with respect to setting AT_SECURE in the new process'
>>> auxiliary vector and, ultimately, having libc scrub (or not scrub) the
>>> environment.
>>>
>>> An exec mode of 'safe' means that the environment will be scrubbed and
>>> this is the default in kernels that support AppArmor profile stacking.
>>> An exec mode of 'unsafe' means that the environment will not be scrubbed
>>> and this is the default and only supported change_profile exec mode in
>>> kernels that do not support AppArmor profile stacking.
>>>
>>> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
>>
>> one problem below, the rest looks good
>>
>>> ---
>>>
>>> * Changes since v1:
>>>   - parser_lex.l: Remove CHANGE_PROFILE_TARGET_MODE, added by v1 of this
>>>     patch, in favor of using the existing SUB_ID rule to handle the
>>>     change_profile transition target.
>>>   - parser_lexl: Add SUB_ID to the list of rules that eats whitespace.
>>>     Without this change, the parser would reject change_profile rules the
>>>     one found in
>>>     parser/tst/simple_tests//vars/vars_simple_assignment_13.sd
>>>   - parser_regex.c: Create a new helper function,
>>>     is_change_profile_mode(), and clearly document why
>>>     (mode & ~AA_CHANGE_PROFILE) is not safe to use.
>>>   - parser_regex.c: Revert the change made in v1 of this patch to identify
>>>     modes that do not correspond to link rules.
>>>
>>>
>>>  parser/parser_lex.l   | 13 +++++++++++--
>>>  parser/parser_regex.c | 44 +++++++++++++++++++++++++++++++++++++-------
>>>  parser/parser_yacc.y  | 28 ++++++++++++++++++++++++----
>>>  3 files changed, 72 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/parser/parser_lex.l b/parser/parser_lex.l
>>> index 49b1f22..cff8a7b 100644
>>> --- a/parser/parser_lex.l
>>> +++ b/parser/parser_lex.l
>>> @@ -268,7 +268,7 @@ LT_EQUAL	<=
>>>  	}
>>>  %}
>>>  
>>> -<INITIAL,INCLUDE,LIST_VAL_MODE,EXTCOND_MODE,LIST_COND_VAL,LIST_COND_PAREN_VAL,LIST_COND_MODE,EXTCONDLIST_MODE,ASSIGN_MODE,NETWORK_MODE,CHANGE_PROFILE_MODE,RLIMIT_MODE,MOUNT_MODE,DBUS_MODE,SIGNAL_MODE,PTRACE_MODE,UNIX_MODE>{
>>> +<INITIAL,SUB_ID,INCLUDE,LIST_VAL_MODE,EXTCOND_MODE,LIST_COND_VAL,LIST_COND_PAREN_VAL,LIST_COND_MODE,EXTCONDLIST_MODE,ASSIGN_MODE,NETWORK_MODE,CHANGE_PROFILE_MODE,RLIMIT_MODE,MOUNT_MODE,DBUS_MODE,SIGNAL_MODE,PTRACE_MODE,UNIX_MODE>{
>>>  	{WS}+	{  DUMP_PREPROCESS; /* Ignoring whitespace */ }
>>
>> ugh, so when I said use sub_id I completely forgot about change_profile_mode eating the WS. This changes the behavior for hats that are defined using a leading ^.
>> currently WS is not allowed the ^ and hat name, and I am not sure we want to change that.
>> This doesn't however change HAT and PROFILE as they explicitly skip WS.
>>
>> We can either keep sub_id by doing
>> <CHANGE_PROFILE_MODE>{
>> 	{ARROW}{WS}*  {
> 
> This doesn't work because WS is using the [:blank:] character class
> which doesn't match newlines. That means that
> parser/tst/simple_tests/vars/vars_simple_assignment_13.sd fails to parse.
> 
> This works:
> 
> 	{ARROW}({WS}|\n)* {
> 		...
> 	}
> 
> Tyler


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160531/455b8f6a/attachment.pgp>