From 7ea59202db8d20806d9ae552acd1875c3a978bcc Mon Sep 17 00:00:00 2001 From: Stephen Smalley Date: Mon, 23 May 2016 10:54:11 -0400 Subject: selinux: Only apply bounds checking to source types The current bounds checking of both source and target types requires allowing any domain that has access to the child domain to also have the same permissions to the parent, which is undesirable. Drop the target bounds checking. KaiGai Kohei originally removed all use of target bounds in commit 7d52a155e38d ("selinux: remove dead code in type_attribute_bounds_av()") but this was reverted in commit 2ae3ba39389b ("selinux: libsepol: remove dead code in check_avtab_hierarchy_callback()") because it would have required explicitly allowing the parent any permissions to the child that the child is allowed to itself. This change in contrast retains the logic for the case where both source and target types are bounded, thereby allowing access if the parent of the source is allowed the corresponding permissions to the parent of the target. Further, this change reworks the logic such that we only perform a single computation for each case and there is no ambiguity as to how to resolve a bounds violation. Under the new logic, if the source type and target types are both bounded, then the parent of the source type must be allowed the same permissions to the parent of the target type. If only the source type is bounded, then the parent of the source type must be allowed the same permissions to the target type. Examples of the new logic and comparisons with the old logic: 1. If we have: typebounds A B; then: allow B self:process ; will satisfy the bounds constraint iff: allow A self:process ; is also allowed in policy. Under the old logic, the allow rule on B satisfies the bounds constraint if any of the following three are allowed: allow A B:process ; or allow B A:process ; or allow A self:process ; However, either of the first two ultimately require the third to satisfy the bounds constraint under the old logic, and therefore this degenerates to the same result (but is more efficient - we only need to perform one compute_av call). 2. If we have: typebounds A B; typebounds A_exec B_exec; then: allow B B_exec:file ; will satisfy the bounds constraint iff: allow A A_exec:file ; is also allowed in policy. This is essentially the same as #1; it is merely included as an example of dealing with object types related to a bounded domain in a manner that satisfies the bounds relationship. Note that this approach is preferable to leaving B_exec unbounded and having: allow A B_exec:file ; in policy because that would allow B's entrypoints to be used to enter A. Similarly for _tmp or other related types. 3. If we have: typebounds A B; and an unbounded type T, then: allow B T:file ; will satisfy the bounds constraint iff: allow A T:file ; is allowed in policy. The old logic would have been identical for this example. 4. If we have: typebounds A B; and an unbounded domain D, then: allow D B:unix_stream_socket ; is not subject to any bounds constraints under the new logic because D is not bounded. This is desirable so that we can allow a domain to e.g. connectto a child domain without having to allow it to do the same to its parent. The old logic would have required: allow D A:unix_stream_socket ; to also be allowed in policy. Signed-off-by: Stephen Smalley [PM: re-wrapped description to appease checkpatch.pl] Signed-off-by: Paul Moore --- security/selinux/ss/services.c | 70 +++++++++++++----------------------------- 1 file changed, 22 insertions(+), 48 deletions(-) (limited to 'security') diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 89df64672b89..082b20c78363 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -543,7 +543,7 @@ static void type_attribute_bounds_av(struct context *scontext, struct av_decision *avd) { struct context lo_scontext; - struct context lo_tcontext; + struct context lo_tcontext, *tcontextp = tcontext; struct av_decision lo_avd; struct type_datum *source; struct type_datum *target; @@ -553,67 +553,41 @@ static void type_attribute_bounds_av(struct context *scontext, scontext->type - 1); BUG_ON(!source); + if (!source->bounds) + return; + target = flex_array_get_ptr(policydb.type_val_to_struct_array, tcontext->type - 1); BUG_ON(!target); - if (source->bounds) { - memset(&lo_avd, 0, sizeof(lo_avd)); - - memcpy(&lo_scontext, scontext, sizeof(lo_scontext)); - lo_scontext.type = source->bounds; + memset(&lo_avd, 0, sizeof(lo_avd)); - context_struct_compute_av(&lo_scontext, - tcontext, - tclass, - &lo_avd, - NULL); - if ((lo_avd.allowed & avd->allowed) == avd->allowed) - return; /* no masked permission */ - masked = ~lo_avd.allowed & avd->allowed; - } + memcpy(&lo_scontext, scontext, sizeof(lo_scontext)); + lo_scontext.type = source->bounds; if (target->bounds) { - memset(&lo_avd, 0, sizeof(lo_avd)); - memcpy(&lo_tcontext, tcontext, sizeof(lo_tcontext)); lo_tcontext.type = target->bounds; - - context_struct_compute_av(scontext, - &lo_tcontext, - tclass, - &lo_avd, - NULL); - if ((lo_avd.allowed & avd->allowed) == avd->allowed) - return; /* no masked permission */ - masked = ~lo_avd.allowed & avd->allowed; + tcontextp = &lo_tcontext; } - if (source->bounds && target->bounds) { - memset(&lo_avd, 0, sizeof(lo_avd)); - /* - * lo_scontext and lo_tcontext are already - * set up. - */ + context_struct_compute_av(&lo_scontext, + tcontextp, + tclass, + &lo_avd, + NULL); - context_struct_compute_av(&lo_scontext, - &lo_tcontext, - tclass, - &lo_avd, - NULL); - if ((lo_avd.allowed & avd->allowed) == avd->allowed) - return; /* no masked permission */ - masked = ~lo_avd.allowed & avd->allowed; - } + masked = ~lo_avd.allowed & avd->allowed; - if (masked) { - /* mask violated permissions */ - avd->allowed &= ~masked; + if (likely(!masked)) + return; /* no masked permission */ - /* audit masked permissions */ - security_dump_masked_av(scontext, tcontext, - tclass, masked, "bounds"); - } + /* mask violated permissions */ + avd->allowed &= ~masked; + + /* audit masked permissions */ + security_dump_masked_av(scontext, tcontext, + tclass, masked, "bounds"); } /* -- cgit v1.2.3 From 2885c1e3e0c29e4a1915214ddc9673925f538299 Mon Sep 17 00:00:00 2001 From: Casey Schaufler Date: Tue, 31 May 2016 17:24:15 -0700 Subject: LSM: Fix for security_inode_getsecurity and -EOPNOTSUPP Serge Hallyn pointed out that the current implementation of security_inode_getsecurity() works if there is only one hook provided for it, but will fail if there is more than one and the attribute requested isn't supplied by the first module. This isn't a problem today, since only SELinux and Smack provide this hook and there is (currently) no way to enable both of those modules at the same time. Serge, however, wants to introduce a capability attribute and an inode_getsecurity hook in the capability security module to handle it. This addresses that upcoming problem, will be required for "extreme stacking" and is just a better implementation. Signed-off-by: Casey Schaufler Acked-by: Serge Hallyn Signed-off-by: James Morris --- security/security.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) (limited to 'security') diff --git a/security/security.c b/security/security.c index 709569305d32..c4bb47db30ee 100644 --- a/security/security.c +++ b/security/security.c @@ -700,18 +700,39 @@ int security_inode_killpriv(struct dentry *dentry) int security_inode_getsecurity(struct inode *inode, const char *name, void **buffer, bool alloc) { + struct security_hook_list *hp; + int rc; + if (unlikely(IS_PRIVATE(inode))) return -EOPNOTSUPP; - return call_int_hook(inode_getsecurity, -EOPNOTSUPP, inode, name, - buffer, alloc); + /* + * Only one module will provide an attribute with a given name. + */ + list_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) { + rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc); + if (rc != -EOPNOTSUPP) + return rc; + } + return -EOPNOTSUPP; } int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags) { + struct security_hook_list *hp; + int rc; + if (unlikely(IS_PRIVATE(inode))) return -EOPNOTSUPP; - return call_int_hook(inode_setsecurity, -EOPNOTSUPP, inode, name, - value, size, flags); + /* + * Only one module will provide an attribute with a given name. + */ + list_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) { + rc = hp->hook.inode_setsecurity(inode, name, value, size, + flags); + if (rc != -EOPNOTSUPP) + return rc; + } + return -EOPNOTSUPP; } int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size) -- cgit v1.2.3 From 40d273782ff16fe1a7445cc05c66a447dfea3433 Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Thu, 19 May 2016 21:37:53 -0700 Subject: security: tomoyo: simplify the gc kthread creation The code is doing the equivalent of the kthread_run macro. Signed-off-by: Mike Danese Acked-by: Tetsuo Handa Signed-off-by: James Morris --- security/tomoyo/gc.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) (limited to 'security') diff --git a/security/tomoyo/gc.c b/security/tomoyo/gc.c index 986a6a756868..540bc29e1b5a 100644 --- a/security/tomoyo/gc.c +++ b/security/tomoyo/gc.c @@ -645,11 +645,6 @@ void tomoyo_notify_gc(struct tomoyo_io_buffer *head, const bool is_register) } } spin_unlock(&tomoyo_io_buffer_list_lock); - if (is_write) { - struct task_struct *task = kthread_create(tomoyo_gc_thread, - NULL, - "GC for TOMOYO"); - if (!IS_ERR(task)) - wake_up_process(task); - } + if (is_write) + kthread_run(tomoyo_gc_thread, NULL, "GC for TOMOYO"); } -- cgit v1.2.3 From 18d872f77cecec2677a394170f26aaeb08562cee Mon Sep 17 00:00:00 2001 From: Rafal Krypa Date: Mon, 4 Apr 2016 11:14:53 +0200 Subject: Smack: ignore null signal in smack_task_kill Kill with signal number 0 is commonly used for checking PID existence. Smack treated such cases like any other kills, although no signal is actually delivered when sig == 0. Checking permissions when sig == 0 didn't prevent an unprivileged caller from learning whether PID exists or not. When it existed, kernel returned EPERM, when it didn't - ESRCH. The only effect of policy check in such case is noise in audit logs. This change lets Smack silently ignore kill() invocations with sig == 0. Signed-off-by: Rafal Krypa Acked-by: Casey Schaufler --- security/smack/smack_lsm.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'security') diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 6777295f4b2b..e96080eaacbb 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -2227,6 +2227,9 @@ static int smack_task_kill(struct task_struct *p, struct siginfo *info, struct smack_known *tkp = smk_of_task_struct(p); int rc; + if (!sig) + return 0; /* null signal; existence test */ + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_TASK); smk_ad_setfield_u_tsk(&ad, p); /* -- cgit v1.2.3 From 8bebe88c0995f331b0614f413285ce2b1d6fe09c Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Thu, 9 Jun 2016 10:40:37 -0400 Subject: selinux: import NetLabel category bitmaps correctly The existing ebitmap_netlbl_import() code didn't correctly handle the case where the ebitmap_node was not aligned/sized to a power of two, this patch fixes this (on x86_64 ebitmap_node contains six bitmaps making a range of 0..383). Signed-off-by: Paul Moore --- security/selinux/ss/ebitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c index 57644b1dc42e..894b6cdc11c5 100644 --- a/security/selinux/ss/ebitmap.c +++ b/security/selinux/ss/ebitmap.c @@ -165,7 +165,7 @@ int ebitmap_netlbl_import(struct ebitmap *ebmap, e_iter = kzalloc(sizeof(*e_iter), GFP_ATOMIC); if (e_iter == NULL) goto netlbl_import_failure; - e_iter->startbit = offset & ~(EBITMAP_SIZE - 1); + e_iter->startbit = offset - (offset % EBITMAP_SIZE); if (e_prev == NULL) ebmap->node = e_iter; else -- cgit v1.2.3 From 965475acca2cbcc1d748a8b6a05f8c7cf57d075a Mon Sep 17 00:00:00 2001 From: David Howells Date: Tue, 14 Jun 2016 10:29:44 +0100 Subject: KEYS: Strip trailing spaces Strip some trailing spaces. Signed-off-by: David Howells --- security/keys/persistent.c | 2 +- security/keys/request_key.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/keys/persistent.c b/security/keys/persistent.c index 2ef45b319dd9..1edc1f0a0ce2 100644 --- a/security/keys/persistent.c +++ b/security/keys/persistent.c @@ -114,7 +114,7 @@ found: ret = key_link(key_ref_to_ptr(dest_ref), persistent); if (ret == 0) { key_set_timeout(persistent, persistent_keyring_expiry); - ret = persistent->serial; + ret = persistent->serial; } } diff --git a/security/keys/request_key.c b/security/keys/request_key.c index a29e3554751e..43affcf10b22 100644 --- a/security/keys/request_key.c +++ b/security/keys/request_key.c @@ -442,7 +442,7 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx, if (ctx->index_key.type == &key_type_keyring) return ERR_PTR(-EPERM); - + user = key_user_lookup(current_fsuid()); if (!user) return ERR_PTR(-ENOMEM); -- cgit v1.2.3 From 309c5fad5de44fb9b1703a8f7bd814a223c57d60 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Fri, 10 Jun 2016 23:14:26 +0200 Subject: selinux: fix type mismatch avc_cache_threshold is of type unsigned int. Do not use a signed new_value in sscanf(page, "%u", &new_value). Signed-off-by: Heinrich Schuchardt [PM: subject prefix fix, description cleanup] Signed-off-by: Paul Moore --- security/selinux/selinuxfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 1b1fd27de632..0765c5b053b5 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -1347,7 +1347,7 @@ static ssize_t sel_write_avc_cache_threshold(struct file *file, { char *page; ssize_t ret; - int new_value; + unsigned int new_value; ret = task_has_security(current, SECURITY__SETSECPARAM); if (ret) -- cgit v1.2.3 From ceba1832b1b2da0149c51de62a847c00bca1677a Mon Sep 17 00:00:00 2001 From: Huw Davies Date: Mon, 27 Jun 2016 15:02:51 -0400 Subject: calipso: Set the calipso socket label to match the secattr. CALIPSO is a hop-by-hop IPv6 option. A lot of this patch is based on the equivalent CISPO code. The main difference is due to manipulating the options in the hop-by-hop header. Signed-off-by: Huw Davies Signed-off-by: Paul Moore --- security/selinux/netlabel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c index 1f989a539fd4..5470f32eca54 100644 --- a/security/selinux/netlabel.c +++ b/security/selinux/netlabel.c @@ -333,7 +333,7 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family) struct sk_security_struct *sksec = sk->sk_security; struct netlbl_lsm_secattr *secattr; - if (family != PF_INET) + if (family != PF_INET && family != PF_INET6) return 0; secattr = selinux_netlbl_sock_genattr(sk); -- cgit v1.2.3 From 1f440c99d3207d684a3ac48d6e528af548b5c915 Mon Sep 17 00:00:00 2001 From: Huw Davies Date: Mon, 27 Jun 2016 15:05:27 -0400 Subject: netlabel: Prevent setsockopt() from changing the hop-by-hop option. If a socket has a netlabel in place then don't let setsockopt() alter the socket's IPv6 hop-by-hop option. This is in the same spirit as the existing check for IPv4. Signed-off-by: Huw Davies Signed-off-by: Paul Moore --- security/selinux/netlabel.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c index 5470f32eca54..2477a75f16e7 100644 --- a/security/selinux/netlabel.c +++ b/security/selinux/netlabel.c @@ -409,6 +409,21 @@ int selinux_netlbl_sock_rcv_skb(struct sk_security_struct *sksec, return rc; } +/** + * selinux_netlbl_option - Is this a NetLabel option + * @level: the socket level or protocol + * @optname: the socket option name + * + * Description: + * Returns true if @level and @optname refer to a NetLabel option. + * Helper for selinux_netlbl_socket_setsockopt(). + */ +static inline int selinux_netlbl_option(int level, int optname) +{ + return (level == IPPROTO_IP && optname == IP_OPTIONS) || + (level == IPPROTO_IPV6 && optname == IPV6_HOPOPTS); +} + /** * selinux_netlbl_socket_setsockopt - Do not allow users to remove a NetLabel * @sock: the socket @@ -431,7 +446,7 @@ int selinux_netlbl_socket_setsockopt(struct socket *sock, struct sk_security_struct *sksec = sk->sk_security; struct netlbl_lsm_secattr secattr; - if (level == IPPROTO_IP && optname == IP_OPTIONS && + if (selinux_netlbl_option(level, optname) && (sksec->nlbl_state == NLBL_LABELED || sksec->nlbl_state == NLBL_CONNLABELED)) { netlbl_secattr_init(&secattr); -- cgit v1.2.3 From e1adea927080821ebfa7505bff752a4015955660 Mon Sep 17 00:00:00 2001 From: Huw Davies Date: Mon, 27 Jun 2016 15:05:29 -0400 Subject: calipso: Allow request sockets to be relabelled by the lsm. Request sockets need to have a label that takes into account the incoming connection as well as their parent's label. This is used for the outgoing SYN-ACK and for their child full-socket. Signed-off-by: Huw Davies Signed-off-by: Paul Moore --- security/selinux/netlabel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c index 2477a75f16e7..ca220c3fbcf9 100644 --- a/security/selinux/netlabel.c +++ b/security/selinux/netlabel.c @@ -284,7 +284,7 @@ int selinux_netlbl_inet_conn_request(struct request_sock *req, u16 family) int rc; struct netlbl_lsm_secattr secattr; - if (family != PF_INET) + if (family != PF_INET && family != PF_INET6) return 0; netlbl_secattr_init(&secattr); -- cgit v1.2.3 From 2917f57b6bc15cc6787496ee5f2fdf17f0e9b7d3 Mon Sep 17 00:00:00 2001 From: Huw Davies Date: Mon, 27 Jun 2016 15:06:15 -0400 Subject: calipso: Allow the lsm to label the skbuff directly. In some cases, the lsm needs to add the label to the skbuff directly. A NF_INET_LOCAL_OUT IPv6 hook is added to selinux to match the IPv4 behaviour. This allows selinux to label the skbuffs that it requires. Signed-off-by: Huw Davies Signed-off-by: Paul Moore --- security/selinux/hooks.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'security') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index a00ab81ab719..cb7c5c8028e7 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5063,6 +5063,15 @@ static unsigned int selinux_ipv4_output(void *priv, return selinux_ip_output(skb, PF_INET); } +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE) +static unsigned int selinux_ipv6_output(void *priv, + struct sk_buff *skb, + const struct nf_hook_state *state) +{ + return selinux_ip_output(skb, PF_INET6); +} +#endif /* IPV6 */ + static unsigned int selinux_ip_postroute_compat(struct sk_buff *skb, int ifindex, u16 family) @@ -6297,6 +6306,12 @@ static struct nf_hook_ops selinux_nf_ops[] = { .hooknum = NF_INET_FORWARD, .priority = NF_IP6_PRI_SELINUX_FIRST, }, + { + .hook = selinux_ipv6_output, + .pf = NFPROTO_IPV6, + .hooknum = NF_INET_LOCAL_OUT, + .priority = NF_IP6_PRI_SELINUX_FIRST, + }, #endif /* IPV6 */ }; -- cgit v1.2.3 From a04e71f631fa3d2fd2aa0404c11484739d1e9073 Mon Sep 17 00:00:00 2001 From: Huw Davies Date: Mon, 27 Jun 2016 15:06:16 -0400 Subject: netlabel: Pass a family parameter to netlbl_skbuff_err(). This makes it possible to route the error to the appropriate labelling engine. CALIPSO is far less verbose than CIPSO when encountering a bogus packet, so there is no need for a CALIPSO error handler. Signed-off-by: Huw Davies Signed-off-by: Paul Moore --- security/selinux/hooks.c | 6 +++--- security/selinux/include/netlabel.h | 4 +++- security/selinux/netlabel.c | 6 +++--- security/smack/smack_lsm.c | 2 +- 4 files changed, 10 insertions(+), 8 deletions(-) (limited to 'security') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index cb7c5c8028e7..51eafe5d3bf4 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4603,13 +4603,13 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) err = selinux_inet_sys_rcv_skb(sock_net(sk), skb->skb_iif, addrp, family, peer_sid, &ad); if (err) { - selinux_netlbl_err(skb, err, 0); + selinux_netlbl_err(skb, family, err, 0); return err; } err = avc_has_perm(sk_sid, peer_sid, SECCLASS_PEER, PEER__RECV, &ad); if (err) { - selinux_netlbl_err(skb, err, 0); + selinux_netlbl_err(skb, family, err, 0); return err; } } @@ -4977,7 +4977,7 @@ static unsigned int selinux_ip_forward(struct sk_buff *skb, err = selinux_inet_sys_rcv_skb(dev_net(indev), indev->ifindex, addrp, family, peer_sid, &ad); if (err) { - selinux_netlbl_err(skb, err, 1); + selinux_netlbl_err(skb, family, err, 1); return NF_DROP; } } diff --git a/security/selinux/include/netlabel.h b/security/selinux/include/netlabel.h index 8c59b8f150e8..75686d53df07 100644 --- a/security/selinux/include/netlabel.h +++ b/security/selinux/include/netlabel.h @@ -40,7 +40,8 @@ #ifdef CONFIG_NETLABEL void selinux_netlbl_cache_invalidate(void); -void selinux_netlbl_err(struct sk_buff *skb, int error, int gateway); +void selinux_netlbl_err(struct sk_buff *skb, u16 family, int error, + int gateway); void selinux_netlbl_sk_security_free(struct sk_security_struct *sksec); void selinux_netlbl_sk_security_reset(struct sk_security_struct *sksec); @@ -72,6 +73,7 @@ static inline void selinux_netlbl_cache_invalidate(void) } static inline void selinux_netlbl_err(struct sk_buff *skb, + u16 family, int error, int gateway) { diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c index ca220c3fbcf9..dfca50dc292a 100644 --- a/security/selinux/netlabel.c +++ b/security/selinux/netlabel.c @@ -151,9 +151,9 @@ void selinux_netlbl_cache_invalidate(void) * present on the packet, NetLabel is smart enough to only act when it should. * */ -void selinux_netlbl_err(struct sk_buff *skb, int error, int gateway) +void selinux_netlbl_err(struct sk_buff *skb, u16 family, int error, int gateway) { - netlbl_skbuff_err(skb, error, gateway); + netlbl_skbuff_err(skb, family, error, gateway); } /** @@ -405,7 +405,7 @@ int selinux_netlbl_sock_rcv_skb(struct sk_security_struct *sksec, return 0; if (nlbl_sid != SECINITSID_UNLABELED) - netlbl_skbuff_err(skb, rc, 0); + netlbl_skbuff_err(skb, family, rc, 0); return rc; } diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 11f79013ae1f..292fdeaaec7e 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3992,7 +3992,7 @@ access_check: rc = smk_bu_note("IPv4 delivery", skp, ssp->smk_in, MAY_WRITE, rc); if (rc != 0) - netlbl_skbuff_err(skb, rc, 0); + netlbl_skbuff_err(skb, sk->sk_family, rc, 0); break; #if IS_ENABLED(CONFIG_IPV6) case PF_INET6: -- cgit v1.2.3 From 4fee5242bf41d9ad641d4c1b821e36eb7ba37fbf Mon Sep 17 00:00:00 2001 From: Huw Davies Date: Mon, 27 Jun 2016 15:06:17 -0400 Subject: calipso: Add a label cache. This works in exactly the same way as the CIPSO label cache. The idea is to allow the lsm to cache the result of a secattr lookup so that it doesn't need to perform the lookup for every skbuff. It introduces two sysctl controls: calipso_cache_enable - enables/disables the cache. calipso_cache_bucket_size - sets the size of a cache bucket. Signed-off-by: Huw Davies Signed-off-by: Paul Moore --- security/selinux/netlabel.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'security') diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c index dfca50dc292a..aaba6677ee2e 100644 --- a/security/selinux/netlabel.c +++ b/security/selinux/netlabel.c @@ -54,6 +54,7 @@ * */ static int selinux_netlbl_sidlookup_cached(struct sk_buff *skb, + u16 family, struct netlbl_lsm_secattr *secattr, u32 *sid) { @@ -63,7 +64,7 @@ static int selinux_netlbl_sidlookup_cached(struct sk_buff *skb, if (rc == 0 && (secattr->flags & NETLBL_SECATTR_CACHEABLE) && (secattr->flags & NETLBL_SECATTR_CACHE)) - netlbl_cache_add(skb, secattr); + netlbl_cache_add(skb, family, secattr); return rc; } @@ -214,7 +215,8 @@ int selinux_netlbl_skbuff_getsid(struct sk_buff *skb, netlbl_secattr_init(&secattr); rc = netlbl_skbuff_getattr(skb, family, &secattr); if (rc == 0 && secattr.flags != NETLBL_SECATTR_NONE) - rc = selinux_netlbl_sidlookup_cached(skb, &secattr, sid); + rc = selinux_netlbl_sidlookup_cached(skb, family, + &secattr, sid); else *sid = SECSID_NULL; *type = secattr.type; @@ -382,7 +384,8 @@ int selinux_netlbl_sock_rcv_skb(struct sk_security_struct *sksec, netlbl_secattr_init(&secattr); rc = netlbl_skbuff_getattr(skb, family, &secattr); if (rc == 0 && secattr.flags != NETLBL_SECATTR_NONE) - rc = selinux_netlbl_sidlookup_cached(skb, &secattr, &nlbl_sid); + rc = selinux_netlbl_sidlookup_cached(skb, family, + &secattr, &nlbl_sid); else nlbl_sid = SECINITSID_UNLABELED; netlbl_secattr_destroy(&secattr); -- cgit v1.2.3 From 96d450bbeccda6f32c70bbb9ee54057f68733cad Mon Sep 17 00:00:00 2001 From: Eric Richter Date: Wed, 1 Jun 2016 13:14:00 -0500 Subject: integrity: add measured_pcrs field to integrity cache To keep track of which measurements have been extended to which PCRs, this patch defines a new integrity_iint_cache field named measured_pcrs. This field is a bitmask of the PCRs measured. Each bit corresponds to a PCR index. For example, bit 10 corresponds to PCR 10. Signed-off-by: Eric Richter Signed-off-by: Mimi Zohar --- security/integrity/iint.c | 2 ++ security/integrity/integrity.h | 1 + 2 files changed, 3 insertions(+) (limited to 'security') diff --git a/security/integrity/iint.c b/security/integrity/iint.c index 345b75997e4c..c710d22042f9 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -79,6 +79,7 @@ static void iint_free(struct integrity_iint_cache *iint) iint->ima_bprm_status = INTEGRITY_UNKNOWN; iint->ima_read_status = INTEGRITY_UNKNOWN; iint->evm_status = INTEGRITY_UNKNOWN; + iint->measured_pcrs = 0; kmem_cache_free(iint_cache, iint); } @@ -159,6 +160,7 @@ static void init_once(void *foo) iint->ima_bprm_status = INTEGRITY_UNKNOWN; iint->ima_read_status = INTEGRITY_UNKNOWN; iint->evm_status = INTEGRITY_UNKNOWN; + iint->measured_pcrs = 0; } static int __init integrity_iintcache_init(void) diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 90bc57d796ec..24520b4ef3b0 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -103,6 +103,7 @@ struct integrity_iint_cache { struct inode *inode; /* back pointer to inode in question */ u64 version; /* track inode changes */ unsigned long flags; + unsigned long measured_pcrs; enum integrity_status ima_file_status:4; enum integrity_status ima_mmap_status:4; enum integrity_status ima_bprm_status:4; -- cgit v1.2.3 From 0260643ce8047d2a58f76222d09f161149622465 Mon Sep 17 00:00:00 2001 From: Eric Richter Date: Wed, 1 Jun 2016 13:14:01 -0500 Subject: ima: add policy support for extending different pcrs This patch defines a new IMA measurement policy rule option "pcr=", which allows extending different PCRs on a per rule basis. For example, the system independent files could extend the default IMA Kconfig specified PCR, while the system dependent files could extend a different PCR. The following is an example of this usage with an SELinux policy; the rule would extend PCR 11 with system configuration files: measure func=FILE_CHECK mask=MAY_READ obj_type=system_conf_t pcr=11 Changelog v3: - FIELD_SIZEOF returns bytes, not bits. Fixed INVALID_PCR Signed-off-by: Eric Richter Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 0f887a564a29..3d35fbe3be0b 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -32,6 +32,7 @@ #define IMA_FSUUID 0x0020 #define IMA_INMASK 0x0040 #define IMA_EUID 0x0080 +#define IMA_PCR 0x0100 #define UNKNOWN 0 #define MEASURE 0x0001 /* same as IMA_MEASURE */ @@ -40,6 +41,9 @@ #define DONT_APPRAISE 0x0008 #define AUDIT 0x0040 +#define INVALID_PCR(a) (((a) < 0) || \ + (a) >= (FIELD_SIZEOF(struct integrity_iint_cache, measured_pcrs) * 8)) + int ima_policy_flag; static int temp_ima_appraise; @@ -60,6 +64,7 @@ struct ima_rule_entry { u8 fsuuid[16]; kuid_t uid; kuid_t fowner; + int pcr; struct { void *rule; /* LSM file metadata specific */ void *args_p; /* audit value */ @@ -478,7 +483,8 @@ enum { Opt_subj_user, Opt_subj_role, Opt_subj_type, Opt_func, Opt_mask, Opt_fsmagic, Opt_fsuuid, Opt_uid, Opt_euid, Opt_fowner, - Opt_appraise_type, Opt_permit_directio + Opt_appraise_type, Opt_permit_directio, + Opt_pcr }; static match_table_t policy_tokens = { @@ -502,6 +508,7 @@ static match_table_t policy_tokens = { {Opt_fowner, "fowner=%s"}, {Opt_appraise_type, "appraise_type=%s"}, {Opt_permit_directio, "permit_directio"}, + {Opt_pcr, "pcr=%s"}, {Opt_err, NULL} }; @@ -773,6 +780,20 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) break; case Opt_permit_directio: entry->flags |= IMA_PERMIT_DIRECTIO; + break; + case Opt_pcr: + if (entry->action != MEASURE) { + result = -EINVAL; + break; + } + ima_log_string(ab, "pcr", args[0].from); + + result = kstrtoint(args[0].from, 10, &entry->pcr); + if (result || INVALID_PCR(entry->pcr)) + result = -EINVAL; + else + entry->flags |= IMA_PCR; + break; case Opt_err: ima_log_string(ab, "UNKNOWN", p); @@ -1011,6 +1032,12 @@ int ima_policy_show(struct seq_file *m, void *v) seq_puts(m, " "); } + if (entry->flags & IMA_PCR) { + snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr); + seq_printf(m, pt(Opt_pcr), tbuf); + seq_puts(m, " "); + } + if (entry->flags & IMA_FSUUID) { seq_printf(m, "fsuuid=%pU", entry->fsuuid); seq_puts(m, " "); -- cgit v1.2.3 From 725de7fabb9fe4ca388c780ad4644352f2f06ccc Mon Sep 17 00:00:00 2001 From: Eric Richter Date: Wed, 1 Jun 2016 13:14:02 -0500 Subject: ima: extend ima_get_action() to return the policy pcr Different policy rules may extend different PCRs. This patch retrieves the specific PCR for the matched rule. Subsequent patches will include the rule specific PCR in the measurement list and extend the appropriate PCR. Signed-off-by: Eric Richter Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 5 +++-- security/integrity/ima/ima_api.c | 5 +++-- security/integrity/ima/ima_appraise.c | 2 +- security/integrity/ima/ima_main.c | 3 ++- security/integrity/ima/ima_policy.c | 6 +++++- 5 files changed, 14 insertions(+), 7 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index d3a939bf2781..3c8e71e9e049 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -154,7 +154,8 @@ enum ima_hooks { }; /* LIM API function definitions */ -int ima_get_action(struct inode *inode, int mask, enum ima_hooks func); +int ima_get_action(struct inode *inode, int mask, + enum ima_hooks func, int *pcr); int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func); int ima_collect_measurement(struct integrity_iint_cache *iint, struct file *file, void *buf, loff_t size, @@ -174,7 +175,7 @@ const char *ima_d_path(const struct path *path, char **pathbuf); /* IMA policy related functions */ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask, - int flags); + int flags, int *pcr); void ima_init_policy(void); void ima_update_policy(void); void ima_update_policy_flag(void); diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 5a2218fe877a..225b9cede300 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -157,6 +157,7 @@ err_out: * @inode: pointer to inode to measure * @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXECUTE) * @func: caller identifier + * @pcr: pointer filled in if matched measure policy sets pcr= * * The policy is defined in terms of keypairs: * subj=, obj=, type=, func=, mask=, fsmagic= @@ -168,13 +169,13 @@ err_out: * Returns IMA_MEASURE, IMA_APPRAISE mask. * */ -int ima_get_action(struct inode *inode, int mask, enum ima_hooks func) +int ima_get_action(struct inode *inode, int mask, enum ima_hooks func, int *pcr) { int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE; flags &= ima_policy_flag; - return ima_match_policy(inode, func, mask, flags); + return ima_match_policy(inode, func, mask, flags, pcr); } /* diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 1bcbc12e03d9..fe8e92360d77 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -41,7 +41,7 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func) if (!ima_appraise) return 0; - return ima_match_policy(inode, func, mask, IMA_APPRAISE); + return ima_match_policy(inode, func, mask, IMA_APPRAISE, NULL); } static int ima_fix_xattr(struct dentry *dentry, diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 68b26c340acd..58b08b25437a 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -162,6 +162,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, char *pathbuf = NULL; const char *pathname = NULL; int rc = -ENOMEM, action, must_appraise; + int pcr = CONFIG_IMA_MEASURE_PCR_IDX; struct evm_ima_xattr_data *xattr_value = NULL; int xattr_len = 0; bool violation_check; @@ -174,7 +175,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, * bitmask based on the appraise/audit/measurement policy. * Included is the appraise submask. */ - action = ima_get_action(inode, mask, func); + action = ima_get_action(inode, mask, func, &pcr); violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) && (ima_policy_flag & IMA_MEASURE)); if (!action && !violation_check) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3d35fbe3be0b..aed47b777a57 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -324,6 +324,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func) * @inode: pointer to an inode for which the policy decision is being made * @func: IMA hook identifier * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC) + * @pcr: set the pcr to extend * * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type) * conditions. @@ -333,7 +334,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func) * than writes so ima_match_policy() is classical RCU candidate. */ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask, - int flags) + int flags, int *pcr) { struct ima_rule_entry *entry; int action = 0, actmask = flags | (flags << 1); @@ -358,6 +359,9 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask, else actmask &= ~(entry->action | entry->action >> 1); + if ((pcr) && (entry->flags & IMA_PCR)) + *pcr = entry->pcr; + if (!actmask) break; } -- cgit v1.2.3 From 14b1da85bbe9a59c5e01123a06dea4c4758a6db9 Mon Sep 17 00:00:00 2001 From: Eric Richter Date: Wed, 1 Jun 2016 13:14:03 -0500 Subject: ima: include pcr for each measurement log entry The IMA measurement list entries include the Kconfig defined PCR value. This patch defines a new ima_template_entry field for including the PCR as specified in the policy rule. Signed-off-by: Eric Richter Signed-off-by: Mimi Zohar --- security/integrity/ima/ima.h | 6 ++++-- security/integrity/ima/ima_api.c | 10 ++++++---- security/integrity/ima/ima_init.c | 3 ++- security/integrity/ima/ima_main.c | 2 +- 4 files changed, 13 insertions(+), 8 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 3c8e71e9e049..db25f54a04fe 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -88,6 +88,7 @@ struct ima_template_desc { }; struct ima_template_entry { + int pcr; u8 digest[TPM_DIGEST_SIZE]; /* sha1 or md5 measurement hash */ struct ima_template_desc *template_desc; /* template descriptor */ u32 template_data_len; @@ -163,13 +164,14 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file, const unsigned char *filename, struct evm_ima_xattr_data *xattr_value, - int xattr_len); + int xattr_len, int pcr); void ima_audit_measurement(struct integrity_iint_cache *iint, const unsigned char *filename); int ima_alloc_init_template(struct ima_event_data *event_data, struct ima_template_entry **entry); int ima_store_template(struct ima_template_entry *entry, int violation, - struct inode *inode, const unsigned char *filename); + struct inode *inode, + const unsigned char *filename, int pcr); void ima_free_template_entry(struct ima_template_entry *entry); const char *ima_d_path(const struct path *path, char **pathbuf); diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 225b9cede300..8363ba384992 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -87,7 +87,7 @@ out: */ int ima_store_template(struct ima_template_entry *entry, int violation, struct inode *inode, - const unsigned char *filename) + const unsigned char *filename, int pcr) { static const char op[] = "add_template_measure"; static const char audit_cause[] = "hashing_error"; @@ -114,6 +114,7 @@ int ima_store_template(struct ima_template_entry *entry, } memcpy(entry->digest, hash.hdr.digest, hash.hdr.length); } + entry->pcr = pcr; result = ima_add_template_entry(entry, violation, op, inode, filename); return result; } @@ -144,7 +145,8 @@ void ima_add_violation(struct file *file, const unsigned char *filename, result = -ENOMEM; goto err_out; } - result = ima_store_template(entry, violation, inode, filename); + result = ima_store_template(entry, violation, inode, + filename, CONFIG_IMA_MEASURE_PCR_IDX); if (result < 0) ima_free_template_entry(entry); err_out: @@ -253,7 +255,7 @@ out: void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file, const unsigned char *filename, struct evm_ima_xattr_data *xattr_value, - int xattr_len) + int xattr_len, int pcr) { static const char op[] = "add_template_measure"; static const char audit_cause[] = "ENOMEM"; @@ -274,7 +276,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, return; } - result = ima_store_template(entry, violation, inode, filename); + result = ima_store_template(entry, violation, inode, filename, pcr); if (!result || result == -EEXIST) iint->flags |= IMA_MEASURED; if (result < 0) diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 5d679a685616..32912bd54ead 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -79,7 +79,8 @@ static int __init ima_add_boot_aggregate(void) } result = ima_store_template(entry, violation, NULL, - boot_aggregate_name); + boot_aggregate_name, + CONFIG_IMA_MEASURE_PCR_IDX); if (result < 0) { ima_free_template_entry(entry); audit_cause = "store_entry"; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 58b08b25437a..3627afdc932e 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -239,7 +239,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, if (action & IMA_MEASURE) ima_store_measurement(iint, file, pathname, - xattr_value, xattr_len); + xattr_value, xattr_len, pcr); if (action & IMA_APPRAISE_SUBMASK) rc = ima_appraise_measurement(func, iint, file, pathname, xattr_value, xattr_len, opened); -- cgit v1.2.3 From 5f6f027b50d8ed9f1ba4447aa5aed3a94b601fe8 Mon Sep 17 00:00:00 2001 From: Eric Richter Date: Wed, 1 Jun 2016 13:14:04 -0500 Subject: ima: change ima_measurements_show() to display the entry specific pcr IMA assumes that the same default Kconfig PCR is extended for each entry. This patch replaces the default configured PCR with the policy defined PCR. Signed-off-by: Eric Richter Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_fs.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index 60d011aaec38..c07a3844ea0a 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -123,7 +123,6 @@ static int ima_measurements_show(struct seq_file *m, void *v) struct ima_template_entry *e; char *template_name; int namelen; - u32 pcr = CONFIG_IMA_MEASURE_PCR_IDX; bool is_ima_template = false; int i; @@ -137,10 +136,10 @@ static int ima_measurements_show(struct seq_file *m, void *v) /* * 1st: PCRIndex - * PCR used is always the same (config option) in - * little-endian format + * PCR used defaults to the same (config option) in + * little-endian format, unless set in policy */ - ima_putc(m, &pcr, sizeof(pcr)); + ima_putc(m, &e->pcr, sizeof(e->pcr)); /* 2nd: template digest */ ima_putc(m, e->digest, TPM_DIGEST_SIZE); @@ -219,7 +218,7 @@ static int ima_ascii_measurements_show(struct seq_file *m, void *v) e->template_desc->name : e->template_desc->fmt; /* 1st: PCR used (config option) */ - seq_printf(m, "%2d ", CONFIG_IMA_MEASURE_PCR_IDX); + seq_printf(m, "%2d ", e->pcr); /* 2nd: SHA1 template hash */ ima_print_digest(m, e->digest, TPM_DIGEST_SIZE); -- cgit v1.2.3 From 67696f6d79923cdc0084b73b4bbe52e6749a43a4 Mon Sep 17 00:00:00 2001 From: Eric Richter Date: Wed, 1 Jun 2016 13:14:05 -0500 Subject: ima: redefine duplicate template entries Template entry duplicates are prevented from being added to the measurement list by checking a hash table that contains the template entry digests. However, the PCR value is not included in this comparison, so duplicate template entry digests with differing PCRs may be dropped. This patch redefines duplicate template entries as template entries with the same digest and same PCR values. Reported-by: Mimi Zohar Signed-off-by: Eric Richter Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_queue.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 552705d5a78d..04a9ac13e85e 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -44,7 +44,8 @@ struct ima_h_table ima_htable = { static DEFINE_MUTEX(ima_extend_list_mutex); /* lookup up the digest value in the hash table, and return the entry */ -static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value) +static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value, + int pcr) { struct ima_queue_entry *qe, *ret = NULL; unsigned int key; @@ -54,7 +55,7 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value) rcu_read_lock(); hlist_for_each_entry_rcu(qe, &ima_htable.queue[key], hnext) { rc = memcmp(qe->entry->digest, digest_value, TPM_DIGEST_SIZE); - if (rc == 0) { + if ((rc == 0) && (qe->entry->pcr == pcr)) { ret = qe; break; } @@ -118,7 +119,7 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, mutex_lock(&ima_extend_list_mutex); if (!violation) { memcpy(digest, entry->digest, sizeof(digest)); - if (ima_lookup_digest_entry(digest)) { + if (ima_lookup_digest_entry(digest, entry->pcr)) { audit_cause = "hash_exists"; result = -EEXIST; goto out; -- cgit v1.2.3 From a422638d492a35316e3fd9bb31bfc9769b249bca Mon Sep 17 00:00:00 2001 From: Eric Richter Date: Wed, 1 Jun 2016 13:14:06 -0500 Subject: ima: change integrity cache to store measured pcr IMA avoids re-measuring files by storing the current state as a flag in the integrity cache. It will then skip adding a new measurement log entry if the cache reports the file as already measured. If a policy measures an already measured file to a new PCR, the measurement will not be added to the list. This patch implements a new bitfield for specifying which PCR the file was measured into, rather than if it was measured. Signed-off-by: Eric Richter Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_api.c | 6 ++++-- security/integrity/ima/ima_appraise.c | 1 + security/integrity/ima/ima_main.c | 7 ++++++- 3 files changed, 11 insertions(+), 3 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 8363ba384992..9df26a2b75ba 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -266,7 +266,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, xattr_len, NULL}; int violation = 0; - if (iint->flags & IMA_MEASURED) + if (iint->measured_pcrs & (0x1 << pcr)) return; result = ima_alloc_init_template(&event_data, &entry); @@ -277,8 +277,10 @@ void ima_store_measurement(struct integrity_iint_cache *iint, } result = ima_store_template(entry, violation, inode, filename, pcr); - if (!result || result == -EEXIST) + if (!result || result == -EEXIST) { iint->flags |= IMA_MEASURED; + iint->measured_pcrs |= (0x1 << pcr); + } if (result < 0) ima_free_template_entry(entry); } diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index fe8e92360d77..4b9b4a4e1b89 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -370,6 +370,7 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig) return; iint->flags &= ~IMA_DONE_MASK; + iint->measured_pcrs = 0; if (digsig) iint->flags |= IMA_DIGSIG; return; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 3627afdc932e..596ef616ac21 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -125,6 +125,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint, if ((iint->version != inode->i_version) || (iint->flags & IMA_NEW_FILE)) { iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE); + iint->measured_pcrs = 0; if (iint->flags & IMA_APPRAISE) ima_update_xattr(iint, file); } @@ -210,7 +211,11 @@ static int process_measurement(struct file *file, char *buf, loff_t size, */ iint->flags |= action; action &= IMA_DO_MASK; - action &= ~((iint->flags & IMA_DONE_MASK) >> 1); + action &= ~((iint->flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1); + + /* If target pcr is already measured, unset IMA_MEASURE action */ + if ((action & IMA_MEASURE) && (iint->measured_pcrs & (0x1 << pcr))) + action ^= IMA_MEASURE; /* Nothing to do, just return existing appraised status */ if (!action) { -- cgit v1.2.3 From 544e1cea03e6674e3c12a3b8e8cc507c3dbeaf0c Mon Sep 17 00:00:00 2001 From: Eric Richter Date: Wed, 1 Jun 2016 13:14:07 -0500 Subject: ima: extend the measurement entry specific pcr Extend the PCR supplied as a parameter, instead of assuming that the measurement entry uses the default configured PCR. Signed-off-by: Eric Richter Signed-off-by: Mimi Zohar --- security/integrity/ima/ima_queue.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'security') diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 04a9ac13e85e..32f6ac0f96df 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -90,14 +90,14 @@ static int ima_add_digest_entry(struct ima_template_entry *entry) return 0; } -static int ima_pcr_extend(const u8 *hash) +static int ima_pcr_extend(const u8 *hash, int pcr) { int result = 0; if (!ima_used_chip) return result; - result = tpm_pcr_extend(TPM_ANY_NUM, CONFIG_IMA_MEASURE_PCR_IDX, hash); + result = tpm_pcr_extend(TPM_ANY_NUM, pcr, hash); if (result != 0) pr_err("Error Communicating to TPM chip, result: %d\n", result); return result; @@ -136,7 +136,7 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation, if (violation) /* invalidate pcr */ memset(digest, 0xff, sizeof(digest)); - tpmresult = ima_pcr_extend(digest); + tpmresult = ima_pcr_extend(digest, entry->pcr); if (tpmresult != 0) { snprintf(tpm_audit_cause, AUDIT_CAUSE_LEN_MAX, "TPM_error(%d)", tpmresult); -- cgit v1.2.3 From dcda617a0c5160c73e0aa02813c871339ea08004 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 11 Apr 2016 16:55:10 -0700 Subject: apparmor: fix refcount bug in profile replacement Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/policy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 705c2879d3a9..222052f64e2c 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -1189,12 +1189,12 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) aa_get_profile(newest); aa_put_profile(parent); rcu_assign_pointer(ent->new->parent, newest); - } else - aa_put_profile(newest); + } /* aafs interface uses replacedby */ rcu_assign_pointer(ent->new->replacedby->profile, aa_get_profile(ent->new)); __list_add_profile(&parent->base.profiles, ent->new); + aa_put_profile(newest); } else { /* aafs interface uses replacedby */ rcu_assign_pointer(ent->new->replacedby->profile, -- cgit v1.2.3 From ec34fa24a934f4c8fd68f39b84abf34c42e5b06a Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 11 Apr 2016 16:57:19 -0700 Subject: apparmor: fix replacement bug that adds new child to old parent When set atomic replacement is used and the parent is updated before the child, and the child did not exist in the old parent so there is no direct replacement then the new child is incorrectly added to the old parent. This results in the new parent not having the child(ren) that it should and the old parent when being destroyed asserting the following error. AppArmor: policy_destroy: internal error, policy '' still contains profiles Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/policy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 222052f64e2c..c92a9f6c1be5 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -1193,7 +1193,7 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) /* aafs interface uses replacedby */ rcu_assign_pointer(ent->new->replacedby->profile, aa_get_profile(ent->new)); - __list_add_profile(&parent->base.profiles, ent->new); + __list_add_profile(&newest->base.profiles, ent->new); aa_put_profile(newest); } else { /* aafs interface uses replacedby */ -- cgit v1.2.3 From b6b1b81b3afba922505b57f4c812bba022f7c4a9 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sun, 8 Jun 2014 11:20:54 -0700 Subject: apparmor: fix uninitialized lsm_audit member BugLink: http://bugs.launchpad.net/bugs/1268727 The task field in the lsm_audit struct needs to be initialized if a change_hat fails, otherwise the following oops will occur BUG: unable to handle kernel paging request at 0000002fbead7d08 IP: [] _raw_spin_lock+0xe/0x50 PGD 1e3f35067 PUD 0 Oops: 0002 [#1] SMP Modules linked in: pppox crc_ccitt p8023 p8022 psnap llc ax25 btrfs raid6_pq xor xfs libcrc32c dm_multipath scsi_dh kvm_amd dcdbas kvm microcode amd64_edac_mod joydev edac_core psmouse edac_mce_amd serio_raw k10temp sp5100_tco i2c_piix4 ipmi_si ipmi_msghandler acpi_power_meter mac_hid lp parport hid_generic usbhid hid pata_acpi mpt2sas ahci raid_class pata_atiixp bnx2 libahci scsi_transport_sas [last unloaded: tipc] CPU: 2 PID: 699 Comm: changehat_twice Tainted: GF O 3.13.0-7-generic #25-Ubuntu Hardware name: Dell Inc. PowerEdge R415/08WNM9, BIOS 1.8.6 12/06/2011 task: ffff8802135c6000 ti: ffff880212986000 task.ti: ffff880212986000 RIP: 0010:[] [] _raw_spin_lock+0xe/0x50 RSP: 0018:ffff880212987b68 EFLAGS: 00010006 RAX: 0000000000020000 RBX: 0000002fbead7500 RCX: 0000000000000000 RDX: 0000000000000292 RSI: ffff880212987ba8 RDI: 0000002fbead7d08 RBP: ffff880212987b68 R08: 0000000000000246 R09: ffff880216e572a0 R10: ffffffff815fd677 R11: ffffea0008469580 R12: ffffffff8130966f R13: ffff880212987ba8 R14: 0000002fbead7d08 R15: ffff8800d8c6b830 FS: 00002b5e6c84e7c0(0000) GS:ffff880216e40000(0000) knlGS:0000000055731700 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000002fbead7d08 CR3: 000000021270f000 CR4: 00000000000006e0 Stack: ffff880212987b98 ffffffff81075f17 ffffffff8130966f 0000000000000009 0000000000000000 0000000000000000 ffff880212987bd0 ffffffff81075f7c 0000000000000292 ffff880212987c08 ffff8800d8c6b800 0000000000000026 Call Trace: [] __lock_task_sighand+0x47/0x80 [] ? apparmor_cred_prepare+0x2f/0x50 [] do_send_sig_info+0x2c/0x80 [] send_sig_info+0x1e/0x30 [] aa_audit+0x13d/0x190 [] aa_audit_file+0xbc/0x130 [] ? apparmor_cred_prepare+0x2f/0x50 [] aa_change_hat+0x202/0x530 [] aa_setprocattr_changehat+0x116/0x1d0 [] apparmor_setprocattr+0x25d/0x300 [] security_setprocattr+0x16/0x20 [] proc_pid_attr_write+0x107/0x130 [] vfs_write+0xb4/0x1f0 [] SyS_write+0x49/0xa0 [] tracesys+0xe1/0xe6 Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/audit.c | 3 ++- security/apparmor/file.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c index 89c78658031f..3a7f1da1425e 100644 --- a/security/apparmor/audit.c +++ b/security/apparmor/audit.c @@ -200,7 +200,8 @@ int aa_audit(int type, struct aa_profile *profile, gfp_t gfp, if (sa->aad->type == AUDIT_APPARMOR_KILL) (void)send_sig_info(SIGKILL, NULL, - sa->u.tsk ? sa->u.tsk : current); + sa->type == LSM_AUDIT_DATA_TASK && sa->u.tsk ? + sa->u.tsk : current); if (sa->aad->type == AUDIT_APPARMOR_ALLOWED) return complain_error(sa->aad->error); diff --git a/security/apparmor/file.c b/security/apparmor/file.c index d186674f973a..4d2af4b01033 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -110,7 +110,8 @@ int aa_audit_file(struct aa_profile *profile, struct file_perms *perms, int type = AUDIT_APPARMOR_AUTO; struct common_audit_data sa; struct apparmor_audit_data aad = {0,}; - sa.type = LSM_AUDIT_DATA_NONE; + sa.type = LSM_AUDIT_DATA_TASK; + sa.u.tsk = NULL; sa.aad = &aad; aad.op = op, aad.fs.request = request; -- cgit v1.2.3 From 9049a7922124d843a2cd26a02b1d00a17596ec0c Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 25 Jul 2014 04:02:03 -0700 Subject: apparmor: exec should not be returning ENOENT when it denies The current behavior is confusing as it causes exec failures to report the executable is missing instead of identifying that apparmor caused the failure. Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index dc0027b28b04..67a7418937a5 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -433,7 +433,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) new_profile = aa_get_newest_profile(ns->unconfined); info = "ux fallback"; } else { - error = -ENOENT; + error = -EACCES; info = "profile not found"; /* remove MAY_EXEC to audit as failure */ perms.allow &= ~MAY_EXEC; -- cgit v1.2.3 From d671e890205a663429da74e1972e652bea4d73ab Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 25 Jul 2014 04:01:56 -0700 Subject: apparmor: fix update the mtime of the profile file on replacement Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/apparmorfs.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'security') diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index ad4fa49ad1db..45a6199ce23b 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -379,6 +379,8 @@ void __aa_fs_profile_migrate_dents(struct aa_profile *old, for (i = 0; i < AAFS_PROF_SIZEOF; i++) { new->dents[i] = old->dents[i]; + if (new->dents[i]) + new->dents[i]->d_inode->i_mtime = CURRENT_TIME; old->dents[i] = NULL; } } -- cgit v1.2.3 From f2e561d190da7ff5ee265fa460e2d7f753dddfda Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 25 Jul 2014 04:02:08 -0700 Subject: apparmor: fix disconnected bind mnts reconnection Bind mounts can fail to be properly reconnected when PATH_CONNECT is specified. Ensure that when PATH_CONNECT is specified the path has a root. BugLink: http://bugs.launchpad.net/bugs/1319984 Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/path.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/apparmor/path.c b/security/apparmor/path.c index edddc026406b..f2616780065a 100644 --- a/security/apparmor/path.c +++ b/security/apparmor/path.c @@ -141,7 +141,10 @@ static int d_namespace_path(const struct path *path, char *buf, int buflen, error = -EACCES; if (*res == '/') *name = res + 1; - } + } else if (*res != '/') + /* CONNECT_PATH with missing root */ + error = prepend(name, *name - buf, "/", 1); + } out: -- cgit v1.2.3 From bd35db8b8ca6e27fc17a9057ef78e1ddfc0de351 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 25 Jul 2014 04:02:10 -0700 Subject: apparmor: internal paths should be treated as disconnected Internal mounts are not mounted anywhere and as such should be treated as disconnected paths. Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/path.c | 64 +++++++++++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 28 deletions(-) (limited to 'security') diff --git a/security/apparmor/path.c b/security/apparmor/path.c index f2616780065a..a8fc7d08c144 100644 --- a/security/apparmor/path.c +++ b/security/apparmor/path.c @@ -25,7 +25,6 @@ #include "include/path.h" #include "include/policy.h" - /* modified from dcache.c */ static int prepend(char **buffer, int buflen, const char *str, int namelen) { @@ -39,6 +38,38 @@ static int prepend(char **buffer, int buflen, const char *str, int namelen) #define CHROOT_NSCONNECT (PATH_CHROOT_REL | PATH_CHROOT_NSCONNECT) +/* If the path is not connected to the expected root, + * check if it is a sysctl and handle specially else remove any + * leading / that __d_path may have returned. + * Unless + * specifically directed to connect the path, + * OR + * if in a chroot and doing chroot relative paths and the path + * resolves to the namespace root (would be connected outside + * of chroot) and specifically directed to connect paths to + * namespace root. + */ +static int disconnect(const struct path *path, char *buf, char **name, + int flags) +{ + int error = 0; + + if (!(flags & PATH_CONNECT_PATH) && + !(((flags & CHROOT_NSCONNECT) == CHROOT_NSCONNECT) && + our_mnt(path->mnt))) { + /* disconnected path, don't return pathname starting + * with '/' + */ + error = -EACCES; + if (**name == '/') + *name = *name + 1; + } else if (**name != '/') + /* CONNECT_PATH with missing root */ + error = prepend(name, *name - buf, "/", 1); + + return error; +} + /** * d_namespace_path - lookup a name associated with a given path * @path: path to lookup (NOT NULL) @@ -74,7 +105,8 @@ static int d_namespace_path(const struct path *path, char *buf, int buflen, * control instead of hard coded /proc */ return prepend(name, *name - buf, "/proc", 5); - } + } else + return disconnect(path, buf, name, flags); return 0; } @@ -120,32 +152,8 @@ static int d_namespace_path(const struct path *path, char *buf, int buflen, goto out; } - /* If the path is not connected to the expected root, - * check if it is a sysctl and handle specially else remove any - * leading / that __d_path may have returned. - * Unless - * specifically directed to connect the path, - * OR - * if in a chroot and doing chroot relative paths and the path - * resolves to the namespace root (would be connected outside - * of chroot) and specifically directed to connect paths to - * namespace root. - */ - if (!connected) { - if (!(flags & PATH_CONNECT_PATH) && - !(((flags & CHROOT_NSCONNECT) == CHROOT_NSCONNECT) && - our_mnt(path->mnt))) { - /* disconnected path, don't return pathname starting - * with '/' - */ - error = -EACCES; - if (*res == '/') - *name = res + 1; - } else if (*res != '/') - /* CONNECT_PATH with missing root */ - error = prepend(name, *name - buf, "/", 1); - - } + if (!connected) + error = disconnect(path, buf, name, flags); out: return error; -- cgit v1.2.3 From 6059f71f1e94486a51cef90e872add11fa7b5775 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 24 Oct 2014 09:16:14 -0700 Subject: apparmor: add parameter to control whether policy hashing is used Signed-off-by: John Johansen Acked-by: Tyler Hicks Acked-by: Seth Arnold --- security/apparmor/Kconfig | 21 +++++++++++++++++---- security/apparmor/include/apparmor.h | 1 + security/apparmor/lsm.c | 4 ++++ security/apparmor/policy_unpack.c | 5 +++-- 4 files changed, 25 insertions(+), 6 deletions(-) (limited to 'security') diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig index 232469baa94f..be5e9414a295 100644 --- a/security/apparmor/Kconfig +++ b/security/apparmor/Kconfig @@ -31,13 +31,26 @@ config SECURITY_APPARMOR_BOOTPARAM_VALUE If you are unsure how to answer this question, answer 1. config SECURITY_APPARMOR_HASH - bool "SHA1 hash of loaded profiles" + bool "Enable introspection of sha1 hashes for loaded profiles" depends on SECURITY_APPARMOR select CRYPTO select CRYPTO_SHA1 default y help - This option selects whether sha1 hashing is done against loaded - profiles and exported for inspection to user space via the apparmor - filesystem. + This option selects whether introspection of loaded policy + is available to userspace via the apparmor filesystem. + +config SECURITY_APPARMOR_HASH_DEFAULT + bool "Enable policy hash introspection by default" + depends on SECURITY_APPARMOR_HASH + default y + + help + This option selects whether sha1 hashing of loaded policy + is enabled by default. The generation of sha1 hashes for + loaded policy provide system administrators a quick way + to verify that policy in the kernel matches what is expected, + however it can slow down policy load on some devices. In + these cases policy hashing can be disabled by default and + enabled only if needed. diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h index e4ea62663866..5d721e990876 100644 --- a/security/apparmor/include/apparmor.h +++ b/security/apparmor/include/apparmor.h @@ -37,6 +37,7 @@ extern enum audit_mode aa_g_audit; extern bool aa_g_audit_header; extern bool aa_g_debug; +extern bool aa_g_hash_policy; extern bool aa_g_lock_policy; extern bool aa_g_logsyscall; extern bool aa_g_paranoid_load; diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 2660fbcf94d1..656b97c2e1ea 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -669,6 +669,10 @@ enum profile_mode aa_g_profile_mode = APPARMOR_ENFORCE; module_param_call(mode, param_set_mode, param_get_mode, &aa_g_profile_mode, S_IRUSR | S_IWUSR); +/* whether policy verification hashing is enabled */ +bool aa_g_hash_policy = CONFIG_SECURITY_APPARMOR_HASH_DEFAULT; +module_param_named(hash_policy, aa_g_hash_policy, aabool, S_IRUSR | S_IWUSR); + /* Debug mode */ bool aa_g_debug; module_param_named(debug, aa_g_debug, aabool, S_IRUSR | S_IWUSR); diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index a689f10930b5..a55fb2f170c9 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -775,8 +775,9 @@ int aa_unpack(void *udata, size_t size, struct list_head *lh, const char **ns) if (error) goto fail_profile; - error = aa_calc_profile_hash(profile, e.version, start, - e.pos - start); + if (aa_g_hash_policy) + error = aa_calc_profile_hash(profile, e.version, start, + e.pos - start); if (error) goto fail_profile; -- cgit v1.2.3 From f351841f8d41072e741e45299070d421a5833a4a Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sat, 16 Apr 2016 13:59:02 -0700 Subject: apparmor: fix put() parent ref after updating the active ref Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/policy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index c92a9f6c1be5..455c9f89f7e2 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -1187,8 +1187,8 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) /* parent replaced in this atomic set? */ if (newest != parent) { aa_get_profile(newest); - aa_put_profile(parent); rcu_assign_pointer(ent->new->parent, newest); + aa_put_profile(parent); } /* aafs interface uses replacedby */ rcu_assign_pointer(ent->new->replacedby->profile, -- cgit v1.2.3 From bf15cf0c641be8e57d45f110a9d91464f5bb461a Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sat, 16 Apr 2016 14:16:50 -0700 Subject: apparmor: fix log failures for all profiles in a set currently only the profile that is causing the failure is logged. This makes it more confusing than necessary about which profiles loaded and which didn't. So make sure to log success and failure messages for all profiles in the set being loaded. Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/policy.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) (limited to 'security') diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 455c9f89f7e2..db31bc5e459f 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -1067,7 +1067,7 @@ static int __lookup_replace(struct aa_namespace *ns, const char *hname, */ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) { - const char *ns_name, *name = NULL, *info = NULL; + const char *ns_name, *info = NULL; struct aa_namespace *ns = NULL; struct aa_load_ent *ent, *tmp; int op = OP_PROF_REPL; @@ -1082,18 +1082,15 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) /* released below */ ns = aa_prepare_namespace(ns_name); if (!ns) { - info = "failed to prepare namespace"; - error = -ENOMEM; - name = ns_name; - goto fail; + error = audit_policy(op, GFP_KERNEL, ns_name, + "failed to prepare namespace", -ENOMEM); + goto free; } mutex_lock(&ns->lock); /* setup parent and ns info */ list_for_each_entry(ent, &lh, list) { struct aa_policy *policy; - - name = ent->new->base.hname; error = __lookup_replace(ns, ent->new->base.hname, noreplace, &ent->old, &info); if (error) @@ -1121,7 +1118,6 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) if (!p) { error = -ENOENT; info = "parent does not exist"; - name = ent->new->base.hname; goto fail_lock; } rcu_assign_pointer(ent->new->parent, aa_get_profile(p)); @@ -1214,9 +1210,22 @@ out: fail_lock: mutex_unlock(&ns->lock); -fail: - error = audit_policy(op, GFP_KERNEL, name, info, error); + /* audit cause of failure */ + op = (!ent->old) ? OP_PROF_LOAD : OP_PROF_REPL; + audit_policy(op, GFP_KERNEL, ent->new->base.hname, info, error); + /* audit status that rest of profiles in the atomic set failed too */ + info = "valid profile in failed atomic policy load"; + list_for_each_entry(tmp, &lh, list) { + if (tmp == ent) { + info = "unchecked profile in failed atomic policy load"; + /* skip entry that caused failure */ + continue; + } + op = (!ent->old) ? OP_PROF_LOAD : OP_PROF_REPL; + audit_policy(op, GFP_KERNEL, tmp->new->base.hname, info, error); + } +free: list_for_each_entry_safe(ent, tmp, &lh, list) { list_del_init(&ent->list); aa_load_ent_free(ent); -- cgit v1.2.3 From 7ee6da25dcce27b6023a8673fdf8be98dcf7cacf Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sat, 16 Apr 2016 14:19:38 -0700 Subject: apparmor: fix audit full profile hname on successful load Currently logging of a successful profile load only logs the basename of the profile. This can result in confusion when a child profile has the same name as the another profile in the set. Logging the hname will ensure there is no confusion. Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/policy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index db31bc5e459f..ca402d028db8 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -1159,7 +1159,7 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) list_del_init(&ent->list); op = (!ent->old && !ent->rename) ? OP_PROF_LOAD : OP_PROF_REPL; - audit_policy(op, GFP_ATOMIC, ent->new->base.name, NULL, error); + audit_policy(op, GFP_ATOMIC, ent->new->base.hname, NULL, error); if (ent->old) { __replace_profile(ent->old, ent->new, 1); -- cgit v1.2.3 From f7da2de01127b58d93cebeab165136d0998e7b1a Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 20 Apr 2016 14:18:18 -0700 Subject: apparmor: ensure the target profile name is always audited The target profile name was not being correctly audited in a few cases because the target variable was not being set and gotos passed the code to set it at apply: Since it is always based on new_profile just drop the target var and conditionally report based on new_profile. Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/domain.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) (limited to 'security') diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 67a7418937a5..fc3036b34e51 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -346,7 +346,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) file_inode(bprm->file)->i_uid, file_inode(bprm->file)->i_mode }; - const char *name = NULL, *target = NULL, *info = NULL; + const char *name = NULL, *info = NULL; int error = 0; if (bprm->cred_prepared) @@ -399,6 +399,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) if (cxt->onexec) { struct file_perms cp; info = "change_profile onexec"; + new_profile = aa_get_newest_profile(cxt->onexec); if (!(perms.allow & AA_MAY_ONEXEC)) goto audit; @@ -413,7 +414,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) if (!(cp.allow & AA_MAY_ONEXEC)) goto audit; - new_profile = aa_get_newest_profile(cxt->onexec); goto apply; } @@ -445,10 +445,8 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) if (!new_profile) { error = -ENOMEM; info = "could not create null profile"; - } else { + } else error = -EACCES; - target = new_profile->base.hname; - } perms.xindex |= AA_X_UNSAFE; } else /* fail exec */ @@ -459,7 +457,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) * fail the exec. */ if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) { - aa_put_profile(new_profile); error = -EPERM; goto cleanup; } @@ -474,10 +471,8 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) { error = may_change_ptraced_domain(new_profile); - if (error) { - aa_put_profile(new_profile); + if (error) goto audit; - } } /* Determine if secure exec is needed. @@ -498,7 +493,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) bprm->unsafe |= AA_SECURE_X_NEEDED; } apply: - target = new_profile->base.hname; /* when transitioning profiles clear unsafe personality bits */ bprm->per_clear |= PER_CLEAR_ON_SETID; @@ -506,15 +500,19 @@ x_clear: aa_put_profile(cxt->profile); /* transfer new profile reference will be released when cxt is freed */ cxt->profile = new_profile; + new_profile = NULL; /* clear out all temporary/transitional state from the context */ aa_clear_task_cxt_trans(cxt); audit: error = aa_audit_file(profile, &perms, GFP_KERNEL, OP_EXEC, MAY_EXEC, - name, target, cond.uid, info, error); + name, + new_profile ? new_profile->base.hname : NULL, + cond.uid, info, error); cleanup: + aa_put_profile(new_profile); aa_put_profile(profile); kfree(buffer); -- cgit v1.2.3 From 23ca7b640b4a55f8747301b6bd984dd05545f6a7 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Thu, 17 Mar 2016 12:02:54 -0700 Subject: apparmor: check that xindex is in trans_table bounds Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/policy_unpack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index a55fb2f170c9..951ae4633979 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -676,7 +676,7 @@ static bool verify_xindex(int xindex, int table_size) int index, xtype; xtype = xindex & AA_X_TYPE_MASK; index = xindex & AA_X_INDEX_MASK; - if (xtype == AA_X_TABLE && index > table_size) + if (xtype == AA_X_TABLE && index >= table_size) return 0; return 1; } -- cgit v1.2.3 From 0b938a2e2cf0b0a2c8bac9769111545aff0fee97 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 18 Nov 2015 11:41:05 -0800 Subject: apparmor: fix ref count leak when profile sha1 hash is read Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/apparmorfs.c | 1 + 1 file changed, 1 insertion(+) (limited to 'security') diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 45a6199ce23b..0d8dd71f989e 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -331,6 +331,7 @@ static int aa_fs_seq_hash_show(struct seq_file *seq, void *v) seq_printf(seq, "%.2x", profile->hash[i]); seq_puts(seq, "\n"); } + aa_put_profile(profile); return 0; } -- cgit v1.2.3 From de7c4cc947f9f56f61520ee7edaf380434a98c8d Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 16 Dec 2015 18:09:10 -0800 Subject: apparmor: fix refcount race when finding a child profile When finding a child profile via an rcu critical section, the profile may be put and scheduled for deletion after the child is found but before its refcount is incremented. Protect against this by repeating the lookup if the profiles refcount is 0 and is one its way to deletion. Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/policy.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index ca402d028db8..780712553651 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -766,7 +766,9 @@ struct aa_profile *aa_find_child(struct aa_profile *parent, const char *name) struct aa_profile *profile; rcu_read_lock(); - profile = aa_get_profile(__find_child(&parent->base.profiles, name)); + do { + profile = __find_child(&parent->base.profiles, name); + } while (profile && !aa_get_profile_not0(profile)); rcu_read_unlock(); /* refcount released by caller */ -- cgit v1.2.3 From 38dbd7d8be36b5e68c96a24b406f3653180c1c03 Mon Sep 17 00:00:00 2001 From: Geliang Tang Date: Mon, 16 Nov 2015 21:46:33 +0800 Subject: apparmor: use list_next_entry instead of list_entry_next list_next_entry has been defined in list.h, so I replace list_entry_next with it. Signed-off-by: Geliang Tang Acked-by: Serge Hallyn Signed-off-by: John Johansen --- security/apparmor/apparmorfs.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'security') diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 0d8dd71f989e..729e595119ed 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -553,8 +553,6 @@ fail2: } -#define list_entry_next(pos, member) \ - list_entry(pos->member.next, typeof(*pos), member) #define list_entry_is_head(pos, head, member) (&pos->member == (head)) /** @@ -585,7 +583,7 @@ static struct aa_namespace *__next_namespace(struct aa_namespace *root, parent = ns->parent; while (ns != root) { mutex_unlock(&ns->lock); - next = list_entry_next(ns, base.list); + next = list_next_entry(ns, base.list); if (!list_entry_is_head(next, &parent->sub_ns, base.list)) { mutex_lock(&next->lock); return next; @@ -639,7 +637,7 @@ static struct aa_profile *__next_profile(struct aa_profile *p) parent = rcu_dereference_protected(p->parent, mutex_is_locked(&p->ns->lock)); while (parent) { - p = list_entry_next(p, base.list); + p = list_next_entry(p, base.list); if (!list_entry_is_head(p, &parent->base.profiles, base.list)) return p; p = parent; @@ -648,7 +646,7 @@ static struct aa_profile *__next_profile(struct aa_profile *p) } /* is next another profile in the namespace */ - p = list_entry_next(p, base.list); + p = list_next_entry(p, base.list); if (!list_entry_is_head(p, &ns->base.profiles, base.list)) return p; -- cgit v1.2.3 From ff118479a76dbece9ae1c65c7c6a3ebe9cfa73e0 Mon Sep 17 00:00:00 2001 From: Jeff Mahoney Date: Fri, 6 Nov 2015 15:17:30 -0500 Subject: apparmor: allow SYS_CAP_RESOURCE to be sufficient to prlimit another task While using AppArmor, SYS_CAP_RESOURCE is insufficient to call prlimit on another task. The only other example of a AppArmor mediating access to another, already running, task (ignoring fork+exec) is ptrace. The AppArmor model for ptrace is that one of the following must be true: 1) The tracer is unconfined 2) The tracer is in complain mode 3) The tracer and tracee are confined by the same profile 4) The tracer is confined but has SYS_CAP_PTRACE 1), 2, and 3) are already true for setrlimit. We can match the ptrace model just by allowing CAP_SYS_RESOURCE. We still test the values of the rlimit since it can always be overridden using a value that means unlimited for a particular resource. Signed-off-by: Jeff Mahoney Signed-off-by: John Johansen --- security/apparmor/resource.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'security') diff --git a/security/apparmor/resource.c b/security/apparmor/resource.c index 748bf0ca6c9f..67a6072ead4b 100644 --- a/security/apparmor/resource.c +++ b/security/apparmor/resource.c @@ -101,9 +101,11 @@ int aa_task_setrlimit(struct aa_profile *profile, struct task_struct *task, /* TODO: extend resource control to handle other (non current) * profiles. AppArmor rules currently have the implicit assumption * that the task is setting the resource of a task confined with - * the same profile. + * the same profile or that the task setting the resource of another + * task has CAP_SYS_RESOURCE. */ - if (profile != task_profile || + if ((profile != task_profile && + aa_capable(profile, CAP_SYS_RESOURCE, 1)) || (profile->rlimits.mask & (1 << resource) && new_rlim->rlim_max > profile->rlimits.limits[resource].rlim_max)) error = -EACCES; -- cgit v1.2.3 From 15756178c6a65b261a080e21af4766f59cafc112 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Thu, 2 Jun 2016 02:37:02 -0700 Subject: apparmor: add missing id bounds check on dfa verification Signed-off-by: John Johansen --- security/apparmor/include/match.h | 1 + security/apparmor/match.c | 2 ++ 2 files changed, 3 insertions(+) (limited to 'security') diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h index 001c43aa0406..a1c04fe86790 100644 --- a/security/apparmor/include/match.h +++ b/security/apparmor/include/match.h @@ -62,6 +62,7 @@ struct table_set_header { #define YYTD_ID_ACCEPT2 6 #define YYTD_ID_NXT 7 #define YYTD_ID_TSIZE 8 +#define YYTD_ID_MAX 8 #define YYTD_DATA8 1 #define YYTD_DATA16 2 diff --git a/security/apparmor/match.c b/security/apparmor/match.c index 727eb4200d5c..f9f57c626f54 100644 --- a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ -47,6 +47,8 @@ static struct table_header *unpack_table(char *blob, size_t bsize) * it every time we use td_id as an index */ th.td_id = be16_to_cpu(*(u16 *) (blob)) - 1; + if (th.td_id > YYTD_ID_MAX) + goto out; th.td_flags = be16_to_cpu(*(u16 *) (blob + 2)); th.td_lolen = be32_to_cpu(*(u32 *) (blob + 8)); blob += sizeof(struct table_header); -- cgit v1.2.3 From 3197f5adf539a3ee6331f433a51483f8c842f890 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 15 Jun 2016 09:57:55 +0300 Subject: apparmor: don't check for vmalloc_addr if kvzalloc() failed Signed-off-by: John Johansen --- security/apparmor/match.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'security') diff --git a/security/apparmor/match.c b/security/apparmor/match.c index f9f57c626f54..32b72eb3d988 100644 --- a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ -75,14 +75,14 @@ static struct table_header *unpack_table(char *blob, size_t bsize) u32, be32_to_cpu); else goto fail; + /* if table was vmalloced make sure the page tables are synced + * before it is used, as it goes live to all cpus. + */ + if (is_vmalloc_addr(table)) + vm_unmap_aliases(); } out: - /* if table was vmalloced make sure the page tables are synced - * before it is used, as it goes live to all cpus. - */ - if (is_vmalloc_addr(table)) - vm_unmap_aliases(); return table; fail: kvfree(table); -- cgit v1.2.3 From 5f20fdfed16bc599a325a145bf0123a8e1c9beea Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 15 Jun 2016 10:00:55 +0300 Subject: apparmor: fix oops in profile_unpack() when policy_db is not present BugLink: http://bugs.launchpad.net/bugs/1592547 If unpack_dfa() returns NULL due to the dfa not being present, profile_unpack() is not checking if the dfa is not present (NULL). Signed-off-by: John Johansen --- security/apparmor/policy_unpack.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'security') diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 951ae4633979..b9b1c66a32a5 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -583,6 +583,9 @@ static struct aa_profile *unpack_profile(struct aa_ext *e) error = PTR_ERR(profile->policy.dfa); profile->policy.dfa = NULL; goto fail; + } else if (!profile->policy.dfa) { + error = -EPROTO; + goto fail; } if (!unpack_u32(e, &profile->policy.start[0], "start")) /* default start state */ -- cgit v1.2.3 From 58acf9d911c8831156634a44d0b022d683e1e50c Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 22 Jun 2016 18:01:08 -0700 Subject: apparmor: fix module parameters can be changed after policy is locked the policy_lock parameter is a one way switch that prevents policy from being further modified. Unfortunately some of the module parameters can effectively modify policy by turning off enforcement. split policy_admin_capable into a view check and a full admin check, and update the admin check to test the policy_lock parameter. Signed-off-by: John Johansen --- security/apparmor/include/policy.h | 2 ++ security/apparmor/lsm.c | 22 ++++++++++------------ security/apparmor/policy.c | 18 +++++++++++++++++- 3 files changed, 29 insertions(+), 13 deletions(-) (limited to 'security') diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h index c28b0f20ab53..52275f040a5f 100644 --- a/security/apparmor/include/policy.h +++ b/security/apparmor/include/policy.h @@ -403,6 +403,8 @@ static inline int AUDIT_MODE(struct aa_profile *profile) return profile->audit; } +bool policy_view_capable(void); +bool policy_admin_capable(void); bool aa_may_manage_policy(int op); #endif /* __AA_POLICY_H */ diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 656b97c2e1ea..300f8336fb8e 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -730,51 +730,49 @@ __setup("apparmor=", apparmor_enabled_setup); /* set global flag turning off the ability to load policy */ static int param_set_aalockpolicy(const char *val, const struct kernel_param *kp) { - if (!capable(CAP_MAC_ADMIN)) + if (!policy_admin_capable()) return -EPERM; - if (aa_g_lock_policy) - return -EACCES; return param_set_bool(val, kp); } static int param_get_aalockpolicy(char *buffer, const struct kernel_param *kp) { - if (!capable(CAP_MAC_ADMIN)) + if (!policy_view_capable()) return -EPERM; return param_get_bool(buffer, kp); } static int param_set_aabool(const char *val, const struct kernel_param *kp) { - if (!capable(CAP_MAC_ADMIN)) + if (!policy_admin_capable()) return -EPERM; return param_set_bool(val, kp); } static int param_get_aabool(char *buffer, const struct kernel_param *kp) { - if (!capable(CAP_MAC_ADMIN)) + if (!policy_view_capable()) return -EPERM; return param_get_bool(buffer, kp); } static int param_set_aauint(const char *val, const struct kernel_param *kp) { - if (!capable(CAP_MAC_ADMIN)) + if (!policy_admin_capable()) return -EPERM; return param_set_uint(val, kp); } static int param_get_aauint(char *buffer, const struct kernel_param *kp) { - if (!capable(CAP_MAC_ADMIN)) + if (!policy_view_capable()) return -EPERM; return param_get_uint(buffer, kp); } static int param_get_audit(char *buffer, struct kernel_param *kp) { - if (!capable(CAP_MAC_ADMIN)) + if (!policy_view_capable()) return -EPERM; if (!apparmor_enabled) @@ -786,7 +784,7 @@ static int param_get_audit(char *buffer, struct kernel_param *kp) static int param_set_audit(const char *val, struct kernel_param *kp) { int i; - if (!capable(CAP_MAC_ADMIN)) + if (!policy_admin_capable()) return -EPERM; if (!apparmor_enabled) @@ -807,7 +805,7 @@ static int param_set_audit(const char *val, struct kernel_param *kp) static int param_get_mode(char *buffer, struct kernel_param *kp) { - if (!capable(CAP_MAC_ADMIN)) + if (!policy_admin_capable()) return -EPERM; if (!apparmor_enabled) @@ -819,7 +817,7 @@ static int param_get_mode(char *buffer, struct kernel_param *kp) static int param_set_mode(const char *val, struct kernel_param *kp) { int i; - if (!capable(CAP_MAC_ADMIN)) + if (!policy_admin_capable()) return -EPERM; if (!apparmor_enabled) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 780712553651..179e68d7dc5f 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -918,6 +918,22 @@ static int audit_policy(int op, gfp_t gfp, const char *name, const char *info, &sa, NULL); } +bool policy_view_capable(void) +{ + struct user_namespace *user_ns = current_user_ns(); + bool response = false; + + if (ns_capable(user_ns, CAP_MAC_ADMIN)) + response = true; + + return response; +} + +bool policy_admin_capable(void) +{ + return policy_view_capable() && !aa_g_lock_policy; +} + /** * aa_may_manage_policy - can the current task manage policy * @op: the policy manipulation operation being done @@ -932,7 +948,7 @@ bool aa_may_manage_policy(int op) return 0; } - if (!capable(CAP_MAC_ADMIN)) { + if (!policy_admin_capable()) { audit_policy(op, GFP_KERNEL, NULL, "not policy admin", -EACCES); return 0; } -- cgit v1.2.3 From f4ee2def2d70692ccff0d55353df4ee594fd0017 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Fri, 10 Jun 2016 23:34:26 +0200 Subject: apparmor: do not expose kernel stack Do not copy uninitalized fields th.td_hilen, th.td_data. Signed-off-by: Heinrich Schuchardt Signed-off-by: John Johansen --- security/apparmor/match.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'security') diff --git a/security/apparmor/match.c b/security/apparmor/match.c index 32b72eb3d988..3f900fcca8fb 100644 --- a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ -63,7 +63,9 @@ static struct table_header *unpack_table(char *blob, size_t bsize) table = kvzalloc(tsize); if (table) { - *table = th; + table->td_id = th.td_id; + table->td_flags = th.td_flags; + table->td_lolen = th.td_lolen; if (th.td_flags == YYTD_DATA8) UNPACK_ARRAY(table->td_data, blob, th.td_lolen, u8, byte_to_byte); -- cgit v1.2.3 From e89b8081327ac9efbf273e790b8677e64fd0361a Mon Sep 17 00:00:00 2001 From: Vegard Nossum Date: Thu, 7 Jul 2016 13:41:11 -0700 Subject: apparmor: fix oops, validate buffer size in apparmor_setprocattr() When proc_pid_attr_write() was changed to use memdup_user apparmor's (interface violating) assumption that the setprocattr buffer was always a single page was violated. The size test is not strictly speaking needed as proc_pid_attr_write() will reject anything larger, but for the sake of robustness we can keep it in. SMACK and SELinux look safe to me, but somebody else should probably have a look just in case. Based on original patch from Vegard Nossum modified for the case that apparmor provides null termination. Fixes: bb646cdb12e75d82258c2f2e7746d5952d3e321a Reported-by: Vegard Nossum Cc: Al Viro Cc: John Johansen Cc: Paul Moore Cc: Stephen Smalley Cc: Eric Paris Cc: Casey Schaufler Cc: stable@kernel.org Signed-off-by: John Johansen Reviewed-by: Tyler Hicks Signed-off-by: James Morris --- security/apparmor/lsm.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) (limited to 'security') diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 300f8336fb8e..c6921a0145ed 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -500,34 +500,34 @@ static int apparmor_setprocattr(struct task_struct *task, char *name, { struct common_audit_data sa; struct apparmor_audit_data aad = {0,}; - char *command, *args = value; + char *command, *largs = NULL, *args = value; size_t arg_size; int error; if (size == 0) return -EINVAL; - /* args points to a PAGE_SIZE buffer, AppArmor requires that - * the buffer must be null terminated or have size <= PAGE_SIZE -1 - * so that AppArmor can null terminate them - */ - if (args[size - 1] != '\0') { - if (size == PAGE_SIZE) - return -EINVAL; - args[size] = '\0'; - } - /* task can only write its own attributes */ if (current != task) return -EACCES; - args = value; + /* AppArmor requires that the buffer must be null terminated atm */ + if (args[size - 1] != '\0') { + /* null terminate */ + largs = args = kmalloc(size + 1, GFP_KERNEL); + if (!args) + return -ENOMEM; + memcpy(args, value, size); + args[size] = '\0'; + } + + error = -EINVAL; args = strim(args); command = strsep(&args, " "); if (!args) - return -EINVAL; + goto out; args = skip_spaces(args); if (!*args) - return -EINVAL; + goto out; arg_size = size - (args - (char *) value); if (strcmp(name, "current") == 0) { @@ -553,10 +553,12 @@ static int apparmor_setprocattr(struct task_struct *task, char *name, goto fail; } else /* only support the "current" and "exec" process attributes */ - return -EINVAL; + goto fail; if (!error) error = size; +out: + kfree(largs); return error; fail: @@ -565,9 +567,9 @@ fail: aad.profile = aa_current_profile(); aad.op = OP_SETPROCATTR; aad.info = name; - aad.error = -EINVAL; + aad.error = error = -EINVAL; aa_audit_msg(AUDIT_APPARMOR_DENIED, &sa, NULL); - return -EINVAL; + goto out; } static int apparmor_task_setrlimit(struct task_struct *task, -- cgit v1.2.3 From d4d03f74a73f3b8b2801d4d02011b6b69778cbcc Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sat, 9 Jul 2016 23:46:33 -0700 Subject: apparmor: fix arg_size computation for when setprocattr is null terminated Signed-off-by: John Johansen --- security/apparmor/lsm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security') diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index c6921a0145ed..3be30c701bfa 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -529,7 +529,7 @@ static int apparmor_setprocattr(struct task_struct *task, char *name, if (!*args) goto out; - arg_size = size - (args - (char *) value); + arg_size = size - (args - (largs ? largs : (char *) value)); if (strcmp(name, "current") == 0) { if (strcmp(command, "changehat") == 0) { error = aa_setprocattr_changehat(args, arg_size, -- cgit v1.2.3 From 7616ac70d1bb4f2e9d25c1a82d283f3368a7b632 Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Mon, 25 Jul 2016 10:59:07 -0700 Subject: apparmor: fix SECURITY_APPARMOR_HASH_DEFAULT parameter handling The newly added Kconfig option could never work and just causes a build error when disabled: security/apparmor/lsm.c:675:25: error: 'CONFIG_SECURITY_APPARMOR_HASH_DEFAULT' undeclared here (not in a function) bool aa_g_hash_policy = CONFIG_SECURITY_APPARMOR_HASH_DEFAULT; The problem is that the macro undefined in this case, and we need to use the IS_ENABLED() helper to turn it into a boolean constant. Another minor problem with the original patch is that the option is even offered in sysfs when SECURITY_APPARMOR_HASH is not enabled, so this also hides the option in that case. Signed-off-by: Arnd Bergmann Fixes: 6059f71f1e94 ("apparmor: add parameter to control whether policy hashing is used") Signed-off-by: John Johansen Signed-off-by: James Morris --- security/apparmor/crypto.c | 3 +++ security/apparmor/lsm.c | 4 +++- security/apparmor/policy_unpack.c | 3 +-- 3 files changed, 7 insertions(+), 3 deletions(-) (limited to 'security') diff --git a/security/apparmor/crypto.c b/security/apparmor/crypto.c index 532471d0b3a0..b75dab0df1cb 100644 --- a/security/apparmor/crypto.c +++ b/security/apparmor/crypto.c @@ -39,6 +39,9 @@ int aa_calc_profile_hash(struct aa_profile *profile, u32 version, void *start, int error = -ENOMEM; u32 le32_version = cpu_to_le32(version); + if (!aa_g_hash_policy) + return 0; + if (!apparmor_tfm) return 0; diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 3be30c701bfa..41b8cb115801 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -671,9 +671,11 @@ enum profile_mode aa_g_profile_mode = APPARMOR_ENFORCE; module_param_call(mode, param_set_mode, param_get_mode, &aa_g_profile_mode, S_IRUSR | S_IWUSR); +#ifdef CONFIG_SECURITY_APPARMOR_HASH /* whether policy verification hashing is enabled */ -bool aa_g_hash_policy = CONFIG_SECURITY_APPARMOR_HASH_DEFAULT; +bool aa_g_hash_policy = IS_ENABLED(CONFIG_SECURITY_APPARMOR_HASH_DEFAULT); module_param_named(hash_policy, aa_g_hash_policy, aabool, S_IRUSR | S_IWUSR); +#endif /* Debug mode */ bool aa_g_debug; diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index b9b1c66a32a5..138120698f83 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -778,8 +778,7 @@ int aa_unpack(void *udata, size_t size, struct list_head *lh, const char **ns) if (error) goto fail_profile; - if (aa_g_hash_policy) - error = aa_calc_profile_hash(profile, e.version, start, + error = aa_calc_profile_hash(profile, e.version, start, e.pos - start); if (error) goto fail_profile; -- cgit v1.2.3