Re: [RFC][PATCH] apparmor: Enforce progressively tighter permissions for no_new_privs
From: John Johansen <john.johansen@canonical.com>
Date: 2021-01-21 00:11:11
Also in:
lkml
On 1/20/21 1:26 PM, Eric W. Biederman wrote:
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:quoted
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.
That is not correct.
commit 7b0d0b40cd78 ("selinux: Permit bounded transitions under
NO_NEW_PRIVS or NOSUID.")
Allows for bound transitions under selinux
and
commit af63f4193f9f selinux: Generalize support for NNP/nosuid SELinux
domain transitions
goes further and "Decouple NNP/nosuid from SELinux transitions".
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.
yes, but that shouldn't matter. As apparmor has not changed its label at any point between when no_new_privs was set and when the check is done. AppArmor is attempting to change it label, and if it finds NNP has been set we capture what the confinement was.
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
right, and apparmor only ever has the task update its own label.
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.
I am open to supporting something like that.
Futhermore given the consistent documentation and how everything else implements no_new_privs, not having the permissions get progressively
Again see above
tighter is a footgun aimed at userspace. I fully expect it to break any
tighter is somewhat relative, nor is it only progressively tighter it is bounded against the snapshot of the label that was on the task.
security sensitive software that uses no_new_privs and was not deliberately designed and tested against apparmor.
Currently the situation has become either an either or choice between the LSM and NNP. We are trying to walk a balance. Ideally apparmor would like to do something similar to selinux and decouple the label transition from NNP and nosuid via an internal capability, but we have not gone there yet.
Avoid the questionable and hard to fix implementation and the potential to confuse userspace by having no_new_privs enforce progressinvely tighter permissions.
This would completely break several use cases.
quoted hunk ↗ jump to hunk
Fixes: 9fcf78cca198 ("apparmor: update domain transitions that are subsets of confinement at nnp") Signed-off-by: Eric W. Biederman <redacted> --- 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