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.johan...@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 <tyhi...@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,
> +}
>
signature.asc
Description: OpenPGP digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
