[apparmor] [PATCH] Fix: make sure overlapping safe and unsafe exec rules conflict

[Tyler Hicks]

On 06/01/2016 04:11 PM, Tyler Hicks wrote:
> On 06/01/2016 01:07 AM, John Johansen wrote:
>> Currently
>>
>>   change_profile /** -> A,
>>   change_profile unsafe /** -> A,
>>
>> do not conflict because the safe rules only set the change_profile
>> permission where the unsafe set unsafe exec. To fix this we have the
>> safe version set exec bits as well with out setting unsafe exec.
>> This allows the exec conflict logic to detect any conflicts.
>>
>> This is safe to do even for older kernels as the exec bits off of the
>> 2nd term encoding in the change_onexec rules are unused.
>>
>> Signed-off-by: John Johansen <john.johansen at canonical.com>
>> ---
>>  parser/parser_yacc.y                               | 23 +++++++++++-----------
>>  parser/tst/equality.sh                             | 14 +++++++++++++
>>  .../change_profile/onx_conflict_unsafe1.sd         |  8 ++++++++
>>  .../change_profile/onx_conflict_unsafe2.sd         |  8 ++++++++
>>  4 files changed, 42 insertions(+), 11 deletions(-)
>>  create mode 100644 parser/tst/simple_tests/change_profile/onx_conflict_unsafe1.sd
>>  create mode 100644 parser/tst/simple_tests/change_profile/onx_conflict_unsafe2.sd
>>
>> diff --git a/parser/parser_yacc.y b/parser/parser_yacc.y
>> index b76634f..3e2bcd2 100644
>> --- a/parser/parser_yacc.y
>> +++ b/parser/parser_yacc.y
>> @@ -1486,15 +1486,16 @@ change_profile: TOK_CHANGE_PROFILE opt_exec_mode opt_id opt_named_transition TOK
>>  		char *exec = $3;
>>  		char *target = $4;
>>  
>> -		if (exec_mode != EXEC_MODE_EMPTY) {
>> -			if (!exec)
>> -				yyerror(_("Exec condition is required when unsafe or safe keywords are present"));
>> -
>> -			if (exec_mode == EXEC_MODE_UNSAFE) {
>> -				mode |= (AA_EXEC_BITS | ALL_AA_EXEC_UNSAFE);
>> -			} else if (exec_mode == EXEC_MODE_SAFE &&
>> -				   !kernel_supports_stacking &&
>> -				   warnflags & WARN_RULE_DOWNGRADED) {
>> +		if (exec) {
>> +			/* exec bits required to trigger rule conflict if
>> +			 * for overlapping safe and unsafe exec rules
>> +			 */
>> +			mode |= AA_EXEC_BITS;
>> +			if (exec_mode == EXEC_MODE_UNSAFE)
>> +				mode |= ALL_AA_EXEC_UNSAFE;
>> +			else if (exec_mode == EXEC_MODE_SAFE &&
>> +				 !kernel_supports_stacking &&
>> +				 warnflags & WARN_RULE_DOWNGRADED) {
>>  				pwarn("downgrading change_profile safe rule to unsafe due to lack of necessary kernel support\n");
>>  				/**
>>  				 * No need to do anything because 'unsafe' exec
>> @@ -1502,8 +1503,8 @@ change_profile: TOK_CHANGE_PROFILE opt_exec_mode opt_id opt_named_transition TOK
>>  				 * change_profile rules in non-stacking kernels
>>  				 */
>>  			}
>> -		}
>> -
>> +		} else if (exec_mode != EXEC_MODE_EMPTY)
>> +			yyerror(_("Exec condition is required when unsafe or safe keywords are present"));
>>  		if (exec && !(exec[0] == '/' || strncmp(exec, "@{", 2) == 0))
>>  			yyerror(_("Exec condition must begin with '/'."));
>>  
>> diff --git a/parser/tst/equality.sh b/parser/tst/equality.sh
>> index 25260ad..18bd286 100755
>> --- a/parser/tst/equality.sh
>> +++ b/parser/tst/equality.sh
>> @@ -461,9 +461,23 @@ verify_binary_equality "Deny of ungranted perm" \
>>  verify_binary_equality "change_profile == change_profile -> **" \
>>  		       "/t { change_profile, }" \
>>  		       "/t { change_profile -> **, }" \
> 
> Nitpick: please drop this trailing '\' when you commit the patch
> 
> With that,
> 
> Acked-by: Tyler Hicks <tyhicks at canonical.com>

I've created a bug to track this since it'll need to be backported with
the change_profile safe/unsafe feature. Please add it to the commit message:

  https://launchpad.net/bugs/1588069

Tyler

> 
> Big thanks for chasing this down and fixing it!
> 
> Tyler
> 
>> +
>> +verify_binary_equality "change_profile /** == change_profile /** -> **" \
>>  		       "/t { change_profile /**, }" \
>>  		       "/t { change_profile /** -> **, }"
>>  
>> +verify_binary_equality "change_profile /** == change_profile /** -> **" \
>> +		       "/t { change_profile unsafe /**, }" \
>> +		       "/t { change_profile unsafe /** -> **, }"
>> +
>> +verify_binary_equality "change_profile /** == change_profile /** -> **" \
>> +		       "/t { change_profile /**, }" \
>> +		       "/t { change_profile safe /** -> **, }"
>> +
>> +verify_binary_inequality "change_profile /** == change_profile /** -> **" \
>> +			 "/t { change_profile /**, }" \
>> +			 "/t { change_profile unsafe /**, }"
>> +
>>  verify_binary_equality "profile name is hname in rule" \
>>  	":ns:/hname { signal peer=/hname, }" \
>>  	":ns:/hname { signal peer=@{profile_name}, }"
>> diff --git a/parser/tst/simple_tests/change_profile/onx_conflict_unsafe1.sd b/parser/tst/simple_tests/change_profile/onx_conflict_unsafe1.sd
>> new file mode 100644
>> index 0000000..05854ae
>> --- /dev/null
>> +++ b/parser/tst/simple_tests/change_profile/onx_conflict_unsafe1.sd
>> @@ -0,0 +1,8 @@
>> +#
>> +#=DESCRIPTION test for conflict safe and unsafe exec condition
>> +#=EXRESULT FAIL
>> +#
>> +/usr/bin/foo {
>> +   change_profile /onexec -> /bin/foo,
>> +   change_profile unsafe /onexec -> /bin/foo,
>> +}
>> diff --git a/parser/tst/simple_tests/change_profile/onx_conflict_unsafe2.sd b/parser/tst/simple_tests/change_profile/onx_conflict_unsafe2.sd
>> new file mode 100644
>> index 0000000..a6c583e
>> --- /dev/null
>> +++ b/parser/tst/simple_tests/change_profile/onx_conflict_unsafe2.sd
>> @@ -0,0 +1,8 @@
>> +#
>> +#=DESCRIPTION test for conflict safe and unsafe exec condition
>> +#=EXRESULT FAIL
>> +#
>> +/usr/bin/foo {
>> +   change_profile safe /onexec -> /bin/foo,
>> +   change_profile unsafe /onexec -> /bin/foo,
>> +}
>>
> 
> 
> 
> 


-------------- 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/20160601/05660f8e/attachment.pgp>