This series is intended to fix the supposedly invalid behaviour of
verify_header() function when checking unpacked profiles namespace
declarations.
Consider having a profiles file called 'testfile' with contents as
profile :ns1:profile1 ... {}
profile :ns2:profile2 ... {}
then packing it in a binary format and trying a load-replace operation
$ apparmor_parser -Q -W -T testfile
$ apparmor_parser -r -B /var/cache/apparmor/ac27e0ee.0/testfile
Per dmesg output this leads the profiles to be loaded into the same
namespace (trimmed for more convenient display):
[ 414.616098] audit: apparmor="STATUS" operation="profile_load" name="profile1" comm="apparmor_parser" ns="ns2"
[ 414.619304] audit: apparmor="STATUS" operation="profile_load" name="profile2" comm="apparmor_parser" ns="ns2"
while the string "ns1" unpacked inside verify_header() is leaked:
unreferenced object 0xffff888012ff9b18 (size 8):
comm "apparmor_parser", pid 1198, jiffies 4295081077 (age 164.892s)
hex dump (first 8 bytes):
6e 73 31 00 80 88 ff ff ns1.....
backtrace:
[<ffffffff81ddb4b2>] __kmem_cache_alloc_node+0x1e2/0x2d0
[<ffffffff81c47034>] __kmalloc_node_track_caller+0x54/0x170
[<ffffffff81c224dc>] kstrdup+0x3c/0x70
[<ffffffff83e2e4ef>] aa_unpack+0xb5f/0x1540
[<ffffffff83e1e9c6>] aa_replace_profiles+0x1f6/0x4040
[<ffffffff83def008>] policy_update+0x238/0x350
[<ffffffff83def33e>] profile_replace+0x20e/0x2a0
[<ffffffff81ead03f>] vfs_write+0x2af/0xe00
[<ffffffff81eae556>] ksys_write+0x126/0x250
[<ffffffff88f54426>] do_syscall_64+0x46/0xf0
[<ffffffff890000ea>] entry_SYSCALL_64_after_hwframe+0x6e/0x76
With the following patches this situation is detected and the whole
profiles load set is denied because of the mixed namespaces.
Note that the following multiple profiles load will also fail after these
patches - quite similar to the namespace check inside aa_replace_profiles().
profile profile1 ... {}
profile :ns:profile2 ... {}
This behaviour may directly affect userspace part of AppArmor, though I've
not been able to find any valid use case when e.g. the user writes
profile profile1 ... {}
profile :ns:profile2 ... {}
and expects `profile1` to be associated with `ns`. I think it's actually
an invalid expectation so such profiles set should be also denied with a
'mixed namespaces' explaining message.
There is no difference in AppArmor regression tests with and without the
changes.
The current `required` flag indicates the packed data start thus requiring
the header to be unpacked at this position.
For the purposes of verify_header() refinement, rename that flag to
`start` so that it can be used with this meaning in other part of the
function.
Found by Linux Verification Center (linuxtesting.org).
Signed-off-by: Fedor Pchelkin <[email protected]>
---
security/apparmor/policy_unpack.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index a91b30100b77..54f7b4346506 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -1111,12 +1111,12 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
/**
* verify_header - unpack serialized stream header
* @e: serialized data read head (NOT NULL)
- * @required: whether the header is required or optional
+ * @start: whether the header is located at the start of data
* @ns: Returns - namespace if one is specified else NULL (NOT NULL)
*
* Returns: error or 0 if header is good
*/
-static int verify_header(struct aa_ext *e, int required, const char **ns)
+static int verify_header(struct aa_ext *e, int start, const char **ns)
{
int error = -EPROTONOSUPPORT;
const char *name = NULL;
@@ -1124,7 +1124,8 @@ static int verify_header(struct aa_ext *e, int required, const char **ns)
/* get the interface version */
if (!aa_unpack_u32(e, &e->version, "version")) {
- if (required) {
+ /* the header is required at the start of data */
+ if (start) {
audit_iface(NULL, NULL, NULL, "invalid profile format",
e, error);
return error;
--
2.43.0
Per commit 04dc715e24d0 ("apparmor: audit policy ns specified in policy
load"), profiles in a single load must specify the same namespace. It is
supposed to be detected inside aa_replace_profiles() and verify_header(),
but seems not to be implemented correctly in the second function.
Consider we have the following profile load
profile :ns1:profile1 ... {}
profile :ns2:profile2 ... {}
The profiles specify different policy namespaces but if they are loaded as
a single binary where the serialized stream headers contain different
namespace values, it eventually leads to the profiles specified with
different namespaces be included into the same one without any specific
indication to user.
*ns is assigned NULL in verify_header(), and the branch indicating an
"invalid ns change" message is a dead code.
Moreover, some of *ns strings is leaked as they are allocated every time
verify_header() reads a namespace string.
Actually, *ns is already assigned NULL in aa_unpack(), before the multiple
profiles unpacking loop. So make verify_header() check if every new
unpacked namespace declaration differs from the previous one in the same
load.
Note that similar to the namespace check in aa_replace_profiles(), if
one profile in the load specifies some namespace declaration in the header
and the other one doesn't specify any namespace in the header - it is also
considered invalid, e.g. the following multiple profiles load will fail
after this patch
profile profile1 ... {}
profile :ns:profile2 ... {}
Found by Linux Verification Center (linuxtesting.org).
Fixes: dd51c8485763 ("apparmor: provide base for multiple profiles to be replaced at once")
Signed-off-by: Fedor Pchelkin <[email protected]>
---
security/apparmor/policy_unpack.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 54f7b4346506..5bd8ab7f1c88 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -1120,7 +1120,6 @@ static int verify_header(struct aa_ext *e, int start, const char **ns)
{
int error = -EPROTONOSUPPORT;
const char *name = NULL;
- *ns = NULL;
/* get the interface version */
if (!aa_unpack_u32(e, &e->version, "version")) {
@@ -1142,20 +1141,35 @@ static int verify_header(struct aa_ext *e, int start, const char **ns)
return error;
}
- /* read the namespace if present */
+ /* read the namespace if present and check it against policy load
+ * namespace specified in the data start header
+ */
if (aa_unpack_str(e, &name, "namespace")) {
if (*name == '\0') {
audit_iface(NULL, NULL, NULL, "invalid namespace name",
e, error);
return error;
}
+
+ /* don't allow different namespaces be specified in the same
+ * policy load set
+ */
+ error = -EACCES;
if (*ns && strcmp(*ns, name)) {
- audit_iface(NULL, NULL, NULL, "invalid ns change", e,
+ audit_iface(NULL, NULL, NULL,
+ "policy load has mixed namespaces", e,
error);
- } else if (!*ns) {
+ return error;
+ } else if (!*ns && start) {
+ /* fill current policy load namespace at data start */
*ns = kstrdup(name, GFP_KERNEL);
if (!*ns)
return -ENOMEM;
+ } else if (!*ns) {
+ audit_iface(NULL, NULL, NULL,
+ "policy load has mixed namespaces", e,
+ error);
+ return error;
}
}
--
2.43.0