Bug#1050256: AppArmor breaks locking non-fs Unix sockets
On 12/30/23 20:24, Mathias Gibbens wrote:
On Sat, 2023-12-30 at 16:44 +0100, Salvatore Bonaccorso wrote:
John, did you had a chance to work on this backport for 6.1.y stable
upstream so we could pick it downstream in Debian in one of the next
stable imports? Cherry-picking 1cf26c3d2c4c ("apparmor: fix apparmor
mediating locking non-fs unix sockets") does not work, if not
havinging the work around e2967ede2297 ("apparmor: compute policydb
permission on profile load") AFAICS, so that needs a 6.1.y specific
backport submitted to stable@vger.kernel.org ?
I think we could have people from this bug as well providing a
Tested-by when necessary. I'm not feeling confident enough to be able
to provide myself such a patch to sent to stable (and you only giving
an Acked-by/Reviewed-by), so if you can help out here with your
upstream hat on that would be more than appreciated and welcome :)
Thanks a lot for your work!
I played around with this a bit the past week as well, and came to
the same conclusion as Salvatore did that commits e2967ede2297 and
1cf26c3d2c4c need to be cherry-picked back to the 6.1 stable tree.
I've attached the two commits rebased onto 6.1.y as patches to this
message. Commit e2967ede2297 needed a little bit of touchup to apply
cleanly, and 1cf26c3d2c4c just needed adjustments for line number
changes. I included some comments at the top of each patch.
With these two commits cherry-picked on top of the 6.1.69 kernel, I
can boot a bookworm system and successfully start a service within a
container that utilizes `PrivateNetwork=yes`. Rebooting back into an
unpatched vanilla 6.1.69 kernel continues to show the problem.
While I didn't see any immediate issues (ie, `aa-status` and log
files looked OK), I don't understand the changes in the first commit
well enough to be confident in sending these patches for inclusion in
the upstream stable tree on my own.
Mathias
Your backports look good to me, and you can stick my acked-by on them.
The changes are strictly more than necessary for the fix. They are
part of a larger change set that is trying to cleanup the runtime
code by changing the permission mapping from a runtime operation
to something that is done only at policy load/unpack time.
The advantage of this approach is that while it is a larger change
than strictly necessary. It is backporting patches that are already
upstream, keep the code closer and making backports easier.
Georgia did a minimal backport fix by keeping the version as part
of policy and doing the permission mapping at runtime. I have
included that patch below. Its advantage is it is a minimal
change to fix the issue.
I am happy with either version going into stable. Do you want to
send them or do you want me to do it?
Acked-by: John Johansen <john.johansen@canonical.com>
From d716d8e6e8b6dc2fa1db2e72ee95c721aef12064 Mon Sep 17 00:00:00 2001
From: Georgia Garcia <georgia.garcia@canonical.com>
Date: Tue, 9 Jan 2024 17:54:52 -0300
Subject: [PATCH] apparmor: backport fix apparmor mediating locking non-fs,
unix sockets
This is a minimal backport of
1cf26c3d2c4c apparmor: fix apparmor mediating locking non-fs unix sockets
instead of pulling in the dependency patch
e2967ede2297 apparmor: compute policydb permission on profile load
which moves the permission mapping to unpack time. We push the version
information into the profile so the permission mapping fix can be
done in the run time aa_compute_perms() instead of the load time
equivalent in compute_perms_entry() introduced by e2967ede2297.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
---
security/apparmor/apparmorfs.c | 3 ++-
security/apparmor/include/perms.h | 2 +-
security/apparmor/include/policy.h | 12 ++++++++++++
security/apparmor/label.c | 9 ++++++---
security/apparmor/lib.c | 4 +++-
security/apparmor/net.c | 3 ++-
security/apparmor/policy_unpack.c | 11 +----------
7 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 7160e7aa58b9..8131c9345900 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -633,7 +633,8 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms,
state = aa_dfa_match_len(dfa, profile->policy.start[0],
match_str, match_len);
if (state)
- aa_compute_perms(dfa, state, &tmp);
+ aa_compute_perms(dfa, state, &tmp,
+ profile->policy.version);
}
aa_apply_modes_to_perms(profile, &tmp);
aa_perms_accum_raw(perms, &tmp);
diff --git a/security/apparmor/include/perms.h b/security/apparmor/include/perms.h
index 13f20c598448..7d5c0c004978 100644
--- a/security/apparmor/include/perms.h
+++ b/security/apparmor/include/perms.h
@@ -142,7 +142,7 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs,
void aa_apply_modes_to_perms(struct aa_profile *profile,
struct aa_perms *perms);
void aa_compute_perms(struct aa_dfa *dfa, unsigned int state,
- struct aa_perms *perms);
+ struct aa_perms *perms, u32 version);
void aa_perms_accum(struct aa_perms *accum, struct aa_perms *addend);
void aa_perms_accum_raw(struct aa_perms *accum, struct aa_perms *addend);
void aa_profile_match_label(struct aa_profile *profile, struct aa_label *label,
diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
index 639b5b248e63..e7973aedc382 100644
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -56,6 +56,17 @@ extern const char *const aa_profile_mode_names[];
#define on_list_rcu(X) (!list_empty(X) && (X)->prev != LIST_POISON2)
+#define K_ABI_MASK 0x3ff
+#define FORCE_COMPLAIN_FLAG 0x800
+#define VERSION_LT(X, Y) (((X) & K_ABI_MASK) < ((Y) & K_ABI_MASK))
+#define VERSION_LE(X, Y) (((X) & K_ABI_MASK) <= ((Y) & K_ABI_MASK))
+#define VERSION_GT(X, Y) (((X) & K_ABI_MASK) > ((Y) & K_ABI_MASK))
+
+#define v5 5 /* base version */
+#define v6 6 /* per entry policydb mediation check */
+#define v7 7
+#define v8 8 /* full network masking */
+
/*
* FIXME: currently need a clean way to replace and remove profiles as a
* set. It should be done at the namespace level.
@@ -78,6 +89,7 @@ struct aa_policydb {
/* Generic policy DFA specific rule types will be subsections of it */
struct aa_dfa *dfa;
unsigned int start[AA_CLASS_LAST + 1];
+ u32 version;
};
diff --git a/security/apparmor/label.c b/security/apparmor/label.c
index a67c5897ee25..316293df07e0 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -1330,7 +1330,8 @@ static int label_compound_match(struct aa_profile *profile,
if (!state)
goto fail;
}
- aa_compute_perms(profile->policy.dfa, state, perms);
+ aa_compute_perms(profile->policy.dfa, state, perms,
+ profile->policy.version);
aa_apply_modes_to_perms(profile, perms);
if ((perms->allow & request) != request)
return -EACCES;
@@ -1381,7 +1382,8 @@ static int label_components_match(struct aa_profile *profile,
return 0;
next:
- aa_compute_perms(profile->policy.dfa, state, &tmp);
+ aa_compute_perms(profile->policy.dfa, state, &tmp,
+ profile->policy.version);
aa_apply_modes_to_perms(profile, &tmp);
aa_perms_accum(perms, &tmp);
label_for_each_cont(i, label, tp) {
@@ -1390,7 +1392,8 @@ static int label_components_match(struct aa_profile *profile,
state = match_component(profile, tp, start);
if (!state)
goto fail;
- aa_compute_perms(profile->policy.dfa, state, &tmp);
+ aa_compute_perms(profile->policy.dfa, state, &tmp,
+ profile->policy.version);
aa_apply_modes_to_perms(profile, &tmp);
aa_perms_accum(perms, &tmp);
}
diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
index 1c72a61108d3..f87d468d02a7 100644
--- a/security/apparmor/lib.c
+++ b/security/apparmor/lib.c
@@ -329,7 +329,7 @@ static u32 map_xbits(u32 x)
}
void aa_compute_perms(struct aa_dfa *dfa, unsigned int state,
- struct aa_perms *perms)
+ struct aa_perms *perms, u32 version)
{
/* This mapping is convulated due to history.
* v1-v4: only file perms
@@ -353,6 +353,8 @@ void aa_compute_perms(struct aa_dfa *dfa, unsigned int state,
* to extend the general perm set
*/
perms->allow |= map_other(dfa_other_allow(dfa, state));
+ if (VERSION_LE(version, v8))
+ perms->allow |= AA_MAY_LOCK;
perms->audit |= map_other(dfa_other_audit(dfa, state));
perms->quiet |= map_other(dfa_other_quiet(dfa, state));
}
diff --git a/security/apparmor/net.c b/security/apparmor/net.c
index 7efe4d17273d..83c9b1ebdcda 100644
--- a/security/apparmor/net.c
+++ b/security/apparmor/net.c
@@ -125,7 +125,8 @@ int aa_profile_af_perm(struct aa_profile *profile, struct common_audit_data *sa,
buffer[1] = cpu_to_be16((u16) type);
state = aa_dfa_match_len(profile->policy.dfa, state, (char *) &buffer,
4);
- aa_compute_perms(profile->policy.dfa, state, &perms);
+ aa_compute_perms(profile->policy.dfa, state, &perms,
+ profile->policy.version);
aa_apply_modes_to_perms(profile, &perms);
return aa_check_perms(profile, &perms, request, sa, audit_net_cb);
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 9c3fec2c7cf6..83b6e6ecda56 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -27,16 +27,6 @@
#include "include/policy.h"
#include "include/policy_unpack.h"
-#define K_ABI_MASK 0x3ff
-#define FORCE_COMPLAIN_FLAG 0x800
-#define VERSION_LT(X, Y) (((X) & K_ABI_MASK) < ((Y) & K_ABI_MASK))
-#define VERSION_GT(X, Y) (((X) & K_ABI_MASK) > ((Y) & K_ABI_MASK))
-
-#define v5 5 /* base version */
-#define v6 6 /* per entry policydb mediation check */
-#define v7 7
-#define v8 8 /* full network masking */
-
/*
* The AppArmor interface treats data as a type byte followed by the
* actual data. The interface has the notion of a named entry
@@ -857,6 +847,7 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
}
if (!unpack_nameX(e, AA_STRUCTEND, NULL))
goto fail;
+ profile->policy.version = e->version;
} else
profile->policy.dfa = aa_get_dfa(nulldfa);
--
2.34.1
Reply to: