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

[Tyler Hicks]

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>

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/721631a8/attachment-0001.pgp>