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