[RFC][PATCH] apparmor: Enforce progressively tighter permissions for no_new_privs
Eric W. Biederman
ebiederm at xmission.com
Wed Jan 20 21:26:56 UTC 2021
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 <ebiederm at 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
More information about the Linux-security-module-archive
mailing list