From 7f3ebcf2b1395e0248e56146041e1e5625fd2f23 Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Fri, 6 Jul 2018 05:25:00 +0000 Subject: [PATCH 1/5] apparmor: Check buffer bounds when mapping permissions mask Don't read past the end of the buffer containing permissions characters or write past the end of the destination string. Detected by CoverityScan CID#1415361, 1415376 ("Out-of-bounds access") Fixes: e53cfe6c7caa ("apparmor: rework perm mapping to a slightly broader set") Signed-off-by: Tyler Hicks Acked-by: Serge Hallyn Signed-off-by: John Johansen --- security/apparmor/file.c | 3 ++- security/apparmor/include/perms.h | 3 ++- security/apparmor/lib.c | 17 +++++++++++++---- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index 224b2fef93ca..4285943f7260 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -47,7 +47,8 @@ static void audit_file_mask(struct audit_buffer *ab, u32 mask) { char str[10]; - aa_perm_mask_to_str(str, aa_file_perm_chrs, map_mask_to_chr_mask(mask)); + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, + map_mask_to_chr_mask(mask)); audit_log_string(ab, str); } diff --git a/security/apparmor/include/perms.h b/security/apparmor/include/perms.h index 38aa6247d00f..b94ec114d1a4 100644 --- a/security/apparmor/include/perms.h +++ b/security/apparmor/include/perms.h @@ -137,7 +137,8 @@ extern struct aa_perms allperms; xcheck(fn_for_each((L1), (P), (FN1)), fn_for_each((L2), (P), (FN2))) -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask); +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, + u32 mask); void aa_audit_perm_names(struct audit_buffer *ab, const char * const *names, u32 mask); void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index a7b3f681b80e..974affe50531 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -198,15 +198,24 @@ const char *aa_file_perm_names[] = { /** * aa_perm_mask_to_str - convert a perm mask to its short string * @str: character buffer to store string in (at least 10 characters) + * @str_size: size of the @str buffer + * @chrs: NUL-terminated character buffer of permission characters * @mask: permission mask to convert */ -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask) +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, u32 mask) { unsigned int i, perm = 1; + size_t num_chrs = strlen(chrs); + + for (i = 0; i < num_chrs; perm <<= 1, i++) { + if (mask & perm) { + /* Ensure that one byte is left for NUL-termination */ + if (WARN_ON_ONCE(str_size <= 1)) + break; - for (i = 0; i < 32; perm <<= 1, i++) { - if (mask & perm) *str++ = chrs[i]; + str_size--; + } } *str = '\0'; } @@ -236,7 +245,7 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, audit_log_format(ab, "\""); if ((mask & chrsmask) && chrs) { - aa_perm_mask_to_str(str, chrs, mask & chrsmask); + aa_perm_mask_to_str(str, sizeof(str), chrs, mask & chrsmask); mask &= ~chrsmask; audit_log_format(ab, "%s", str); if (mask & namesmask) From f4585bc20fc785d94192cf780ee925e25891ddfd Mon Sep 17 00:00:00 2001 From: Tyler Hicks Date: Fri, 6 Jul 2018 05:25:01 +0000 Subject: [PATCH 2/5] apparmor: Fully initialize aa_perms struct when answering userspace query Fully initialize the aa_perms struct in profile_query_cb() to avoid the potential of using an uninitialized struct member's value in a response to a query from userspace. Detected by CoverityScan CID#1415126 ("Uninitialized scalar variable") Fixes: 4f3b3f2d79a4 ("apparmor: add profile permission query ability") Signed-off-by: Tyler Hicks Acked-by: Serge Hallyn Signed-off-by: John Johansen --- security/apparmor/apparmorfs.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 949dd8a48164..e09fe4d7307c 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -603,7 +603,7 @@ static const struct file_operations aa_fs_ns_revision_fops = { static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, const char *match_str, size_t match_len) { - struct aa_perms tmp; + struct aa_perms tmp = { }; struct aa_dfa *dfa; unsigned int state = 0; @@ -613,7 +613,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, dfa = profile->file.dfa; state = aa_dfa_match_len(dfa, profile->file.start, match_str + 1, match_len - 1); - tmp = nullperms; if (state) { struct path_cond cond = { }; @@ -627,8 +626,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, match_str, match_len); if (state) aa_compute_perms(dfa, state, &tmp); - else - tmp = nullperms; } aa_apply_modes_to_perms(profile, &tmp); aa_perms_accum_raw(perms, &tmp); From 24b87a16fee0ebd49b66e8523186ace7ea09ad12 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 20 Jul 2018 03:25:25 -0700 Subject: [PATCH 3/5] apparmor: Fix failure to audit context info in build_change_hat Cleans up clang warning: warning: variable 'info' set but not used [-Wunused-but-set-variable] Fixes: 89dbf1962aa63 ("apparmor: move change_hat mediation to using labels") Reported-by: Colin Ian King Signed-off-by: John Johansen --- security/apparmor/domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 098d546d8253..08c88de0ffda 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -1036,7 +1036,7 @@ static struct aa_label *build_change_hat(struct aa_profile *profile, audit: aa_audit_file(profile, &nullperms, OP_CHANGE_HAT, AA_MAY_CHANGEHAT, name, hat ? hat->base.hname : NULL, - hat ? &hat->label : NULL, GLOBAL_ROOT_UID, NULL, + hat ? &hat->label : NULL, GLOBAL_ROOT_UID, info, error); if (!hat || (error && error != -ENOENT)) return ERR_PTR(error); From 0a6b29230ec336189bab32498df3f06c8a6944d8 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Thu, 2 Aug 2018 11:38:23 +0300 Subject: [PATCH 4/5] apparmor: fix an error code in __aa_create_ns() We should return error pointers in this function. Returning NULL results in a NULL dereference in the caller. Fixes: 73688d1ed0b8 ("apparmor: refactor prepare_ns() and make usable from different views") Signed-off-by: Dan Carpenter Signed-off-by: John Johansen --- security/apparmor/policy_ns.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/apparmor/policy_ns.c b/security/apparmor/policy_ns.c index b0f9dc3f765a..1a7cec5d9cac 100644 --- a/security/apparmor/policy_ns.c +++ b/security/apparmor/policy_ns.c @@ -255,7 +255,7 @@ static struct aa_ns *__aa_create_ns(struct aa_ns *parent, const char *name, ns = alloc_ns(parent->base.hname, name); if (!ns) - return NULL; + return ERR_PTR(-ENOMEM); ns->level = parent->level + 1; mutex_lock_nested(&ns->lock, ns->level); error = __aafs_ns_mkdir(ns, ns_subns_dir(parent), name, dir); From c037bd615885f1d9d3bdb48531bace79fae1505d Mon Sep 17 00:00:00 2001 From: John Johansen Date: Tue, 21 Aug 2018 17:19:53 -0700 Subject: [PATCH 5/5] apparmor: remove no-op permission check in policy_unpack The patch 736ec752d95e: "AppArmor: policy routines for loading and unpacking policy" from Jul 29, 2010, leads to the following static checker warning: security/apparmor/policy_unpack.c:410 verify_accept() warn: bitwise AND condition is false here security/apparmor/policy_unpack.c:413 verify_accept() warn: bitwise AND condition is false here security/apparmor/policy_unpack.c 392 #define DFA_VALID_PERM_MASK 0xffffffff 393 #define DFA_VALID_PERM2_MASK 0xffffffff 394 395 /** 396 * verify_accept - verify the accept tables of a dfa 397 * @dfa: dfa to verify accept tables of (NOT NULL) 398 * @flags: flags governing dfa 399 * 400 * Returns: 1 if valid accept tables else 0 if error 401 */ 402 static bool verify_accept(struct aa_dfa *dfa, int flags) 403 { 404 int i; 405 406 /* verify accept permissions */ 407 for (i = 0; i < dfa->tables[YYTD_ID_ACCEPT]->td_lolen; i++) { 408 int mode = ACCEPT_TABLE(dfa)[i]; 409 410 if (mode & ~DFA_VALID_PERM_MASK) 411 return 0; 412 413 if (ACCEPT_TABLE2(dfa)[i] & ~DFA_VALID_PERM2_MASK) 414 return 0; fixes: 736ec752d95e ("AppArmor: policy routines for loading and unpacking policy") Reported-by: Dan Carpenter Signed-off-by: John Johansen --- security/apparmor/policy_unpack.c | 32 ------------------------------- 1 file changed, 32 deletions(-) diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 0e566a01d217..21cb384d712a 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -389,32 +389,6 @@ static int unpack_strdup(struct aa_ext *e, char **string, const char *name) return res; } -#define DFA_VALID_PERM_MASK 0xffffffff -#define DFA_VALID_PERM2_MASK 0xffffffff - -/** - * verify_accept - verify the accept tables of a dfa - * @dfa: dfa to verify accept tables of (NOT NULL) - * @flags: flags governing dfa - * - * Returns: 1 if valid accept tables else 0 if error - */ -static bool verify_accept(struct aa_dfa *dfa, int flags) -{ - int i; - - /* verify accept permissions */ - for (i = 0; i < dfa->tables[YYTD_ID_ACCEPT]->td_lolen; i++) { - int mode = ACCEPT_TABLE(dfa)[i]; - - if (mode & ~DFA_VALID_PERM_MASK) - return 0; - - if (ACCEPT_TABLE2(dfa)[i] & ~DFA_VALID_PERM2_MASK) - return 0; - } - return 1; -} /** * unpack_dfa - unpack a file rule dfa @@ -445,15 +419,9 @@ static struct aa_dfa *unpack_dfa(struct aa_ext *e) if (IS_ERR(dfa)) return dfa; - if (!verify_accept(dfa, flags)) - goto fail; } return dfa; - -fail: - aa_put_dfa(dfa); - return ERR_PTR(-EPROTO); } /**