[apparmor] [PATCH]: unpack_*() functions should reset e->pos on failure
Mike Salvatore
mike.salvatore at canonical.com
Tue Apr 9 05:25:45 UTC 2019
John,
Below are some patches for bug that is duplicated across many of the unpack_*()
funtions in security/apparmor/policy_unpack.c.
Each function that manipulates the aa_ext struct should reset it's "pos"
member on failure. This ensures that, on failure, no changes are made to
the state of the aa_ext struct. The unpack_*() functions were not resetting
e->pos when the call to inbounds() returned false.
The below patches supersede the patches I previously sent you. Since unpack_u8()
is not yet present on all apparmor branches, I have split it out into its own
commit to avoid future merge conflicts.
Regards,
Mike Salvatore
From e0df2ee532292b4c18c2f2dac695f327b9feccbb Mon Sep 17 00:00:00 2001
From: Mike Salvatore <mike.salvatore at canonical.com>
Date: Fri, 1 Feb 2019 12:35:16 -0500
Subject: [PATCH 1/3] apparmor: reset pos on failure of unpack_u16_chunk() or
unpack_str()
Each function that manipulates the aa_ext struct should reset it's "pos"
member on failure. This ensures that, on failure, no changes are made to
the state of the aa_ext struct.
A bug was found in unpack_str() where if the call to unpack_u16_chunk()
failed, the call unpack_str() call would fail but not reset the state of
the aa_ext struct. It was also found that unpack_u16_chunk() does not
reset the state of the aa_ext struct if it fails. Both unpack_u16_chunk()
and unpack_str() were modified so that the "pos" member of the aa_ext
struct is reset on failure.
Signed-off-by: Mike Salvatore <mike.salvatore at canonical.com>
---
security/apparmor/policy_unpack.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 379682e2a8d5..5b9d8efed8e2 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -223,16 +223,21 @@ static void *kvmemdup(const void *src, size_t len)
static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
{
size_t size = 0;
+ void *pos = e->pos;
if (!inbounds(e, sizeof(u16)))
- return 0;
+ goto fail;
size = le16_to_cpu(get_unaligned((__le16 *) e->pos));
e->pos += sizeof(__le16);
if (!inbounds(e, size))
- return 0;
+ goto fail;
*chunk = e->pos;
e->pos += size;
return size;
+
+fail:
+ e->pos = pos;
+ return 0;
}
/* unpack control byte */
@@ -374,9 +379,10 @@ static int unpack_str(struct aa_ext *e, const char **string, const char *name)
if (src_str[size - 1] != 0)
goto fail;
*string = src_str;
+
+ return size;
}
}
- return size;
fail:
e->pos = pos;
--
2.17.1
From b941db066b4ffe51a121f6322c197b095b8dbd8d Mon Sep 17 00:00:00 2001
From: Mike Salvatore <mike.salvatore at canonical.com>
Date: Thu, 7 Feb 2019 15:52:27 -0500
Subject: [PATCH 2/3] apparmor: reset pos on failure to unpack for various
functions
Each function that manipulates the aa_ext struct should reset it's "pos"
member on failure. This ensures that, on failure, no changes are made to
the state of the aa_ext struct.
A bug was found in unpack_u32(), unpack_u64(), unpack_array(), and
unpack_blob() where if the call to inbounds() fails, the pos member of
the aa_ext struct is not reset. All of the aforementioned functions have
been updated so that on failure the pos member of the aa_ext struct is
reset.
Signed-off-by: Mike Salvatore <mike.salvatore at canonical.com>
---
security/apparmor/policy_unpack.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 5b9d8efed8e2..c2252e2bbddb 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -312,49 +312,66 @@ static bool unpack_u8(struct aa_ext *e, u8 *data, const char *name)
static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
{
+ void *pos = e->pos;
+
if (unpack_nameX(e, AA_U32, name)) {
if (!inbounds(e, sizeof(u32)))
- return 0;
+ goto fail;
if (data)
*data = le32_to_cpu(get_unaligned((__le32 *) e->pos));
e->pos += sizeof(u32);
return 1;
}
+
+fail:
+ e->pos = pos;
return 0;
}
static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
{
+ void *pos = e->pos;
+
if (unpack_nameX(e, AA_U64, name)) {
if (!inbounds(e, sizeof(u64)))
- return 0;
+ goto fail;
if (data)
*data = le64_to_cpu(get_unaligned((__le64 *) e->pos));
e->pos += sizeof(u64);
return 1;
}
+
+fail:
+ e->pos = pos;
return 0;
}
static size_t unpack_array(struct aa_ext *e, const char *name)
{
+ void *pos = e->pos;
+
if (unpack_nameX(e, AA_ARRAY, name)) {
int size;
if (!inbounds(e, sizeof(u16)))
- return 0;
+ goto fail;
size = (int)le16_to_cpu(get_unaligned((__le16 *) e->pos));
e->pos += sizeof(u16);
return size;
}
+
+fail:
+ e->pos = pos;
return 0;
}
static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
{
+ void *pos = e->pos;
+
if (unpack_nameX(e, AA_BLOB, name)) {
u32 size;
if (!inbounds(e, sizeof(u32)))
- return 0;
+ goto fail;
size = le32_to_cpu(get_unaligned((__le32 *) e->pos));
e->pos += sizeof(u32);
if (inbounds(e, (size_t) size)) {
@@ -363,6 +380,9 @@ static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
return size;
}
}
+
+fail:
+ e->pos = pos;
return 0;
}
--
2.17.1
From 96d4ec56211b6e0ab4f6870ba1df5243819f860e Mon Sep 17 00:00:00 2001
From: Mike Salvatore <mike.salvatore at canonical.com>
Date: Tue, 9 Apr 2019 00:47:12 -0400
Subject: [PATCH 3/3] apparmor: reset pos on failure of unpack_u8()
Each function that manipulates the aa_ext struct should reset it's "pos"
member on failure. This ensures that, on failure, no changes are made to
the state of the aa_ext struct.
A bug was found in unpack_u8() where if the call to inbounds() fails,
the pos member of the aa_ext struct is not reset. All of the
aforementioned functions have been updated so that on failure the pos
member of the aa_ext struct is reset.
Signed-off-by: Mike Salvatore <mike.salvatore at canonical.com>
---
security/apparmor/policy_unpack.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index c2252e2bbddb..593aaab23ba2 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -299,14 +299,19 @@ static bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name)
static bool unpack_u8(struct aa_ext *e, u8 *data, const char *name)
{
+ void *pos = e->pos;
+
if (unpack_nameX(e, AA_U8, name)) {
if (!inbounds(e, sizeof(u8)))
- return 0;
+ goto fail;
if (data)
*data = get_unaligned((u8 *)e->pos);
e->pos += sizeof(u8);
return 1;
}
+
+fail:
+ e->pos = pos;
return 0;
}
--
2.17.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20190409/2b3e5177/attachment.sig>
More information about the AppArmor
mailing list