Re: [PATCH] selinux: vsock: Set SID for socket returned by accept()
On Wed, Mar 17, 2021 at 11:44 AM David Brazdil wrote:
>
> For AF_VSOCK, accept() currently returns sockets that are unlabelled.
> Other socket families derive the child's SID from the SID of the parent
> and the SID of the incoming packet. This is typically done as the
> connected socket is placed in the queue that accept() removes from.
>
> Implement an LSM hook 'vsock_sk_clone' that takes the parent (server)
> and child (connection) struct socks, and assigns the parent SID to the
> child. There is no packet SID in this case.
>
> Signed-off-by: David Brazdil
> ---
> This is my first patch in this part of the kernel so please comment if I
> missed anything, specifically whether there is a packet SID that should
> be mixed into the child SID.
>
> Tested on Android.
>
> include/linux/lsm_hook_defs.h | 1 +
> include/linux/lsm_hooks.h | 7 +++
> include/linux/security.h | 5 +
> net/vmw_vsock/af_vsock.c | 1 +
> security/security.c | 5 +
> security/selinux/hooks.c | 10 ++
> 6 files changed, 29 insertions(+)
Additional comments below, but I think it would be a good idea for you
to test your patches on a more traditional Linux distribution as well
as Android.
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 5546710d8ac1..a9bf3b90cb2f 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -755,6 +755,7 @@ static struct sock *__vsock_create(struct net *net,
> vsk->buffer_size = psk->buffer_size;
> vsk->buffer_min_size = psk->buffer_min_size;
> vsk->buffer_max_size = psk->buffer_max_size;
> + security_vsock_sk_clone(parent, sk);
Did you try calling the existing security_sk_clone() hook here? I
would be curious to hear why it doesn't work in this case.
Feel free to educate me on AF_VSOCK, it's entirely possible I'm
misunderstanding something here :)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ddd097790d47..7b92d6f2e0fd 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5616,6 +5616,15 @@ static int selinux_tun_dev_open(void *security)
> return 0;
> }
>
> +static void selinux_socket_vsock_sk_clone(struct sock *sock, struct sock
> *newsk)
> +{
> + struct sk_security_struct *sksec_sock = sock->sk_security;
> + struct sk_security_struct *sksec_new = newsk->sk_security;
> +
> + /* Always returns 0 when packet SID is SECSID_NULL. */
> + WARN_ON_ONCE(selinux_conn_sid(sksec_sock->sid, SECSID_NULL,
> &sksec_new->sid));
> +}
If you are using selinux_conn_sid() with the second argument always
SECSID_NULL it probably isn't the best choice; it ends up doing a
simple "sksec_new->sid = sksec_sock->sid" ... which gets us back to
this function looking like a reimplementation of
selinux_sk_clone_security(), minus the peer_sid and sclass
initializations (which should be important things to have).
I strongly suggest you try making use of the existing
security_sk_clone() hook in the vsock code, it seems like a better way
to solve this problem.
--
paul moore
www.paul-moore.com
Re: [PATCH v2] CIPSO: Fix unaligned memory access in cipso_v4_gentag_hdr
On Fri, Mar 5, 2021 at 3:05 AM Sergey Nazarov wrote:
>
> We need to use put_unaligned when writing 32-bit DOI value
> in cipso_v4_gentag_hdr to avoid unaligned memory access.
>
> v2: unneeded type cast removed as Ondrej Mosnacek suggested.
>
> Signed-off-by: Sergey Nazarov
> ---
> net/ipv4/cipso_ipv4.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Paul Moore
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 471d33a..6e59902 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1162,7 +1162,7 @@ static void cipso_v4_gentag_hdr(const struct
> cipso_v4_doi *doi_def,
> {
> buf[0] = IPOPT_CIPSO;
> buf[1] = CIPSO_V4_HDR_LEN + len;
> - *(__be32 *)&buf[2] = htonl(doi_def->doi);
> + put_unaligned_be32(doi_def->doi, &buf[2]);
> }
>
> /**
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
Re: [PATCH] cipso,calipso: resolve a number of problems with the DOI refcounts
On Thu, Mar 4, 2021 at 5:33 PM David Miller wrote: > From: Paul Moore > Date: Thu, 04 Mar 2021 16:29:51 -0500 > > > +static void calipso_doi_putdef(struct calipso_doi *doi_def); > > + > > This is a global symbol, so why the static decl here? To resolve this: CC net/ipv6/calipso.o net/ipv6/calipso.c: In function ‘calipso_doi_remove’: net/ipv6/calipso.c:453:2: error: implicit declaration of function ‘calipso_doi_p utdef’ I think there are some odd things with how the CALIPSO prototypes are handled, some of that I'm guessing is due to handling IPv6 as-a-module, but regardless of the reason it seemed like the smallest fix was to add the forward declaration at the top of the file. Considering that I believe this should be sent to -stable I figured a smaller patch, with less chance for merge conflicts, would be more desirable. Or are you simply concerned about the static tag? I simply kept the static from the function definition; removing it causes a mismatch which makes the compiler unhappy. Either way, if you want this done another way, let me know what you want and I'll respin it. -- paul moore www.paul-moore.com
Re: [PATCH] cipso,calipso: resolve a number of problems with the DOI refcounts
On Thu, Mar 4, 2021 at 4:29 PM Paul Moore wrote:
>
> The current CIPSO and CALIPSO refcounting scheme for the DOI
> definitions is a bit flawed in that we:
>
> 1. Don't correctly match gets/puts in netlbl_cipsov4_list().
> 2. Decrement the refcount on each attempt to remove the DOI from the
>DOI list, only removing it from the list once the refcount drops
>to zero.
>
> This patch fixes these problems by adding the missing "puts" to
> netlbl_cipsov4_list() and introduces a more conventional, i.e.
> not-buggy, refcounting mechanism to the DOI definitions. Upon the
> addition of a DOI to the DOI list, it is initialized with a refcount
> of one, removing a DOI from the list removes it from the list and
> drops the refcount by one; "gets" and "puts" behave as expected with
> respect to refcounts, increasing and decreasing the DOI's refcount by
> one.
>
> Fixes: b1edeb102397 ("netlabel: Replace protocol/NetLabel linking with
> refrerence counts")
> Fixes: d7cce01504a0 ("netlabel: Add support for removing a CALIPSO DOI.")
> Reported-by: syzbot+9ec037722d2603a9f...@syzkaller.appspotmail.com
> Signed-off-by: Paul Moore
> ---
> net/ipv4/cipso_ipv4.c| 11 +--
> net/ipv6/calipso.c | 14 +-
> net/netlabel/netlabel_cipso_v4.c |3 +++
> 3 files changed, 9 insertions(+), 19 deletions(-)
As a FYI, this patch has been tested by looping through a number of
NetLabel/CALIPSO/CIPSO tests overnight, a reproducer from one of the
syzbot reports (multiple times), and the selinux-testsuite tests;
everything looked good at the end of the testing.
Thanks to syzbot and Dmitry for finding and reporting the bug.
--
paul moore
www.paul-moore.com
[PATCH] cipso,calipso: resolve a number of problems with the DOI refcounts
The current CIPSO and CALIPSO refcounting scheme for the DOI
definitions is a bit flawed in that we:
1. Don't correctly match gets/puts in netlbl_cipsov4_list().
2. Decrement the refcount on each attempt to remove the DOI from the
DOI list, only removing it from the list once the refcount drops
to zero.
This patch fixes these problems by adding the missing "puts" to
netlbl_cipsov4_list() and introduces a more conventional, i.e.
not-buggy, refcounting mechanism to the DOI definitions. Upon the
addition of a DOI to the DOI list, it is initialized with a refcount
of one, removing a DOI from the list removes it from the list and
drops the refcount by one; "gets" and "puts" behave as expected with
respect to refcounts, increasing and decreasing the DOI's refcount by
one.
Fixes: b1edeb102397 ("netlabel: Replace protocol/NetLabel linking with
refrerence counts")
Fixes: d7cce01504a0 ("netlabel: Add support for removing a CALIPSO DOI.")
Reported-by: syzbot+9ec037722d2603a9f...@syzkaller.appspotmail.com
Signed-off-by: Paul Moore
---
net/ipv4/cipso_ipv4.c| 11 +--
net/ipv6/calipso.c | 14 +-
net/netlabel/netlabel_cipso_v4.c |3 +++
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 471d33a0d095..be09c7669a79 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -519,16 +519,10 @@ int cipso_v4_doi_remove(u32 doi, struct netlbl_audit
*audit_info)
ret_val = -ENOENT;
goto doi_remove_return;
}
- if (!refcount_dec_and_test(&doi_def->refcount)) {
- spin_unlock(&cipso_v4_doi_list_lock);
- ret_val = -EBUSY;
- goto doi_remove_return;
- }
list_del_rcu(&doi_def->list);
spin_unlock(&cipso_v4_doi_list_lock);
- cipso_v4_cache_invalidate();
- call_rcu(&doi_def->rcu, cipso_v4_doi_free_rcu);
+ cipso_v4_doi_putdef(doi_def);
ret_val = 0;
doi_remove_return:
@@ -585,9 +579,6 @@ void cipso_v4_doi_putdef(struct cipso_v4_doi *doi_def)
if (!refcount_dec_and_test(&doi_def->refcount))
return;
- spin_lock(&cipso_v4_doi_list_lock);
- list_del_rcu(&doi_def->list);
- spin_unlock(&cipso_v4_doi_list_lock);
cipso_v4_cache_invalidate();
call_rcu(&doi_def->rcu, cipso_v4_doi_free_rcu);
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index 51184a70ac7e..1578ed9e97d8 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -83,6 +83,9 @@ struct calipso_map_cache_entry {
static struct calipso_map_cache_bkt *calipso_cache;
+static void calipso_cache_invalidate(void);
+static void calipso_doi_putdef(struct calipso_doi *doi_def);
+
/* Label Mapping Cache Functions
*/
@@ -444,15 +447,10 @@ static int calipso_doi_remove(u32 doi, struct
netlbl_audit *audit_info)
ret_val = -ENOENT;
goto doi_remove_return;
}
- if (!refcount_dec_and_test(&doi_def->refcount)) {
- spin_unlock(&calipso_doi_list_lock);
- ret_val = -EBUSY;
- goto doi_remove_return;
- }
list_del_rcu(&doi_def->list);
spin_unlock(&calipso_doi_list_lock);
- call_rcu(&doi_def->rcu, calipso_doi_free_rcu);
+ calipso_doi_putdef(doi_def);
ret_val = 0;
doi_remove_return:
@@ -508,10 +506,8 @@ static void calipso_doi_putdef(struct calipso_doi *doi_def)
if (!refcount_dec_and_test(&doi_def->refcount))
return;
- spin_lock(&calipso_doi_list_lock);
- list_del_rcu(&doi_def->list);
- spin_unlock(&calipso_doi_list_lock);
+ calipso_cache_invalidate();
call_rcu(&doi_def->rcu, calipso_doi_free_rcu);
}
diff --git a/net/netlabel/netlabel_cipso_v4.c b/net/netlabel/netlabel_cipso_v4.c
index 726dda95934c..4f50a64315cf 100644
--- a/net/netlabel/netlabel_cipso_v4.c
+++ b/net/netlabel/netlabel_cipso_v4.c
@@ -575,6 +575,7 @@ static int netlbl_cipsov4_list(struct sk_buff *skb, struct
genl_info *info)
break;
}
+ cipso_v4_doi_putdef(doi_def);
rcu_read_unlock();
genlmsg_end(ans_skb, data);
@@ -583,12 +584,14 @@ static int netlbl_cipsov4_list(struct sk_buff *skb,
struct genl_info *info)
list_retry:
/* XXX - this limit is a guesstimate */
if (nlsze_mult < 4) {
+ cipso_v4_doi_putdef(doi_def);
rcu_read_unlock();
kfree_skb(ans_skb);
nlsze_mult *= 2;
goto list_start;
}
list_failure_lock:
+ cipso_v4_doi_putdef(doi_def);
rcu_read_unlock();
list_failure:
kfree_skb(ans_skb);
Re: KASAN: use-after-free Write in cipso_v4_doi_putdef
On Wed, Mar 3, 2021 at 11:20 AM Paul Moore wrote: > On Wed, Mar 3, 2021 at 10:53 AM syzbot > wrote: > > > > Hello, > > > > syzbot found the following issue on: > > > > HEAD commit:7a7fd0de Merge branch 'kmap-conversion-for-5.12' of git://.. > > git tree: upstream > > console output: https://syzkaller.appspot.com/x/log.txt?x=164a74dad0 > > kernel config: https://syzkaller.appspot.com/x/.config?x=779a2568b654c1c6 > > dashboard link: https://syzkaller.appspot.com/bug?extid=521772a90166b3fca21f > > compiler: Debian clang version 11.0.1-2 > > > > Unfortunately, I don't have any reproducer for this issue yet. > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > > Reported-by: syzbot+521772a90166b3fca...@syzkaller.appspotmail.com > > > > == > > BUG: KASAN: use-after-free in instrument_atomic_read_write > > include/linux/instrumented.h:101 [inline] > > BUG: KASAN: use-after-free in atomic_fetch_sub_release > > include/asm-generic/atomic-instrumented.h:220 [inline] > > BUG: KASAN: use-after-free in __refcount_sub_and_test > > include/linux/refcount.h:272 [inline] > > BUG: KASAN: use-after-free in __refcount_dec_and_test > > include/linux/refcount.h:315 [inline] > > BUG: KASAN: use-after-free in refcount_dec_and_test > > include/linux/refcount.h:333 [inline] > > BUG: KASAN: use-after-free in cipso_v4_doi_putdef+0x2d/0x190 > > net/ipv4/cipso_ipv4.c:586 > > Write of size 4 at addr 8880179ecb18 by task syz-executor.5/20110 > > Almost surely the same problem as the others, I'm currently chasing > down a few remaining spots to make sure the fix I'm working on is > correct. I think I've now managed to convince myself that the patch I've got here is reasonable. I'm looping over a series of tests right now and plan to let it continue overnight; assuming everything still looks good in the morning I'll post it. Thanks for your help. -- paul moore www.paul-moore.com
Re: KASAN: use-after-free Write in cipso_v4_doi_putdef
On Wed, Mar 3, 2021 at 10:53 AM syzbot wrote: > > Hello, > > syzbot found the following issue on: > > HEAD commit:7a7fd0de Merge branch 'kmap-conversion-for-5.12' of git://.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=164a74dad0 > kernel config: https://syzkaller.appspot.com/x/.config?x=779a2568b654c1c6 > dashboard link: https://syzkaller.appspot.com/bug?extid=521772a90166b3fca21f > compiler: Debian clang version 11.0.1-2 > > Unfortunately, I don't have any reproducer for this issue yet. > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+521772a90166b3fca...@syzkaller.appspotmail.com > > == > BUG: KASAN: use-after-free in instrument_atomic_read_write > include/linux/instrumented.h:101 [inline] > BUG: KASAN: use-after-free in atomic_fetch_sub_release > include/asm-generic/atomic-instrumented.h:220 [inline] > BUG: KASAN: use-after-free in __refcount_sub_and_test > include/linux/refcount.h:272 [inline] > BUG: KASAN: use-after-free in __refcount_dec_and_test > include/linux/refcount.h:315 [inline] > BUG: KASAN: use-after-free in refcount_dec_and_test > include/linux/refcount.h:333 [inline] > BUG: KASAN: use-after-free in cipso_v4_doi_putdef+0x2d/0x190 > net/ipv4/cipso_ipv4.c:586 > Write of size 4 at addr 8880179ecb18 by task syz-executor.5/20110 Almost surely the same problem as the others, I'm currently chasing down a few remaining spots to make sure the fix I'm working on is correct. -- paul moore www.paul-moore.com
Re: KASAN: use-after-free Read in cipso_v4_genopt
On Tue, Mar 2, 2021 at 2:15 PM Dmitry Vyukov wrote: ... > Not sure if it's the root cause or not, but I am looking at this > reference drop in cipso_v4_doi_remove: > https://elixir.bootlin.com/linux/v5.12-rc1/source/net/ipv4/cipso_ipv4.c#L522 > The thing is that it does not remove from the list if reference is not > 0, right? So what if I send 1000 of netlink remove messages? Will it > drain refcount to 0? > I did not read all involved code, but the typical pattern is to drop > refcount and always remove from the list. Then the last use will > delete the object. > Does it make any sense? Looking at it quickly, the logic above seems sane. I wrote this code a *long* time ago, so let me get my head back into it and make sure that still holds. -- paul moore www.paul-moore.com
Re: KASAN: use-after-free Read in cipso_v4_genopt
On Tue, Mar 2, 2021 at 6:03 AM Dmitry Vyukov wrote: > ... > Besides these 2 crashes, we've also seen one on a 4.19 based kernel, see > below. > Based on the reports with mismatching stacks, it looks like > cipso_v4_genopt is doing some kind of wild pointer access (uninit > pointer?). Hmm, interesting. Looking quickly at the stack dump, it appears that the problem occurs (at least in the recent kernel) when accessing the cipso_v4_doi.tags[] array which is embedded in the cipso_v4_doi struct. Based on the code in cipso_v4_genopt() it doesn't appear that we are shooting past the end of the array/struct and the cipso_v4_doi struct appears to be refcounted correctly in cipso_v4_doi_getdef() and cipso_v4_doi_putdef(). I'll look at it some more today to see if something jumps out at me, but obviously a reproducer would be very helpful if you are able to find one. It's also worth adding that this code really hasn't changed much in a *long* time, not that this means it isn't broken, just that it might also be worth looking at other odd memory bugs to see if there is chance they are wandering around and stomping on memory ... -- paul moore www.paul-moore.com
Re: [PATCH ghak90 v10 01/11] audit: collect audit task parameters
On Mon, Dec 21, 2020 at 11:57 AM Richard Guy Briggs wrote: > > The audit-related parameters in struct task_struct should ideally be > collected together and accessed through a standard audit API and the audit > structures made opaque to other kernel subsystems. > > Collect the existing loginuid, sessionid and audit_context together in a > new opaque struct audit_task_info called "audit" in struct task_struct. > > Use kmem_cache to manage this pool of memory. > Un-inline audit_free() to be able to always recover that memory. > > Please see the upstream github issues > https://github.com/linux-audit/audit-kernel/issues/81 > https://github.com/linux-audit/audit-kernel/issues/90 > > Signed-off-by: Richard Guy Briggs > Acked-by: Neil Horman > Reviewed-by: Ondrej Mosnacek Did Neil and Ondrej really ACK/Review the changes that you made here in v10 or are you just carrying over the ACK/Review? I'm hopeful it is the former, because I'm going to be a little upset if it is the latter. > --- > fs/io-wq.c| 8 +-- > fs/io_uring.c | 16 ++--- > include/linux/audit.h | 49 +- > include/linux/sched.h | 7 +- > init/init_task.c | 3 +- > init/main.c | 2 + > kernel/audit.c| 154 +- > kernel/audit.h| 7 ++ > kernel/auditsc.c | 24 ++++--- > kernel/fork.c | 1 - > 10 files changed, 205 insertions(+), 66 deletions(-) -- paul moore www.paul-moore.com
Re: [PATCH] lsm,selinux: pass flowi_common instead of flowi to the LSM hooks
On Thu, Nov 19, 2020 at 10:02 PM James Morris wrote: > On Thu, 19 Nov 2020, Paul Moore wrote: > > As pointed out by Herbert in a recent related patch, the LSM hooks do > > not have the necessary address family information to use the flowi > > struct safely. As none of the LSMs currently use any of the protocol > > specific flowi information, replace the flowi pointers with pointers > > to the address family independent flowi_common struct. > > > > Reported-by: Herbert Xu > > Signed-off-by: Paul Moore > > Acked-by: James Morris Thanks. Seeing no further comments or objections, and given the discussion in the previous draft of the patch, I've gone again and merged this into selinux/next. -- paul moore www.paul-moore.com
[PATCH] lsm,selinux: pass flowi_common instead of flowi to the LSM hooks
As pointed out by Herbert in a recent related patch, the LSM hooks do
not have the necessary address family information to use the flowi
struct safely. As none of the LSMs currently use any of the protocol
specific flowi information, replace the flowi pointers with pointers
to the address family independent flowi_common struct.
Reported-by: Herbert Xu
Signed-off-by: Paul Moore
---
.../chelsio/inline_crypto/chtls/chtls_cm.c |2 +-
drivers/net/wireguard/socket.c |4 ++-
include/linux/lsm_hook_defs.h |4 ++-
include/linux/lsm_hooks.h |2 +-
include/linux/security.h | 23
include/net/flow.h | 10 +
include/net/route.h|6 +++--
net/dccp/ipv4.c|2 +-
net/dccp/ipv6.c|6 +++--
net/ipv4/icmp.c|4 ++-
net/ipv4/inet_connection_sock.c|4 ++-
net/ipv4/ip_output.c |2 +-
net/ipv4/ping.c|2 +-
net/ipv4/raw.c |2 +-
net/ipv4/syncookies.c |2 +-
net/ipv4/udp.c |2 +-
net/ipv6/af_inet6.c|2 +-
net/ipv6/datagram.c|2 +-
net/ipv6/icmp.c|6 +++--
net/ipv6/inet6_connection_sock.c |4 ++-
net/ipv6/netfilter/nf_reject_ipv6.c|2 +-
net/ipv6/ping.c|2 +-
net/ipv6/raw.c |2 +-
net/ipv6/syncookies.c |2 +-
net/ipv6/tcp_ipv6.c|4 ++-
net/ipv6/udp.c |2 +-
net/l2tp/l2tp_ip6.c|2 +-
net/netfilter/nf_synproxy_core.c |2 +-
net/xfrm/xfrm_state.c |6 +++--
security/security.c| 17 ---
security/selinux/hooks.c |4 ++-
security/selinux/include/xfrm.h|2 +-
security/selinux/xfrm.c| 13 ++-
33 files changed, 85 insertions(+), 66 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
index ec4f79049a06..42e4e43cdd91 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
@@ -1148,7 +1148,7 @@ static struct sock *chtls_recv_sock(struct sock *lsk,
fl6.daddr = ip6h->saddr;
fl6.fl6_dport = inet_rsk(oreq)->ir_rmt_port;
fl6.fl6_sport = htons(inet_rsk(oreq)->ir_num);
- security_req_classify_flow(oreq, flowi6_to_flowi(&fl6));
+ security_req_classify_flow(oreq, flowi6_to_flowi_common(&fl6));
dst = ip6_dst_lookup_flow(sock_net(lsk), lsk, &fl6, NULL);
if (IS_ERR(dst))
goto free_sk;
diff --git a/drivers/net/wireguard/socket.c b/drivers/net/wireguard/socket.c
index c33e2c81635f..410b318e57fb 100644
--- a/drivers/net/wireguard/socket.c
+++ b/drivers/net/wireguard/socket.c
@@ -49,7 +49,7 @@ static int send4(struct wg_device *wg, struct sk_buff *skb,
rt = dst_cache_get_ip4(cache, &fl.saddr);
if (!rt) {
- security_sk_classify_flow(sock, flowi4_to_flowi(&fl));
+ security_sk_classify_flow(sock, flowi4_to_flowi_common(&fl));
if (unlikely(!inet_confirm_addr(sock_net(sock), NULL, 0,
fl.saddr, RT_SCOPE_HOST))) {
endpoint->src4.s_addr = 0;
@@ -129,7 +129,7 @@ static int send6(struct wg_device *wg, struct sk_buff *skb,
dst = dst_cache_get_ip6(cache, &fl.saddr);
if (!dst) {
- security_sk_classify_flow(sock, flowi6_to_flowi(&fl));
+ security_sk_classify_flow(sock, flowi6_to_flowi_common(&fl));
if (unlikely(!ipv6_addr_any(&fl.saddr) &&
!ipv6_chk_addr(sock_net(sock), &fl.saddr, NULL,
0))) {
endpoint->src6 = fl.saddr = in6addr_any;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 32a940117e7a..f70984c8318b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -311,7 +311,7 @@ LSM_HOOK(int, 0, secmark_relabel_packet, u32 secid)
LSM_HOOK(void, LSM_RET_VOID, secmark_refcount_
[PATCH] netlabel: fix an uninitialized warning in netlbl_unlabel_staticlist()
Static checking revealed that a previous fix to
netlbl_unlabel_staticlist() leaves a stack variable uninitialized,
this patches fixes that.
Fixes: 866358ec331f ("netlabel: fix our progress tracking in
netlbl_unlabel_staticlist()")
Reported-by: Dan Carpenter
Signed-off-by: Paul Moore
---
net/netlabel/netlabel_unlabeled.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netlabel/netlabel_unlabeled.c
b/net/netlabel/netlabel_unlabeled.c
index fc55c9116da0..ccb491642811 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -1167,7 +1167,7 @@ static int netlbl_unlabel_staticlist(struct sk_buff *skb,
u32 skip_bkt = cb->args[0];
u32 skip_chain = cb->args[1];
u32 skip_addr4 = cb->args[2];
- u32 iter_bkt, iter_chain, iter_addr4 = 0, iter_addr6 = 0;
+ u32 iter_bkt, iter_chain = 0, iter_addr4 = 0, iter_addr6 = 0;
struct netlbl_unlhsh_iface *iface;
struct list_head *iter_list;
struct netlbl_af4list *addr4;
[PATCH] netlabel: fix our progress tracking in netlbl_unlabel_staticlist()
The current NetLabel code doesn't correctly keep track of the netlink
dump state in some cases, in particular when multiple interfaces with
large configurations are loaded. The problem manifests itself by not
reporting the full configuration to userspace, even though it is
loaded and active in the kernel. This patch fixes this by ensuring
that the dump state is properly reset when necessary inside the
netlbl_unlabel_staticlist() function.
Fixes: 8cc44579d1bd ("NetLabel: Introduce static network labels for unlabeled
connections")
Signed-off-by: Paul Moore
---
net/netlabel/netlabel_unlabeled.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/net/netlabel/netlabel_unlabeled.c
b/net/netlabel/netlabel_unlabeled.c
index 2e8e3f7b2111..fc55c9116da0 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -1166,12 +1166,13 @@ static int netlbl_unlabel_staticlist(struct sk_buff
*skb,
struct netlbl_unlhsh_walk_arg cb_arg;
u32 skip_bkt = cb->args[0];
u32 skip_chain = cb->args[1];
- u32 iter_bkt;
- u32 iter_chain = 0, iter_addr4 = 0, iter_addr6 = 0;
+ u32 skip_addr4 = cb->args[2];
+ u32 iter_bkt, iter_chain, iter_addr4 = 0, iter_addr6 = 0;
struct netlbl_unlhsh_iface *iface;
struct list_head *iter_list;
struct netlbl_af4list *addr4;
#if IS_ENABLED(CONFIG_IPV6)
+ u32 skip_addr6 = cb->args[3];
struct netlbl_af6list *addr6;
#endif
@@ -1182,7 +1183,7 @@ static int netlbl_unlabel_staticlist(struct sk_buff *skb,
rcu_read_lock();
for (iter_bkt = skip_bkt;
iter_bkt < rcu_dereference(netlbl_unlhsh)->size;
-iter_bkt++, iter_chain = 0, iter_addr4 = 0, iter_addr6 = 0) {
+iter_bkt++) {
iter_list = &rcu_dereference(netlbl_unlhsh)->tbl[iter_bkt];
list_for_each_entry_rcu(iface, iter_list, list) {
if (!iface->valid ||
@@ -1190,7 +1191,7 @@ static int netlbl_unlabel_staticlist(struct sk_buff *skb,
continue;
netlbl_af4list_foreach_rcu(addr4,
&iface->addr4_list) {
- if (iter_addr4++ < cb->args[2])
+ if (iter_addr4++ < skip_addr4)
continue;
if (netlbl_unlabel_staticlist_gen(
NLBL_UNLABEL_C_STATICLIST,
@@ -1203,10 +1204,12 @@ static int netlbl_unlabel_staticlist(struct sk_buff
*skb,
goto unlabel_staticlist_return;
}
}
+ iter_addr4 = 0;
+ skip_addr4 = 0;
#if IS_ENABLED(CONFIG_IPV6)
netlbl_af6list_foreach_rcu(addr6,
&iface->addr6_list) {
- if (iter_addr6++ < cb->args[3])
+ if (iter_addr6++ < skip_addr6)
continue;
if (netlbl_unlabel_staticlist_gen(
NLBL_UNLABEL_C_STATICLIST,
@@ -1219,8 +1222,12 @@ static int netlbl_unlabel_staticlist(struct sk_buff *skb,
goto unlabel_staticlist_return;
}
}
+ iter_addr6 = 0;
+ skip_addr6 = 0;
#endif /* IPv6 */
}
+ iter_chain = 0;
+ skip_chain = 0;
}
unlabel_staticlist_return:
Re: [RFC PATCH] lsm,selinux: pass the family information along with xfrm flow
On Wed, Sep 30, 2020 at 9:44 AM Paul Moore wrote: > On Tue, Sep 29, 2020 at 7:09 PM James Morris wrote: > > I'm not keen on adding a parameter which nobody is using. Perhaps a note > > in the header instead? > > On Wed, Sep 30, 2020 at 6:14 AM Herbert Xu > wrote: > > Please at least change to the struct flowi to flowi_common if we're > > not adding a family field. > > It did feel a bit weird adding a (currently) unused parameter, so I > can understand the concern, I just worry that a comment in the code > will be easily overlooked. I also thought about passing a pointer to > the nested flowi_common struct, but it doesn't appear that this is > done anywhere else in the stack so it felt wrong to do it here. With the merge window behind us, where do stand on this? I see the ACK from Casey and some grumbling about adding an unused parameter (which is a valid argument, I just feel the alternative is worse), but I haven't seen any serious NACKs. Any objections or other strong feelings to me merging this via the selinux/next branch? -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls
On Fri, Oct 23, 2020 at 4:40 PM Richard Guy Briggs wrote: > On 2020-10-22 21:21, Paul Moore wrote: > > On Wed, Oct 21, 2020 at 12:39 PM Richard Guy Briggs wrote: > > > Here is an exmple I was able to generate after updating the testsuite > > > script to include a signalling example of a nested audit container > > > identifier: > > > > > > > > > type=PROCTITLE msg=audit(2020-10-21 10:31:16.655:6731) : > > > proctitle=/usr/bin/perl -w containerid/test > > > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > > > contid=7129731255799087104^941723245477888 > > > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115583 > > > oauid=root ouid=root oses=1 > > > obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl > > > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > > > contid=941723245477888 > > > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115580 > > > oauid=root ouid=root oses=1 > > > obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl > > > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > > > contid=8098399240850112512^941723245477888 > > > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115582 > > > oauid=root ouid=root oses=1 > > > obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl > > > type=SYSCALL msg=audit(2020-10-21 10:31:16.655:6731) : arch=x86_64 > > > syscall=kill success=yes exit=0 a0=0xfffe3c84 a1=SIGTERM a2=0x4d524554 > > > a3=0x0 items=0 ppid=115564 pid=115567 auid=root uid=root gid=root > > > euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 > > > ses=1 comm=perl exe=/usr/bin/perl > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > > key=testsuite-1603290671-AcLtUulY > > > > > > > > > There are three CONTAINER_ID records which need some way of associating > > > with OBJ_PID records. An additional CONTAINER_ID record would be present > > > if the killing process itself had an audit container identifier. I think > > > the most obvious way to connect them is with a pid= field in the > > > CONTAINER_ID record. > > > > Using a "pid=" field as a way to link CONTAINER_ID records to other > > records raises a few questions. What happens if/when we need to > > represent those PIDs in the context of a namespace? Are we ever going > > to need to link to records which don't have a "pid=" field? I haven't > > done the homework to know if either of these are a concern right now, > > but I worry that this might become a problem in the future. > > Good point about PID namespaces in the future but those accompanying > records will already have to be conditioned for the PID namespace > context that is requesting it, so I don't see this as a showstopper. Possibly, it just gets very messy. Doubly so when you start looking at potentially adjusting for multiple audit daemons. Thankfully it doesn't look like using the PID is a good idea for other reasons. > I've forgotten about an important one we already hit, which is a network > event that only has a NETFILTER_PKT record, but in that case, there is > no ambiguity since there are no other records associated with that > event. So the second is already an issue now. Using > task_tgid_nr(current), in the contid testsuite script network event it > attributed it to ping which caused the event, but we cannot use this > since it wasn't triggered by a syscall and doesn't accurately reflect > the kernel thread that received it. It could just be set to zero for > network events. Possibly. It just seems like too much hackery to start; that's the stuff you do once it has been in a kernel release for years and need to find a workaround that doesn't break everything. I think we should aim a bit higher right now. > > The idea of using something like "item=" is interesting. As you > > mention, the "item=" field does present some overlap problems with the > > PATH record, but perhaps we can do something similar. What if we > > added a "record=" (or similar, I'm not worried about names at this > > point) to each record, reset to 0/1 at the start of each event, and > > when we needed to link records somehow we could add a "related=1,..,N" > > field. This would potentially be useful beyond just the audit > > container ID work. > > Does it make any sense to use the same keyword in each type of record > such as record/records as in PATH/SYSCALL: item/items ? That was mentioned above, if you can assure yourself and the rest of us that it can be safely reused I think that might be okay, but I'm not convinced that is safe at the moment. Although I will admit those are fears are not based on an exhaustive search through the code (or a determined "think"). > (I prefer 0-indexed like item=...) I have no preference on where we start the index, but it makes sense to keep the same index starting point as the PATH records. -- paul moore www.paul-moore.com
Re: [PATCH net-next] net: netlabel: Fix kerneldoc warnings
On Tue, Oct 27, 2020 at 8:54 PM Andrew Lunn wrote: > > net/netlabel/netlabel_calipso.c:376: warning: Function parameter or member > 'ops' not described in 'netlbl_calipso_ops_register' > > Signed-off-by: Andrew Lunn > --- > net/netlabel/netlabel_calipso.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/netlabel/netlabel_calipso.c b/net/netlabel/netlabel_calipso.c > index 4e62f2ad3575..a4efa99fb1f8 100644 > --- a/net/netlabel/netlabel_calipso.c > +++ b/net/netlabel/netlabel_calipso.c > @@ -366,6 +366,7 @@ static const struct netlbl_calipso_ops *calipso_ops; > > /** > * netlbl_calipso_ops_register - Register the CALIPSO operations > + * @ops: Ops to register If we are being nitpicky, it might be better to drop the capitalization for the sake of consistency, e.g. "@ops: ops to register". Acked-by: Paul Moore > * > * Description: > * Register the CALIPSO packet engine operations. -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls
On Wed, Oct 21, 2020 at 12:39 PM Richard Guy Briggs wrote: > Here is an exmple I was able to generate after updating the testsuite > script to include a signalling example of a nested audit container > identifier: > > > type=PROCTITLE msg=audit(2020-10-21 10:31:16.655:6731) : > proctitle=/usr/bin/perl -w containerid/test > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > contid=7129731255799087104^941723245477888 > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115583 oauid=root > ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > ocomm=perl > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > contid=941723245477888 > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115580 oauid=root > ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > ocomm=perl > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > contid=8098399240850112512^941723245477888 > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115582 oauid=root > ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > ocomm=perl > type=SYSCALL msg=audit(2020-10-21 10:31:16.655:6731) : arch=x86_64 > syscall=kill success=yes exit=0 a0=0xfffe3c84 a1=SIGTERM a2=0x4d524554 a3=0x0 > items=0 ppid=115564 pid=115567 auid=root uid=root gid=root euid=root > suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl > exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > key=testsuite-1603290671-AcLtUulY > > > There are three CONTAINER_ID records which need some way of associating with > OBJ_PID records. An additional CONTAINER_ID record would be present if the > killing process itself had an audit container identifier. I think the most > obvious way to connect them is with a pid= field in the CONTAINER_ID record. Using a "pid=" field as a way to link CONTAINER_ID records to other records raises a few questions. What happens if/when we need to represent those PIDs in the context of a namespace? Are we ever going to need to link to records which don't have a "pid=" field? I haven't done the homework to know if either of these are a concern right now, but I worry that this might become a problem in the future. The idea of using something like "item=" is interesting. As you mention, the "item=" field does present some overlap problems with the PATH record, but perhaps we can do something similar. What if we added a "record=" (or similar, I'm not worried about names at this point) to each record, reset to 0/1 at the start of each event, and when we needed to link records somehow we could add a "related=1,..,N" field. This would potentially be useful beyond just the audit container ID work. -- paul moore www.paul-moore.com
Re: [PATCH 3/3] selinux: Add SELinux GTP support
On Mon, Oct 12, 2020 at 5:40 AM Harald Welte wrote: > > Hi Paul, > > On Sun, Oct 11, 2020 at 10:09:11PM -0400, Paul Moore wrote: > > Harald, Pablo - I know you both suggested taking a slow iterative > > approach to merging functionality, perhaps you could also help those > > of us on the SELinux side better understand some of the common GTP use > > cases? > > There really only is one use case for this code: The GGSN or P-GW function > in the 3GPP network architecture ... > > Hope this helps, > Harald It does, thank you. It looks like this patchset is not really a candidate for merging in its current form, but I didn't want to lose this information (both the patches and Harald's comments) so I created a GH issue to track this at the URL below. * https://github.com/SELinuxProject/selinux-kernel/issues/54 -- paul moore www.paul-moore.com
Re: [PATCH 3/3] selinux: Add SELinux GTP support
On Wed, Sep 30, 2020 at 9:39 AM Harald Welte wrote: > Hi Richard, > > On Wed, Sep 30, 2020 at 01:25:27PM +0100, Richard Haines wrote: > > > As in the reply to Pablo, I did it for no particular reason other than > > idle curiosity, and given the attempted move to Open 5G I thought > > adding MAC support might be useful somewhere along the line. > > ... > > I think it would not be the best idea to merge SELinux support patches for the > GTP kernel driver without thoroughly understanding the use case, and/or having > some actual userspace implementations that make use of them. In the end, we > may > be introducing code that nobody uses, and which only turns out to be > insufficient > for what later actual users may want. > > So like Pablo suggested, it would probably be best to focus on > submitting / merging features for things that are either well-defined (e.g. > specified in a standerd), and/or have existing userspace implementations. Having a solid use case or two is also helpful for those of us who don't have a GTP/GPRS background. I did spend some time reading a few things on GTP, but I don't feel like I've made much of a dent on understanding how it is actually used. Harald, Pablo - I know you both suggested taking a slow iterative approach to merging functionality, perhaps you could also help those of us on the SELinux side better understand some of the common GTP use cases? -- paul moore www.paul-moore.com
Re: [PATCH 4/5] net/ipv6: use semicolons rather than commas to separate statements
On Sun, Oct 11, 2020 at 7:18 AM Julia Lawall wrote: > > Replace commas with semicolons. Commas introduce unnecessary > variability in the code structure and are hard to see. What is done > is essentially described by the following Coccinelle semantic patch > (http://coccinelle.lip6.fr/): > > // > @@ expression e1,e2; @@ > e1 > -, > +; > e2 > ... when any > // > > Signed-off-by: Julia Lawall > > --- > net/ipv6/calipso.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks Julia. Acked-by: Paul Moore > diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c > index 8d3f66c310db..78f766019b7e 100644 > --- a/net/ipv6/calipso.c > +++ b/net/ipv6/calipso.c > @@ -761,7 +761,7 @@ static int calipso_genopt(unsigned char *buf, u32 start, > u32 buf_len, > calipso[1] = len - 2; > *(__be32 *)(calipso + 2) = htonl(doi_def->doi); > calipso[6] = (len - CALIPSO_HDR_LEN) / 4; > - calipso[7] = secattr->attr.mls.lvl, > + calipso[7] = secattr->attr.mls.lvl; > crc = ~crc_ccitt(0xffff, calipso, len); > calipso[8] = crc & 0xff; > calipso[9] = (crc >> 8) & 0xff; -- paul moore www.paul-moore.com
Re: [RFC PATCH] lsm,selinux: pass the family information along with xfrm flow
On Tue, Sep 29, 2020 at 7:09 PM James Morris wrote: > I'm not keen on adding a parameter which nobody is using. Perhaps a note > in the header instead? On Wed, Sep 30, 2020 at 6:14 AM Herbert Xu wrote: > Please at least change to the struct flowi to flowi_common if we're > not adding a family field. It did feel a bit weird adding a (currently) unused parameter, so I can understand the concern, I just worry that a comment in the code will be easily overlooked. I also thought about passing a pointer to the nested flowi_common struct, but it doesn't appear that this is done anywhere else in the stack so it felt wrong to do it here. -- paul moore www.paul-moore.com
[RFC PATCH] lsm,selinux: pass the family information along with xfrm flow
As pointed out by Herbert in a recent related patch, the LSM hooks
should pass the address family in addition to the xfrm flow as the
family information is needed to safely access the flow.
While this is not technically a problem for the current LSM/SELinux
code as it only accesses fields common to all address families, we
should still pass the address family so that the LSM hook isn't
inherently flawed. An alternate solution could be to simply pass
the LSM secid instead of flow, but this introduces the problem of
the LSM hook callers sending the wrong secid which would be much
worse.
Reported-by: Herbert Xu
Signed-off-by: Paul Moore
---
include/linux/lsm_hook_defs.h |2 +-
include/linux/lsm_hooks.h |1 +
include/linux/security.h|7 +--
net/xfrm/xfrm_state.c |4 ++--
security/security.c |5 +++--
security/selinux/include/xfrm.h |3 ++-
security/selinux/xfrm.c |3 ++-
7 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2a8c74d99015..e3c3b5d20469 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -349,7 +349,7 @@ LSM_HOOK(int, 0, xfrm_state_delete_security, struct
xfrm_state *x)
LSM_HOOK(int, 0, xfrm_policy_lookup, struct xfrm_sec_ctx *ctx, u32 fl_secid,
u8 dir)
LSM_HOOK(int, 1, xfrm_state_pol_flow_match, struct xfrm_state *x,
-struct xfrm_policy *xp, const struct flowi *fl)
+struct xfrm_policy *xp, const struct flowi *fl, unsigned short family)
LSM_HOOK(int, 0, xfrm_decode_session, struct sk_buff *skb, u32 *secid,
int ckall)
#endif /* CONFIG_SECURITY_NETWORK_XFRM */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 9e2e3e63719d..ea088aacfdad 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1093,6 +1093,7 @@
* @x contains the state to match.
* @xp contains the policy to check for a match.
* @fl contains the flow to check for a match.
+ * @family the flow's address family.
* Return 1 if there is a match.
* @xfrm_decode_session:
* @skb points to skb to decode.
diff --git a/include/linux/security.h b/include/linux/security.h
index 0a0a03b36a3b..701b41eb090c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1625,7 +1625,8 @@ void security_xfrm_state_free(struct xfrm_state *x);
int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8
dir);
int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
struct xfrm_policy *xp,
- const struct flowi *fl);
+ const struct flowi *fl,
+ unsigned short family);
int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid);
void security_skb_classify_flow(struct sk_buff *skb, struct flowi *fl);
@@ -1679,7 +1680,9 @@ static inline int security_xfrm_policy_lookup(struct
xfrm_sec_ctx *ctx, u32 fl_s
}
static inline int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
- struct xfrm_policy *xp, const struct flowi *fl)
+struct xfrm_policy *xp,
+const struct flowi *fl,
+unsigned short family)
{
return 1;
}
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 69520ad3d83b..f90d2f1da44a 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1020,7 +1020,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol,
struct xfrm_state *x,
if (x->km.state == XFRM_STATE_VALID) {
if ((x->sel.family &&
!xfrm_selector_match(&x->sel, fl, x->sel.family)) ||
- !security_xfrm_state_pol_flow_match(x, pol, fl))
+ !security_xfrm_state_pol_flow_match(x, pol, fl, family))
return;
if (!*best ||
@@ -1033,7 +1033,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol,
struct xfrm_state *x,
} else if (x->km.state == XFRM_STATE_ERROR ||
x->km.state == XFRM_STATE_EXPIRED) {
if (xfrm_selector_match(&x->sel, fl, x->sel.family) &&
- security_xfrm_state_pol_flow_match(x, pol, fl))
+ security_xfrm_state_pol_flow_match(x, pol, fl, family))
*error = -ESRCH;
}
}
diff --git a/security/security.c b/security/security.c
index 70a7ad357bc6..62dd0af7c6bc 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2391,7 +2391,8 @@ int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx,
u32 fl_secid, u8 dir)
int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
Re: KASAN: stack-out-of-bounds Read in xfrm_selector_match (2)
On Thu, Sep 24, 2020 at 11:08 PM Herbert Xu wrote: > On Mon, Sep 21, 2020 at 07:56:20AM -0700, syzbot wrote: > > Hello, > > > > syzbot found the following issue on: > > > > HEAD commit:eb5f95f1 Merge tag 's390-5.9-6' of git://git.kernel.org/pu.. > > git tree: upstream > > console output: https://syzkaller.appspot.com/x/log.txt?x=13996ad590 > > kernel config: https://syzkaller.appspot.com/x/.config?x=ffe85b197a57c180 > > dashboard link: https://syzkaller.appspot.com/bug?extid=577fbac3145a6eb2e7a5 > > compiler: gcc (GCC) 10.1.0-syz 20200507 > > > > Unfortunately, I don't have any reproducer for this issue yet. > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > > Reported-by: syzbot+577fbac3145a6eb2e...@syzkaller.appspotmail.com > > > > == > > BUG: KASAN: stack-out-of-bounds in xfrm_flowi_dport include/net/xfrm.h:877 > > [inline] > > BUG: KASAN: stack-out-of-bounds in __xfrm6_selector_match > > net/xfrm/xfrm_policy.c:216 [inline] > > BUG: KASAN: stack-out-of-bounds in xfrm_selector_match+0xf36/0xf60 > > net/xfrm/xfrm_policy.c:229 > > Read of size 2 at addr c9001914f55c by task syz-executor.4/15633 > > > > CPU: 0 PID: 15633 Comm: syz-executor.4 Not tainted 5.9.0-rc5-syzkaller #0 > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > Google 01/01/2011 > > Call Trace: > > __dump_stack lib/dump_stack.c:77 [inline] > > dump_stack+0x198/0x1fd lib/dump_stack.c:118 > > print_address_description.constprop.0.cold+0x5/0x497 mm/kasan/report.c:383 > > __kasan_report mm/kasan/report.c:513 [inline] > > kasan_report.cold+0x1f/0x37 mm/kasan/report.c:530 > > xfrm_flowi_dport include/net/xfrm.h:877 [inline] > > This one goes back more than ten years. This patch should fix > it. > > ---8<--- > The struct flowi must never be interpreted by itself as its size > depends on the address family. Therefore it must always be grouped > with its original family value. > > In this particular instance, the original family value is lost in > the function xfrm_state_find. Therefore we get a bogus read when > it's coupled with the wrong family which would occur with inter- > family xfrm states. > > This patch fixes it by keeping the original family value. > > Note that the same bug could potentially occur in LSM through > the xfrm_state_pol_flow_match hook. I checked the current code > there and it seems to be safe for now as only secid is used which > is part of struct flowi_common. But that API should be changed > so that so that we don't get new bugs in the future. We could > do that by replacing fl with just secid or adding a family field. I'm thinking it might be better to pass the family along with the flow instead of passing just the secid (less worry of passing an incorrect secid that way). Let me see if I can cobble together a quick patch for testing before bed ... -- paul moore www.paul-moore.com
Re: [PATCH net-next] netlabel: Fix some kernel-doc warnings
On Tue, Sep 8, 2020 at 10:09 AM Wang Hai wrote: > > Fixes the following W=1 kernel build warning(s): > > net/netlabel/netlabel_calipso.c:438: warning: Excess function parameter > 'audit_secid' description in 'calipso_doi_remove' > net/netlabel/netlabel_calipso.c:605: warning: Excess function parameter 'reg' > description in 'calipso_req_delattr' > > Reported-by: Hulk Robot > Signed-off-by: Wang Hai > --- > net/netlabel/netlabel_calipso.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Looks good to me, thanks. Acked-by: Paul Moore > diff --git a/net/netlabel/netlabel_calipso.c b/net/netlabel/netlabel_calipso.c > index 249da67d50a2..1a98247ab148 100644 > --- a/net/netlabel/netlabel_calipso.c > +++ b/net/netlabel/netlabel_calipso.c > @@ -426,7 +426,7 @@ void calipso_doi_free(struct calipso_doi *doi_def) > /** > * calipso_doi_remove - Remove an existing DOI from the CALIPSO protocol > engine > * @doi: the DOI value > - * @audit_secid: the LSM secid to use in the audit message > + * @audit_info: NetLabel audit information > * > * Description: > * Removes a DOI definition from the CALIPSO engine. The NetLabel routines > will > @@ -595,7 +595,7 @@ int calipso_req_setattr(struct request_sock *req, > > /** > * calipso_req_delattr - Delete the CALIPSO option from a request socket > - * @reg: the request socket > + * @req: the request socket > * > * Description: > * Removes the CALIPSO option from a request socket, if present. > -- > 2.17.1 > -- paul moore www.paul-moore.com
Re: [PATCH net-next] cipso: fix 'audit_secid' kernel-doc warning in cipso_ipv4.c
On Tue, Sep 8, 2020 at 10:02 AM Wang Hai wrote: > > Fixes the following W=1 kernel build warning(s): > > net/ipv4/cipso_ipv4.c:510: warning: Excess function parameter 'audit_secid' > description in 'cipso_v4_doi_remove' > > Reported-by: Hulk Robot > Signed-off-by: Wang Hai > --- > net/ipv4/cipso_ipv4.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks for catching this and submitting the fix. Acked-by: Paul Moore > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > index 2eb71579f4d2..471d33a0d095 100644 > --- a/net/ipv4/cipso_ipv4.c > +++ b/net/ipv4/cipso_ipv4.c > @@ -498,7 +498,7 @@ static void cipso_v4_doi_free_rcu(struct rcu_head *entry) > /** > * cipso_v4_doi_remove - Remove an existing DOI from the CIPSO protocol > engine > * @doi: the DOI value > - * @audit_secid: the LSM secid to use in the audit message > + * @audit_info: NetLabel audit information > * > * Description: > * Removes a DOI definition from the CIPSO engine. The NetLabel routines > will > -- > 2.17.1 > -- paul moore www.paul-moore.com
Re: [PATCH v20 18/23] NET: Store LSM netlabel data in a lsmblob
On Wed, Aug 26, 2020 at 11:21 AM Casey Schaufler wrote: > > Netlabel uses LSM interfaces requiring an lsmblob and > the internal storage is used to pass information between > these interfaces, so change the internal data from a secid > to a lsmblob. Update the netlabel interfaces and their > callers to accommodate the change. This requires that the > modules using netlabel use the lsm_id.slot to access the > correct secid when using netlabel. > > Reviewed-by: Kees Cook > Reviewed-by: John Johansen > Acked-by: Stephen Smalley > Signed-off-by: Casey Schaufler > Cc: netdev@vger.kernel.org > --- > include/net/netlabel.h | 8 +-- > net/ipv4/cipso_ipv4.c | 27 ++ > net/netlabel/netlabel_kapi.c| 6 +-- > net/netlabel/netlabel_unlabeled.c | 79 + > net/netlabel/netlabel_unlabeled.h | 2 +- > security/selinux/hooks.c| 2 +- > security/selinux/include/security.h | 1 + > security/selinux/netlabel.c | 2 +- > security/selinux/ss/services.c | 4 +- > security/smack/smack.h | 1 + > security/smack/smack_lsm.c | 5 +- > security/smack/smackfs.c| 10 ++-- > 12 files changed, 65 insertions(+), 82 deletions(-) Minor change suggested to a comment below, but looks good otherwise. Acked-by: Paul Moore > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > index 2eb71579f4d2..8182b923e802 100644 > --- a/net/ipv4/cipso_ipv4.c > +++ b/net/ipv4/cipso_ipv4.c > @@ -106,15 +106,17 @@ int cipso_v4_rbm_strictvalid = 1; > /* Base length of the local tag (non-standard tag). > * Tag definition (may change between kernel versions) > * > - * 0 8 16 24 32 > - * +--+--+--+--+ > - * | 1000 | 0110 | 32-bit secid value | > - * +--+--+--+--+ > - * | in (host byte order)| > - * +--+--+ > - * > + * 0 8 1616 + sizeof(struct lsmblob) > + * +--+--+-+ > + * | 1000 | 0110 | LSM blob data | > + * +--+--+-+ > + * > + * All secid and flag fields are in host byte order. > + * The lsmblob structure size varies depending on which > + * Linux security modules are built in the kernel. > + * The data is opaque. > */ > -#define CIPSO_V4_TAG_LOC_BLEN 6 > +#define CIPSO_V4_TAG_LOC_BLEN (2 + sizeof(struct lsmblob)) > > /* > * Helper Functions > @@ -1469,7 +1471,12 @@ static int cipso_v4_gentag_loc(const struct > cipso_v4_doi *doi_def, > > buffer[0] = CIPSO_V4_TAG_LOCAL; > buffer[1] = CIPSO_V4_TAG_LOC_BLEN; > - *(u32 *)&buffer[2] = secattr->attr.secid; > + /* Ensure that there is sufficient space in the CIPSO header > +* for the LSM data. This should never become an issue. > +* The check is made from an abundance of caution. */ > + BUILD_BUG_ON(CIPSO_V4_TAG_LOC_BLEN > CIPSO_V4_OPT_LEN_MAX); I like the BUILD_BUG_ON() check, but for reasons very similar to the unix_skb_params changes I don't really like the "should never become an issue" commentary. Granted, it is unlikely, but I don't think it is wise to thumb our nose at fate. > + memcpy(&buffer[2], &secattr->attr.lsmblob, > + sizeof(secattr->attr.lsmblob)); > > return CIPSO_V4_TAG_LOC_BLEN; > } -- paul moore www.paul-moore.com
Re: [PATCH v20 17/23] LSM: security_secid_to_secctx in netlink netfilter
On Wed, Aug 26, 2020 at 11:20 AM Casey Schaufler wrote:
>
> Change netlink netfilter interfaces to use lsmcontext
> pointers, and remove scaffolding.
>
> Reviewed-by: Kees Cook
> Reviewed-by: John Johansen
> Acked-by: Stephen Smalley
> Signed-off-by: Casey Schaufler
> cc: netdev@vger.kernel.org
> ---
> net/netfilter/nfnetlink_queue.c | 31 ---
> 1 file changed, 12 insertions(+), 19 deletions(-)
...
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index d3f8e808c5d3..c830401f7792 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -401,8 +399,7 @@ nfqnl_build_packet_message(struct net *net, struct
> nfqnl_instance *queue,
> enum ip_conntrack_info ctinfo;
> struct nfnl_ct_hook *nfnl_ct;
> bool csum_verify;
> - struct lsmcontext scaff; /* scaffolding */
> - char *secdata = NULL;
> + struct lsmcontext context = { };
> u32 seclen = 0;
>
> size = nlmsg_total_size(sizeof(struct nfgenmsg))
> @@ -469,7 +466,7 @@ nfqnl_build_packet_message(struct net *net, struct
> nfqnl_instance *queue,
> }
>
> if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
> - seclen = nfqnl_get_sk_secctx(entskb, &secdata);
> + seclen = nfqnl_get_sk_secctx(entskb, &context);
> if (seclen)
> size += nla_total_size(seclen);
> }
I think we can get rid of the local "seclen" variable, right? We can
embed the nfqnl_get_sk_secctx() in the conditional and then simply
reference "context.len" everywhere else, yes? For example:
if (nfqnl_get_sk_secctx(..., &context))
size += nla_total_size(context.len);
--
paul moore
www.paul-moore.com
Re: [PATCH v20 14/23] LSM: Ensure the correct LSM context releaser
On Wed, Aug 26, 2020 at 11:16 AM Casey Schaufler wrote:
>
> Add a new lsmcontext data structure to hold all the information
> about a "security context", including the string, its size and
> which LSM allocated the string. The allocation information is
> necessary because LSMs have different policies regarding the
> lifecycle of these strings. SELinux allocates and destroys
> them on each use, whereas Smack provides a pointer to an entry
> in a list that never goes away.
>
> Reviewed-by: Kees Cook
> Reviewed-by: John Johansen
> Acked-by: Stephen Smalley
> Signed-off-by: Casey Schaufler
> Cc: linux-integr...@vger.kernel.org
> Cc: netdev@vger.kernel.org
> ---
> drivers/android/binder.c| 10 ---
> fs/ceph/xattr.c | 6 -
> fs/nfs/nfs4proc.c | 8 --
> fs/nfsd/nfs4xdr.c | 7 +++--
> include/linux/security.h| 35 +++--
> include/net/scm.h | 5 +++-
> kernel/audit.c | 14 +++---
> kernel/auditsc.c| 12 ++---
> net/ipv4/ip_sockglue.c | 4 ++-
> net/netfilter/nf_conntrack_netlink.c| 4 ++-
> net/netfilter/nf_conntrack_standalone.c | 4 ++-
> net/netfilter/nfnetlink_queue.c | 13 ++---
> net/netlabel/netlabel_unlabeled.c | 19 +++---
> net/netlabel/netlabel_user.c| 4 ++-
> security/security.c | 11
> 15 files changed, 121 insertions(+), 35 deletions(-)
One small comment below, but otherwise ...
Acked-by: Paul Moore
> +/**
> + * lsmcontext_init - initialize an lsmcontext structure.
> + * @cp: Pointer to the context to initialize
> + * @context: Initial context, or NULL
> + * @size: Size of context, or 0
> + * @slot: Which LSM provided the context
> + *
> + * Fill in the lsmcontext from the provided information.
> + * This is a scaffolding function that will be removed when
> + * lsmcontext integration is complete.
> + */
> +static inline void lsmcontext_init(struct lsmcontext *cp, char *context,
> + u32 size, int slot)
> +{
> + cp->slot = slot;
> + cp->context = context;
> + cp->len = size;
> +}
Here is another case where some of the intermediate code, and perhaps
some of the final code, can probably be simplified if
lsmcontext_init() returns the lsmcontext pointer.
--
paul moore
www.paul-moore.com
Re: [PATCH v20 06/23] LSM: Use lsmblob in security_secctx_to_secid
On Wed, Aug 26, 2020 at 11:08 AM Casey Schaufler wrote:
> Change security_secctx_to_secid() to fill in a lsmblob instead
> of a u32 secid. Multiple LSMs may be able to interpret the
> string, and this allows for setting whichever secid is
> appropriate. Change security_secmark_relabel_packet() to use a
> lsmblob instead of a u32 secid. In some other cases there is
> scaffolding where interfaces have yet to be converted.
>
> Reviewed-by: Kees Cook
> Signed-off-by: Casey Schaufler
> Cc: netdev@vger.kernel.org
> ---
> include/linux/security.h | 30 +++
> include/net/scm.h | 7 +--
> kernel/cred.c | 4 +---
> net/ipv4/ip_sockglue.c| 6 --
> net/netfilter/nft_meta.c | 18 +---
> net/netfilter/xt_SECMARK.c| 9 ++--
> net/netlabel/netlabel_unlabeled.c | 23 +
> security/security.c | 34 ++-
> 8 files changed, 98 insertions(+), 33 deletions(-)
I imagine there may be ways around the xt_secmark_target_info
limitation, but that would require userspace changes to take advantage
of it, and the way forward is clearly nftables so it probably isn't
worth the effort.
I'm okay with this patch with the understanding that several chunks in
the patch are replaced by later patches in the series.
Acked-by: Paul Moore
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ae623b89cdf4..f8770c228356 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -190,6 +190,27 @@ static inline bool lsmblob_equal(struct lsmblob *bloba,
> struct lsmblob *blobb)
> return !memcmp(bloba, blobb, sizeof(*bloba));
> }
>
> +/**
> + * lsmblob_value - find the first non-zero value in an lsmblob structure.
> + * @blob: Pointer to the data
> + *
> + * This needs to be used with extreme caution, as the cases where
> + * it is appropriate are rare.
> + *
> + * Return the first secid value set in the lsmblob.
> + * There should only be one.
> + */
> +static inline u32 lsmblob_value(const struct lsmblob *blob)
> +{
> + int i;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++)
> + if (blob->secid[i])
> + return blob->secid[i];
> +
> + return 0;
> +}
> +
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
>int cap, unsigned int opts);
> @@ -503,7 +524,8 @@ int security_setprocattr(const char *lsm, const char
> *name, void *value,
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
> +int security_secctx_to_secid(const char *secdata, u32 seclen,
> +struct lsmblob *blob);
> void security_release_secctx(char *secdata, u32 seclen);
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> @@ -1322,7 +1344,7 @@ static inline int security_secid_to_secctx(u32 secid,
> char **secdata, u32 *secle
>
> static inline int security_secctx_to_secid(const char *secdata,
>u32 seclen,
> - u32 *secid)
> + struct lsmblob *blob)
> {
> return -EOPNOTSUPP;
> }
> @@ -1412,7 +1434,7 @@ void security_inet_csk_clone(struct sock *newsk,
> const struct request_sock *req);
> void security_inet_conn_established(struct sock *sk,
> struct sk_buff *skb);
> -int security_secmark_relabel_packet(u32 secid);
> +int security_secmark_relabel_packet(struct lsmblob *blob);
> void security_secmark_refcount_inc(void);
> void security_secmark_refcount_dec(void);
> int security_tun_dev_alloc_security(void **security);
> @@ -1585,7 +1607,7 @@ static inline void
> security_inet_conn_established(struct sock *sk,
> {
> }
>
> -static inline int security_secmark_relabel_packet(u32 secid)
> +static inline int security_secmark_relabel_packet(struct lsmblob *blob)
> {
> return 0;
> }
> diff --git a/include/net/scm.h b/include/net/scm.h
> index e2e71c4bf9d0..c09f2dfeec88 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -97,8 +97,11 @@ static inline void scm_passec(struct socket *sock, struct
> msghdr *msg, struct sc
> int err;
>
> if (test_
Re: [PATCH v2] netlabel: remove unused param from audit_log_format()
On Fri, Aug 28, 2020 at 9:56 AM Alex Dewar wrote:
>
> Commit d3b990b7f327 ("netlabel: fix problems with mapping removal")
> added a check to return an error if ret_val != 0, before ret_val is
> later used in a log message. Now it will unconditionally print "...
> res=1". So just drop the check.
>
> Addresses-Coverity: ("Dead code")
> Fixes: d3b990b7f327 ("netlabel: fix problems with mapping removal")
> Signed-off-by: Alex Dewar
> ---
> v2: Still print the res field, because it's useful (Paul)
>
> net/netlabel/netlabel_domainhash.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Thanks Alex.
Acked-by: Paul Moore
> diff --git a/net/netlabel/netlabel_domainhash.c
> b/net/netlabel/netlabel_domainhash.c
> index f73a8382c275e..dc8c39f51f7d3 100644
> --- a/net/netlabel/netlabel_domainhash.c
> +++ b/net/netlabel/netlabel_domainhash.c
> @@ -612,9 +612,8 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map
> *entry,
> audit_buf = netlbl_audit_start_common(AUDIT_MAC_MAP_DEL, audit_info);
> if (audit_buf != NULL) {
> audit_log_format(audit_buf,
> -" nlbl_domain=%s res=%u",
> -entry->domain ? entry->domain : "(default)",
> -ret_val == 0 ? 1 : 0);
> +" nlbl_domain=%s res=1",
> +entry->domain ? entry->domain : "(default)");
> audit_log_end(audit_buf);
> }
>
> --
> 2.28.0
>
--
paul moore
www.paul-moore.com
Re: [PATCH RFC] netlabel: remove unused param from audit_log_format()
On Thu, Aug 27, 2020 at 1:20 PM Alex Dewar wrote:
> On Thu, Aug 27, 2020 at 06:06:34PM +0100, Alex Dewar wrote:
> > On Thu, Aug 27, 2020 at 01:00:58PM -0400, Paul Moore wrote:
> > > On Thu, Aug 27, 2020 at 12:39 PM Alex Dewar
> > > wrote:
> > > >
> > > > Commit d3b990b7f327 ("netlabel: fix problems with mapping removal")
> > > > added a check to return an error if ret_val != 0, before ret_val is
> > > > later used in a log message. Now it will unconditionally print "...
> > > > res=0". So don't print res anymore.
> > > >
> > > > Addresses-Coverity: ("Dead code")
> > > > Fixes: d3b990b7f327 ("netlabel: fix problems with mapping removal")
> > > > Signed-off-by: Alex Dewar
> > > > ---
> > > >
> > > > I wasn't sure whether it was intended that something other than ret_val
> > > > be printed in the log, so that's why I'm sending this as an RFC.
> > >
> > > It's intentional for a couple of reasons:
> > >
> > > * The people who care about audit logs like to see success/fail (e.g.
> > > "res=X") for audit events/records, so printing this out gives them the
> > > warm fuzzies.
> > >
> > > * For a lot of awful reasons that I won't bore you with, we really
> > > don't want to add/remove fields in the middle of an audit record so we
> > > pretty much need to keep the "res=0" there even if it seems a bit
> > > redundant.
> > >
> > > So NACK from me, but thanks for paying attention just the same :)
> >
> > Would you rather just have an explicit "res=0" in there, without looking
> > at ret_val? The thing is that ret_val will *always* be zero at this point in
> > the code, because, if not, the function will already have returned.
> > That's why Coverity flagged it up as a redundant check.
>
> Sorry, I meant "res=1". The code will always print res=1, because
> ret_val is always 0.
That's okay, I can't tell you how many times I've made that mistake
with "res=" :)
Anyway, yes at that point ret_val should always be 0, and "res=X"
should always be "res=1", so if you wanted to change it to a fixed
value so you could get rid of the ternary statement that would be
okay.
> > > > net/netlabel/netlabel_domainhash.c | 5 ++---
> > > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/net/netlabel/netlabel_domainhash.c
> > > > b/net/netlabel/netlabel_domainhash.c
> > > > index f73a8382c275..526762b2f3a9 100644
> > > > --- a/net/netlabel/netlabel_domainhash.c
> > > > +++ b/net/netlabel/netlabel_domainhash.c
> > > > @@ -612,9 +612,8 @@ int netlbl_domhsh_remove_entry(struct
> > > > netlbl_dom_map *entry,
> > > > audit_buf = netlbl_audit_start_common(AUDIT_MAC_MAP_DEL,
> > > > audit_info);
> > > > if (audit_buf != NULL) {
> > > > audit_log_format(audit_buf,
> > > > -" nlbl_domain=%s res=%u",
> > > > -entry->domain ? entry->domain :
> > > > "(default)",
> > > > -ret_val == 0 ? 1 : 0);
> > > > +" nlbl_domain=%s",
> > > > +entry->domain ? entry->domain :
> > > > "(default)");
> > > > audit_log_end(audit_buf);
> > > > }
> > > >
--
paul moore
www.paul-moore.com
Re: [PATCH RFC] netlabel: remove unused param from audit_log_format()
On Thu, Aug 27, 2020 at 12:39 PM Alex Dewar wrote:
>
> Commit d3b990b7f327 ("netlabel: fix problems with mapping removal")
> added a check to return an error if ret_val != 0, before ret_val is
> later used in a log message. Now it will unconditionally print "...
> res=0". So don't print res anymore.
>
> Addresses-Coverity: ("Dead code")
> Fixes: d3b990b7f327 ("netlabel: fix problems with mapping removal")
> Signed-off-by: Alex Dewar
> ---
>
> I wasn't sure whether it was intended that something other than ret_val
> be printed in the log, so that's why I'm sending this as an RFC.
It's intentional for a couple of reasons:
* The people who care about audit logs like to see success/fail (e.g.
"res=X") for audit events/records, so printing this out gives them the
warm fuzzies.
* For a lot of awful reasons that I won't bore you with, we really
don't want to add/remove fields in the middle of an audit record so we
pretty much need to keep the "res=0" there even if it seems a bit
redundant.
So NACK from me, but thanks for paying attention just the same :)
> net/netlabel/netlabel_domainhash.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/netlabel/netlabel_domainhash.c
> b/net/netlabel/netlabel_domainhash.c
> index f73a8382c275..526762b2f3a9 100644
> --- a/net/netlabel/netlabel_domainhash.c
> +++ b/net/netlabel/netlabel_domainhash.c
> @@ -612,9 +612,8 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map
> *entry,
> audit_buf = netlbl_audit_start_common(AUDIT_MAC_MAP_DEL, audit_info);
> if (audit_buf != NULL) {
> audit_log_format(audit_buf,
> -" nlbl_domain=%s res=%u",
> -entry->domain ? entry->domain : "(default)",
> -ret_val == 0 ? 1 : 0);
> +" nlbl_domain=%s",
> +entry->domain ? entry->domain : "(default)");
> audit_log_end(audit_buf);
> }
>
--
paul moore
www.paul-moore.com
Re: [net-next PATCH] netlabel: fix problems with mapping removal
On Fri, Aug 21, 2020 at 2:38 PM David Miller wrote: > From: Paul Moore > Date: Thu, 20 Aug 2020 21:46:14 -0400 > > > This patch fixes two main problems seen when removing NetLabel > > mappings: memory leaks and potentially extra audit noise. > > These are bug fixes therefore this needs to target the 'net' tree > and you must also provide appropriate "Fixes:" tags. > > Thank you. Reposted as requested: https://lore.kernel.org/selinux/159804209207.16190.14955035148979265114.stgit@sifl -- paul moore www.paul-moore.com
[net PATCH] netlabel: fix problems with mapping removal
This patch fixes two main problems seen when removing NetLabel
mappings: memory leaks and potentially extra audit noise.
The memory leaks are caused by not properly free'ing the mapping's
address selector struct when free'ing the entire entry as well as
not properly cleaning up a temporary mapping entry when adding new
address selectors to an existing entry. This patch fixes both these
problems such that kmemleak reports no NetLabel associated leaks
after running the SELinux test suite.
The potentially extra audit noise was caused by the auditing code in
netlbl_domhsh_remove_entry() being called regardless of the entry's
validity. If another thread had already marked the entry as invalid,
but not removed/free'd it from the list of mappings, then it was
possible that an additional mapping removal audit record would be
generated. This patch fixes this by returning early from the removal
function when the entry was previously marked invalid. This change
also had the side benefit of improving the code by decreasing the
indentation level of large chunk of code by one (accounting for most
of the diffstat).
Fixes: 63c416887437 ("netlabel: Add network address selectors to the
NetLabel/LSM domain mapping")
Reported-by: Stephen Smalley
Signed-off-by: Paul Moore
---
net/netlabel/netlabel_domainhash.c | 59 ++--
1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/net/netlabel/netlabel_domainhash.c
b/net/netlabel/netlabel_domainhash.c
index d07de2c0fbc7..f73a8382c275 100644
--- a/net/netlabel/netlabel_domainhash.c
+++ b/net/netlabel/netlabel_domainhash.c
@@ -85,6 +85,7 @@ static void netlbl_domhsh_free_entry(struct rcu_head *entry)
kfree(netlbl_domhsh_addr6_entry(iter6));
}
#endif /* IPv6 */
+ kfree(ptr->def.addrsel);
}
kfree(ptr->domain);
kfree(ptr);
@@ -537,6 +538,8 @@ int netlbl_domhsh_add(struct netlbl_dom_map *entry,
goto add_return;
}
#endif /* IPv6 */
+ /* cleanup the new entry since we've moved everything over */
+ netlbl_domhsh_free_entry(&entry->rcu);
} else
ret_val = -EINVAL;
@@ -580,6 +583,12 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map
*entry,
{
int ret_val = 0;
struct audit_buffer *audit_buf;
+ struct netlbl_af4list *iter4;
+ struct netlbl_domaddr4_map *map4;
+#if IS_ENABLED(CONFIG_IPV6)
+ struct netlbl_af6list *iter6;
+ struct netlbl_domaddr6_map *map6;
+#endif /* IPv6 */
if (entry == NULL)
return -ENOENT;
@@ -597,6 +606,9 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map *entry,
ret_val = -ENOENT;
spin_unlock(&netlbl_domhsh_lock);
+ if (ret_val)
+ return ret_val;
+
audit_buf = netlbl_audit_start_common(AUDIT_MAC_MAP_DEL, audit_info);
if (audit_buf != NULL) {
audit_log_format(audit_buf,
@@ -606,40 +618,29 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map
*entry,
audit_log_end(audit_buf);
}
- if (ret_val == 0) {
- struct netlbl_af4list *iter4;
- struct netlbl_domaddr4_map *map4;
-#if IS_ENABLED(CONFIG_IPV6)
- struct netlbl_af6list *iter6;
- struct netlbl_domaddr6_map *map6;
-#endif /* IPv6 */
-
- switch (entry->def.type) {
- case NETLBL_NLTYPE_ADDRSELECT:
- netlbl_af4list_foreach_rcu(iter4,
-&entry->def.addrsel->list4) {
- map4 = netlbl_domhsh_addr4_entry(iter4);
- cipso_v4_doi_putdef(map4->def.cipso);
- }
+ switch (entry->def.type) {
+ case NETLBL_NLTYPE_ADDRSELECT:
+ netlbl_af4list_foreach_rcu(iter4, &entry->def.addrsel->list4) {
+ map4 = netlbl_domhsh_addr4_entry(iter4);
+ cipso_v4_doi_putdef(map4->def.cipso);
+ }
#if IS_ENABLED(CONFIG_IPV6)
- netlbl_af6list_foreach_rcu(iter6,
-&entry->def.addrsel->list6) {
- map6 = netlbl_domhsh_addr6_entry(iter6);
- calipso_doi_putdef(map6->def.calipso);
- }
+ netlbl_af6list_foreach_rcu(iter6, &entry->def.addrsel->list6) {
+ map6 = netlbl_domhsh_addr6_entry(iter6);
+ calipso_doi_putdef(map6->def.calipso);
+ }
#endif /* IPv6 */
- break;
- case NETLBL_NLTYPE_CIPSOV4:
- cipso_v4_doi_putdef(entry->def.cipso);
-
Re: [PATCH ghak90 V9 11/13] audit: contid check descendancy and nesting
On Fri, Aug 7, 2020 at 1:10 PM Richard Guy Briggs wrote: > On 2020-07-05 11:11, Paul Moore wrote: > > On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs wrote: > > > Require the target task to be a descendant of the container > > > orchestrator/engine. If you want to get formal about this, you need to define "target" in the sentence above. Target of what? FWIW, I read the above to basically mean that a task can only set the audit container ID of processes which are beneath it in the "process tree" where the "process tree" is defined as the relationship between a parent and children processes such that the children processes are branches below the parent process. I have no problem with that, with the understanding that nesting complicates it somewhat. For example, this isn't true when one of the children is a nested orchestrator, is it? > > > You would only change the audit container ID from one set or inherited > > > value to another if you were nesting containers. I thought we decided we were going to allow an orchestrator to move a process between audit container IDs, yes? no? > > > If changing the contid, the container orchestrator/engine must be a > > > descendant and not same orchestrator as the one that set it so it is not > > > possible to change the contid of another orchestrator's container. Try rephrasing the above please, it isn't clear to me what you are trying to say. > Are we able to agree on the premises above? Is anything asserted that > should not be and is there anything missing? See above. If you want to go back to the definitions/assumptions stage, it probably isn't worth worrying about the other comments until we get the above sorted. -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V9 02/13] audit: add container id
On Wed, Jul 29, 2020 at 4:06 PM Richard Guy Briggs wrote:
> On 2020-07-05 11:09, Paul Moore wrote:
> > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote:
...
> > > @@ -212,6 +219,33 @@ void __init audit_task_init(void)
> > > 0, SLAB_PANIC, NULL);
> > > }
> > >
> > > +/* rcu_read_lock must be held by caller unless new */
> > > +static struct audit_contobj *_audit_contobj_hold(struct audit_contobj
> > > *cont)
> > > +{
> > > + if (cont)
> > > + refcount_inc(&cont->refcount);
> > > + return cont;
> > > +}
> > > +
> > > +static struct audit_contobj *_audit_contobj_get(struct task_struct *tsk)
> > > +{
> > > + if (!tsk->audit)
> > > + return NULL;
> > > + return _audit_contobj_hold(tsk->audit->cont);
> > > +}
> > > +
> > > +/* rcu_read_lock must be held by caller */
> > > +static void _audit_contobj_put(struct audit_contobj *cont)
> > > +{
> > > + if (!cont)
> > > + return;
> > > + if (refcount_dec_and_test(&cont->refcount)) {
> > > + put_task_struct(cont->owner);
> > > + list_del_rcu(&cont->list);
> >
> > You should check your locking; I'm used to seeing exclusive locks
> > (e.g. the spinlock) around list adds/removes, it just reads/traversals
> > that can be done with just the RCU lock held.
>
> Ok, I've redone the locking yet again. I knew this on one level but
> that didn't translate consistently to code...
>
> > > + kfree_rcu(cont, rcu);
> > > + }
> > > +}
> >
> > Another nitpick, but it might be nice to have similar arguments to the
> > _get() and _put() functions, e.g. struct audit_contobj, but that is
> > some serious bikeshedding (basically rename _hold() to _get() and
> > rename _hold to audit_task_contid_hold() or similar).
>
> I have some idea what you are trying to say, but I think you misspoke.
> Did you mean rename _hold to _get, rename _get to
> audit_task_contobj_hold()?
It reads okay to me, but I know what I'm intending here :) I agree it
could be a bit confusing. Let me try to put my suggestion into some
quick pseudo-code function prototypes to make things a bit more
concrete.
The _audit_contobj_hold() function would become:
struct audit_contobj *_audit_contobj_hold(struct task_struct *tsk);
The _audit_contobj_get() function would become:
struct audit_contobj *_audit_contobj_get(struct audit_contobj *cont);
The _audit_contobj_put() function would become:
void _audit_contobj_put(struct audit_contobj *cont);
Basically swap the _get() and _hold() function names so that the
arguments are the same for both the _get() and _set() functions. Does
this make more sense?
> > > /**
> > > * audit_alloc - allocate an audit info block for a task
> > > * @tsk: task
> > > @@ -232,6 +266,9 @@ int audit_alloc(struct task_struct *tsk)
> > > }
> > > info->loginuid = audit_get_loginuid(current);
> > > info->sessionid = audit_get_sessionid(current);
> > > + rcu_read_lock();
> > > + info->cont = _audit_contobj_get(current);
> > > + rcu_read_unlock();
> >
> > The RCU locks aren't strictly necessary here, are they? In fact I
> > suppose we could probably just replace the _get() call with a
> > refcount_set(1) just as we do in audit_set_contid(), yes?
>
> I don't understand what you are getting at here. It needs a *contobj,
> along with bumping up the refcount of the existing contobj.
Sorry, you can disregard. My mental definition for audit_alloc() is
permanently messed up; I usually double check myself before commenting
on related code, but I must have forgotten here.
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls
On Wed, Jul 29, 2020 at 3:41 PM Richard Guy Briggs wrote:
> On 2020-07-05 11:10, Paul Moore wrote:
> > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote:
...
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index f03d3eb0752c..9e79645e5c0e 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -1458,6 +1466,7 @@ static void audit_log_exit(void)
> > > struct audit_buffer *ab;
> > > struct audit_aux_data *aux;
> > > struct audit_names *n;
> > > + struct audit_contobj *cont;
> > >
> > > context->personality = current->personality;
> > >
> > > @@ -1541,7 +1550,7 @@ static void audit_log_exit(void)
> > > for (aux = context->aux_pids; aux; aux = aux->next) {
> > > struct audit_aux_data_pids *axs = (void *)aux;
> > >
> > > - for (i = 0; i < axs->pid_count; i++)
> > > + for (i = 0; i < axs->pid_count; i++) {
> > > if (audit_log_pid_context(context,
> > > axs->target_pid[i],
> > > axs->target_auid[i],
> > > axs->target_uid[i],
> > > @@ -1549,14 +1558,20 @@ static void audit_log_exit(void)
> > > axs->target_sid[i],
> > > axs->target_comm[i]))
> > > call_panic = 1;
> > > + audit_log_container_id(context,
> > > axs->target_cid[i]);
> > > + }
> >
> > It might be nice to see an audit event example including the
> > ptrace/signal information. I'm concerned there may be some confusion
> > about associating the different audit container IDs with the correct
> > information in the event.
>
> This is the subject of ghat81, which is a test for ptrace and signal
> records.
>
> This was the reason I had advocated for an op= field since there is a
> possibility of multiple contid records per event.
I think an "op=" field is the wrong way to link audit container ID to
a particular record. It may be convenient, but I fear that it would
be overloading the field too much.
Like I said above, I think it would be good to see an audit event
example including the ptrace/signal information. This way we can talk
about it on-list and hash out the various solutions if it proves to be
a problem.
> > > @@ -1575,6 +1590,14 @@ static void audit_log_exit(void)
> > >
> > > audit_log_proctitle();
> > >
> > > + rcu_read_lock();
> > > + cont = _audit_contobj_get(current);
> > > + rcu_read_unlock();
> > > + audit_log_container_id(context, cont);
> > > + rcu_read_lock();
> > > + _audit_contobj_put(cont);
> > > + rcu_read_unlock();
> >
> > Do we need to grab an additional reference for the audit container
> > object here? We don't create any additional references here that
> > persist beyond the lifetime of this function, right?
>
> Why do we need another reference? There's one for each pointer pointing
> to it and so far we have just one from this task. Or are you thinking
> of the contid hash list, which is only added to when a task points to it
> and gets removed from that list when the last task stops pointing to it.
> Later that gets more complicated with network namespaces and nested
> container objects. For now we just needed it while generating the
> record, then it gets freed.
I don't think we need to grab an additional reference here, that is
why I asked the question. The code above grabs a reference for the
audit container ID object associated with the current task and then
drops it before returning; if the current task, and it's associated
audit container ID object, disappears in the middle of the function
we've got much bigger worries :)
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 06/13] audit: add contid support for signalling the audit daemon
On Wed, Jul 29, 2020 at 3:00 PM Richard Guy Briggs wrote:
> On 2020-07-05 11:10, Paul Moore wrote:
> > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote:
> > >
> > > Add audit container identifier support to the action of signalling the
> > > audit daemon.
> > >
> > > Since this would need to add an element to the audit_sig_info struct,
> > > a new record type AUDIT_SIGNAL_INFO2 was created with a new
> > > audit_sig_info2 struct. Corresponding support is required in the
> > > userspace code to reflect the new record request and reply type.
> > > An older userspace won't break since it won't know to request this
> > > record type.
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > include/linux/audit.h | 8
> > > include/uapi/linux/audit.h | 1 +
> > > kernel/audit.c | 95
> > > -
> > > security/selinux/nlmsgtab.c | 1 +
> > > 4 files changed, 104 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index 5eeba0efffc2..89cf7c66abe6 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -22,6 +22,13 @@ struct audit_sig_info {
> > > charctx[];
> > > };
> > >
> > > +struct audit_sig_info2 {
> > > + uid_t uid;
> > > + pid_t pid;
> > > + u32 cid_len;
> > > + chardata[];
> > > +};
> > > +
> > > struct audit_buffer;
> > > struct audit_context;
> > > struct inode;
> > > @@ -105,6 +112,7 @@ struct audit_contobj {
> > > u64 id;
> > > struct task_struct *owner;
> > > refcount_t refcount;
> > > + refcount_t sigflag;
> > > struct rcu_head rcu;
> > > };
> >
> > It seems like we need some protection in audit_set_contid() so that we
> > don't allow reuse of an audit container ID when "refcount == 0 &&
> > sigflag != 0", yes?
>
> We have it, see -ESHUTDOWN below.
That check in audit_set_contid() is checking ->refcount and not
->sigflag; ->sigflag is more important in this context, yes?
> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index fd98460c983f..a56ad77069b9 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -72,6 +72,7 @@
> > > #define AUDIT_SET_FEATURE 1018/* Turn an audit feature on or
> > > off */
> > > #define AUDIT_GET_FEATURE 1019/* Get which features are enabled
> > > */
> > > #define AUDIT_CONTAINER_OP 1020/* Define the container id and
> > > info */
> > > +#define AUDIT_SIGNAL_INFO2 1021/* Get info auditd signal sender
> > > */
> > >
> > > #define AUDIT_FIRST_USER_MSG 1100/* Userspace messages mostly
> > > uninteresting to kernel */
> > > #define AUDIT_USER_AVC 1107/* We filter this differently */
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index a09f8f661234..54dd2cb69402 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -126,6 +126,8 @@ struct auditd_connection {
> > > kuid_t audit_sig_uid = INVALID_UID;
> > > pid_t audit_sig_pid = -1;
> > > u32audit_sig_sid = 0;
> > > +static struct audit_contobj *audit_sig_cid;
> > > +static struct task_struct *audit_sig_atsk;
> >
> > This looks like a typo, or did you mean "atsk" for some reason?
>
> No, I meant atsk to refer specifically to the audit daemon task and not
> any other random one that is doing the signalling. I can change it is
> there is a strong objection.
Esh, yeah, "atsk" looks too much like a typo ;) At the very leask add
a 'd' in there, e.g. "adtsk", but something better than that would be
welcome.
> > > @@ -2532,6 +2620,11 @@ int audit_set_contid(struct task_struct *task, u64
> > > contid)
> > > if (cont->id == contid) {
> > > /* task injection to existing container */
> > > if (current == cont->owner) {
> > > + if
&
Re: [PATCH ghak90 V9 08/13] audit: add containerid support for user records
On Fri, Jul 17, 2020 at 8:44 PM Richard Guy Briggs wrote: > On 2020-07-05 11:11, Paul Moore wrote: > > On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs wrote: > > > > > > Add audit container identifier auxiliary record to user event standalone > > > records. > > > > > > Signed-off-by: Richard Guy Briggs > > > Acked-by: Neil Horman > > > Reviewed-by: Ondrej Mosnacek > > > --- > > > kernel/audit.c | 19 --- > > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > index 54dd2cb69402..997c34178ee8 100644 > > > --- a/kernel/audit.c > > > +++ b/kernel/audit.c > > > @@ -1507,6 +1504,14 @@ static int audit_receive_msg(struct sk_buff *skb, > > > struct nlmsghdr *nlh) > > > audit_log_n_untrustedstring(ab, str, > > > data_len); > > > } > > > audit_log_end(ab); > > > + rcu_read_lock(); > > > + cont = _audit_contobj_get(current); > > > + rcu_read_unlock(); > > > + audit_log_container_id(context, cont); > > > + rcu_read_lock(); > > > + _audit_contobj_put(cont); > > > + rcu_read_unlock(); > > > + audit_free_context(context); > > > > I haven't searched the entire patchset, but it seems like the pattern > > above happens a couple of times in this patchset, yes? If so would it > > make sense to wrap the above get/log/put in a helper function? > > I've redone the locking with an rcu lock around the get and a spinlock > around the put. It occurs to me that putting an rcu lock around the > whole thing and doing a get without the refcount increment would save > us the spinlock and put and be fine since we'd be fine with stale but > consistent information traversing the contobj list from this point to > report it. Problem with that is needing to use GFP_ATOMIC due to the > rcu lock. If I stick with the spinlock around the put then I can use > GFP_KERNEL and just grab the spinlock while traversing the contobj list. > > > Not a big deal either way, I'm pretty neutral on it at this point in > > the patchset but thought it might be worth mentioning in case you > > noticed the same and were on the fence. > > There is only one other place this is used, in audit_log_exit in > auditsc.c. I had noted the pattern but wasn't sure it was worth it. > Inline or not? Should we just let the compiler decide? I'm generally not a fan of explicit inlines unless it has been shown to be a real problem. -- paul moore www.paul-moore.com
[net-next PATCH] netlabel: fix problems with mapping removal
This patch fixes two main problems seen when removing NetLabel
mappings: memory leaks and potentially extra audit noise.
The memory leaks are caused by not properly free'ing the mapping's
address selector struct when free'ing the entire entry as well as
not properly cleaning up a temporary mapping entry when adding new
address selectors to an existing entry. This patch fixes both these
problems such that kmemleak reports no NetLabel associated leaks
after running the SELinux test suite.
The potentially extra audit noise was caused by the auditing code in
netlbl_domhsh_remove_entry() being called regardless of the entry's
validity. If another thread had already marked the entry as invalid,
but not removed/free'd it from the list of mappings, then it was
possible that an additional mapping removal audit record would be
generated. This patch fixes this by returning early from the removal
function when the entry was previously marked invalid. This change
also had the side benefit of improving the code by decreasing the
indentation level of large chunk of code by one (accounting for most
of the diffstat).
Reported-by: Stephen Smalley
Signed-off-by: Paul Moore
---
net/netlabel/netlabel_domainhash.c | 59 ++--
1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/net/netlabel/netlabel_domainhash.c
b/net/netlabel/netlabel_domainhash.c
index d07de2c0fbc7..f73a8382c275 100644
--- a/net/netlabel/netlabel_domainhash.c
+++ b/net/netlabel/netlabel_domainhash.c
@@ -85,6 +85,7 @@ static void netlbl_domhsh_free_entry(struct rcu_head *entry)
kfree(netlbl_domhsh_addr6_entry(iter6));
}
#endif /* IPv6 */
+ kfree(ptr->def.addrsel);
}
kfree(ptr->domain);
kfree(ptr);
@@ -537,6 +538,8 @@ int netlbl_domhsh_add(struct netlbl_dom_map *entry,
goto add_return;
}
#endif /* IPv6 */
+ /* cleanup the new entry since we've moved everything over */
+ netlbl_domhsh_free_entry(&entry->rcu);
} else
ret_val = -EINVAL;
@@ -580,6 +583,12 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map
*entry,
{
int ret_val = 0;
struct audit_buffer *audit_buf;
+ struct netlbl_af4list *iter4;
+ struct netlbl_domaddr4_map *map4;
+#if IS_ENABLED(CONFIG_IPV6)
+ struct netlbl_af6list *iter6;
+ struct netlbl_domaddr6_map *map6;
+#endif /* IPv6 */
if (entry == NULL)
return -ENOENT;
@@ -597,6 +606,9 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map *entry,
ret_val = -ENOENT;
spin_unlock(&netlbl_domhsh_lock);
+ if (ret_val)
+ return ret_val;
+
audit_buf = netlbl_audit_start_common(AUDIT_MAC_MAP_DEL, audit_info);
if (audit_buf != NULL) {
audit_log_format(audit_buf,
@@ -606,40 +618,29 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map
*entry,
audit_log_end(audit_buf);
}
- if (ret_val == 0) {
- struct netlbl_af4list *iter4;
- struct netlbl_domaddr4_map *map4;
-#if IS_ENABLED(CONFIG_IPV6)
- struct netlbl_af6list *iter6;
- struct netlbl_domaddr6_map *map6;
-#endif /* IPv6 */
-
- switch (entry->def.type) {
- case NETLBL_NLTYPE_ADDRSELECT:
- netlbl_af4list_foreach_rcu(iter4,
-&entry->def.addrsel->list4) {
- map4 = netlbl_domhsh_addr4_entry(iter4);
- cipso_v4_doi_putdef(map4->def.cipso);
- }
+ switch (entry->def.type) {
+ case NETLBL_NLTYPE_ADDRSELECT:
+ netlbl_af4list_foreach_rcu(iter4, &entry->def.addrsel->list4) {
+ map4 = netlbl_domhsh_addr4_entry(iter4);
+ cipso_v4_doi_putdef(map4->def.cipso);
+ }
#if IS_ENABLED(CONFIG_IPV6)
- netlbl_af6list_foreach_rcu(iter6,
-&entry->def.addrsel->list6) {
- map6 = netlbl_domhsh_addr6_entry(iter6);
- calipso_doi_putdef(map6->def.calipso);
- }
+ netlbl_af6list_foreach_rcu(iter6, &entry->def.addrsel->list6) {
+ map6 = netlbl_domhsh_addr6_entry(iter6);
+ calipso_doi_putdef(map6->def.calipso);
+ }
#endif /* IPv6 */
- break;
- case NETLBL_NLTYPE_CIPSOV4:
- cipso_v4_doi_putdef(entry->def.cipso);
- break;
+ break;
+ case NETLBL_NLTYPE_CIPSOV4:
+ cipso_v4_doi_putdef(entry
Re: [PATCH net-next] cipso: Remove unused inline functions
On Tue, Jul 14, 2020 at 10:21 PM YueHaibing wrote:
>
> They are not used any more since commit b1edeb102397 ("netlabel: Replace
> protocol/NetLabel linking with refrerence counts")
>
> Signed-off-by: YueHaibing
> ---
> include/net/cipso_ipv4.h | 12
> 1 file changed, 12 deletions(-)
Looks good to me, thanks for the patch.
Acked-by: Paul Moore
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 01/13] audit: collect audit task parameters
On Mon, Jul 13, 2020 at 4:30 PM Richard Guy Briggs wrote: > On 2020-07-07 21:42, Paul Moore wrote: > > On Mon, Jul 6, 2020 at 10:50 PM Richard Guy Briggs wrote: > > > On 2020-07-05 11:09, Paul Moore wrote: > > > > On Sat, Jun 27, 2020 at 9:21 AM Richard Guy Briggs > > > > wrote: ... > > > > In the early days of this patchset we talked a lot about how to handle > > > > the task_struct and the changes that would be necessary, ultimately > > > > deciding that encapsulating all of the audit fields into an > > > > audit_task_info struct. However, what is puzzling me a bit at this > > > > moment is why we are only including audit_task_info in task_info by > > > > reference *and* making it a build time conditional (via CONFIG_AUDIT). > > > > > > > > If audit is enabled at build time it would seem that we are always > > > > going to allocate an audit_task_info struct, so I have to wonder why > > > > we don't simply embed it inside the task_info struct (similar to the > > > > seccomp struct in the snippet above? Of course the audit_context > > > > struct needs to remain as is, I'm talking only about the > > > > task_info/audit_task_info struct. > > > > > > I agree that including the audit_task_info struct in the struct > > > task_struct would have been preferred to simplify allocation and free, > > > but the reason it was included by reference instead was to make the > > > task_struct size independent of audit so that future changes would not > > > cause as many kABI challenges. This first change will cause kABI > > > challenges regardless, but it was future ones that we were trying to > > > ease. > > > > > > Does that match with your recollection? > > > > I guess, sure. I suppose what I was really asking was if we had a > > "good" reason for not embedding the audit_task_info struct. > > Regardless, thanks for the explanation, that was helpful. > > Making it dynamic was actually your idea back in the spring of 2018: > https://lkml.org/lkml/2018/4/18/759 If you read my comments from 2018 carefully, or even not so carefully I think, you'll notice that my primary motivation for using a pointer was to "hide" the audit_task_info struct contents so that they couldn't be abused by other kernel subsystems looking for a general container identifier inside the kernel. As we've discussed many times before, this patchset is not a general purpose container identifier, this is an ***audit*** container ID; limiting the scope and usage of this identifier is what has allowed us to gain the begrudging acceptance we've had thus far and I believe it is the key to success. For whatever it is worth, this patchset doesn't hide the audit_task_struct definition in a kernel/audit*.c file, it lives in a header file which is easily accessed by other subsystems. In my opinion we should pick one of two options: leave it as a pointer reference and "hide" the struct definition, or just embed the struct and simplify the code. I see little value in openly defining the audit_task_info struct and using a pointer reference; if you believe you have a valid argument for why this makes sense I'm open to hearing it, but your comments thus far have been unconvincing. > > > > Richard, I'm sure you can answer this off the top of your head, but > > > > I'd have to go digging through the archives to pull out the relevant > > > > discussions so I figured I would just ask you for a reminder ... ? I > > > > imagine it's also possible things have changed a bit since those early > > > > discussions and the solution we arrived at then no longer makes as > > > > much sense as it did before. > > > > > > Agreed, it doesn't make as much sense now as it did when proposed, but > > > will make more sense in the future depending on when this change gets > > > accepted upstream. This is why I wanted this patch to go through as > > > part of ghak81 at the time the rest of it did so that future kABI issues > > > would be easier to handle, but that ship has long sailed. > > > > To be clear, kABI issues with task_struct really aren't an issue with > > the upstream kernel. I know that you know all of this already > > Richard, I'm mostly talking to everyone else on the To/CC line in case > > they are casually watching this discussion. > > kABI issues may not as much of an upstream issue, but part of the goal > here was upstream kernel issues, isolating the kernel audit
Re: [PATCH net-next 11/20] net: netlabel: kerneldoc fixes
On Sun, Jul 12, 2020 at 7:15 PM Andrew Lunn wrote: > > Simple fixes which require no deep knowledge of the code. > > Cc: Paul Moore > Signed-off-by: Andrew Lunn > --- > net/netlabel/netlabel_domainhash.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Looks good to me, thanks! Acked-by: Paul Moore > diff --git a/net/netlabel/netlabel_domainhash.c > b/net/netlabel/netlabel_domainhash.c > index a1f2320ecc16..d07de2c0fbc7 100644 > --- a/net/netlabel/netlabel_domainhash.c > +++ b/net/netlabel/netlabel_domainhash.c > @@ -92,7 +92,7 @@ static void netlbl_domhsh_free_entry(struct rcu_head *entry) > > /** > * netlbl_domhsh_hash - Hashing function for the domain hash table > - * @domain: the domain name to hash > + * @key: the domain name to hash > * > * Description: > * This is the hashing function for the domain hash table, it returns the > -- > 2.27.0.rc2 -- paul moore www.paul-moore.com
Re: [PATCH net-next 06/20] net: ipv4: kerneldoc fixes
On Sun, Jul 12, 2020 at 7:15 PM Andrew Lunn wrote: > > Simple fixes which require no deep knowledge of the code. > > Cc: Paul Moore > Cc: Alexey Kuznetsov > Cc: Eric Dumazet > Signed-off-by: Andrew Lunn > --- > net/ipv4/cipso_ipv4.c | 6 -- > net/ipv4/ipmr.c | 3 +++ > net/ipv4/tcp_input.c | 1 - > net/ipv4/tcp_output.c | 2 ++ > net/ipv4/tcp_timer.c | 2 +- > net/ipv4/udp.c| 6 +++--- > 6 files changed, 13 insertions(+), 7 deletions(-) Thanks Andrew. For the cipso_ipv4.c changes ... Acked-by: Paul Moore > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > index 0f1b9065c0a6..2eb71579f4d2 100644 > --- a/net/ipv4/cipso_ipv4.c > +++ b/net/ipv4/cipso_ipv4.c > @@ -283,7 +283,7 @@ static int cipso_v4_cache_check(const unsigned char *key, > > /** > * cipso_v4_cache_add - Add an entry to the CIPSO cache > - * @skb: the packet > + * @cipso_ptr: pointer to CIPSO IP option > * @secattr: the packet's security attributes > * > * Description: > @@ -1535,6 +1535,7 @@ unsigned char *cipso_v4_optptr(const struct sk_buff > *skb) > > /** > * cipso_v4_validate - Validate a CIPSO option > + * @skb: the packet > * @option: the start of the option, on error it is set to point to the error > * > * Description: > @@ -2066,7 +2067,7 @@ void cipso_v4_sock_delattr(struct sock *sk) > > /** > * cipso_v4_req_delattr - Delete the CIPSO option from a request socket > - * @reg: the request socket > + * @req: the request socket > * > * Description: > * Removes the CIPSO option from a request socket, if present. > @@ -2158,6 +2159,7 @@ int cipso_v4_sock_getattr(struct sock *sk, struct > netlbl_lsm_secattr *secattr) > /** > * cipso_v4_skbuff_setattr - Set the CIPSO option on a packet > * @skb: the packet > + * @doi_def: the DOI structure > * @secattr: the security attributes > * > * Description: -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V9 01/13] audit: collect audit task parameters
On Mon, Jul 6, 2020 at 10:50 PM Richard Guy Briggs wrote:
> On 2020-07-05 11:09, Paul Moore wrote:
> > On Sat, Jun 27, 2020 at 9:21 AM Richard Guy Briggs wrote:
> > >
> > > The audit-related parameters in struct task_struct should ideally be
> > > collected together and accessed through a standard audit API.
> > >
> > > Collect the existing loginuid, sessionid and audit_context together in a
> > > new struct audit_task_info called "audit" in struct task_struct.
> > >
> > > Use kmem_cache to manage this pool of memory.
> > > Un-inline audit_free() to be able to always recover that memory.
> > >
> > > Please see the upstream github issue
> > > https://github.com/linux-audit/audit-kernel/issues/81
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > Acked-by: Neil Horman
> > > Reviewed-by: Ondrej Mosnacek
> > > ---
> > > include/linux/audit.h | 49 +++
> > > include/linux/sched.h | 7 +
> > > init/init_task.c | 3 +--
> > > init/main.c | 2 ++
> > > kernel/audit.c| 71
> > > +--
> > > kernel/audit.h| 5
> > > kernel/auditsc.c | 26 ++-
> > > kernel/fork.c | 1 -
> > > 8 files changed, 124 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index 3fcd9ee49734..c2150415f9df 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -100,6 +100,16 @@ enum audit_nfcfgop {
> > > AUDIT_XT_OP_UNREGISTER,
> > > };
> > >
> > > +struct audit_task_info {
> > > + kuid_t loginuid;
> > > + unsigned intsessionid;
> > > +#ifdef CONFIG_AUDITSYSCALL
> > > + struct audit_context*ctx;
> > > +#endif
> > > +};
> > > +
> > > +extern struct audit_task_info init_struct_audit;
> > > +
> > > extern int is_audit_feature_set(int which);
> > >
> > > extern int __init audit_register_class(int class, unsigned *list);
> >
> > ...
> >
> > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > index b62e6aaf28f0..2213ac670386 100644
> > > --- a/include/linux/sched.h
> > > +++ b/include/linux/sched.h
> > > @@ -34,7 +34,6 @@
> > > #include
> > >
> > > /* task_struct member predeclarations (sorted alphabetically): */
> > > -struct audit_context;
> > > struct backing_dev_info;
> > > struct bio_list;
> > > struct blk_plug;
> > > @@ -946,11 +945,7 @@ struct task_struct {
> > > struct callback_head*task_works;
> > >
> > > #ifdef CONFIG_AUDIT
> > > -#ifdef CONFIG_AUDITSYSCALL
> > > - struct audit_context*audit_context;
> > > -#endif
> > > - kuid_t loginuid;
> > > - unsigned intsessionid;
> > > + struct audit_task_info *audit;
> > > #endif
> > > struct seccomp seccomp;
> >
> > In the early days of this patchset we talked a lot about how to handle
> > the task_struct and the changes that would be necessary, ultimately
> > deciding that encapsulating all of the audit fields into an
> > audit_task_info struct. However, what is puzzling me a bit at this
> > moment is why we are only including audit_task_info in task_info by
> > reference *and* making it a build time conditional (via CONFIG_AUDIT).
> >
> > If audit is enabled at build time it would seem that we are always
> > going to allocate an audit_task_info struct, so I have to wonder why
> > we don't simply embed it inside the task_info struct (similar to the
> > seccomp struct in the snippet above? Of course the audit_context
> > struct needs to remain as is, I'm talking only about the
> > task_info/audit_task_info struct.
>
> I agree that including the audit_task_info struct in the struct
> task_struct would have been preferred to simplify allocation and free,
> but the reason it was included by reference instead was to make the
> task_struct size independent of audit so that future changes would not
> cause as many kABI challenges. This first change will cause kABI
> challenges regardless, but it was future ones that we were trying to
> ease.
>
Re: [PATCH ghak90 V9 12/13] audit: track container nesting
On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs wrote:
>
> Track the parent container of a container to be able to filter and
> report nesting.
>
> Now that we have a way to track and check the parent container of a
> container, modify the contid field format to be able to report that
> nesting using a carrat ("^") modifier to indicate nesting. The
> original field format was "contid=" for task-associated records
> and "contid=[,[...]]" for network-namespace-associated
> records. The new field format is
> "contid=[,^[...]][,[...]]".
I feel like this is a case which could really benefit from an example
in the commit description showing multiple levels of nesting, with
some leaf audit container IDs at each level. This way we have a
canonical example for people who want to understand how to parse the
list and properly sort out the inheritance.
> Signed-off-by: Richard Guy Briggs
> ---
> include/linux/audit.h | 1 +
> kernel/audit.c| 60
> ++-
> kernel/audit.h| 2 ++
> kernel/auditfilter.c | 17 ++-
> kernel/auditsc.c | 2 +-
> 5 files changed, 70 insertions(+), 12 deletions(-)
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 08/13] audit: add containerid support for user records
On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs wrote: > > Add audit container identifier auxiliary record to user event standalone > records. > > Signed-off-by: Richard Guy Briggs > Acked-by: Neil Horman > Reviewed-by: Ondrej Mosnacek > --- > kernel/audit.c | 19 --- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index 54dd2cb69402..997c34178ee8 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1507,6 +1504,14 @@ static int audit_receive_msg(struct sk_buff *skb, > struct nlmsghdr *nlh) > audit_log_n_untrustedstring(ab, str, > data_len); > } > audit_log_end(ab); > + rcu_read_lock(); > + cont = _audit_contobj_get(current); > + rcu_read_unlock(); > + audit_log_container_id(context, cont); > + rcu_read_lock(); > + _audit_contobj_put(cont); > + rcu_read_unlock(); > + audit_free_context(context); I haven't searched the entire patchset, but it seems like the pattern above happens a couple of times in this patchset, yes? If so would it make sense to wrap the above get/log/put in a helper function? Not a big deal either way, I'm pretty neutral on it at this point in the patchset but thought it might be worth mentioning in case you noticed the same and were on the fence. -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V9 10/13] audit: add support for containerid to network namespaces
ontns, contobj_list, list)
> + if (contns->obj == cont) {
> + contns->count--;
> + if (contns->count < 1) {
One could simplify this with "(--countns->count) < 1", although if it
is changed to a refcount_t (which seems like a smart thing), the
normal decrement/test would be the best choice.
> + list_del_rcu(&contns->list);
> + kfree_rcu(contns, rcu);
> + }
> + break;
> + }
> + spin_unlock(&aunet->contobj_list_lock);
> + rcu_read_unlock();
> +}
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 13/13] audit: add capcontid to set contid outside init_user_ns
On Sat, Jun 27, 2020 at 9:24 AM Richard Guy Briggs wrote: > > Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a > process in a non-init user namespace the capability to set audit > container identifiers of individual children. > > Provide the /proc/$PID/audit_capcontid interface to capcontid. > Valid values are: 1==enabled, 0==disabled > > Writing a "1" to this special file for the target process $PID will > enable the target process to set audit container identifiers of its > descendants. > > A process must already have CAP_AUDIT_CONTROL in the initial user > namespace or have had audit_capcontid enabled by a previous use of this > feature by its parent on this process in order to be able to enable it > for another process. The target process must be a descendant of the > calling process. > > Report this action in new message type AUDIT_SET_CAPCONTID 1022 with > fields opid= capcontid= old-capcontid= > > Signed-off-by: Richard Guy Briggs > --- > fs/proc/base.c | 57 > +- > include/linux/audit.h | 14 > include/uapi/linux/audit.h | 1 + > kernel/audit.c | 38 ++- > 4 files changed, 108 insertions(+), 2 deletions(-) This seems very similar to the capable/ns_capable combination I mentioned in patch 11/13; any reasons why you feel that this might be a better approach? My current thinking is that the capable/ns_capable approach is preferable as it leverages existing kernel mechanisms and doesn't require us to reinvent the wheel in the audit subsystem. -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V9 07/13] audit: add support for non-syscall auxiliary records
On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote:
>
> Standalone audit records have the timestamp and serial number generated
> on the fly and as such are unique, making them standalone. This new
> function audit_alloc_local() generates a local audit context that will
> be used only for a standalone record and its auxiliary record(s). The
> context is discarded immediately after the local associated records are
> produced.
We've had some good discussions on the list about why we can't reuse
the "in_syscall" field and need to add a "local" field, I think it
would be good to address that here in the commit description.
> Signed-off-by: Richard Guy Briggs
> Acked-by: Serge Hallyn
> Acked-by: Neil Horman
> Reviewed-by: Ondrej Mosnacek
> ---
> include/linux/audit.h | 8
> kernel/audit.h| 1 +
> kernel/auditsc.c | 33 -
> 3 files changed, 37 insertions(+), 5 deletions(-)
...
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 9e79645e5c0e..935eb3d2cde9 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -908,11 +908,13 @@ static inline void audit_free_aux(struct audit_context
> *context)
> }
> }
>
> -static inline struct audit_context *audit_alloc_context(enum audit_state
> state)
> +static inline struct audit_context *audit_alloc_context(enum audit_state
> state,
> + gfp_t gfpflags)
> {
> struct audit_context *context;
>
> - context = kzalloc(sizeof(*context), GFP_KERNEL);
> + /* We can be called in atomic context via audit_tg() */
At this point I think it's clear we need a respin so I'm not going to
preface all of my nitpick comments as such, although this definitely
would qualify ...
I don't believe audit_tg() doesn't exist yet, likely coming later in
this patchset, so please remove this comment as it doesn't make sense
in this context.
To be frank, don't re-add the comment later in the patchset either.
Comments like these tend to be fragile and don't really add any great
insight. The audit_tg() function can, and most likely will, be
modified at some point in the future such that the comment above no
longer applies, and there is a reasonable chance that when it does the
above comment will not be updated. Further, anyone modifying the
audit_alloc_context() is going to look at the callers (rather they
*should* look at the callers) and will notice the no-sleep
requirements.
> @@ -960,8 +963,27 @@ int audit_alloc_syscall(struct task_struct *tsk)
> return 0;
> }
>
> -static inline void audit_free_context(struct audit_context *context)
> +struct audit_context *audit_alloc_local(gfp_t gfpflags)
> {
> + struct audit_context *context = NULL;
> +
> + context = audit_alloc_context(AUDIT_RECORD_CONTEXT, gfpflags);
> + if (!context) {
> + audit_log_lost("out of memory in audit_alloc_local");
> + goto out;
You might as well just return NULL here, no need to jump and then return NULL.
> + }
> + context->serial = audit_serial();
> + ktime_get_coarse_real_ts64(&context->ctime);
> + context->local = true;
> +out:
> + return context;
> +}
> +EXPORT_SYMBOL(audit_alloc_local);
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 11/13] audit: contid check descendancy and nesting
On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs wrote:
>
> Require the target task to be a descendant of the container
> orchestrator/engine.
>
> You would only change the audit container ID from one set or inherited
> value to another if you were nesting containers.
>
> If changing the contid, the container orchestrator/engine must be a
> descendant and not same orchestrator as the one that set it so it is not
> possible to change the contid of another orchestrator's container.
>
> Since the task_is_descendant() function is used in YAMA and in audit,
> remove the duplication and pull the function into kernel/core/sched.c
>
> Signed-off-by: Richard Guy Briggs
> ---
> include/linux/sched.h| 3 +++
> kernel/audit.c | 23 +--
> kernel/sched/core.c | 33 +
> security/yama/yama_lsm.c | 33 -
> 4 files changed, 57 insertions(+), 35 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 2213ac670386..06938d0b9e0c 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2047,4 +2047,7 @@ static inline void rseq_syscall(struct pt_regs *regs)
>
> const struct cpumask *sched_trace_rd_span(struct root_domain *rd);
>
> +extern int task_is_descendant(struct task_struct *parent,
> + struct task_struct *child);
> +
> #endif
> diff --git a/kernel/audit.c b/kernel/audit.c
> index a862721dfd9b..efa65ec01239 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2713,6 +2713,20 @@ int audit_signal_info(int sig, struct task_struct *t)
> return audit_signal_info_syscall(t);
> }
>
> +static bool audit_contid_isnesting(struct task_struct *tsk)
> +{
> + bool isowner = false;
> + bool ownerisparent = false;
> +
> + rcu_read_lock();
> + if (tsk->audit && tsk->audit->cont) {
> + isowner = current == tsk->audit->cont->owner;
> + ownerisparent = task_is_descendant(tsk->audit->cont->owner,
> current);
I want to make sure I'm understanding this correctly and I keep
mentally tripping over something: it seems like for a given audit
container ID a task is either the owner or a descendent, there is no
third state, is that correct?
Assuming that is true, can the descendent check simply be a negative
owner check given they both have the same audit container ID?
> + }
> + rcu_read_unlock();
> + return !isowner && ownerisparent;
> +}
> +
> /*
> * audit_set_contid - set current task's audit contid
> * @task: target task
> @@ -2755,8 +2769,13 @@ int audit_set_contid(struct task_struct *task, u64
> contid)
> rc = -EBUSY;
> goto unlock;
> }
> - /* if contid is already set, deny */
> - if (audit_contid_set(task))
> + /* if task is not descendant, block */
> + if (task == current || !task_is_descendant(current, task)) {
I'm also still fuzzy on why we can't let a task set it's own audit
container ID, assuming it meets all the criteria established in patch
2/13. It somewhat made sense when you were tracking inherited vs
explicitly set audit container IDs, but that doesn't appear to be the
case so far in this patchset, yes?
> + rc = -EXDEV;
I'm fairly confident we had a discussion about not using all these
different error codes, but that may be a moot point given my next
comment.
> + goto unlock;
> + }
> + /* only allow contid setting again if nesting */
> + if (audit_contid_set(task) && !audit_contid_isnesting(task))
> rc = -EEXIST;
It seems like what we need in audit_set_contid() is a check to ensure
that the task being modified is only modified by the owner of the
audit container ID, yes? If so, I would think we could do this quite
easily with the following, or similar logic, (NOTE: assumes both
current and tsk are properly setup):
if ((current->audit->cont != tsk->audit->cont) ||
(current->audit->cont->owner != current))
return -EACCESS;
This is somewhat independent of the above issue, but we may also want
to add to the capability check. Patch 2 adds a
"capable(CAP_AUDIT_CONTROL)" which is good, but perhaps we also need a
"ns_capable(CAP_AUDIT_CONTROL)" to allow a given audit container ID
orchestrator/owner the ability to control which of it's descendants
can change their audit container ID, for example:
if (!capable(CAP_AUDIT_CONTROL) ||
!ns_capable(current->nsproxy->user_ns, CAP_AUDIT_CONTROL))
return -EPERM;
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 06/13] audit: add contid support for signalling the audit daemon
return audit_signal_info_syscall(t);
> @@ -2532,6 +2620,11 @@ int audit_set_contid(struct task_struct *task, u64
> contid)
> if (cont->id == contid) {
> /* task injection to existing container */
> if (current == cont->owner) {
> + if (!refcount_read(&cont->refcount)) {
> + rc = -ESHUTDOWN;
Reuse -ENOTUNIQ; I'm not overly excited about providing a lot of
detail here as these are global system objects. If you must have a
different errno (and I would prefer you didn't), use something like
-EBUSY.
> +
> spin_unlock(&audit_contobj_list_lock);
> + goto conterror;
> + }
> _audit_contobj_hold(cont);
> newcont = cont;
> } else {
> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> index b69231918686..8303bb7a63d0 100644
> --- a/security/selinux/nlmsgtab.c
> +++ b/security/selinux/nlmsgtab.c
> @@ -137,6 +137,7 @@ struct nlmsg_perm {
> { AUDIT_DEL_RULE, NETLINK_AUDIT_SOCKET__NLMSG_WRITE},
> { AUDIT_USER, NETLINK_AUDIT_SOCKET__NLMSG_RELAY},
> { AUDIT_SIGNAL_INFO,NETLINK_AUDIT_SOCKET__NLMSG_READ },
> + { AUDIT_SIGNAL_INFO2, NETLINK_AUDIT_SOCKET__NLMSG_READ },
> { AUDIT_TRIM, NETLINK_AUDIT_SOCKET__NLMSG_WRITE},
> { AUDIT_MAKE_EQUIV, NETLINK_AUDIT_SOCKET__NLMSG_WRITE},
> { AUDIT_TTY_GET,NETLINK_AUDIT_SOCKET__NLMSG_READ },
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls
for (aux = context->aux_pids; aux; aux = aux->next) {
> struct audit_aux_data_pids *axs = (void *)aux;
>
> - for (i = 0; i < axs->pid_count; i++)
> + for (i = 0; i < axs->pid_count; i++) {
> if (audit_log_pid_context(context, axs->target_pid[i],
> axs->target_auid[i],
> axs->target_uid[i],
> @@ -1549,14 +1558,20 @@ static void audit_log_exit(void)
> axs->target_sid[i],
> axs->target_comm[i]))
> call_panic = 1;
> + audit_log_container_id(context, axs->target_cid[i]);
> + }
It might be nice to see an audit event example including the
ptrace/signal information. I'm concerned there may be some confusion
about associating the different audit container IDs with the correct
information in the event.
> }
>
> - if (context->target_pid &&
> - audit_log_pid_context(context, context->target_pid,
> - context->target_auid, context->target_uid,
> - context->target_sessionid,
> - context->target_sid, context->target_comm))
> + if (context->target_pid) {
> + if (audit_log_pid_context(context, context->target_pid,
> + context->target_auid,
> + context->target_uid,
> + context->target_sessionid,
> + context->target_sid,
> + context->target_comm))
> call_panic = 1;
> + audit_log_container_id(context, context->target_cid);
> + }
>
> if (context->pwd.dentry && context->pwd.mnt) {
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_CWD);
> @@ -1575,6 +1590,14 @@ static void audit_log_exit(void)
>
> audit_log_proctitle();
>
> + rcu_read_lock();
> + cont = _audit_contobj_get(current);
> + rcu_read_unlock();
> + audit_log_container_id(context, cont);
> + rcu_read_lock();
> + _audit_contobj_put(cont);
> + rcu_read_unlock();
Do we need to grab an additional reference for the audit container
object here? We don't create any additional references here that
persist beyond the lifetime of this function, right?
> audit_log_container_drop();
>
> /* Send end of event record to help user space know we are finished */
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 04/13] audit: log drop of contid on exit of last task
On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote:
>
> Since we are tracking the life of each audit container indentifier, we
> can match the creation event with the destruction event. Log the
> destruction of the audit container identifier when the last process in
> that container exits.
>
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/audit.c | 20
> kernel/audit.h | 2 ++
> kernel/auditsc.c | 2 ++
> 3 files changed, 24 insertions(+)
If you end up respinning this patchset it seems like this should be
merged in with patch 2/13. This way patch 2/13 would include both the
"set" and "drop" records, making that patch a bit more useful on it's
own.
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 6d387793f702..9e0b38ce1ead 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2558,6 +2558,26 @@ int audit_set_contid(struct task_struct *task, u64
> contid)
> return rc;
> }
>
> +void audit_log_container_drop(void)
> +{
> + struct audit_buffer *ab;
> + struct audit_contobj *cont;
> +
> + rcu_read_lock();
> + cont = _audit_contobj_get(current);
> + _audit_contobj_put(cont);
> + if (!cont || refcount_read(&cont->refcount) > 1)
> + goto out;
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
You may want to check on sleeping with RCU locks held, or just use
GFP_ATOMIC to be safe.
> + if (!ab)
> + goto out;
> + audit_log_format(ab, "op=drop opid=%d contid=%llu old-contid=%llu",
> +task_tgid_nr(current), cont->id, cont->id);
> + audit_log_end(ab);
> +out:
> + rcu_read_unlock();
> +}
> +
> /**
> * audit_log_end - end one audit record
> * @ab: the audit_buffer
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 182fc76ea276..d07093903008 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -254,6 +254,8 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
> extern struct tty_struct *audit_get_tty(void);
> extern void audit_put_tty(struct tty_struct *tty);
>
> +extern void audit_log_container_drop(void);
> +
> /* audit watch/mark/tree functions */
> #ifdef CONFIG_AUDITSYSCALL
> extern unsigned int audit_serial(void);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f00c1da587ea..f03d3eb0752c 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1575,6 +1575,8 @@ static void audit_log_exit(void)
>
> audit_log_proctitle();
>
> + audit_log_container_drop();
> +
> /* Send end of event record to help user space know we are finished */
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
> if (ab)
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 01/13] audit: collect audit task parameters
p_init();
> + audit_task_init();
> taskstats_init_early();
> delayacct_init();
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 8c201f414226..5d8147a29291 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -203,6 +203,73 @@ struct audit_reply {
> struct sk_buff *skb;
> };
>
> +static struct kmem_cache *audit_task_cache;
> +
> +void __init audit_task_init(void)
> +{
> + audit_task_cache = kmem_cache_create("audit_task",
> +sizeof(struct audit_task_info),
> +0, SLAB_PANIC, NULL);
> +}
> +
> +/**
> + * audit_alloc - allocate an audit info block for a task
> + * @tsk: task
> + *
> + * Call audit_alloc_syscall to filter on the task information and
> + * allocate a per-task audit context if necessary. This is called from
> + * copy_process, so no lock is needed.
> + */
> +int audit_alloc(struct task_struct *tsk)
> +{
> + int ret = 0;
> + struct audit_task_info *info;
> +
> + info = kmem_cache_alloc(audit_task_cache, GFP_KERNEL);
> + if (!info) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + info->loginuid = audit_get_loginuid(current);
> + info->sessionid = audit_get_sessionid(current);
> + tsk->audit = info;
> +
> + ret = audit_alloc_syscall(tsk);
> + if (ret) {
> + tsk->audit = NULL;
> + kmem_cache_free(audit_task_cache, info);
> + }
> +out:
> + return ret;
> +}
This is a big nitpick, and I'm only mentioning this in the case you
need to respin this patchset: the "out" label is unnecessary in the
function above. Simply return the error code, there is no need to
jump to "out" only to immediately return the error code there and
nothing more.
> +struct audit_task_info init_struct_audit = {
> + .loginuid = INVALID_UID,
> + .sessionid = AUDIT_SID_UNSET,
> +#ifdef CONFIG_AUDITSYSCALL
> + .ctx = NULL,
> +#endif
> +};
> +
> +/**
> + * audit_free - free per-task audit info
> + * @tsk: task whose audit info block to free
> + *
> + * Called from copy_process and do_exit
> + */
> +void audit_free(struct task_struct *tsk)
> +{
> + struct audit_task_info *info = tsk->audit;
> +
> + audit_free_syscall(tsk);
> + /* Freeing the audit_task_info struct must be performed after
> +* audit_log_exit() due to need for loginuid and sessionid.
> +*/
> + info = tsk->audit;
> + tsk->audit = NULL;
> + kmem_cache_free(audit_task_cache, info);
Another nitpick, and this one may even become a moot point given the
question posed above. However, is there any reason we couldn't get
rid of "info" and simplify this a bit?
audit_free_syscall(tsk);
kmem_cache_free(audit_task_cache, tsk->audit);
tsk->audit = NULL;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 468a23390457..f00c1da587ea 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1612,7 +1615,6 @@ void __audit_free(struct task_struct *tsk)
> if (context->current_state == AUDIT_RECORD_CONTEXT)
> audit_log_exit();
> }
> -
> audit_set_context(tsk, NULL);
> audit_free_context(context);
> }
This nitpick is barely worth the time it is taking me to write this,
but the whitespace change above isn't strictly necessary.
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 02/13] audit: add container id
ssary? From what I can see we only
jump to "unlock" in case of error where we are not going to set the
audit container ID, yet the "unlock" target is placed in a misleading
location in the middle of the function. It may be that everything
works correctly, but I would argue this is a bad practice that
increases the likelihood of buggy behavior in future code changes.
If you can't find way to arrange the code nicely, just duplicate the
"tasklist_lock" unlock operation in the error handlers before jumping
down to the end of the function. It isn't perfect, but I believe it
will be a lot less fragile than the current approach.
> + read_unlock(&tasklist_lock);
> + rcu_read_lock();
> + oldcont = _audit_contobj_get(task);
> + if (!rc) {
> + struct audit_contobj *cont = NULL, *newcont = NULL;
> + int h = audit_hash_contid(contid);
> +
> + spin_lock(&audit_contobj_list_lock);
> + list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
> + if (cont->id == contid) {
> + /* task injection to existing container */
> + if (current == cont->owner) {
> + _audit_contobj_hold(cont);
> + newcont = cont;
> + } else {
> + rc = -ENOTUNIQ;
> + spin_unlock(&audit_contobj_list_lock);
> + goto conterror;
> + }
> + break;
> + }
> + if (!newcont) {
> + newcont = kmalloc(sizeof(*newcont), GFP_ATOMIC);
> + if (newcont) {
> + INIT_LIST_HEAD(&newcont->list);
> + newcont->id = contid;
> + newcont->owner = get_task_struct(current);
> + refcount_set(&newcont->refcount, 1);
> + list_add_rcu(&newcont->list,
> + &audit_contid_hash[h]);
> + } else {
> + rc = -ENOMEM;
> + spin_unlock(&audit_contobj_list_lock);
> + goto conterror;
> + }
> + }
> + spin_unlock(&audit_contobj_list_lock);
> + task->audit->cont = newcont;
> + _audit_contobj_put(oldcont);
> + }
> +conterror:
> + task_unlock(task);
> +
> + if (!audit_enabled)
> + return rc;
> +
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
> + if (!ab)
> + return rc;
> +
> + audit_log_format(ab,
> +"op=set opid=%d contid=%llu old-contid=%llu",
> +task_tgid_nr(task), contid, oldcont ? oldcont->id :
> -1);
> + _audit_contobj_put(oldcont);
> + rcu_read_unlock();
> + audit_log_end(ab);
> + return rc;
> +}
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 02/13] audit: add container id
On Sat, Jul 4, 2020 at 9:29 AM Paul Moore wrote: > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote: > > > > Implement the proc fs write to set the audit container identifier of a > > process, emitting an AUDIT_CONTAINER_OP record to document the event. Sorry about the email misfire, you can safely ignore that last empty message.
Re: [PATCH ghak90 V9 02/13] audit: add container id
rc = -ENOTUNIQ;
> + spin_unlock(&audit_contobj_list_lock);
> + goto conterror;
> + }
> + break;
> + }
> + if (!newcont) {
> + newcont = kmalloc(sizeof(*newcont), GFP_ATOMIC);
> + if (newcont) {
> + INIT_LIST_HEAD(&newcont->list);
> + newcont->id = contid;
> + newcont->owner = get_task_struct(current);
> + refcount_set(&newcont->refcount, 1);
> + list_add_rcu(&newcont->list,
> +&audit_contid_hash[h]);
> + } else {
> + rc = -ENOMEM;
> + spin_unlock(&audit_contobj_list_lock);
> + goto conterror;
> + }
> + }
> + spin_unlock(&audit_contobj_list_lock);
> + task->audit->cont = newcont;
> + _audit_contobj_put(oldcont);
> + }
> +conterror:
> + task_unlock(task);
> +
> + if (!audit_enabled)
> + return rc;
> +
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
> + if (!ab)
> + return rc;
> +
> + audit_log_format(ab,
> +"op=set opid=%d contid=%llu old-contid=%llu",
> +task_tgid_nr(task), contid, oldcont ? oldcont->id :
> -1);
> + _audit_contobj_put(oldcont);
> + rcu_read_unlock();
> + audit_log_end(ab);
> + return rc;
> +}
> +
> /**
> * audit_log_end - end one audit record
> * @ab: the audit_buffer
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 9bee09757068..182fc76ea276 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -210,6 +210,14 @@ static inline int audit_hash_ino(u32 ino)
> return (ino & (AUDIT_INODE_BUCKETS-1));
> }
>
> +#define AUDIT_CONTID_BUCKETS 32
> +extern struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
> +
> +static inline int audit_hash_contid(u64 contid)
> +{
> + return (contid & (AUDIT_CONTID_BUCKETS-1));
> +}
> +
> /* Indicates that audit should log the full pathname. */
> #define AUDIT_NAME_FULL -1
>
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V8 07/16] audit: add contid support for signalling the audit daemon
On Mon, Jun 8, 2020 at 2:04 PM Richard Guy Briggs wrote: > On 2020-04-22 13:24, Paul Moore wrote: > > On Fri, Apr 17, 2020 at 6:26 PM Eric W. Biederman > > wrote: > > > Paul Moore writes: > > > > On Thu, Apr 16, 2020 at 4:36 PM Eric W. Biederman > > > > wrote: > > > >> Paul Moore writes: > > > >> > On Mon, Mar 30, 2020 at 1:49 PM Richard Guy Briggs > > > >> > wrote: > > > >> >> On 2020-03-30 13:34, Paul Moore wrote: > > > >> >> > On Mon, Mar 30, 2020 at 12:22 PM Richard Guy Briggs > > > >> >> > wrote: > > > >> >> > > On 2020-03-30 10:26, Paul Moore wrote: > > > >> >> > > > On Mon, Mar 30, 2020 at 9:47 AM Richard Guy Briggs > > > >> >> > > > wrote: > > > >> >> > > > > On 2020-03-28 23:11, Paul Moore wrote: > > > >> >> > > > > > On Tue, Mar 24, 2020 at 5:02 PM Richard Guy Briggs > > > >> >> > > > > > wrote: > > > >> >> > > > > > > On 2020-03-23 20:16, Paul Moore wrote: > > > >> >> > > > > > > > On Thu, Mar 19, 2020 at 6:03 PM Richard Guy Briggs > > > >> >> > > > > > > > wrote: > > > >> >> > > > > > > > > On 2020-03-18 18:06, Paul Moore wrote: > > > >> > > > > >> > ... > > > >> > > > > >> >> > > Well, every time a record gets generated, *any* record gets > > > >> >> > > generated, > > > >> >> > > we'll need to check for which audit daemons this record is in > > > >> >> > > scope and > > > >> >> > > generate a different one for each depending on the content and > > > >> >> > > whether > > > >> >> > > or not the content is influenced by the scope. > > > >> >> > > > > >> >> > That's the problem right there - we don't want to have to > > > >> >> > generate a > > > >> >> > unique record for *each* auditd on *every* record. That is a > > > >> >> > recipe > > > >> >> > for disaster. > > > >> >> > > > > >> >> > Solving this for all of the known audit records is not something > > > >> >> > we > > > >> >> > need to worry about in depth at the moment (although giving it > > > >> >> > some > > > >> >> > casual thought is not a bad thing), but solving this for the audit > > > >> >> > container ID information *is* something we need to worry about > > > >> >> > right > > > >> >> > now. > > > >> >> > > > >> >> If you think that a different nested contid value string per daemon > > > >> >> is > > > >> >> not acceptable, then we are back to issuing a record that has only > > > >> >> *one* > > > >> >> contid listed without any nesting information. This brings us back > > > >> >> to > > > >> >> the original problem of keeping *all* audit log history since the > > > >> >> boot > > > >> >> of the machine to be able to track the nesting of any particular > > > >> >> contid. > > > >> > > > > >> > I'm not ruling anything out, except for the "let's just completely > > > >> > regenerate every record for each auditd instance". > > > >> > > > >> Paul I am a bit confused about what you are referring to when you say > > > >> regenerate every record. > > > >> > > > >> Are you saying that you don't want to repeat the sequence: > > > >> audit_log_start(...); > > > >> audit_log_format(...); > > > >> audit_log_end(...); > > > >> for every nested audit daemon? > > > > > > > > If it can be avoided yes. Audit performance is already not-awesome, > > > > this would make it even worse. > > > > > > As far as I can see not repeating sequences like that is fundamental > > >
Re: [PATCH net] netlabel: cope with NULL catmap
On Tue, May 12, 2020 at 8:44 AM Paolo Abeni wrote:
>
> The cipso and calipso code can set the MLS_CAT attribute on
> successful parsing, even if the corresponding catmap has
> not been allocated, as per current configuration and external
> input.
>
> Later, selinux code tries to access the catmap if the MLS_CAT flag
> is present via netlbl_catmap_getlong(). That may cause null ptr
> dereference while processing incoming network traffic.
>
> Address the issue setting the MLS_CAT flag only if the catmap is
> really allocated. Additionally let netlbl_catmap_getlong() cope
> with NULL catmap.
>
> Reported-by: Matthew Sheets
> Fixes: 4b8feff251da ("netlabel: fix the horribly broken catmap functions")
> Fixes: ceba1832b1b2 ("calipso: Set the calipso socket label to match the
> secattr.")
> Signed-off-by: Paolo Abeni
> ---
> net/ipv4/cipso_ipv4.c| 6 --
> net/ipv6/calipso.c | 3 ++-
> net/netlabel/netlabel_kapi.c | 6 ++
> 3 files changed, 12 insertions(+), 3 deletions(-)
Seems reasonable to me, thanks Paolo.
Acked-by: Paul Moore
--
paul moore
www.paul-moore.com
Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
On Thu, Aug 29, 2019 at 3:45 AM Michal Kubecek wrote: > On Tue, Aug 27, 2019 at 04:47:04PM -0400, Paul Moore wrote: > > > > I'm also not a big fan of inserting the hook in rtnl_fill_ifinfo(); as > > presented it is way too specific for a LSM hook for me to be happy. > > However, I do agree that giving the LSMs some control over netlink > > messages makes sense. As others have pointed out, it's all a matter > > of where to place the hook. > > > > If we only care about netlink messages which leverage nlattrs I > > suppose one option that I haven't seen mentioned would be to place a > > hook in nla_put(). While it is a bit of an odd place for a hook, it > > would allow the LSM easy access to the skb and attribute type to make > > decisions, and all of the callers should already be checking the > > return code (although we would need to verify this). One notable > > drawback (not the only one) is that the hook is going to get hit > > multiple times for each message. > > For most messages, "multiple times" would mean tens, for many even > hundreds of calls. For each, you would have to check corresponding > socket (and possibly also genetlink header) to see which netlink based > protocol it is and often even parse existing part of the message to get > the context (because the same numeric attribute type can mean something > completely different if it appears in a nested attribute). > > Also, nla_put() (or rather __nla_put()) is not used for all attributes, > one may also use nla_reserve() and then compose the attribute date in > place. I never said it was a great idea, just an idea ;) Honestly I'm just trying to spur some discussion on this so we can hopefully arrive at a solution which allows a LSM to control kernel generated netlink messages that we can all accept. -- paul moore www.paul-moore.com
Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
On Fri, Aug 23, 2019 at 7:41 AM Jeffrey Vander Stoep wrote: > On Fri, Aug 23, 2019 at 1:19 AM David Miller wrote: > > From: Jeff Vander Stoep > > Date: Wed, 21 Aug 2019 15:45:47 +0200 > > > > > MAC addresses are often considered sensitive because they are > > > usually unique and can be used to identify/track a device or > > > user [1]. > > > > > > The MAC address is accessible via the RTM_NEWLINK message type of a > > > netlink route socket[2]. Ideally we could grant/deny access to the > > > MAC address on a case-by-case basis without blocking the entire > > > RTM_NEWLINK message type which contains a lot of other useful > > > information. This can be achieved using a new LSM hook on the netlink > > > message receive path. Using this new hook, individual LSMs can select > > > which processes are allowed access to the real MAC, otherwise a > > > default value of zeros is returned. Offloading access control > > > decisions like this to an LSM is convenient because it preserves the > > > status quo for most Linux users while giving the various LSMs > > > flexibility to make finer grained decisions on access to sensitive > > > data based on policy. > > > > > > [1] https://adamdrake.com/mac-addresses-udids-and-privacy.html > > > [2] Other access vectors like ioctl(SIOCGIFHWADDR) are already covered > > > by existing LSM hooks. > > > > > > Signed-off-by: Jeff Vander Stoep > > > > I'm sure the MAC address will escape into userspace via other means, > > dumping pieces of networking config in other contexts, etc. I mean, > > if I can get a link dump, I can dump the neighbor table as well. > > These are already gated by existing LSM hooks and capability checks. > They are not allowed on mandatory access control systems unless explicitly > granted. > > > I kinda think this is all very silly whack-a-mole kind of stuff, to > > be quite honest. > > We evaluated mechanisms for the MAC to reach unprivileged apps. > A number of researchers have published on this as well such as: > https://www.usenix.org/conference/usenixsecurity19/presentation/reardon > > Three "leaks" were identified, two have already been fixed. > -ioctl(SIOCGIFHWADDR). Fixed using finer grained LSM checks > on socket ioctls (similar to this change). > -IPv6 IP addresses. Fixed by no longer including the MAC as part > of the IP address. > -RTM_NEWLINK netlink route messages. The last mole to be whacked. > > > And like others have said, tomorrow you'll be like "oh crap, we should > > block X too" and we'll get another hook, another config knob, another > > rulset update, etc. > > This seems like an issue inherent with permissions/capabilities. I don’t > think we should abandon the concept of permissions because someone > can forget to add a check. Likewise, if someone adds new code to the > kernel and omits a capable(CAP_NET_*) check, I would expect it to be > fixed like any other bug without the idea of capability checks being tossed > out. > > We need to do something because this information is being abused. Any > recommendations? This seemed like the simplest approach, but I can > definitely appreciate that it has downsides. > > I could make this really generic by adding a single hook to the end of > sock_msgrecv() which would allow an LSM to modify the message to omit > the MAC address and any other information that we deem as sensitive in the > future. Basically what Casey was suggesting. Thoughts on that approach? I apologize for the delay in responding; I'm blaming LSS-NA travel. I'm also not a big fan of inserting the hook in rtnl_fill_ifinfo(); as presented it is way too specific for a LSM hook for me to be happy. However, I do agree that giving the LSMs some control over netlink messages makes sense. As others have pointed out, it's all a matter of where to place the hook. If we only care about netlink messages which leverage nlattrs I suppose one option that I haven't seen mentioned would be to place a hook in nla_put(). While it is a bit of an odd place for a hook, it would allow the LSM easy access to the skb and attribute type to make decisions, and all of the callers should already be checking the return code (although we would need to verify this). One notable drawback (not the only one) is that the hook is going to get hit multiple times for each message. -- paul moore www.paul-moore.com
Re: New skb extension for use by LSMs (skb "security blob")?
On Thu, Aug 22, 2019 at 3:03 AM Florian Westphal wrote: > Paul Moore wrote: > > Hello netdev, > > > > I was just made aware of the skb extension work, and it looks very > > appealing from a LSM perspective. As some of you probably remember, > > we (the LSM folks) have wanted a proper security blob in the skb for > > quite some time, but netdev has been resistant to this idea thus far. > > Is that "blob" in addition to skb->secmark, or a replacement? That's a good question. While I thought about that, I wasn't sure if that was worth bringing up as previous attempts to trade the secmark field for a void pointer met with failure. Last time I played with it I was able to take the additional 32-bits from holes in the skb, and possibly even improve some of the cacheline groupings (but that is always going to be a dependent on use case I think), but that wasn't enough. I think we could consider freeing up the secmark in the main skb, and move it to a skb extension, but this would potentially increase the chances that we would need to add an extension to a skb. I don't have any hard numbers, but based on discussions and questions I suspect Secmark is more widely used than NetLabel and/or labeled IPsec; although I'm confident it is still a minor percentage of the overall Linux installed base. For me the big question is what would it take for us to get a security blob associated with the skb? Would moving the secmark into the skb extension be enough? Something else? Or is this simply never going to happen? I want to remain optimistic, but I've been trying for this off-and-on for over a decade and keep running into a brick wall ;) -- paul moore www.paul-moore.com
Re: New skb extension for use by LSMs (skb "security blob")?
On Wed, Aug 21, 2019 at 6:50 PM David Miller wrote: > From: Paul Moore > Date: Wed, 21 Aug 2019 18:00:09 -0400 > > > I was just made aware of the skb extension work, and it looks very > > appealing from a LSM perspective. As some of you probably remember, > > we (the LSM folks) have wanted a proper security blob in the skb for > > quite some time, but netdev has been resistant to this idea thus far. > > > > If I were to propose a patchset to add a SKB_EXT_SECURITY skb > > extension (a single extension ID to be shared among the different > > LSMs), would that be something that netdev would consider merging, or > > is there still a philosophical objection to things like this? > > Unlike it's main intended user (MPTCP), it sounds like LSM's would use > this in a way such that it would be enabled on most systems all the > time. > > That really defeats the whole purpose of making it dynamic. :-/ I would be okay with only adding a skb extension when we needed it, which I'm currently thinking would only be when we had labeled networking actually configured at runtime and not just built into the kernel. In SELinux we do something similar today when it comes to our per-packet access controls; if labeled networking is not configured we bail out of the LSM hooks early to improve performance (we would just be comparing unlabeled_t to unlabeled_t anyway). I think the other LSMs would be okay with this usage as well. While a number of distros due enable some form of LSM and the labeled networking bits at build time, vary few (if any?) provide a default configuration so I would expect no additional overhead in the common case. Would that be acceptable? -- paul moore www.paul-moore.com
New skb extension for use by LSMs (skb "security blob")?
Hello netdev, I was just made aware of the skb extension work, and it looks very appealing from a LSM perspective. As some of you probably remember, we (the LSM folks) have wanted a proper security blob in the skb for quite some time, but netdev has been resistant to this idea thus far. If I were to propose a patchset to add a SKB_EXT_SECURITY skb extension (a single extension ID to be shared among the different LSMs), would that be something that netdev would consider merging, or is there still a philosophical objection to things like this? -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V6 02/10] audit: add container id
On Mon, Jul 8, 2019 at 2:06 PM Richard Guy Briggs wrote: > On 2019-05-30 15:29, Paul Moore wrote: ... > > [REMINDER: It is an "*audit* container ID" and not a general > > "container ID" ;) Smiley aside, I'm not kidding about that part.] > > > > I'm not interested in supporting/merging something that isn't useful; > > if this doesn't work for your use case then we need to figure out what > > would work. It sounds like nested containers are much more common in > > the lxc world, can you elaborate a bit more on this? > > > > As far as the possible solutions you mention above, I'm not sure I > > like the per-userns audit container IDs, I'd much rather just emit the > > necessary tracking information via the audit record stream and let the > > log analysis tools figure it out. However, the bigger question is how > > to limit (re)setting the audit container ID when you are in a non-init > > userns. For reasons already mentioned, using capable() is a non > > starter for everything but the initial userns, and using ns_capable() > > is equally poor as it essentially allows any userns the ability to > > munge it's audit container ID (obviously not good). It appears we > > need a different method for controlling access to the audit container > > ID. > > We're not quite ready yet for multiple audit daemons and possibly not > yet for audit namespaces, but this is starting to look a lot like the > latter. A few quick comments on audit namespaces: the audit container ID is not envisioned as a new namespace (even in nested form) and neither do I consider running multiple audit daemons to be a new namespace. >From my perspective we create namespaces to allow us to redefine a global resource for some subset of the system, e.g. providing a unique /tmp for some number of processes on the system. While it may be tempting to think of the audit container ID as something we could "namespace", especially when multiple audit daemons are concerned, in some ways this would be counter productive; the audit container ID is intended to be a global ID that can be used to associate audit event records with a "container" where the "container" is defined by an orchestrator outside the audit subsystem. The global nature of the audit container ID allows us to maintain a sane(ish) view of the system in the audit log, if we were to "namespace" the audit container ID such that the value was no longer guaranteed to be unique throughout the system, we would need to additionally track the audit namespace along with the audit container ID which starts to border on insanity IMHO. > If we can't trust ns_capable() then why are we passing on > CAP_AUDIT_CONTROL? It is being passed down and not stripped purposely > by the orchestrator/engine. If ns_capable() isn't inherited how is it > gained otherwise? Can it be inserted by cotainer image? I think the > answer is "no". Either we trust ns_capable() or we have audit > namespaces (recommend based on user namespace) (or both). My thinking is that since ns_capable() checks the credentials with respect to the current user namespace we can't rely on it to control access since it would be possible for a privileged process running inside an unprivileged container to manipulate the audit container ID (containerized process has CAP_AUDIT_CONTROL, e.g. running as root in the container, while the container itself does not). > At this point I would say we are at an impasse unless we trust > ns_capable() or we implement audit namespaces. I'm not sure how we can trust ns_capable(), but if you can think of a way I would love to hear it. I'm also not sure how namespacing audit is helpful (see my above comments), but if you think it is please explain. > I don't think another mechanism to trust nested orchestrators/engines > will buy us anything. > > Am I missing something? Based on your questions/comments above it looks like your understanding of ns_capable() does not match mine; if I'm thinking about ns_capable() incorrectly, please educate me. -- paul moore www.paul-moore.com
Re: [PATCH v2] hooks: fix a missing-check bug in selinux_sb_eat_lsm_opts()
On Thu, May 30, 2019 at 9:34 PM Gen Zhang wrote:
>
> In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> returns NULL when fails. So 'arg' should be checked.
>
> Signed-off-by: Gen Zhang
> Reviewed-by: Ondrej Mosnacek
> Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
One quick note about the subject line, instead of using "hooks:" you
should use "selinux:" since this is specific to SELinux. If the patch
did apply to the LSM framework as a whole, I would suggest using
"lsm:" instead of "hooks:" as "hooks" is too ambiguous of a prefix.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702c..5a9e959 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2635,6 +2635,8 @@ static int selinux_sb_eat_lsm_opts(char *options, void
> **mnt_opts)
> *q++ = c;
> }
> arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
> + if (!arg)
> + return -ENOMEM;
It seems like we should also check for, and potentially free *mnt_opts
as the selinux_add_opt() error handling does just below this change,
yes? If that is the case we might want to move that error handling
code to the bottom of the function and jump there on error.
> }
> rc = selinux_add_opt(token, arg, mnt_opts);
> if (unlikely(rc)) {
--
paul moore
www.paul-moore.com
Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
On Thu, May 9, 2019 at 4:40 AM Paolo Abeni wrote:
> On Wed, 2019-05-08 at 17:17 -0400, Paul Moore wrote:
> > On Wed, May 8, 2019 at 2:55 PM Stephen Smalley wrote:
> > > On 5/8/19 2:27 PM, Marcelo Ricardo Leitner wrote:
> > > > On Wed, May 08, 2019 at 02:13:17PM -0400, Stephen Smalley wrote:
> > > > > On 5/8/19 2:12 PM, Stephen Smalley wrote:
> > > > > > On 5/8/19 9:32 AM, Paolo Abeni wrote:
> > > > > > > calling connect(AF_UNSPEC) on an already connected TCP socket is
> > > > > > > an
> > > > > > > established way to disconnect() such socket. After commit
> > > > > > > 68741a8adab9
> > > > > > > ("selinux: Fix ltp test connect-syscall failure") it no longer
> > > > > > > works
> > > > > > > and, in the above scenario connect() fails with EAFNOSUPPORT.
> > > > > > >
> > > > > > > Fix the above falling back to the generic/old code when the
> > > > > > > address
> > > > > > > family
> > > > > > > is not AF_INET{4,6}, but leave the SCTP code path untouched, as
> > > > > > > it has
> > > > > > > specific constraints.
> > > > > > >
> > > > > > > Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall
> > > > > > > failure")
> > > > > > > Reported-by: Tom Deseyn
> > > > > > > Signed-off-by: Paolo Abeni
> > > > > > > ---
> > > > > > >security/selinux/hooks.c | 8
> > > > > > >1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > > > > index c61787b15f27..d82b87c16b0a 100644
> > > > > > > --- a/security/selinux/hooks.c
> > > > > > > +++ b/security/selinux/hooks.c
> > > > > > > @@ -4649,7 +4649,7 @@ static int
> > > > > > > selinux_socket_connect_helper(struct socket *sock,
> > > > > > >struct lsm_network_audit net = {0,};
> > > > > > >struct sockaddr_in *addr4 = NULL;
> > > > > > >struct sockaddr_in6 *addr6 = NULL;
> > > > > > > -unsigned short snum;
> > > > > > > +unsigned short snum = 0;
> > > > > > >u32 sid, perm;
> > > > > > >/* sctp_connectx(3) calls via
> > > > > > > selinux_sctp_bind_connect()
> > > > > > > @@ -4674,12 +4674,12 @@ static int
> > > > > > > selinux_socket_connect_helper(struct socket *sock,
> > > > > > >break;
> > > > > > >default:
> > > > > > >/* Note that SCTP services expect -EINVAL, whereas
> > > > > > > - * others expect -EAFNOSUPPORT.
> > > > > > > + * others must handle this at the protocol level:
> > > > > > > + * connect(AF_UNSPEC) on a connected socket is
> > > > > > > + * a documented way disconnect the socket.
> > > > > > > */
> > > > > > >if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> > > > > > >return -EINVAL;
> > > > > > > -else
> > > > > > > -return -EAFNOSUPPORT;
> > > > > >
> > > > > > I think we need to return 0 here. Otherwise, we'll fall through
> > > > > > with an
> > > > > > uninitialized snum, triggering a random/bogus permission check.
> > > > >
> > > > > Sorry, I see that you initialize snum above. Nonetheless, I think the
> > > > > correct behavior here is to skip the check since this is a
> > > > > disconnect, not a
> > > > > connect.
> > > >
> > > > Skipping the check would make it less controllable. So should it
> > > > somehow re-use shutdown() stuff? It gets very confusing, and after
> > > > all, it still is, in essence, a connect() syscall.
> > >
> > > The function checks CONNECT permission on entry, before reaching this
> > > point
Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
On Wed, May 8, 2019 at 2:55 PM Stephen Smalley wrote:
> On 5/8/19 2:27 PM, Marcelo Ricardo Leitner wrote:
> > On Wed, May 08, 2019 at 02:13:17PM -0400, Stephen Smalley wrote:
> >> On 5/8/19 2:12 PM, Stephen Smalley wrote:
> >>> On 5/8/19 9:32 AM, Paolo Abeni wrote:
> >>>> calling connect(AF_UNSPEC) on an already connected TCP socket is an
> >>>> established way to disconnect() such socket. After commit 68741a8adab9
> >>>> ("selinux: Fix ltp test connect-syscall failure") it no longer works
> >>>> and, in the above scenario connect() fails with EAFNOSUPPORT.
> >>>>
> >>>> Fix the above falling back to the generic/old code when the address
> >>>> family
> >>>> is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has
> >>>> specific constraints.
> >>>>
> >>>> Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure")
> >>>> Reported-by: Tom Deseyn
> >>>> Signed-off-by: Paolo Abeni
> >>>> ---
> >>>>security/selinux/hooks.c | 8
> >>>>1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>>> index c61787b15f27..d82b87c16b0a 100644
> >>>> --- a/security/selinux/hooks.c
> >>>> +++ b/security/selinux/hooks.c
> >>>> @@ -4649,7 +4649,7 @@ static int
> >>>> selinux_socket_connect_helper(struct socket *sock,
> >>>>struct lsm_network_audit net = {0,};
> >>>>struct sockaddr_in *addr4 = NULL;
> >>>>struct sockaddr_in6 *addr6 = NULL;
> >>>> -unsigned short snum;
> >>>> +unsigned short snum = 0;
> >>>>u32 sid, perm;
> >>>>/* sctp_connectx(3) calls via selinux_sctp_bind_connect()
> >>>> @@ -4674,12 +4674,12 @@ static int
> >>>> selinux_socket_connect_helper(struct socket *sock,
> >>>>break;
> >>>>default:
> >>>>/* Note that SCTP services expect -EINVAL, whereas
> >>>> - * others expect -EAFNOSUPPORT.
> >>>> + * others must handle this at the protocol level:
> >>>> + * connect(AF_UNSPEC) on a connected socket is
> >>>> + * a documented way disconnect the socket.
> >>>> */
> >>>>if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> >>>>return -EINVAL;
> >>>> -else
> >>>> -return -EAFNOSUPPORT;
> >>>
> >>> I think we need to return 0 here. Otherwise, we'll fall through with an
> >>> uninitialized snum, triggering a random/bogus permission check.
> >>
> >> Sorry, I see that you initialize snum above. Nonetheless, I think the
> >> correct behavior here is to skip the check since this is a disconnect, not
> >> a
> >> connect.
> >
> > Skipping the check would make it less controllable. So should it
> > somehow re-use shutdown() stuff? It gets very confusing, and after
> > all, it still is, in essence, a connect() syscall.
>
> The function checks CONNECT permission on entry, before reaching this
> point. This logic is only in preparation for a further check
> (NAME_CONNECT) on the port. In this case, there is no further check to
> perform and we can just return.
I agree with Stephen, in the connect(AF_UNSPEC) case the right thing
to do is to simply return with no error.
I would also suggest that since this patch only touches the SELinux
code it really should go in via the SELinux tree and not netdev; this
will help avoid merge conflicts in the linux-next tree and during the
merge window. I think the right thing to do at this point is to
create a revert patch (or have DaveM do it, I'm not sure what he
prefers in situations like this) for this commit, make the adjustments
that Stephen mentioned and submit them for the SELinux tree.
Otherwise, thanks for catching this and submitting a fix :)
--
paul moore
www.paul-moore.com
Re: [PATCH] net: socket: Always initialize family field at move_addr_to_kernel().
On Thu, Apr 11, 2019 at 7:32 AM Tetsuo Handa wrote: > On 2019/04/04 13:49, David Miller wrote: > > From: Tetsuo Handa > > Date: Wed, 3 Apr 2019 06:07:40 +0900 > > > >> On 2019/04/03 5:23, David Miller wrote: > >>> Please fix RDS and other protocols to examine the length properly > >>> instead. > >> > >> Do you prefer adding branches only for allow reading the family of socket > >> address? > > > > If the length is zero, there is no point in reading the family. > > > > (Adding LSM people.) > > syzbot is reporting that RDS is not checking valid length of address given > from userspace. > It turned out that there are several users who access "struct > sockaddr"->family without > checking valid length (which will be reported by KMSAN). > > Unfortunately, since tipc_bind() in net/tipc/socket.c accepts length == 0 as > a valid input, > we can't reject length < offsetofend(struct sockaddr, sa_family) with -EINVAL > at > move_addr_to_kernel() which are called from bind()/connect() system calls. I > proposed > always setting "struct sockaddr"->family at move_addr_to_kernel() but David > does not > like such trick. > > Therefore, LSM modules which checks address and/or port have to check valid > length > before accessing "struct sockaddr"->family in order to determine IPv4 or IPv6 > or UNIX. > > Below is all-in-one change (for x86_64 allmodconfig). Well, I've just > realized that > move_addr_to_kernel() is also called by sendmsg() system call and for now > added changes > for only security/ directory. Is this change appropriate for you? (I wish we > could > simplify this by automatically initializing "struct sockaddr_storage" with 0 > at > move_addr_to_kernel()...) > --- > drivers/isdn/mISDN/socket.c | 4 ++-- > net/bluetooth/sco.c | 4 ++-- > net/core/filter.c | 2 ++ > net/ipv6/udp.c | 2 ++ > net/llc/af_llc.c| 3 +-- > net/netlink/af_netlink.c| 3 ++- > net/rds/af_rds.c| 3 +++ > net/rds/bind.c | 2 ++ > net/rxrpc/af_rxrpc.c| 3 ++- > net/sctp/socket.c | 3 ++- > security/apparmor/lsm.c | 6 ++ > security/selinux/hooks.c| 12 ++++ > security/smack/smack_lsm.c | 39 +++ > security/tomoyo/network.c | 12 > 14 files changed, 85 insertions(+), 13 deletions(-) Some minor nits/comments below, but I think this is still okay as-is from a SELinux perspective. Acked-by: Paul Moore > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index d5fdcb0..710d386 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4503,6 +4503,12 @@ static int selinux_socket_bind(struct socket *sock, > struct sockaddr *address, in > err = sock_has_perm(sk, SOCKET__BIND); > if (err) > goto out; > + /* > +* Nothing more to do if valid length is too short to check > +* address->sa_family. > +*/ > + if (addrlen < offsetofend(struct sockaddr, sa_family)) > + goto out; SELinux already checks the address length further down in selinux_socket_bind() for the address families it care about, although it does read sa_family before the address length check so no unfortunately it's of no help. I imagine you could move this new length check inside the PF_INET/PF_INET6 if-then code block to minimize the impact to other address families, but I suppose there is some value in keeping it where it is too. > /* If PF_INET or PF_INET6, check name_bind permission for the port. */ > family = sk->sk_family; > @@ -4634,6 +4640,12 @@ static int selinux_socket_connect_helper(struct socket > *sock, > err = sock_has_perm(sk, SOCKET__CONNECT); > if (err) > return err; > + /* > +* Nothing more to do if valid length is too short to check > +* address->sa_family. > +*/ > + if (addrlen < offsetofend(struct sockaddr, sa_family)) > + return 0; Similar comments as above, including moving the check inside the if-then block. > /* > * If a TCP, DCCP or SCTP socket, check name_connect permission -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V6 05/10] audit: add contid support for signalling the audit daemon
On Tue, Apr 9, 2019 at 9:49 AM Neil Horman wrote: > On Tue, Apr 09, 2019 at 09:40:58AM -0400, Paul Moore wrote: > > On Tue, Apr 9, 2019 at 8:58 AM Ondrej Mosnacek wrote: > > > > > > On Tue, Apr 9, 2019 at 5:40 AM Richard Guy Briggs wrote: > > > > Add audit container identifier support to the action of signalling the > > > > audit daemon. > > > > > > > > Since this would need to add an element to the audit_sig_info struct, > > > > a new record type AUDIT_SIGNAL_INFO2 was created with a new > > > > audit_sig_info2 struct. Corresponding support is required in the > > > > userspace code to reflect the new record request and reply type. > > > > An older userspace won't break since it won't know to request this > > > > record type. > > > > > > > > Signed-off-by: Richard Guy Briggs > > > > > > This looks good to me. > > > > > > Reviewed-by: Ondrej Mosnacek > > > > > > Although I'm wondering if we shouldn't try to future-proof the > > > AUDIT_SIGNAL_INFO2 format somehow, so that we don't need to add > > > another AUDIT_SIGNAL_INFO3 when the need arises to add yet-another > > > identifier to it... The simplest solution I can come up with is to add > > > a "version" field at the beginning (set to 2 initially), then v_len > > > at the beginning of data for version . But maybe this is too > > > complicated for too little gain... > > > > FWIW, I believe the long term solution to this is the fabled netlink > > attribute approach that we haven't talked about in some time, but I > > keep dreaming about (it has been mostly on the back burner becasue 1) > > time and 2) didn't want to impact the audit container ID work). While > > I'm not opposed to trying to make things like this a bit more robust > > by adding version fields and similar things, there are still so many > > (so very many) problems with the audit kernel/userspace interface that > > still need to be addressed. > > Agreed, this change as-is is in keeping with the message structure that audit > has today, and so is ok with me, but the long term goal should be a conversion > to netlink attributes for all audit messages. Thats a big undertaking and > should be addressed separately though. You've likely missed all the conversations around this from some time ago, but this is the direction I want us to go towards eventually, and yes, this is a huge undertaking (much larger than the audit container ID work) that will need to be done in stages. The first step is moving away from audit_log_format() to an in-kernel audit API that separates the data from the record format; I've got a lot of ideas on that, but as I said earlier, it's mostly on the back burner so it doesn't hold up the audit container ID work. -- paul moore www.paul-moore.com
Re: [PATCH net] selinux: add the missing walk_size + len check in selinux_sctp_bind_connect
On Fri, Mar 8, 2019 at 9:47 PM Paul Moore wrote:
> On Fri, Mar 8, 2019 at 12:08 PM Marcelo Ricardo Leitner
> wrote:
> > On Sat, Mar 09, 2019 at 12:07:34AM +0800, Xin Long wrote:
> > > As does in __sctp_connect(), when checking addrs in a while loop, after
> > > get the addr len according to sa_family, it's necessary to do the check
> > > walk_size + af->sockaddr_len > addrs_size to make sure it won't access
> > > an out-of-bounds addr.
> > >
> > > The same thing is needed in selinux_sctp_bind_connect(), otherwise an
> > > out-of-bounds issue can be triggered:
> > >
> > > [14548.772313] BUG: KASAN: slab-out-of-bounds in
> > > selinux_sctp_bind_connect+0x1aa/0x1f0
> > > [14548.927083] Call Trace:
> > > [14548.938072] dump_stack+0x9a/0xe9
> > > [14548.953015] print_address_description+0x65/0x22e
> > > [14548.996524] kasan_report.cold.6+0x92/0x1a6
> > > [14549.015335] selinux_sctp_bind_connect+0x1aa/0x1f0
> > > [14549.036947] security_sctp_bind_connect+0x58/0x90
> > > [14549.058142] __sctp_setsockopt_connectx+0x5a/0x150 [sctp]
> > > [14549.081650] sctp_setsockopt.part.24+0x1322/0x3ce0 [sctp]
> > >
> > > Fixes: d452930fd3b9 ("selinux: Add SCTP support")
> > > Reported-by: Chunyu Hu
> > > Signed-off-by: Xin Long
> >
> > Reviewed-by: Marcelo Ricardo Leitner
>
> Thanks, this patch looks good to me.
>
> > Paul, how can we get this into -stable trees? SELinux process may be
> > different from -net trees.
>
> For patches that warrant inclusion in -stable, I merge them into the
> current selinux/stable-X.Y and send it up to Linus once it passes some
> basic sanity tests. Since we're still only half-way through the merge
> window, and this is an obvious bug fix, I think this qualifies as
> -stable material.
>
> I'm traveling at the moment with not-so-great connectivity, but I'll
> get this merged and verified early next week.
FYI, I just merged this into selinux/stable-5.1. Assuming it tests
okay (and I can't imagine it would cause a regression), I'll send it
to Linus tomorrow.
--
paul moore
www.paul-moore.com
Re: [PATCH net] selinux: add the missing walk_size + len check in selinux_sctp_bind_connect
On Fri, Mar 8, 2019 at 12:08 PM Marcelo Ricardo Leitner
wrote:
> On Sat, Mar 09, 2019 at 12:07:34AM +0800, Xin Long wrote:
> > As does in __sctp_connect(), when checking addrs in a while loop, after
> > get the addr len according to sa_family, it's necessary to do the check
> > walk_size + af->sockaddr_len > addrs_size to make sure it won't access
> > an out-of-bounds addr.
> >
> > The same thing is needed in selinux_sctp_bind_connect(), otherwise an
> > out-of-bounds issue can be triggered:
> >
> > [14548.772313] BUG: KASAN: slab-out-of-bounds in
> > selinux_sctp_bind_connect+0x1aa/0x1f0
> > [14548.927083] Call Trace:
> > [14548.938072] dump_stack+0x9a/0xe9
> > [14548.953015] print_address_description+0x65/0x22e
> > [14548.996524] kasan_report.cold.6+0x92/0x1a6
> > [14549.015335] selinux_sctp_bind_connect+0x1aa/0x1f0
> > [14549.036947] security_sctp_bind_connect+0x58/0x90
> > [14549.058142] __sctp_setsockopt_connectx+0x5a/0x150 [sctp]
> > [14549.081650] sctp_setsockopt.part.24+0x1322/0x3ce0 [sctp]
> >
> > Fixes: d452930fd3b9 ("selinux: Add SCTP support")
> > Reported-by: Chunyu Hu
> > Signed-off-by: Xin Long
>
> Reviewed-by: Marcelo Ricardo Leitner
Thanks, this patch looks good to me.
> Paul, how can we get this into -stable trees? SELinux process may be
> different from -net trees.
For patches that warrant inclusion in -stable, I merge them into the
current selinux/stable-X.Y and send it up to Linus once it passes some
basic sanity tests. Since we're still only half-way through the merge
window, and this is an obvious bug fix, I think this qualifies as
-stable material.
I'm traveling at the moment with not-so-great connectivity, but I'll
get this merged and verified early next week.
--
paul moore
www.paul-moore.com
Re: [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create()
On Fri, Mar 8, 2019 at 1:31 AM maowenan wrote:
> On 2019/3/8 4:36, Paul Moore wrote:
> > On Wed, Mar 6, 2019 at 9:44 PM Mao Wenan wrote:
> >>
> >> If netlbl_sock_setattr() is failed, it directly returns rc and forgets
> >> to free secattr.
> >>
> >> BUG: memory leak
> >> unreferenced object 0x8883c3ea4200 (size 2664):
> >> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> >> hex dump (first 32 bytes):
> >> 7f 00 00 01 7f 00 00 01 eb 7f ed 71 4e 24 00 00 ...qN$..
> >> 02 00 07 40 00 00 00 00 00 00 00 00 00 00 00 00 ...@
> >> backtrace:
> >> [<4c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> >> [<535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> >> [<9aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> >> [<4274b384>] sock_create net/socket.c:1315 [inline]
> >> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >> [<4ae3186e>] do_syscall_64+0xc8/0x580
> >> arch/x86/entry/common.c:290
> >> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >> [<f737e62f>] 0x
> >>
> >> BUG: memory leak
> >> unreferenced object 0x8883de23d570 (size 32):
> >> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> >> hex dump (first 32 bytes):
> >> 02 00 00 00 00 00 00 00 60 a9 40 ce 83 88 ff ff `.@.
> >> 01 00 00 00 03 00 00 00 10 00 00 00 00 00 00 00
> >> backtrace:
> >> [<35ba8b75>] security_sk_alloc+0x5e/0xb0
> >> security/security.c:1473
> >> [<302cc426>] sk_prot_alloc+0x8e/0x290 net/core/sock.c:1472
> >> [<4c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> >> [<535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> >> [<9aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> >> [<4274b384>] sock_create net/socket.c:1315 [inline]
> >> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >> [<4ae3186e>] do_syscall_64+0xc8/0x580
> >> arch/x86/entry/common.c:290
> >> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >> [<f737e62f>] 0x
> >>
> >> BUG: memory leak
> >> unreferenced object 0x8883ce40a960 (size 64):
> >> comm "syz-executor.2", pid 8813, jiffies 4297264420 (age 156.089s)
> >> hex dump (first 32 bytes):
> >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> backtrace:
> >> [<29add401>] selinux_netlbl_socket_post_create+0x68/0x130
> >> security/selinux/netlabel.c:416
> >> [<5368a19c>] selinux_socket_post_create+0x31a/0x7f0
> >> security/selinux/hooks.c:4597
> >> [<bd4730e2>] security_socket_post_create+0x70/0xc0
> >> security/security.c:1385
> >> [<671052a4>] __sock_create+0x5a6/0x790 net/socket.c:1291
> >> [<4274b384>] sock_create net/socket.c:1315 [inline]
> >> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >> [<4ae3186e>] do_syscall_64+0xc8/0x580
> >> arch/x86/entry/common.c:290
> >> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >> [<f737e62f>] 0x
> >>
> >> Fixes: 389fb800ac8b("netlabel: Label incoming TCP
Re: [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create()
On Wed, Mar 6, 2019 at 9:44 PM Mao Wenan wrote:
>
> If netlbl_sock_setattr() is failed, it directly returns rc and forgets
> to free secattr.
>
> BUG: memory leak
> unreferenced object 0x8883c3ea4200 (size 2664):
> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> hex dump (first 32 bytes):
> 7f 00 00 01 7f 00 00 01 eb 7f ed 71 4e 24 00 00 ...qN$..
> 02 00 07 40 00 00 00 00 00 00 00 00 00 00 00 00 ...@
> backtrace:
> [<4c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> [<535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> [<9aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> [<4274b384>] sock_create net/socket.c:1315 [inline]
> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> [<4ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<f737e62f>] 0x
>
> BUG: memory leak
> unreferenced object 0x8883de23d570 (size 32):
> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> hex dump (first 32 bytes):
> 02 00 00 00 00 00 00 00 60 a9 40 ce 83 88 ff ff `.@.
> 01 00 00 00 03 00 00 00 10 00 00 00 00 00 00 00
> backtrace:
> [<35ba8b75>] security_sk_alloc+0x5e/0xb0 security/security.c:1473
> [<302cc426>] sk_prot_alloc+0x8e/0x290 net/core/sock.c:1472
> [<4c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> [<535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> [<9aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> [<4274b384>] sock_create net/socket.c:1315 [inline]
> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> [<4ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<f737e62f>] 0x
>
> BUG: memory leak
> unreferenced object 0x8883ce40a960 (size 64):
> comm "syz-executor.2", pid 8813, jiffies 4297264420 (age 156.089s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> backtrace:
> [<29add401>] selinux_netlbl_socket_post_create+0x68/0x130
> security/selinux/netlabel.c:416
> [<5368a19c>] selinux_socket_post_create+0x31a/0x7f0
> security/selinux/hooks.c:4597
> [<bd4730e2>] security_socket_post_create+0x70/0xc0
> security/security.c:1385
> [<671052a4>] __sock_create+0x5a6/0x790 net/socket.c:1291
> [<4274b384>] sock_create net/socket.c:1315 [inline]
> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> [<4ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<f737e62f>] 0x
>
> Fixes: 389fb800ac8b("netlabel: Label incoming TCP connections correctly in
> SELinux")
> Reported-by: Hulk Robot
> Signed-off-by: Mao Wenan
> ---
> security/selinux/netlabel.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index 186e727b737b..f3da05338580 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -426,6 +426,9 @@ int selinux_netlbl_socket_post_create(struct sock *sk,
> u16 family)
> rc = 0;
> break;
> }
> + if (rc != 0) {
> + netlbl_secattr_free(secattr);
> + }
This is likely going to cause a problem as the
sock->sk_security->nlbl_secattr still has a reference to the secattr
pointer you are releasing here. Assuming things are working correctly
elsewhere, I believe freeing secattr here will result in a double free
when the network stacks cleans up after the failed socket creation
(via the sock_release() call in the error handling code).
It looks like you may have found this via a test tool (syzbot?), do
you have a reproducer you can share?
--
paul moore
www.paul-moore.com
[PATCH] netlabel: fix out-of-bounds memory accesses
There are two array out-of-bounds memory accesses, one in
cipso_v4_map_lvl_valid(), the other in netlbl_bitmap_walk(). Both
errors are embarassingly simple, and the fixes are straightforward.
As a FYI for anyone backporting this patch to kernels prior to v4.8,
you'll want to apply the netlbl_bitmap_walk() patch to
cipso_v4_bitmap_walk() as netlbl_bitmap_walk() doesn't exist before
Linux v4.8.
Reported-by: Jann Horn
Fixes: 446fda4f2682 ("[NetLabel]: CIPSOv4 engine")
Fixes: 3faa8f982f95 ("netlabel: Move bitmap manipulation functions to the
NetLabel core.")
Signed-off-by: Paul Moore
---
net/ipv4/cipso_ipv4.c|3 ++-
net/netlabel/netlabel_kapi.c |3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 777fa3b7fb13..f4b83de2263e 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -667,7 +667,8 @@ static int cipso_v4_map_lvl_valid(const struct cipso_v4_doi
*doi_def, u8 level)
case CIPSO_V4_MAP_PASS:
return 0;
case CIPSO_V4_MAP_TRANS:
- if (doi_def->map.std->lvl.cipso[level] < CIPSO_V4_INV_LVL)
+ if ((level < doi_def->map.std->lvl.cipso_size) &&
+ (doi_def->map.std->lvl.cipso[level] < CIPSO_V4_INV_LVL))
return 0;
break;
}
diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
index ea7c67050792..ee3e5b6471a6 100644
--- a/net/netlabel/netlabel_kapi.c
+++ b/net/netlabel/netlabel_kapi.c
@@ -903,7 +903,8 @@ int netlbl_bitmap_walk(const unsigned char *bitmap, u32
bitmap_len,
(state == 0 && (byte & bitmask) == 0))
return bit_spot;
- bit_spot++;
+ if (++bit_spot >= bitmap_len)
+ return -1;
bitmask >>= 1;
if (bitmask == 0) {
byte = bitmap[++byte_offset];
Re: [PATCH v2 1/2] NETWORKING: avoid use IPCB in cipso_v4_error
On Mon, Feb 25, 2019 at 5:33 PM David Miller wrote: > From: Nazarov Sergey > Date: Mon, 25 Feb 2019 19:24:15 +0300 > > > Add __icmp_send function having ip_options struct parameter > > > > Signed-off-by: Sergey Nazarov > > Applied with Subject line fixes up. This commit doesn't even make > changes to cipse_v4_error(). > > Anyone who ACK'd this change or added their Reviewed-by did not read > this email and are just rubber stamping crap. I should have checked the subject line closer that's my fault (Gmail does interesting things to threads sometimes, and obscured the subject line), however, I did look at the content of patch before giving it my thumbs up. Claiming the email wasn't read isn't correct (although you could rightly argue I didn't read the subject line), or I'm "rubber stamping crap" isn't correct. -- paul moore www.paul-moore.com
Re: [PATCH v2 1/2] NETWORKING: avoid use IPCB in cipso_v4_error
On Mon, Feb 25, 2019 at 11:24 AM Nazarov Sergey wrote:
>
> Add __icmp_send function having ip_options struct parameter
>
> Signed-off-by: Sergey Nazarov
> ---
> include/net/icmp.h|9 -
> net/ipv4/icmp.c |7 ---
> 2 files changed, 12 insertions(+), 4 deletions(-)
Reviewed-by: Paul Moore
> diff --git a/include/net/icmp.h b/include/net/icmp.h
> index 6ac3a5b..e0f709d 100644
> --- a/include/net/icmp.h
> +++ b/include/net/icmp.h
> @@ -22,6 +22,7 @@
>
> #include
> #include
> +#include
>
> struct icmp_err {
>int errno;
> @@ -39,7 +40,13 @@ struct icmp_err {
> struct sk_buff;
> struct net;
>
> -void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info);
> +void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
> +const struct ip_options *opt);
> +static inline void icmp_send(struct sk_buff *skb_in, int type, int code,
> __be32 info)
> +{
> + __icmp_send(skb_in, type, code, info, &IPCB(skb_in)->opt);
> +}
> +
> int icmp_rcv(struct sk_buff *skb);
> int icmp_err(struct sk_buff *skb, u32 info);
> int icmp_init(void);
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 065997f..3f24414 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -570,7 +570,8 @@ static void icmp_reply(struct icmp_bxm *icmp_param,
> struct sk_buff *skb)
> * MUST reply to only the first fragment.
> */
>
> -void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
> +void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
> +const struct ip_options *opt)
> {
> struct iphdr *iph;
> int room;
> @@ -691,7 +692,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int
> code, __be32 info)
> iph->tos;
> mark = IP4_REPLY_MARK(net, skb_in->mark);
>
> - if (ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in))
> + if (__ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in,
> opt))
> goto out_unlock;
>
>
> @@ -742,7 +743,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int
> code, __be32 info)
> local_bh_enable();
> out:;
> }
> -EXPORT_SYMBOL(icmp_send);
> +EXPORT_SYMBOL(__icmp_send);
>
>
> static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
> ---
>
--
paul moore
www.paul-moore.com
Re: [PATCH v2 2/2] NETWORKING: avoid use IPCB in cipso_v4_error
On Mon, Feb 25, 2019 at 11:27 AM Nazarov Sergey wrote:
>
> Extract IP options in cipso_v4_error and use __icmp_send.
>
> Signed-off-by: Sergey Nazarov
> ---
> include/net/ip.h |2 ++
> net/ipv4/cipso_ipv4.c | 17 +++--
> net/ipv4/ip_options.c | 22 +-
> 3 files changed, 34 insertions(+), 7 deletions(-)
Thanks Sergey.
Acked-by: Paul Moore
> diff --git a/include/net/ip.h b/include/net/ip.h
> index 8866bfc..f0e8d06 100644
> --- a/include/net/ip.h
> +++ b/include/net/ip.h
> @@ -667,6 +667,8 @@ static inline int ip_options_echo(struct net *net, struct
> ip_options *dopt,
> }
>
> void ip_options_fragment(struct sk_buff *skb);
> +int __ip_options_compile(struct net *net, struct ip_options *opt,
> +struct sk_buff *skb, __be32 *info);
> int ip_options_compile(struct net *net, struct ip_options *opt,
>struct sk_buff *skb);
> int ip_options_get(struct net *net, struct ip_options_rcu **optp,
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 777fa3b..eff86a7 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1735,13 +1735,26 @@ int cipso_v4_validate(const struct sk_buff *skb,
> unsigned char **option)
> */
> void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
> {
> + unsigned char optbuf[sizeof(struct ip_options) + 40];
> + struct ip_options *opt = (struct ip_options *)optbuf;
> +
> if (ip_hdr(skb)->protocol == IPPROTO_ICMP || error != -EACCES)
> return;
>
> + /*
> +* We might be called above the IP layer,
> +* so we can not use icmp_send and IPCB here.
> +*/
> +
> + memset(opt, 0, sizeof(struct ip_options));
> + opt->optlen = ip_hdr(skb)->ihl*4 - sizeof(struct iphdr);
> + if (__ip_options_compile(dev_net(skb->dev), opt, skb, NULL))
> + return;
> +
> if (gateway)
> - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0);
> + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0, opt);
> else
> - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0);
> + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0, opt);
> }
>
> /**
> diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
> index ed194d4..32a3504 100644
> --- a/net/ipv4/ip_options.c
> +++ b/net/ipv4/ip_options.c
> @@ -251,8 +251,9 @@ static void spec_dst_fill(__be32 *spec_dst, struct
> sk_buff *skb)
> * If opt == NULL, then skb->data should point to IP header.
> */
>
> -int ip_options_compile(struct net *net,
> - struct ip_options *opt, struct sk_buff *skb)
> +int __ip_options_compile(struct net *net,
> +struct ip_options *opt, struct sk_buff *skb,
> +__be32 *info)
> {
> __be32 spec_dst = htonl(INADDR_ANY);
> unsigned char *pp_ptr = NULL;
> @@ -468,11 +469,22 @@ int ip_options_compile(struct net *net,
> return 0;
>
> error:
> - if (skb) {
> - icmp_send(skb, ICMP_PARAMETERPROB, 0,
> htonl((pp_ptr-iph)<<24));
> - }
> + if (info)
> + *info = htonl((pp_ptr-iph)<<24);
> return -EINVAL;
> }
> +
> +int ip_options_compile(struct net *net,
> + struct ip_options *opt, struct sk_buff *skb)
> +{
> + int ret;
> + __be32 info;
> +
> + ret = __ip_options_compile(net, opt, skb, &info);
> + if (ret != 0 && skb)
> + icmp_send(skb, ICMP_PARAMETERPROB, 0, info);
> + return ret;
> +}
> EXPORT_SYMBOL(ip_options_compile);
>
> /*
> ---
>
--
paul moore
www.paul-moore.com
Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error
On Fri, Feb 15, 2019 at 3:00 PM David Miller wrote:
> From: Paul Moore
> Date: Fri, 15 Feb 2019 14:02:31 -0500
>
> > On Thu, Feb 14, 2019 at 11:43 AM David Miller wrote:
> >> From: Nazarov Sergey
> >> Date: Tue, 12 Feb 2019 18:10:03 +0300
> >>
> >> > Since cipso_v4_error might be called from different network stack
> >> > layers, we can't safely use icmp_send there.
> >> > icmp_send copies IP options with ip_option_echo, which uses IPCB to take
> >> > access to IP header compiled data.
> >> > But after commit 971f10ec ("tcp: better TCP_SKB_CB layout to reduce
> >> > cache line misses"), IPCB can't be used
> >> > above IP layer.
> >> > This patch fixes the problem by creating in cipso_v4_error a local copy
> >> > of compiled IP options and using it with
> >> > introduced __icmp_send function. This looks some overloaded, but in
> >> > quite rare error conditions only.
> >> >
> >> > The original discussion is here:
> >> > https://lore.kernel.org/linux-security-module/16659801547571...@sas1-890ba5c2334a.qloud-c.yandex.net/
> >> >
> >> > Signed-off-by: Sergey Nazarov
> >>
> >> This problem is not unique to Cipso, net/atm/clip.c's error handler
> >> has the same exact issue.
> >>
> >> I didn't scan more of the tree, there are probably a couple more
> >> locations as well.
> >
> > David, are you happy with Sergey's solution as a fix for this?
> >
> > If so, would you prefer a respin of this patch to apply the to the
> > other broken callers (e.g. net/atm/clip.c), or would you rather merge
> > this patch and deal with the other callers in separate patches?
>
> I'd like the other broken callers to be handled.
Sergey, do you think you could fix the other callers too, or do you
want some help with that?
--
paul moore
www.paul-moore.com
Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error
On Thu, Feb 14, 2019 at 11:43 AM David Miller wrote:
> From: Nazarov Sergey
> Date: Tue, 12 Feb 2019 18:10:03 +0300
>
> > Since cipso_v4_error might be called from different network stack layers,
> > we can't safely use icmp_send there.
> > icmp_send copies IP options with ip_option_echo, which uses IPCB to take
> > access to IP header compiled data.
> > But after commit 971f10ec ("tcp: better TCP_SKB_CB layout to reduce cache
> > line misses"), IPCB can't be used
> > above IP layer.
> > This patch fixes the problem by creating in cipso_v4_error a local copy of
> > compiled IP options and using it with
> > introduced __icmp_send function. This looks some overloaded, but in quite
> > rare error conditions only.
> >
> > The original discussion is here:
> > https://lore.kernel.org/linux-security-module/16659801547571...@sas1-890ba5c2334a.qloud-c.yandex.net/
> >
> > Signed-off-by: Sergey Nazarov
>
> This problem is not unique to Cipso, net/atm/clip.c's error handler
> has the same exact issue.
>
> I didn't scan more of the tree, there are probably a couple more
> locations as well.
David, are you happy with Sergey's solution as a fix for this?
If so, would you prefer a respin of this patch to apply the to the
other broken callers (e.g. net/atm/clip.c), or would you rather merge
this patch and deal with the other callers in separate patches?
--
paul moore
www.paul-moore.com
Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error
On Tue, Feb 12, 2019 at 10:10 AM Nazarov Sergey wrote:
> Since cipso_v4_error might be called from different network stack layers, we
> can't safely use icmp_send there.
> icmp_send copies IP options with ip_option_echo, which uses IPCB to take
> access to IP header compiled data.
> But after commit 971f10ec ("tcp: better TCP_SKB_CB layout to reduce cache
> line misses"), IPCB can't be used
> above IP layer.
> This patch fixes the problem by creating in cipso_v4_error a local copy of
> compiled IP options and using it with
> introduced __icmp_send function. This looks some overloaded, but in quite
> rare error conditions only.
>
> The original discussion is here:
> https://lore.kernel.org/linux-security-module/16659801547571...@sas1-890ba5c2334a.qloud-c.yandex.net/
>
> Signed-off-by: Sergey Nazarov
> ---
> include/net/icmp.h|9 -
> net/ipv4/cipso_ipv4.c | 18 --
> net/ipv4/icmp.c |7 ---
> 3 files changed, 28 insertions(+), 6 deletions(-)
Hi Sergey,
Thanks for your work on finding this and putting a fix together. As
we discussed previously, I think this looks good, but can you describe
the testing you did to verify that this works correctly?
> diff --git a/include/net/icmp.h b/include/net/icmp.h
> index 6ac3a5b..e0f709d 100644
> --- a/include/net/icmp.h
> +++ b/include/net/icmp.h
> @@ -22,6 +22,7 @@
>
> #include
> #include
> +#include
>
> struct icmp_err {
>int errno;
> @@ -39,7 +40,13 @@ struct icmp_err {
> struct sk_buff;
> struct net;
>
> -void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info);
> +void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
> +const struct ip_options *opt);
> +static inline void icmp_send(struct sk_buff *skb_in, int type, int code,
> __be32 info)
> +{
> + __icmp_send(skb_in, type, code, info, &IPCB(skb_in)->opt);
> +}
> +
> int icmp_rcv(struct sk_buff *skb);
> int icmp_err(struct sk_buff *skb, u32 info);
> int icmp_init(void);
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 777fa3b..234d12e 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1735,13 +1735,27 @@ int cipso_v4_validate(const struct sk_buff *skb,
> unsigned char **option)
> */
> void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
> {
> + unsigned char optbuf[sizeof(struct ip_options) + 40];
> + struct ip_options *opt = (struct ip_options *)optbuf;
> +
> if (ip_hdr(skb)->protocol == IPPROTO_ICMP || error != -EACCES)
> return;
>
> + /*
> +* We might be called above the IP layer,
> +* so we can not use icmp_send and IPCB here.
> +*/
> +
> + memset(opt, 0, sizeof(struct ip_options));
> + opt->optlen = ip_hdr(skb)->ihl*4 - sizeof(struct iphdr);
> + memcpy(opt->__data, (unsigned char *)&(ip_hdr(skb)[1]), opt->optlen);
> + if (ip_options_compile(dev_net(skb->dev), opt, NULL))
> + return;
> +
> if (gateway)
> - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0);
> + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0, opt);
> else
> - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0);
> + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0, opt);
> }
>
> /**
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 065997f..3f24414 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -570,7 +570,8 @@ static void icmp_reply(struct icmp_bxm *icmp_param,
> struct sk_buff *skb)
> * MUST reply to only the first fragment.
> */
>
> -void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
> +void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
> +const struct ip_options *opt)
> {
> struct iphdr *iph;
> int room;
> @@ -691,7 +692,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int
> code, __be32 info)
> iph->tos;
> mark = IP4_REPLY_MARK(net, skb_in->mark);
>
> - if (ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in))
> + if (__ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in,
> opt))
> goto out_unlock;
>
>
> @@ -742,7 +743,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int
> code, __be32 info)
> local_bh_enable();
> out:;
> }
> -EXPORT_SYMBOL(icmp_send);
> +EXPORT_SYMBOL(__icmp_send);
>
>
> static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
> --
>
--
paul moore
www.paul-moore.com
Re: Kernel memory corruption in CIPSO labeled TCP packets processing.
On Mon, Feb 11, 2019 at 4:21 PM Nazarov Sergey wrote: > Hi, Paul! > What I need to do for this? If you haven't already done so, go read Documentation/process/submitting-patches.rst, that should guide you through the process. I would also suggest looking at both the git log and the mailing list archives to see what others have done in terms of commit descriptions, etc. After that, if you have any questions let me know and I can help you out. Thanks. > 11.02.2019, 23:37, "Paul Moore" : > > On Thu, Jan 31, 2019 at 8:20 AM Nazarov Sergey wrote: > >> 31.01.2019, 05:10, "Paul Moore" : > >> > This isn't how the rest of the stack works, look at > >> > ip_local_deliver_finish() for one example. Perhaps the behavior you > >> > are proposing is correct, but please show me where in the various RFC > >> > specs it is defined so that I can better understand why it should work > >> > this way. > >> > -- > >> > paul moore > >> > www.paul-moore.com > >> > >> Sorry, I was inattentive. ip_options_compile modifies srr option data, > >> only if > >> skb is NULL. My last message could be ignored. > > > > Hi Nazarov, > > > > Do you plan on submitting these patches as a proper patchset for > > review and merging? -- paul moore www.paul-moore.com
Re: Kernel memory corruption in CIPSO labeled TCP packets processing.
On Thu, Jan 31, 2019 at 8:20 AM Nazarov Sergey wrote: > 31.01.2019, 05:10, "Paul Moore" : > > This isn't how the rest of the stack works, look at > > ip_local_deliver_finish() for one example. Perhaps the behavior you > > are proposing is correct, but please show me where in the various RFC > > specs it is defined so that I can better understand why it should work > > this way. > > -- > > paul moore > > www.paul-moore.com > > Sorry, I was inattentive. ip_options_compile modifies srr option data, only if > skb is NULL. My last message could be ignored. Hi Nazarov, Do you plan on submitting these patches as a proper patchset for review and merging? -- paul moore www.paul-moore.com
Re: KASAN: use-after-free Read in selinux_netlbl_socket_setsockopt
On Fri, Feb 1, 2019 at 1:26 AM Dmitry Vyukov wrote: > On Wed, Jan 30, 2019 at 10:30 PM Paul Moore wrote: > > > > On Wed, Jan 30, 2019 at 4:01 PM syzbot > > wrote: > > > > > > Hello, > > > > > > syzbot found the following crash on: > > > > > > HEAD commit:62967898789d Merge > > > git://git.kernel.org/pub/scm/linux/kern.. > > > git tree: upstream > > > console output: https://syzkaller.appspot.com/x/log.txt?x=167fdef8c0 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=4fceea9e2d99ac20 > > > dashboard link: > > > https://syzkaller.appspot.com/bug?extid=1bfc00ca3aabe5bcd4cb > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > > > Unfortunately, I don't have any reproducer for this crash yet. > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > Reported-by: syzbot+1bfc00ca3aabe5bcd...@syzkaller.appspotmail.com > > > > > > 8021q: adding VLAN 0 to HW filter on device team0 > > > == > > > BUG: KASAN: use-after-free in selinux_netlbl_socket_setsockopt+0x49b/0x510 > > > security/selinux/netlabel.c:525 > > > Read of size 8 at addr 8880a63cf078 by task syz-executor3/18477 > > > > > > CPU: 1 PID: 18477 Comm: syz-executor3 Not tainted 5.0.0-rc4+ #51 > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > > Google 01/01/2011 > > > Call Trace: > > > __dump_stack lib/dump_stack.c:77 [inline] > > > dump_stack+0x1db/0x2d0 lib/dump_stack.c:113 > > > print_address_description.cold+0x7c/0x20d mm/kasan/report.c:187 > > > kasan_report.cold+0x1b/0x40 mm/kasan/report.c:317 > > > __asan_report_load8_noabort+0x14/0x20 mm/kasan/generic_report.c:135 > > > selinux_netlbl_socket_setsockopt+0x49b/0x510 > > > security/selinux/netlabel.c:525 > > > > At first glance this seems odd. The line above is simply > > dereferencing sock->sk_security (getting the "sksec"), which we also > > do higher up selinux_socket_setsockopt() via sock_has_perm(). Unless > > somehow the socket is being released/freed in the middle of a > > setsockopt() syscall this looks like maybe it's something else? > > Hi Paul, > > Searching for af_netrom across other syzbot bugs: > https://groups.google.com/forum/#!searchin/syzkaller-bugs/af_netrom%7Csort:date > > I see at least: > https://syzkaller.appspot.com/bug?extid=b0b1952f5864b4009b09 > https://syzkaller.appspot.com/bug?extid=febf3c50d4262e578b1c > https://syzkaller.appspot.com/bug?extid=defa700d16f1bd1b9a05 > > Which suggests there are some serious lifetime problems in netrom > sockets. That would probably explain this crash as well. That definitely looks plausible. While I'm not one to say it could *never* be the SELinux/NetLabel code, I can say that the SELinux code path in question hasn't changed in some time so I would be a little surprised if it was suddenly broken. > +netrom maintainers, does this explanation look plausible to you? > should we dup this crash onto one of the other netrom bugs? and > perhaps these netrom bugs across themselves too? -- paul moore www.paul-moore.com
Re: Kernel memory corruption in CIPSO labeled TCP packets processing.
On Wed, Jan 30, 2019 at 8:11 AM Nazarov Sergey wrote:
> 30.01.2019, 01:42, "Paul Moore" :
> > There are several cases where the stack ends up calling icmp_send()
> > after the skb has been through ip_options_compile(), that should be
> > okay.
> >
> > --
> > paul moore
> > www.paul-moore.com
>
> In those cases precompiled ip_options struct used, without the need to reuse
> ip_options_compile.
> I think, for error ICMP packet, we can discard all other options except
> CIPSO. It will be better, than
> send packet, contains wrong option's data. Modified patch 2:
> ---
> net/ipv4/cipso_ipv4.c | 24 ++--
> 1 files changed, 22 insertions(+), 2 deletions(-)
This isn't how the rest of the stack works, look at
ip_local_deliver_finish() for one example. Perhaps the behavior you
are proposing is correct, but please show me where in the various RFC
specs it is defined so that I can better understand why it should work
this way.
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 777fa3b..797826c 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1735,13 +1735,33 @@ int cipso_v4_validate(const struct sk_buff *skb,
> unsigned char **option)
> */
> void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
> {
> + struct ip_options opt;
> + unsigned char *optptr;
> +
> if (ip_hdr(skb)->protocol == IPPROTO_ICMP || error != -EACCES)
> return;
>
> + /*
> +* We might be called above the IP layer,
> +* so we can not use icmp_send and IPCB here.
> +*
> +* For the generated ICMP packet, we create a
> +* temporary ip _options structure, contains
> +* the CIPSO option only, since the other options data
> +* could be modified when the original packet receiving.
> +*/
> +
> + memset(&opt, 0, sizeof(struct ip_options));
> + optptr = cipso_v4_optptr(skb);
> + if (optptr) {
> + opt.optlen = optptr[1];
> + opt.cipso = optptr - skb_network_header(skb);
> + }
> +
> if (gateway)
> - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0);
> + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0, &opt);
> else
> - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0);
> + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0, &opt);
> }
>
> /**
>
--
paul moore
www.paul-moore.com
Re: Kernel memory corruption in CIPSO labeled TCP packets processing.
On Tue, Jan 29, 2019 at 2:23 AM Nazarov Sergey wrote: > 29.01.2019, 01:18, "Paul Moore" : > > If we don't pass a skb into ip_options_compile(), meaning both "skb" > > and "rt" will be NULL, then I don't believe the option data will > > change. Am I missing something? > > I mean, in cipso_v4_error we copy option data from skb before > ip_options_compile call: > + memcpy(opt->__data, (unsigned char *)&(ip_hdr(skb)[1]), opt->optlen); > But skb IP header data could be already changed by first call of > ip_options_compile > when packet received. There are several cases where the stack ends up calling icmp_send() after the skb has been through ip_options_compile(), that should be okay. -- paul moore www.paul-moore.com
Re: Kernel memory corruption in CIPSO labeled TCP packets processing.
On Mon, Jan 28, 2019 at 8:10 AM Nazarov Sergey wrote: > 25.01.2019, 19:46, "Paul Moore" : > > Hmm, I think the above calculation should take into account the actual > > length of the IP options, and not just the max size (calculate it > > based on iphdr->ihl). > > > > Beyond that fix, I think it's time to put together a proper patchset > > and post it to the lists for formal review/merging. > > > > Thanks for your work on this. > > > > -- > > paul moore > > www.paul-moore.com > > Where we can take actual IP options length? Sorry, I'm not so familiar with > linux network stack. I'm the one who needs to apologize; you're doing it correctly. Not sure what I was thinking there, sorry about that. > And also, ip_options_compile could change IP options data (SSRR, LSRR, RR, > TIMESTAMP options), > so, we can't use ip_options_compile again for these options. Am I right? If we don't pass a skb into ip_options_compile(), meaning both "skb" and "rt" will be NULL, then I don't believe the option data will change. Am I missing something? -- paul moore www.paul-moore.com
Re: Kernel memory corruption in CIPSO labeled TCP packets processing.
On Thu, Jan 24, 2019 at 9:46 AM Nazarov Sergey wrote:
> 22.01.2019, 20:48, "Paul Moore" :
> >
> > Yes, exactly. If you don't pass the skb it shouldn't attempt to call
> > icmp_send() in case of error.
> >
> > --
> > paul moore
> > www.paul-moore.com
>
> You are right, sorry. We can do that without ip_options_compile modification.
> Simplified patch 2:
> ---
> net/ipv4/cipso_ipv4.c | 18 --
> 1 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 777fa3b..a4ed0a4 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1735,13 +1735,27 @@ int cipso_v4_validate(const struct sk_buff *skb,
> unsigned char **option)
> */
> void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
> {
> + unsigned char optbuf[sizeof(struct ip_options) + 40];
> + struct ip_options *opt = (struct ip_options *)optbuf;
> +
> if (ip_hdr(skb)->protocol == IPPROTO_ICMP || error != -EACCES)
> return;
>
> + /*
> +* We might be called above the IP layer,
> +* so we can not use icmp_send and IPCB here.
> +*/
> +
> + memset(opt, 0, sizeof(struct ip_options));
> + opt->optlen = ip_hdr(skb)->ihl*4 - sizeof(struct iphdr);
Hmm, I think the above calculation should take into account the actual
length of the IP options, and not just the max size (calculate it
based on iphdr->ihl).
Beyond that fix, I think it's time to put together a proper patchset
and post it to the lists for formal review/merging.
Thanks for your work on this.
> + memcpy(opt->__data, (unsigned char *)&(ip_hdr(skb)[1]), opt->optlen);
> + if (ip_options_compile(dev_net(skb->dev), opt, NULL))
> + return;
> +
> if (gateway)
> - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0);
> + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0, opt);
> else
> - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0);
> + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0, opt);
> }
--
paul moore
www.paul-moore.com
Re: Kernel memory corruption in CIPSO labeled TCP packets processing.
On Tue, Jan 22, 2019 at 12:35 PM Nazarov Sergey wrote: > 22.01.2019, 19:49, "Paul Moore" : > > > > Granted I'm looking at this rather quickly, so I may be missing > > something, but why the changes to ip_options_compile()? Couldn't you > > simply set opt->data manually (set the ptr) in cipso_v4_error() before > > calling ip_options_compile() and arrive at the same result without > > having to modify ip_options_compile()? I suppose there is the rtable > > value to worry about, but ip_options_echo() should take care of that, > > yes? No? > > ip_options_compile calls icmp_send, if someting wrong. So, we'll go back > to trying to fix. ip_options_compile changes needed to avoid this. Yes, exactly. If you don't pass the skb it shouldn't attempt to call icmp_send() in case of error. -- paul moore www.paul-moore.com
Re: Kernel memory corruption in CIPSO labeled TCP packets processing.
On Mon, Jan 21, 2019 at 12:11 PM Nazarov Sergey wrote:
> 18.01.2019, 20:17, "Paul Moore" :
> > Yes, this is extra overhead, but it is in an error path so I'm not
> > overly concerned about the performance impact on the given connection.
> >
> > I will admit that it isn't an ideal solution from a LSM/NetLabel
> > perspective, but historically the netdev folks have not been overly
> > receptive to changes which negatively impact the core network stack
> > regardless of how important it may be for the LSMs. If we could
> > modify icmp_send() so that it works regardless of the caller's
> > location in the stack, that would be great, I'm just not sure it would
> > be deemed acceptable. I suggested the approach I did because I think
> > that has the best chance of getting merged.
> >
> > Regardless, I would encourage you to put together a patch and post it
> > for review, I think that's the best way to get a response. If you
> > aren't able, or aren't comfortable, submitting a patch please let me
> > know.
> > --
> > paul moore
> > www.paul-moore.com
>
> May be something like that?
>
> [PATCH 1/2] Add IP options parameter to icmp_send
> ---
> include/net/icmp.h|9 -
> net/ipv4/icmp.c |7 ---
> 2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/icmp.h b/include/net/icmp.h
> index 6ac3a5b..e0f709d 100644
> --- a/include/net/icmp.h
> +++ b/include/net/icmp.h
> @@ -22,6 +22,7 @@
>
> #include
> #include
> +#include
>
> struct icmp_err {
>int errno;
> @@ -39,7 +40,13 @@ struct icmp_err {
> struct sk_buff;
> struct net;
>
> -void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info);
> +void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
> +const struct ip_options *opt);
> +static inline void icmp_send(struct sk_buff *skb_in, int type, int code,
> __be32 info)
> +{
> + __icmp_send(skb_in, type, code, info, &IPCB(skb_in)->opt);
> +}
> +
> int icmp_rcv(struct sk_buff *skb);
> int icmp_err(struct sk_buff *skb, u32 info);
> int icmp_init(void);
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 065997f..3f24414 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -570,7 +570,8 @@ static void icmp_reply(struct icmp_bxm *icmp_param,
> struct sk_buff *skb)
> * MUST reply to only the first fragment.
> */
>
> -void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
> +void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
> +const struct ip_options *opt)
> {
> struct iphdr *iph;
> int room;
> @@ -691,7 +692,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int
> code, __be32 info)
> iph->tos;
> mark = IP4_REPLY_MARK(net, skb_in->mark);
>
> - if (ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in))
> + if (__ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in,
> opt))
> goto out_unlock;
>
>
> @@ -742,7 +743,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int
> code, __be32 info)
> local_bh_enable();
> out:;
> }
> -EXPORT_SYMBOL(icmp_send);
> +EXPORT_SYMBOL(__icmp_send);
This looks reasonable, and I think this should be acceptable to the
netdev folks. Although a minor heads-up that somebody might object to
icmp_send() no longer being exported.
More below...
> ---
> [PATCH 2/2] Extract IP options in cipso_v4_error
> ---
> include/net/ip.h |2 ++
> net/ipv4/cipso_ipv4.c | 11 +--
> net/ipv4/ip_options.c | 22 +-
> 3 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/ip.h b/include/net/ip.h
> index 8866bfc..f0e8d06 100644
> --- a/include/net/ip.h
> +++ b/include/net/ip.h
> @@ -667,6 +667,8 @@ static inline int ip_options_echo(struct net *net, struct
> ip_options *dopt,
> }
>
> void ip_options_fragment(struct sk_buff *skb);
> +int __ip_options_compile(struct net *net, struct ip_options *opt,
> +struct sk_buff *skb, __be32 *info);
> int ip_options_compile(struct net *net, struct ip_options *opt,
>struct sk_buff *skb);
> int ip_options_get(struct net *net, struct ip_options_rcu **optp,
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 777fa3b..86599e9 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1735,13 +1735,20 @@ int cipso_v4_validate(const
Re: Kernel memory corruption in CIPSO labeled TCP packets processing.
On Fri, Jan 18, 2019 at 11:34 AM Nazarov Sergey wrote: > Hi, Paul! > I don't like this. As you said, fix any calls to icmp_send is a trivial. > But in cipso_v4_optptr, we repeating the work already done, and in > cipso_v4_error > we will need to do even more again. Yes, this is extra overhead, but it is in an error path so I'm not overly concerned about the performance impact on the given connection. I will admit that it isn't an ideal solution from a LSM/NetLabel perspective, but historically the netdev folks have not been overly receptive to changes which negatively impact the core network stack regardless of how important it may be for the LSMs. If we could modify icmp_send() so that it works regardless of the caller's location in the stack, that would be great, I'm just not sure it would be deemed acceptable. I suggested the approach I did because I think that has the best chance of getting merged. Regardless, I would encourage you to put together a patch and post it for review, I think that's the best way to get a response. If you aren't able, or aren't comfortable, submitting a patch please let me know. > Any code, working with IP header data on several > levels of TCP/IP stack need to do this again and again. Is looks too > overloaded! > In my opinion, this is a problem of TCP/IP stack design, comes from standing > compiled IP header data in different places at different stack layers. > May be better to add some flag or pointer to struct sk_buff? > But, I think, we need netdev developers advice for this. > Regards, Sergey. > > 18.01.2019, 17:53, "Paul Moore" : > > On Tue, Jan 15, 2019 at 2:52 PM Paul Moore wrote: > > > > It's been a few days now with no comments from the netdev folks, so I > > think it's probably best to start putting together a RFC patch for > > review/comment. Nazarov, would you like to do that? If not, that's > > okay, just let me know. > > > > I'm still concerned about calling ip_options_compile() in icmp_send() > > and I'm thinking we might be better off to add a new ip_options > > parameter to icmp_send(); if the parameter is NULL we behave as we do > > today, but if it is non-NULL we use the given ip_options parameter in > > place of calling ip_options_echo(). With that change in place, we > > would need to update cipso_v4_error() to do the extra work of calling > > ip_options_compile() and __ip_options_echo(). There looks to be maybe > > a dozen (or two?) existing icmp_send() callers, but it should be > > pretty trivial to update them to pass NULL for the new parameter. > > > > What do you think? -- paul moore www.paul-moore.com
Re: Kernel memory corruption in CIPSO labeled TCP packets processing.
On Tue, Jan 15, 2019 at 2:52 PM Paul Moore wrote:
> On Tue, Jan 15, 2019 at 12:55 PM Casey Schaufler
> wrote:
> > On 1/15/2019 9:06 AM, Nazarov Sergey wrote:
> > > Hello!
> > > Security modules (selinux, smack) use icmp_send for discarded incorrectly
> > > labeled network packets.
> > > This could be on TCP level too (security_sock_rcv_skb -> cipso_v4_error
> > > for INET stream connection, for example).
> > > icmp_send calls ip_option_echo, which uses IPCB to take compiled IP
> > > options.
> > > After moving IP header data to the end of the struct tcp_skb_cb (since
> > > 3.18 kernel), this could lead to
> > > kernel memory corruption when IP options copying.
> >
> > Can you explain how that corruption might occur?
> > Do you have a test case?
>
> Thanks for pointing this out Nazarov.
>
> Presumably we are going to hit a problem whenever icmp_send is called
> from outside the IP layer in the stack. We fixed a similar problem a
> few years back with 04f81f0154e4 ("cipso: don't use IPCB() to locate
> the CIPSO IP option").
>
> I've CC'd netdev, as I'm guessing they will have some thoughts on
> this, but my initial reaction is that your proposed patch isn't as
> generic as it should be for code that lives in icmp_send(). I suspect
> the safe thing to do would be to call ip_options_compile() again on
> skb_in and build a local copy of the ip_options struct that could then
> be used in the call to __ip_options_echo(); the code could either live
> in icmp_send() or some new ip_options_echo() variant
> (ip_options_echo_safe()? I dunno). Unfortunately, calling
> ip_options_compile() is going to add some overhead, and may be a
> non-starter for the netdev folks, but this is error path code, so it
> might be acceptable. Hopefully the netdev folks will have some
> better, more clever suggestions.
It's been a few days now with no comments from the netdev folks, so I
think it's probably best to start putting together a RFC patch for
review/comment. Nazarov, would you like to do that? If not, that's
okay, just let me know.
I'm still concerned about calling ip_options_compile() in icmp_send()
and I'm thinking we might be better off to add a new ip_options
parameter to icmp_send(); if the parameter is NULL we behave as we do
today, but if it is non-NULL we use the given ip_options parameter in
place of calling ip_options_echo(). With that change in place, we
would need to update cipso_v4_error() to do the extra work of calling
ip_options_compile() and __ip_options_echo(). There looks to be maybe
a dozen (or two?) existing icmp_send() callers, but it should be
pretty trivial to update them to pass NULL for the new parameter.
What do you think?
> > > This patch fix a bug, but I'm not sure, that this is a best solution.
> > > Perhaps someone more familiar with the
> > > linux TCP/IP stack will offer a better one.
> > > Thanks.
> > >
> > > --- a/net/ipv4/icmp.c
> > > +++ b/net/ipv4/icmp.c
> > > @@ -679,7 +679,8 @@ void icmp_send(struct sk_buff *skb_in, i
> > > iph->tos;
> > > mark = IP4_REPLY_MARK(net, skb_in->mark);
> > >
> > > - if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in))
> > > + if (__ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in,
> > > + ip_hdr(skb_in)->protocol == IPPROTO_TCP ?
> > > &TCP_SKB_CB(skb_in)->header.h4.opt : &IPCB(skb_in)->opt))
> > > goto out_unlock;
> > >
> > >
--
paul moore
www.paul-moore.com
Re: Kernel memory corruption in CIPSO labeled TCP packets processing.
On Tue, Jan 15, 2019 at 12:55 PM Casey Schaufler wrote:
> On 1/15/2019 9:06 AM, Nazarov Sergey wrote:
> > Hello!
> > Security modules (selinux, smack) use icmp_send for discarded incorrectly
> > labeled network packets.
> > This could be on TCP level too (security_sock_rcv_skb -> cipso_v4_error for
> > INET stream connection, for example).
> > icmp_send calls ip_option_echo, which uses IPCB to take compiled IP options.
> > After moving IP header data to the end of the struct tcp_skb_cb (since 3.18
> > kernel), this could lead to
> > kernel memory corruption when IP options copying.
>
> Can you explain how that corruption might occur?
> Do you have a test case?
Thanks for pointing this out Nazarov.
Presumably we are going to hit a problem whenever icmp_send is called
from outside the IP layer in the stack. We fixed a similar problem a
few years back with 04f81f0154e4 ("cipso: don't use IPCB() to locate
the CIPSO IP option").
I've CC'd netdev, as I'm guessing they will have some thoughts on
this, but my initial reaction is that your proposed patch isn't as
generic as it should be for code that lives in icmp_send(). I suspect
the safe thing to do would be to call ip_options_compile() again on
skb_in and build a local copy of the ip_options struct that could then
be used in the call to __ip_options_echo(); the code could either live
in icmp_send() or some new ip_options_echo() variant
(ip_options_echo_safe()? I dunno). Unfortunately, calling
ip_options_compile() is going to add some overhead, and may be a
non-starter for the netdev folks, but this is error path code, so it
might be acceptable. Hopefully the netdev folks will have some
better, more clever suggestions.
> > This patch fix a bug, but I'm not sure, that this is a best solution.
> > Perhaps someone more familiar with the
> > linux TCP/IP stack will offer a better one.
> > Thanks.
> >
> > --- a/net/ipv4/icmp.c
> > +++ b/net/ipv4/icmp.c
> > @@ -679,7 +679,8 @@ void icmp_send(struct sk_buff *skb_in, i
> > iph->tos;
> > mark = IP4_REPLY_MARK(net, skb_in->mark);
> >
> > - if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in))
> > + if (__ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in,
> > + ip_hdr(skb_in)->protocol == IPPROTO_TCP ?
> > &TCP_SKB_CB(skb_in)->header.h4.opt : &IPCB(skb_in)->opt))
> > goto out_unlock;
> >
> >
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 (was ghak32) V4 00/10] audit: implement container identifier
On Thu, Jan 3, 2019 at 12:36 PM Richard Guy Briggs wrote: > On 2019-01-03 08:15, Guenter Roeck wrote: > > Hi, > > > > On Tue, Jul 31, 2018 at 04:07:35PM -0400, Richard Guy Briggs wrote: > > > Implement kernel audit container identifier. > > > > I don't see a follow-up submission of this patch series. Has it been > > abandoned, > > or do I use the wrong search terms ? > > Guenter, thanks for your interest in this patchset. I haven't > abandoned it. I've pushed some updates to my own (ill-publicized) > public git repo. This effort has been going on more than 5 years with 8 > previous revisions trying to document task namespaces and deciding that > was insufficient. > > For this patchset I waited 11.5 weeks (80 days, Jules Verne anyone?) > before the primary intended maintainer did the first review, then I > responded within 2 weeks with further questions and a followup patch > proposal and then waited another 8 weeks for any response before adding > another query for that followup patch proposal review at which point I > got a rude answer saying I had disappointed and exhausted the > maintainer's goodwill with some hints at how to proceed just before new > year's. For what it is worth, I've found your emails to me to be rather "rude" as well (to borrow the term), and I responded with what I felt was appropriate. Perhaps our interactions may have been seen as overly, or quickly, harsh but I would remind those that we have several years of history that extends far beyond the lists which obviously affects how we interact. Our expectations for each other are clearly higher than either of us are delivering, so I'm going to suggest what I've suggested before, albeit privately: let's stick to the code, that's where we can find common ground. There were only a few outstanding threads/questions from your last posting, you should have responses to those sitting in your inbox now. > I'd be delighted with other upstream review to get other angles and to > take some of the load and responsibility off the primary maintainer. > > I expect to submit a v5 within a week without having had those questions > directly answered, but with some ideas of what to check and verify > before I resubmit. Most of the changes have been sitting in that branch > for two months, already rebased one kernel version and will need > updating again. -- paul moore www.paul-moore.com
Re: [PATCH ghak90 (was ghak32) V4 09/10] audit: NETFILTER_PKT: record each container ID associated with a netNS
On Thu, Dec 27, 2018 at 10:34 AM Richard Guy Briggs wrote:
> On 2018-10-31 15:30, Richard Guy Briggs wrote:
> > On 2018-10-19 19:18, Paul Moore wrote:
> > > On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs wrote:
> > > > Add audit container identifier auxiliary record(s) to NETFILTER_PKT
> > > > event standalone records. Iterate through all potential audit container
> > > > identifiers associated with a network namespace.
> > > >
> > > > Signed-off-by: Richard Guy Briggs
> > > > ---
> > > > include/linux/audit.h| 5 +
> > > > kernel/audit.c | 26 ++
> > > > net/netfilter/xt_AUDIT.c | 12 ++--
> > > > 3 files changed, 41 insertions(+), 2 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > index 9a02095..8755f4d 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -169,6 +169,8 @@ extern int audit_log_contid(struct audit_context
> > > > *context,
> > > > extern void audit_netns_contid_add(struct net *net, u64 contid);
> > > > extern void audit_netns_contid_del(struct net *net, u64 contid);
> > > > extern void audit_switch_task_namespaces(struct nsproxy *ns, struct
> > > > task_struct *p);
> > > > +extern void audit_log_netns_contid_list(struct net *net,
> > > > +struct audit_context *context);
> > > >
> > > > extern int audit_update_lsm_rules(void);
> > > >
> > > > @@ -228,6 +230,9 @@ static inline void audit_netns_contid_del(struct
> > > > net *net, u64 contid)
> > > > { }
> > > > static inline void audit_switch_task_namespaces(struct nsproxy *ns,
> > > > struct task_struct *p)
> > > > { }
> > > > +static inline void audit_log_netns_contid_list(struct net *net,
> > > > + struct audit_context *context)
> > > > +{ }
> > > >
> > > > #define audit_enabled AUDIT_OFF
> > > > #endif /* CONFIG_AUDIT */
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index c5fed3b..b23711c 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -392,6 +392,32 @@ void audit_switch_task_namespaces(struct nsproxy
> > > > *ns, struct task_struct *p)
> > > > audit_netns_contid_add(new->net_ns, contid);
> > > > }
> > > >
> > > > +void audit_log_netns_contid_list(struct net *net, struct audit_context
> > > > *context)
> > > > +{
> > > > + spinlock_t *lock = audit_get_netns_contid_list_lock(net);
> > > > + struct audit_buffer *ab;
> > > > + struct audit_contid *cont;
> > > > + bool first = true;
> > > > +
> > > > + /* Generate AUDIT_CONTAINER record with container ID CSV list */
> > > > + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_CONTAINER);
> > > > + if (!ab) {
> > > > + audit_log_lost("out of memory in
> > > > audit_log_netns_contid_list");
> > > > + return;
> > > > + }
> > > > + audit_log_format(ab, "contid=");
> > > > + spin_lock(lock);
> > > > + list_for_each_entry(cont, audit_get_netns_contid_list(net),
> > > > list) {
> > > > + if (!first)
> > > > + audit_log_format(ab, ",");
> > > > + audit_log_format(ab, "%llu", cont->id);
> > > > + first = false;
> > > > + }
> > > > + spin_unlock(lock);
> > >
> > > This is looking like potentially a lot of work to be doing under a
> > > spinlock, not to mention a single spinlock that is shared across CPUs.
> > > Considering that I expect changes to the list to be somewhat
> > > infrequent, this might be a good candidate for a RCU based locking
> > > scheme.
> >
> > Would something like this look reasonable?
> > (This is on top of a patch to make contid list lock and unlock
> > functions.)
>
> Paul, could I please get your review on this locking approach I proposed
> almost two months ago so I can be more
Re: [PATCH] selinux: add support for RTM_NEWCHAIN, RTM_DELCHAIN, and RTM_GETCHAIN
On Wed, Nov 28, 2018 at 1:44 PM Paul Moore wrote:
> Commit 32a4f5ecd738 ("net: sched: introduce chain object to uapi")
> added new RTM_* definitions without properly updating SELinux, this
> patch adds the necessary SELinux support.
>
> While there was a BUILD_BUG_ON() in the SELinux code to protect from
> exactly this case, it was bypassed in the broken commit. In order to
> hopefully prevent this from happening in the future, add additional
> comments which provide some instructions on how to resolve the
> BUILD_BUG_ON() failures.
>
> Fixes: 32a4f5ecd738 ("net: sched: introduce chain object to uapi")
> Cc: # 4.19
> Signed-off-by: Paul Moore
> ---
> security/selinux/nlmsgtab.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
I'm building a test kernel right now, assuming all goes well I'm going
to send this up to Linus for v4.20.
> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> index 74b951f55608..9cec81209617 100644
> --- a/security/selinux/nlmsgtab.c
> +++ b/security/selinux/nlmsgtab.c
> @@ -80,6 +80,9 @@ static const struct nlmsg_perm nlmsg_route_perms[] =
> { RTM_NEWSTATS, NETLINK_ROUTE_SOCKET__NLMSG_READ },
> { RTM_GETSTATS, NETLINK_ROUTE_SOCKET__NLMSG_READ },
> { RTM_NEWCACHEREPORT, NETLINK_ROUTE_SOCKET__NLMSG_READ },
> + { RTM_NEWCHAIN, NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
> + { RTM_DELCHAIN, NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
> + { RTM_GETCHAIN, NETLINK_ROUTE_SOCKET__NLMSG_READ },
> };
>
> static const struct nlmsg_perm nlmsg_tcpdiag_perms[] =
> @@ -158,7 +161,11 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32
> *perm)
>
> switch (sclass) {
> case SECCLASS_NETLINK_ROUTE_SOCKET:
> - /* RTM_MAX always point to RTM_SET, ie RTM_NEWxxx + 3 */
> + /* RTM_MAX always points to RTM_SET, ie RTM_NEWxxx + 3.
> +* If the BUILD_BUG_ON() below fails you must update the
> +* structures at the top of this file with the new mappings
> +* before updating the BUILD_BUG_ON() macro!
> +*/
> BUILD_BUG_ON(RTM_MAX != (RTM_NEWCHAIN + 3));
> err = nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms,
> sizeof(nlmsg_route_perms));
> @@ -170,6 +177,10 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32
> *perm)
> break;
>
> case SECCLASS_NETLINK_XFRM_SOCKET:
> + /* If the BUILD_BUG_ON() below fails you must update the
> +* structures at the top of this file with the new mappings
> +* before updating the BUILD_BUG_ON() macro!
> +*/
> BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_MAPPING);
> err = nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms,
> sizeof(nlmsg_xfrm_perms));
>
--
paul moore
www.paul-moore.com
[PATCH] selinux: add support for RTM_NEWCHAIN, RTM_DELCHAIN, and RTM_GETCHAIN
Commit 32a4f5ecd738 ("net: sched: introduce chain object to uapi")
added new RTM_* definitions without properly updating SELinux, this
patch adds the necessary SELinux support.
While there was a BUILD_BUG_ON() in the SELinux code to protect from
exactly this case, it was bypassed in the broken commit. In order to
hopefully prevent this from happening in the future, add additional
comments which provide some instructions on how to resolve the
BUILD_BUG_ON() failures.
Fixes: 32a4f5ecd738 ("net: sched: introduce chain object to uapi")
Cc: # 4.19
Signed-off-by: Paul Moore
---
security/selinux/nlmsgtab.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
index 74b951f55608..9cec81209617 100644
--- a/security/selinux/nlmsgtab.c
+++ b/security/selinux/nlmsgtab.c
@@ -80,6 +80,9 @@ static const struct nlmsg_perm nlmsg_route_perms[] =
{ RTM_NEWSTATS, NETLINK_ROUTE_SOCKET__NLMSG_READ },
{ RTM_GETSTATS, NETLINK_ROUTE_SOCKET__NLMSG_READ },
{ RTM_NEWCACHEREPORT, NETLINK_ROUTE_SOCKET__NLMSG_READ },
+ { RTM_NEWCHAIN, NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
+ { RTM_DELCHAIN, NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
+ { RTM_GETCHAIN, NETLINK_ROUTE_SOCKET__NLMSG_READ },
};
static const struct nlmsg_perm nlmsg_tcpdiag_perms[] =
@@ -158,7 +161,11 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32
*perm)
switch (sclass) {
case SECCLASS_NETLINK_ROUTE_SOCKET:
- /* RTM_MAX always point to RTM_SET, ie RTM_NEWxxx + 3 */
+ /* RTM_MAX always points to RTM_SET, ie RTM_NEWxxx + 3.
+* If the BUILD_BUG_ON() below fails you must update the
+* structures at the top of this file with the new mappings
+* before updating the BUILD_BUG_ON() macro!
+*/
BUILD_BUG_ON(RTM_MAX != (RTM_NEWCHAIN + 3));
err = nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms,
sizeof(nlmsg_route_perms));
@@ -170,6 +177,10 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32
*perm)
break;
case SECCLASS_NETLINK_XFRM_SOCKET:
+ /* If the BUILD_BUG_ON() below fails you must update the
+* structures at the top of this file with the new mappings
+* before updating the BUILD_BUG_ON() macro!
+*/
BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_MAPPING);
err = nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms,
sizeof(nlmsg_xfrm_perms));
Re: [PATCH v2 net] net/ipv4: defensive cipso option parsing
On Mon, Sep 17, 2018 at 1:49 PM Stefan Nuernberger wrote:
> commit 40413955ee26 ("Cipso: cipso_v4_optptr enter infinite loop") fixed
> a possible infinite loop in the IP option parsing of CIPSO. The fix
> assumes that ip_options_compile filtered out all zero length options and
> that no other one-byte options beside IPOPT_END and IPOPT_NOOP exist.
> While this assumption currently holds true, add explicit checks for zero
> length and invalid length options to be safe for the future. Even though
> ip_options_compile should have validated the options, the introduction of
> new one-byte options can still confuse this code without the additional
> checks.
>
> Signed-off-by: Stefan Nuernberger
> Cc: David Woodhouse
> Cc: Simon Veith
> Cc: sta...@vger.kernel.org
> ---
> net/ipv4/cipso_ipv4.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
See my previous comments about the necessity of this patch, but beyond
that it looks fine to me.
Acked-by: Paul Moore
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 82178cc69c96..777fa3b7fb13 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1512,7 +1512,7 @@ static int cipso_v4_parsetag_loc(const struct
> cipso_v4_doi *doi_def,
> *
> * Description:
> * Parse the packet's IP header looking for a CIPSO option. Returns a
> pointer
> - * to the start of the CIPSO option on success, NULL if one if not found.
> + * to the start of the CIPSO option on success, NULL if one is not found.
> *
> */
> unsigned char *cipso_v4_optptr(const struct sk_buff *skb)
> @@ -1522,10 +1522,8 @@ unsigned char *cipso_v4_optptr(const struct sk_buff
> *skb)
> int optlen;
> int taglen;
>
> - for (optlen = iph->ihl*4 - sizeof(struct iphdr); optlen > 0; ) {
> + for (optlen = iph->ihl*4 - sizeof(struct iphdr); optlen > 1; ) {
Not worth re-spinning this patch, but looking at this a bit closer, we
could probably optimize the "optlen > 1" tweak a bit further by using
CIPSO_V4_HDR_LEN instead of "1" since we only care about CIPSO headers
here.
Although given the nature of IPv4 options, I'm not sure this would
ever really have an impact, let alone a noticeable impact.
> switch (optptr[0]) {
> - case IPOPT_CIPSO:
> - return optptr;
> case IPOPT_END:
> return NULL;
> case IPOPT_NOOP:
> @@ -1534,6 +1532,11 @@ unsigned char *cipso_v4_optptr(const struct sk_buff
> *skb)
> default:
> taglen = optptr[1];
> }
> + if (!taglen || taglen > optlen)
> + return NULL;
> + if (optptr[0] == IPOPT_CIPSO)
> + return optptr;
> +
> optlen -= taglen;
> optptr += taglen;
> }
> --
> 2.19.0
--
paul moore
www.paul-moore.com
