The current understanding of apparmor with respect to no_new_privs is at odds with how no_new_privs is implemented and understood by the rest of the kernel.
The documentation of no_new_privs states: > With ``no_new_privs`` set, ``execve()`` promises not to grant the > privilege to do anything that could not have been done without the > execve call. And reading through the kernel except for apparmor that description matches what is implemented. There are two major divergences of apparmor from this definition: - proc_setattr enforces limitations when no_new_privs are set. - the limitation is enforced from the apparent time when no_new_privs is set instead of guaranteeing that each execve has progressively more narrow permissions. The code in apparmor that attempts to discover the apparmor label at the point where no_new_privs is set is not robust. The capture happens a long time after no_new_privs is set. Capturing the label at the point where no_new_privs is set is practically impossible to implement robustly. Today the rule is struct cred can only be changed by it's current task. Today SECCOMP_FILTER_FLAG_TSYNC sets no_new_privs from another thread. A robust implementation would require changing something fundamental in how creds are managed for SECCOMP_FILTER_FLAG_TSYNC to be able to capture the cred at the point it is set. Futhermore given the consistent documentation and how everything else implements no_new_privs, not having the permissions get progressively tighter is a footgun aimed at userspace. I fully expect it to break any security sensitive software that uses no_new_privs and was not deliberately designed and tested against apparmor. Avoid the questionable and hard to fix implementation and the potential to confuse userspace by having no_new_privs enforce progressinvely tighter permissions. Fixes: 9fcf78cca198 ("apparmor: update domain transitions that are subsets of confinement at nnp") Signed-off-by: Eric W. Biederman <ebied...@xmission.com> --- I came accross this while examining the places cred_guard_mutex is used and trying to find a way to make those code paths less insane. If it would be more pallatable I would not mind removing the task_no_new_privs test entirely from aa_change_hat and aa_change_profile as those are not part of exec, so arguably no_new_privs should not care about them at all. Can we please get rid of the huge semantic wart and pain in the implementation? security/apparmor/domain.c | 39 ++++---------------------------- security/apparmor/include/task.h | 4 ---- security/apparmor/task.c | 7 ------ 3 files changed, 4 insertions(+), 46 deletions(-) diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index f919ebd042fd..8f77059bf890 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -869,17 +869,6 @@ int apparmor_bprm_creds_for_exec(struct linux_binprm *bprm) label = aa_get_newest_label(cred_label(bprm->cred)); - /* - * Detect no new privs being set, and store the label it - * occurred under. Ideally this would happen when nnp - * is set but there isn't a good way to do that yet. - * - * Testing for unconfined must be done before the subset test - */ - if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) && !unconfined(label) && - !ctx->nnp) - ctx->nnp = aa_get_label(label); - /* buffer freed below, name is pointer into buffer */ buffer = aa_get_buffer(false); if (!buffer) { @@ -915,7 +904,7 @@ int apparmor_bprm_creds_for_exec(struct linux_binprm *bprm) */ if ((bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) && !unconfined(label) && - !aa_label_is_unconfined_subset(new, ctx->nnp)) { + !aa_label_is_unconfined_subset(new, label)) { error = -EPERM; info = "no new privs"; goto audit; @@ -1158,16 +1147,6 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) label = aa_get_newest_cred_label(cred); previous = aa_get_newest_label(ctx->previous); - /* - * Detect no new privs being set, and store the label it - * occurred under. Ideally this would happen when nnp - * is set but there isn't a good way to do that yet. - * - * Testing for unconfined must be done before the subset test - */ - if (task_no_new_privs(current) && !unconfined(label) && !ctx->nnp) - ctx->nnp = aa_get_label(label); - if (unconfined(label)) { info = "unconfined can not change_hat"; error = -EPERM; @@ -1193,7 +1172,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) * reduce restrictions. */ if (task_no_new_privs(current) && !unconfined(label) && - !aa_label_is_unconfined_subset(new, ctx->nnp)) { + !aa_label_is_unconfined_subset(new, label)) { /* not an apparmor denial per se, so don't log it */ AA_DEBUG("no_new_privs - change_hat denied"); error = -EPERM; @@ -1214,7 +1193,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) * reduce restrictions. */ if (task_no_new_privs(current) && !unconfined(label) && - !aa_label_is_unconfined_subset(previous, ctx->nnp)) { + !aa_label_is_unconfined_subset(previous, label)) { /* not an apparmor denial per se, so don't log it */ AA_DEBUG("no_new_privs - change_hat denied"); error = -EPERM; @@ -1303,16 +1282,6 @@ int aa_change_profile(const char *fqname, int flags) label = aa_get_current_label(); - /* - * Detect no new privs being set, and store the label it - * occurred under. Ideally this would happen when nnp - * is set but there isn't a good way to do that yet. - * - * Testing for unconfined must be done before the subset test - */ - if (task_no_new_privs(current) && !unconfined(label) && !ctx->nnp) - ctx->nnp = aa_get_label(label); - if (!fqname || !*fqname) { aa_put_label(label); AA_DEBUG("no profile name"); @@ -1409,7 +1378,7 @@ int aa_change_profile(const char *fqname, int flags) * reduce restrictions. */ if (task_no_new_privs(current) && !unconfined(label) && - !aa_label_is_unconfined_subset(new, ctx->nnp)) { + !aa_label_is_unconfined_subset(new, label)) { /* not an apparmor denial per se, so don't log it */ AA_DEBUG("no_new_privs - change_hat denied"); error = -EPERM; diff --git a/security/apparmor/include/task.h b/security/apparmor/include/task.h index f13d12373b25..8a9c258e2018 100644 --- a/security/apparmor/include/task.h +++ b/security/apparmor/include/task.h @@ -17,13 +17,11 @@ static inline struct aa_task_ctx *task_ctx(struct task_struct *task) /* * struct aa_task_ctx - information for current task label change - * @nnp: snapshot of label at time of no_new_privs * @onexec: profile to transition to on next exec (MAY BE NULL) * @previous: profile the task may return to (MAY BE NULL) * @token: magic value the task must know for returning to @previous_profile */ struct aa_task_ctx { - struct aa_label *nnp; struct aa_label *onexec; struct aa_label *previous; u64 token; @@ -42,7 +40,6 @@ struct aa_label *aa_get_task_label(struct task_struct *task); static inline void aa_free_task_ctx(struct aa_task_ctx *ctx) { if (ctx) { - aa_put_label(ctx->nnp); aa_put_label(ctx->previous); aa_put_label(ctx->onexec); } @@ -57,7 +54,6 @@ static inline void aa_dup_task_ctx(struct aa_task_ctx *new, const struct aa_task_ctx *old) { *new = *old; - aa_get_label(new->nnp); aa_get_label(new->previous); aa_get_label(new->onexec); } diff --git a/security/apparmor/task.c b/security/apparmor/task.c index d17130ee6795..4b9ec370a171 100644 --- a/security/apparmor/task.c +++ b/security/apparmor/task.c @@ -41,7 +41,6 @@ struct aa_label *aa_get_task_label(struct task_struct *task) int aa_replace_current_label(struct aa_label *label) { struct aa_label *old = aa_current_raw_label(); - struct aa_task_ctx *ctx = task_ctx(current); struct cred *new; AA_BUG(!label); @@ -56,12 +55,6 @@ int aa_replace_current_label(struct aa_label *label) if (!new) return -ENOMEM; - if (ctx->nnp && label_is_stale(ctx->nnp)) { - struct aa_label *tmp = ctx->nnp; - - ctx->nnp = aa_get_newest_label(tmp); - aa_put_label(tmp); - } if (unconfined(label) || (labels_ns(old) != labels_ns(label))) /* * if switching to unconfined or a different label namespace -- 2.20.1 Eric