From 5f997580e8b12b9f585e34cc16304925d26ce49e Mon Sep 17 00:00:00 2001 From: Tony Jones Date: Thu, 6 Sep 2018 21:33:57 -0700 Subject: [PATCH 01/11] apparmor: Fix network performance issue in aa_label_sk_perm The netperf benchmark shows a 5.73% reduction in throughput for small (64 byte) transfers by unconfined tasks. DEFINE_AUDIT_SK() in aa_label_sk_perm() should not be performed unconditionally, rather only when the label is confined. netperf-tcp 56974a6fc^ 56974a6fc Min 64 563.48 ( 0.00%) 531.17 ( -5.73%) Min 128 1056.92 ( 0.00%) 999.44 ( -5.44%) Min 256 1945.95 ( 0.00%) 1867.97 ( -4.01%) Min 1024 6761.40 ( 0.00%) 6364.23 ( -5.87%) Min 2048 11110.53 ( 0.00%) 10606.20 ( -4.54%) Min 3312 13692.67 ( 0.00%) 13158.41 ( -3.90%) Min 4096 14926.29 ( 0.00%) 14457.46 ( -3.14%) Min 8192 18399.34 ( 0.00%) 18091.65 ( -1.67%) Min 16384 21384.13 ( 0.00%) 21158.05 ( -1.06%) Hmean 64 564.96 ( 0.00%) 534.38 ( -5.41%) Hmean 128 1064.42 ( 0.00%) 1010.12 ( -5.10%) Hmean 256 1965.85 ( 0.00%) 1879.16 ( -4.41%) Hmean 1024 6839.77 ( 0.00%) 6478.70 ( -5.28%) Hmean 2048 11154.80 ( 0.00%) 10671.13 ( -4.34%) Hmean 3312 13838.12 ( 0.00%) 13249.01 ( -4.26%) Hmean 4096 15009.99 ( 0.00%) 14561.36 ( -2.99%) Hmean 8192 18975.57 ( 0.00%) 18326.54 ( -3.42%) Hmean 16384 21440.44 ( 0.00%) 21324.59 ( -0.54%) Stddev 64 1.24 ( 0.00%) 2.85 (-130.64%) Stddev 128 4.51 ( 0.00%) 6.53 ( -44.84%) Stddev 256 11.67 ( 0.00%) 8.50 ( 27.16%) Stddev 1024 48.33 ( 0.00%) 75.07 ( -55.34%) Stddev 2048 54.82 ( 0.00%) 65.16 ( -18.86%) Stddev 3312 153.57 ( 0.00%) 56.29 ( 63.35%) Stddev 4096 100.25 ( 0.00%) 88.50 ( 11.72%) Stddev 8192 358.13 ( 0.00%) 169.99 ( 52.54%) Stddev 16384 43.99 ( 0.00%) 141.82 (-222.39%) Signed-off-by: Tony Jones Fixes: 56974a6fcfef ("apparmor: add base infastructure for socket mediation") Signed-off-by: John Johansen --- security/apparmor/net.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/security/apparmor/net.c b/security/apparmor/net.c index bb24cfa0a164..d5d72dd1ca1f 100644 --- a/security/apparmor/net.c +++ b/security/apparmor/net.c @@ -146,17 +146,20 @@ int aa_af_perm(struct aa_label *label, const char *op, u32 request, u16 family, static int aa_label_sk_perm(struct aa_label *label, const char *op, u32 request, struct sock *sk) { - struct aa_profile *profile; - DEFINE_AUDIT_SK(sa, op, sk); + int error = 0; AA_BUG(!label); AA_BUG(!sk); - if (unconfined(label)) - return 0; + if (!unconfined(label)) { + struct aa_profile *profile; + DEFINE_AUDIT_SK(sa, op, sk); - return fn_for_each_confined(label, profile, - aa_profile_af_sk_perm(profile, &sa, request, sk)); + error = fn_for_each_confined(label, profile, + aa_profile_af_sk_perm(profile, &sa, request, sk)); + } + + return error; } int aa_sk_perm(const char *op, u32 request, struct sock *sk) From 1f8266ff58840d698a1e96d2274189de1bdf7969 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Thu, 13 Sep 2018 18:12:09 +0200 Subject: [PATCH 02/11] apparmor: don't try to replace stale label in ptrace access check As a comment above begin_current_label_crit_section() explains, begin_current_label_crit_section() must run in sleepable context because when label_is_stale() is true, aa_replace_current_label() runs, which uses prepare_creds(), which can sleep. Until now, the ptrace access check (which runs with a task lock held) violated this rule. Also add a might_sleep() assertion to begin_current_label_crit_section(), because asserts are less likely to be ignored than comments. Fixes: b2d09ae449ced ("apparmor: move ptrace checks to using labels") Signed-off-by: Jann Horn Signed-off-by: John Johansen --- security/apparmor/include/cred.h | 2 ++ security/apparmor/lsm.c | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/security/apparmor/include/cred.h b/security/apparmor/include/cred.h index e287b7d0d4be..265ae6641a06 100644 --- a/security/apparmor/include/cred.h +++ b/security/apparmor/include/cred.h @@ -151,6 +151,8 @@ static inline struct aa_label *begin_current_label_crit_section(void) { struct aa_label *label = aa_current_raw_label(); + might_sleep(); + if (label_is_stale(label)) { label = aa_get_newest_label(label); if (aa_replace_current_label(label) == 0) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 74f17376202b..f09fea0b4db7 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -114,13 +114,13 @@ static int apparmor_ptrace_access_check(struct task_struct *child, struct aa_label *tracer, *tracee; int error; - tracer = begin_current_label_crit_section(); + tracer = __begin_current_label_crit_section(); tracee = aa_get_task_label(child); error = aa_may_ptrace(tracer, tracee, (mode & PTRACE_MODE_READ) ? AA_PTRACE_READ : AA_PTRACE_TRACE); aa_put_label(tracee); - end_current_label_crit_section(tracer); + __end_current_label_crit_section(tracer); return error; } From 617a629c08bfffb05249131079d9a38322902e5b Mon Sep 17 00:00:00 2001 From: Matthew Garrett Date: Thu, 24 May 2018 13:27:45 -0700 Subject: [PATCH 03/11] apparmor: Add a wildcard secid Reserve a secid value that we can use as a wildcard, allowing us to define policy that's expected to match against all secids. Signed-off-by: Matthew Garrett Signed-off-by: John Johansen --- security/apparmor/include/secid.h | 3 +++ security/apparmor/secid.c | 3 +-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/security/apparmor/include/secid.h b/security/apparmor/include/secid.h index dee6fa3b6081..fa2062711b63 100644 --- a/security/apparmor/include/secid.h +++ b/security/apparmor/include/secid.h @@ -22,6 +22,9 @@ struct aa_label; /* secid value that will not be allocated */ #define AA_SECID_INVALID 0 +/* secid value that matches any other secid */ +#define AA_SECID_WILDCARD 1 + struct aa_label *aa_secid_to_label(u32 secid); int apparmor_secid_to_secctx(u32 secid, char **secdata, u32 *seclen); int apparmor_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid); diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c index f2f22d00db18..8c951c493beb 100644 --- a/security/apparmor/secid.c +++ b/security/apparmor/secid.c @@ -32,8 +32,7 @@ * secids - do not pin labels with a refcount. They rely on the label * properly updating/freeing them */ - -#define AA_FIRST_SECID 1 +#define AA_FIRST_SECID 2 static DEFINE_IDR(aa_secids); static DEFINE_SPINLOCK(secid_lock); From 9caafbe2b4cf4c635826a2832e93cf648605de8b Mon Sep 17 00:00:00 2001 From: Matthew Garrett Date: Thu, 24 May 2018 13:27:46 -0700 Subject: [PATCH 04/11] apparmor: Parse secmark policy Add support for parsing secmark policy provided by userspace, and store that in the overall policy. Signed-off-by: Matthew Garrett Signed-off-by: John Johansen --- security/apparmor/include/net.h | 10 +++++ security/apparmor/include/policy.h | 3 ++ security/apparmor/policy.c | 3 ++ security/apparmor/policy_unpack.c | 61 ++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+) diff --git a/security/apparmor/include/net.h b/security/apparmor/include/net.h index ec7228e857a9..7334ac966d01 100644 --- a/security/apparmor/include/net.h +++ b/security/apparmor/include/net.h @@ -83,6 +83,13 @@ struct aa_sk_ctx { __e; \ }) +struct aa_secmark { + u8 audit; + u8 deny; + u32 secid; + char *label; +}; + extern struct aa_sfs_entry aa_sfs_entry_network[]; void audit_net_cb(struct audit_buffer *ab, void *va); @@ -103,4 +110,7 @@ int aa_sk_perm(const char *op, u32 request, struct sock *sk); int aa_sock_file_perm(struct aa_label *label, const char *op, u32 request, struct socket *sock); +int apparmor_secmark_check(struct aa_label *label, char *op, u32 request, + u32 secid, struct sock *sk); + #endif /* __AA_NET_H */ diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h index ab64c6b5db5a..8e6707c837be 100644 --- a/security/apparmor/include/policy.h +++ b/security/apparmor/include/policy.h @@ -155,6 +155,9 @@ struct aa_profile { struct aa_rlimit rlimits; + int secmark_count; + struct aa_secmark *secmark; + struct aa_loaddata *rawdata; unsigned char *hash; char *dirname; diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 1590e2de4e84..8d846a747b84 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -231,6 +231,9 @@ void aa_free_profile(struct aa_profile *profile) for (i = 0; i < profile->xattr_count; i++) kzfree(profile->xattrs[i]); kzfree(profile->xattrs); + for (i=0; i < profile->secmark_count; i++) + kzfree(profile->secmark[i].label); + kzfree(profile->secmark); kzfree(profile->dirname); aa_put_dfa(profile->xmatch); aa_put_dfa(profile->policy.dfa); diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 21cb384d712a..379682e2a8d5 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -292,6 +292,19 @@ fail: return 0; } +static bool unpack_u8(struct aa_ext *e, u8 *data, const char *name) +{ + if (unpack_nameX(e, AA_U8, name)) { + if (!inbounds(e, sizeof(u8))) + return 0; + if (data) + *data = get_unaligned((u8 *)e->pos); + e->pos += sizeof(u8); + return 1; + } + return 0; +} + static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name) { if (unpack_nameX(e, AA_U32, name)) { @@ -529,6 +542,49 @@ fail: return 0; } +static bool unpack_secmark(struct aa_ext *e, struct aa_profile *profile) +{ + void *pos = e->pos; + int i, size; + + if (unpack_nameX(e, AA_STRUCT, "secmark")) { + size = unpack_array(e, NULL); + + profile->secmark = kcalloc(size, sizeof(struct aa_secmark), + GFP_KERNEL); + if (!profile->secmark) + goto fail; + + profile->secmark_count = size; + + for (i = 0; i < size; i++) { + if (!unpack_u8(e, &profile->secmark[i].audit, NULL)) + goto fail; + if (!unpack_u8(e, &profile->secmark[i].deny, NULL)) + goto fail; + if (!unpack_strdup(e, &profile->secmark[i].label, NULL)) + goto fail; + } + if (!unpack_nameX(e, AA_ARRAYEND, NULL)) + goto fail; + if (!unpack_nameX(e, AA_STRUCTEND, NULL)) + goto fail; + } + + return 1; + +fail: + if (profile->secmark) { + for (i = 0; i < size; i++) + kfree(profile->secmark[i].label); + kfree(profile->secmark); + profile->secmark_count = 0; + } + + e->pos = pos; + return 0; +} + static bool unpack_rlimits(struct aa_ext *e, struct aa_profile *profile) { void *pos = e->pos; @@ -727,6 +783,11 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name) goto fail; } + if (!unpack_secmark(e, profile)) { + info = "failed to unpack profile secmark rules"; + goto fail; + } + if (unpack_nameX(e, AA_STRUCT, "policydb")) { /* generic policy dfa - optional and may be NULL */ info = "failed to unpack policydb"; From ab9f2115081ab7ba63b77a759e0f3eb5d6463d7f Mon Sep 17 00:00:00 2001 From: Matthew Garrett Date: Thu, 24 May 2018 13:27:47 -0700 Subject: [PATCH 05/11] apparmor: Allow filtering based on secmark policy Add support for dropping or accepting packets based on their secmark tags. Signed-off-by: Matthew Garrett Signed-off-by: John Johansen --- security/apparmor/lsm.c | 112 +++++++++++++++++++++++++++++++++++++++- security/apparmor/net.c | 66 +++++++++++++++++++++++ 2 files changed, 177 insertions(+), 1 deletion(-) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index f09fea0b4db7..2c842f24821b 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -23,6 +23,8 @@ #include #include #include +#include +#include #include #include "include/apparmor.h" @@ -1030,7 +1032,13 @@ static int apparmor_socket_shutdown(struct socket *sock, int how) */ static int apparmor_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) { - return 0; + struct aa_sk_ctx *ctx = SK_CTX(sk); + + if (!skb->secmark) + return 0; + + return apparmor_secmark_check(ctx->label, OP_RECVMSG, AA_MAY_RECEIVE, + skb->secmark, sk); } @@ -1126,6 +1134,18 @@ static void apparmor_sock_graft(struct sock *sk, struct socket *parent) ctx->label = aa_get_current_label(); } +static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb, + struct request_sock *req) +{ + struct aa_sk_ctx *ctx = SK_CTX(sk); + + if (!skb->secmark) + return 0; + + return apparmor_secmark_check(ctx->label, OP_CONNECT, AA_MAY_CONNECT, + skb->secmark, sk); +} + static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check), LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme), @@ -1183,6 +1203,7 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(socket_getpeersec_dgram, apparmor_socket_getpeersec_dgram), LSM_HOOK_INIT(sock_graft, apparmor_sock_graft), + LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request), LSM_HOOK_INIT(cred_alloc_blank, apparmor_cred_alloc_blank), LSM_HOOK_INIT(cred_free, apparmor_cred_free), @@ -1538,6 +1559,95 @@ static inline int apparmor_init_sysctl(void) } #endif /* CONFIG_SYSCTL */ +static unsigned int apparmor_ip_postroute(void *priv, + struct sk_buff *skb, + const struct nf_hook_state *state) +{ + struct aa_sk_ctx *ctx; + struct sock *sk; + + if (!skb->secmark) + return NF_ACCEPT; + + sk = skb_to_full_sk(skb); + if (sk == NULL) + return NF_ACCEPT; + + ctx = SK_CTX(sk); + if (!apparmor_secmark_check(ctx->label, OP_SENDMSG, AA_MAY_SEND, + skb->secmark, sk)) + return NF_ACCEPT; + + return NF_DROP_ERR(-ECONNREFUSED); + +} + +static unsigned int apparmor_ipv4_postroute(void *priv, + struct sk_buff *skb, + const struct nf_hook_state *state) +{ + return apparmor_ip_postroute(priv, skb, state); +} + +static unsigned int apparmor_ipv6_postroute(void *priv, + struct sk_buff *skb, + const struct nf_hook_state *state) +{ + return apparmor_ip_postroute(priv, skb, state); +} + +static const struct nf_hook_ops apparmor_nf_ops[] = { + { + .hook = apparmor_ipv4_postroute, + .pf = NFPROTO_IPV4, + .hooknum = NF_INET_POST_ROUTING, + .priority = NF_IP_PRI_SELINUX_FIRST, + }, +#if IS_ENABLED(CONFIG_IPV6) + { + .hook = apparmor_ipv6_postroute, + .pf = NFPROTO_IPV6, + .hooknum = NF_INET_POST_ROUTING, + .priority = NF_IP6_PRI_SELINUX_FIRST, + }, +#endif +}; + +static int __net_init apparmor_nf_register(struct net *net) +{ + int ret; + + ret = nf_register_net_hooks(net, apparmor_nf_ops, + ARRAY_SIZE(apparmor_nf_ops)); + return ret; +} + +static void __net_exit apparmor_nf_unregister(struct net *net) +{ + nf_unregister_net_hooks(net, apparmor_nf_ops, + ARRAY_SIZE(apparmor_nf_ops)); +} + +static struct pernet_operations apparmor_net_ops = { + .init = apparmor_nf_register, + .exit = apparmor_nf_unregister, +}; + +static int __init apparmor_nf_ip_init(void) +{ + int err; + + if (!apparmor_enabled) + return 0; + + err = register_pernet_subsys(&apparmor_net_ops); + if (err) + panic("Apparmor: register_pernet_subsys: error %d\n", err); + + return 0; +} +__initcall(apparmor_nf_ip_init); + static int __init apparmor_init(void) { int error; diff --git a/security/apparmor/net.c b/security/apparmor/net.c index d5d72dd1ca1f..f9a678ce994f 100644 --- a/security/apparmor/net.c +++ b/security/apparmor/net.c @@ -18,6 +18,7 @@ #include "include/label.h" #include "include/net.h" #include "include/policy.h" +#include "include/secid.h" #include "net_names.h" @@ -188,3 +189,68 @@ int aa_sock_file_perm(struct aa_label *label, const char *op, u32 request, return aa_label_sk_perm(label, op, request, sock->sk); } + +static int apparmor_secmark_init(struct aa_secmark *secmark) +{ + struct aa_label *label; + + if (secmark->label[0] == '*') { + secmark->secid = AA_SECID_WILDCARD; + return 0; + } + + label = aa_label_strn_parse(&root_ns->unconfined->label, + secmark->label, strlen(secmark->label), + GFP_ATOMIC, false, false); + + if (IS_ERR(label)) + return PTR_ERR(label); + + secmark->secid = label->secid; + + return 0; +} + +static int aa_secmark_perm(struct aa_profile *profile, u32 request, u32 secid, + struct common_audit_data *sa, struct sock *sk) +{ + int i, ret; + struct aa_perms perms = { }; + + if (profile->secmark_count == 0) + return 0; + + for (i = 0; i < profile->secmark_count; i++) { + if (!profile->secmark[i].secid) { + ret = apparmor_secmark_init(&profile->secmark[i]); + if (ret) + return ret; + } + + if (profile->secmark[i].secid == secid || + profile->secmark[i].secid == AA_SECID_WILDCARD) { + if (profile->secmark[i].deny) + perms.deny = ALL_PERMS_MASK; + else + perms.allow = ALL_PERMS_MASK; + + if (profile->secmark[i].audit) + perms.audit = ALL_PERMS_MASK; + } + } + + aa_apply_modes_to_perms(profile, &perms); + + return aa_check_perms(profile, &perms, request, sa, audit_net_cb); +} + +int apparmor_secmark_check(struct aa_label *label, char *op, u32 request, + u32 secid, struct sock *sk) +{ + struct aa_profile *profile; + DEFINE_AUDIT_SK(sa, op, sk); + + return fn_for_each_confined(label, profile, + aa_secmark_perm(profile, request, secid, + &sa, sk)); +} From 0fb871cc42537465e322f727bec6abfd375faa83 Mon Sep 17 00:00:00 2001 From: Lance Roy Date: Tue, 2 Oct 2018 22:39:01 -0700 Subject: [PATCH 06/11] apparmor: Replace spin_is_locked() with lockdep lockdep_assert_held() is better suited to checking locking requirements, since it won't get confused when someone else holds the lock. This is also a step towards possibly removing spin_is_locked(). Signed-off-by: Lance Roy Cc: John Johansen Cc: James Morris Cc: "Serge E. Hallyn" Cc: Signed-off-by: John Johansen --- security/apparmor/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index 4285943f7260..d0afed9ebd0e 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -496,7 +496,7 @@ static void update_file_ctx(struct aa_file_ctx *fctx, struct aa_label *label, /* update caching of label on file_ctx */ spin_lock(&fctx->lock); old = rcu_dereference_protected(fctx->label, - spin_is_locked(&fctx->lock)); + lockdep_is_held(&fctx->lock)); l = aa_label_merge(old, label, GFP_ATOMIC); if (l) { if (l != old) { From ca3fde5214e1d24f78269b337d3f22afd6bf445e Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Sat, 29 Sep 2018 03:49:26 +0200 Subject: [PATCH 07/11] apparmor: don't try to replace stale label in ptraceme check begin_current_label_crit_section() must run in sleepable context because when label_is_stale() is true, aa_replace_current_label() runs, which uses prepare_creds(), which can sleep. Until now, the ptraceme access check (which runs with tasklist_lock held) violated this rule. Fixes: b2d09ae449ced ("apparmor: move ptrace checks to using labels") Reported-by: Cyrill Gorcunov Reported-by: kernel test robot Signed-off-by: Jann Horn Signed-off-by: John Johansen --- security/apparmor/lsm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 2c842f24821b..d08aac05c65a 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -132,11 +132,11 @@ static int apparmor_ptrace_traceme(struct task_struct *parent) struct aa_label *tracer, *tracee; int error; - tracee = begin_current_label_crit_section(); + tracee = __begin_current_label_crit_section(); tracer = aa_get_task_label(parent); error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE); aa_put_label(tracer); - end_current_label_crit_section(tracee); + __end_current_label_crit_section(tracee); return error; } From 250f2da49cb8e582215a65c03f50e8ddf5cd119c Mon Sep 17 00:00:00 2001 From: Zubin Mithra Date: Thu, 27 Sep 2018 14:49:17 -0700 Subject: [PATCH 08/11] apparmor: Fix uninitialized value in aa_split_fqname Syzkaller reported a OOB-read with the stacktrace below. This occurs inside __aa_lookupn_ns as `n` is not initialized. `n` is obtained from aa_splitn_fqname. In cases where `name` is invalid, aa_splitn_fqname returns without initializing `ns_name` and `ns_len`. Fix this by always initializing `ns_name` and `ns_len`. __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113 print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256 kasan_report_error mm/kasan/report.c:354 [inline] kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412 __asan_report_load1_noabort+0x14/0x20 mm/kasan/report.c:430 memcmp+0xe3/0x160 lib/string.c:861 strnstr+0x4b/0x70 lib/string.c:934 __aa_lookupn_ns+0xc1/0x570 security/apparmor/policy_ns.c:209 aa_lookupn_ns+0x88/0x1e0 security/apparmor/policy_ns.c:240 aa_fqlookupn_profile+0x1b9/0x1010 security/apparmor/policy.c:468 fqlookupn_profile+0x80/0xc0 security/apparmor/label.c:1844 aa_label_strn_parse+0xa3a/0x1230 security/apparmor/label.c:1908 aa_label_parse+0x42/0x50 security/apparmor/label.c:1943 aa_change_profile+0x513/0x3510 security/apparmor/domain.c:1362 apparmor_setprocattr+0xaa4/0x1150 security/apparmor/lsm.c:658 security_setprocattr+0x66/0xc0 security/security.c:1298 proc_pid_attr_write+0x301/0x540 fs/proc/base.c:2555 __vfs_write+0x119/0x9f0 fs/read_write.c:485 vfs_write+0x1fc/0x560 fs/read_write.c:549 ksys_write+0x101/0x260 fs/read_write.c:598 __do_sys_write fs/read_write.c:610 [inline] __se_sys_write fs/read_write.c:607 [inline] __x64_sys_write+0x73/0xb0 fs/read_write.c:607 do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290 entry_SYSCALL_64_after_hwframe+0x49/0xbe Fixes: 3b0aaf5866bf ("apparmor: add lib fn to find the "split" for fqnames") Reported-by: syzbot+61e4b490d9d2da591b50@syzkaller.appspotmail.com Signed-off-by: Zubin Mithra Reviewed-by: Kees Cook Signed-off-by: John Johansen --- security/apparmor/lib.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index 974affe50531..76491e7f4177 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -90,10 +90,12 @@ const char *aa_splitn_fqname(const char *fqname, size_t n, const char **ns_name, const char *end = fqname + n; const char *name = skipn_spaces(fqname, n); - if (!name) - return NULL; *ns_name = NULL; *ns_len = 0; + + if (!name) + return NULL; + if (name[0] == ':') { char *split = strnchr(&name[1], end - &name[1], ':'); *ns_name = skipn_spaces(&name[1], end - &name[1]); From e1af4779617928efa84562de4de5dc071e7deb08 Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Fri, 5 Oct 2018 18:11:47 +0200 Subject: [PATCH 09/11] apparmor: add #ifdef checks for secmark filtering The newly added code fails to build when either SECMARK or NETFILTER are disabled: security/apparmor/lsm.c: In function 'apparmor_socket_sock_rcv_skb': security/apparmor/lsm.c:1138:12: error: 'struct sk_buff' has no member named 'secmark'; did you mean 'mark'? security/apparmor/lsm.c:1671:21: error: 'struct nf_hook_state' declared inside parameter list will not be visible outside of this definition or declaration [-Werror] Add a set of #ifdef checks around it to only enable the code that we can compile and that makes sense in that configuration. Fixes: ab9f2115081a ("apparmor: Allow filtering based on secmark policy") Signed-off-by: Arnd Bergmann Signed-off-by: John Johansen --- security/apparmor/lsm.c | 10 ++++++++++ security/apparmor/net.c | 2 ++ 2 files changed, 12 insertions(+) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index d08aac05c65a..656a143ce8fe 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -1022,6 +1022,7 @@ static int apparmor_socket_shutdown(struct socket *sock, int how) return aa_sock_perm(OP_SHUTDOWN, AA_MAY_SHUTDOWN, sock); } +#ifdef CONFIG_NETWORK_SECMARK /** * apparmor_socket_sock_recv_skb - check perms before associating skb to sk * @@ -1040,6 +1041,7 @@ static int apparmor_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) return apparmor_secmark_check(ctx->label, OP_RECVMSG, AA_MAY_RECEIVE, skb->secmark, sk); } +#endif static struct aa_label *sk_peer_label(struct sock *sk) @@ -1134,6 +1136,7 @@ static void apparmor_sock_graft(struct sock *sk, struct socket *parent) ctx->label = aa_get_current_label(); } +#ifdef CONFIG_NETWORK_SECMARK static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb, struct request_sock *req) { @@ -1145,6 +1148,7 @@ static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb, return apparmor_secmark_check(ctx->label, OP_CONNECT, AA_MAY_CONNECT, skb->secmark, sk); } +#endif static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check), @@ -1197,13 +1201,17 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(socket_getsockopt, apparmor_socket_getsockopt), LSM_HOOK_INIT(socket_setsockopt, apparmor_socket_setsockopt), LSM_HOOK_INIT(socket_shutdown, apparmor_socket_shutdown), +#ifdef CONFIG_NETWORK_SECMARK LSM_HOOK_INIT(socket_sock_rcv_skb, apparmor_socket_sock_rcv_skb), +#endif LSM_HOOK_INIT(socket_getpeersec_stream, apparmor_socket_getpeersec_stream), LSM_HOOK_INIT(socket_getpeersec_dgram, apparmor_socket_getpeersec_dgram), LSM_HOOK_INIT(sock_graft, apparmor_sock_graft), +#ifdef CONFIG_NETWORK_SECMARK LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request), +#endif LSM_HOOK_INIT(cred_alloc_blank, apparmor_cred_alloc_blank), LSM_HOOK_INIT(cred_free, apparmor_cred_free), @@ -1559,6 +1567,7 @@ static inline int apparmor_init_sysctl(void) } #endif /* CONFIG_SYSCTL */ +#if defined(CONFIG_NETFILTER) && defined(CONFIG_NETWORK_SECMARK) static unsigned int apparmor_ip_postroute(void *priv, struct sk_buff *skb, const struct nf_hook_state *state) @@ -1647,6 +1656,7 @@ static int __init apparmor_nf_ip_init(void) return 0; } __initcall(apparmor_nf_ip_init); +#endif static int __init apparmor_init(void) { diff --git a/security/apparmor/net.c b/security/apparmor/net.c index f9a678ce994f..c07fde444792 100644 --- a/security/apparmor/net.c +++ b/security/apparmor/net.c @@ -190,6 +190,7 @@ int aa_sock_file_perm(struct aa_label *label, const char *op, u32 request, return aa_label_sk_perm(label, op, request, sock->sk); } +#ifdef CONFIG_NETWORK_SECMARK static int apparmor_secmark_init(struct aa_secmark *secmark) { struct aa_label *label; @@ -254,3 +255,4 @@ int apparmor_secmark_check(struct aa_label *label, char *op, u32 request, aa_secmark_perm(profile, request, secid, &sa, sk)); } +#endif From 76af016e65e74023a11b2710e18a98b68b86e3cd Mon Sep 17 00:00:00 2001 From: John Johansen Date: Thu, 1 Nov 2018 22:28:17 -0700 Subject: [PATCH 10/11] apparmor: fix checkpatch error in Parse secmark policy Fix missed spacing error reported by checkpatch for 9caafbe2b4cf ("Parse secmark policy") Signed-off-by: John Johansen --- security/apparmor/policy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 8d846a747b84..df9c5890a878 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -231,7 +231,7 @@ void aa_free_profile(struct aa_profile *profile) for (i = 0; i < profile->xattr_count; i++) kzfree(profile->xattrs[i]); kzfree(profile->xattrs); - for (i=0; i < profile->secmark_count; i++) + for (i = 0; i < profile->secmark_count; i++) kzfree(profile->secmark[i].label); kzfree(profile->secmark); kzfree(profile->dirname); From 566f52ece7bd1099d20dfe2f6f0801896643cf8f Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Tue, 30 Oct 2018 14:11:31 +0000 Subject: [PATCH 11/11] apparmor: clean an indentation issue, remove extraneous space Trivial fix to clean up an indentation issue, remove space Signed-off-by: Colin Ian King Signed-off-by: John Johansen --- security/apparmor/apparmorfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index e09fe4d7307c..8963203319ea 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -1742,7 +1742,7 @@ static int ns_rmdir_op(struct inode *dir, struct dentry *dentry) if (error) return error; - parent = aa_get_ns(dir->i_private); + parent = aa_get_ns(dir->i_private); /* rmdir calls the generic securityfs functions to remove files * from the apparmor dir. It is up to the apparmor ns locking * to avoid races.