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 <[email protected]> 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 <[email protected]> --- 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 <[email protected]> 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 <[email protected]> --- 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 <[email protected]> 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 <[email protected]> --- 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
signature.asc
Description: OpenPGP digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
