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
Re: [PATCH net] net/ipv4: defensive cipso option parsing
On Mon, Sep 17, 2018 at 11:12 AM 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
> Reviewed-by: David Woodhouse
> Reviewed-by: Simon Veith
> Cc: sta...@vger.kernel.org
> ---
> net/ipv4/cipso_ipv4.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 82178cc69c96..f291b57b8474 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,9 +1522,11 @@ 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; ) {
> switch (optptr[0]) {
> case IPOPT_CIPSO:
> + if (!optptr[1] || optptr[1] > optlen)
> + return NULL;
> return optptr;
> case IPOPT_END:
> return NULL;
> @@ -1534,6 +1536,10 @@ unsigned char *cipso_v4_optptr(const struct sk_buff
> *skb)
> default:
> taglen = optptr[1];
> }
> +
> + if (!taglen || taglen > optlen)
> + break;
I tend to think that you reach a point where you simply need to trust
that the stack is doing the right thing and that by the time you hit a
certain point you can safely assume that the packet is well formed,
but I'm not going to fight about that here.
Regardless of the above, I don't like how you're doing the option
length check twice in this code, that looks ugly to me, I think we can
do better. How about something like this:
for (...) {
switch(optptr[0]) {
case IPOPT_END:
return NULL;
case IPOPT_NOOP:
taglen = 1;
default:
taglen = optptr[1];
}
if (taglen == 0 || taglen > optlen)
return NULL;
if (optptr[0] == IPOPT_CIPSO)
return optptr;
}
> optlen -= taglen;
> optptr += taglen;
> }
--
paul moore
www.paul-moore.com
[RFC PATCH] ipv6: make ipv6_renew_options() interrupt/kernel safe
From: Paul Moore
At present the ipv6_renew_options_kern() function ends up calling into
access_ok() which is problematic if done from inside an interrupt as
access_ok() calls WARN_ON_IN_IRQ() on some (all?) architectures
(x86-64 is affected). Example warning/backtrace is shown below:
WARNING: CPU: 1 PID: 3144 at lib/usercopy.c:11 _copy_from_user+0x85/0x90
...
Call Trace:
ipv6_renew_option+0xb2/0xf0
ipv6_renew_options+0x26a/0x340
ipv6_renew_options_kern+0x2c/0x40
calipso_req_setattr+0x72/0xe0
netlbl_req_setattr+0x126/0x1b0
selinux_netlbl_inet_conn_request+0x80/0x100
selinux_inet_conn_request+0x6d/0xb0
security_inet_conn_request+0x32/0x50
tcp_conn_request+0x35f/0xe00
? __lock_acquire+0x250/0x16c0
? selinux_socket_sock_rcv_skb+0x1ae/0x210
? tcp_rcv_state_process+0x289/0x106b
tcp_rcv_state_process+0x289/0x106b
? tcp_v6_do_rcv+0x1a7/0x3c0
tcp_v6_do_rcv+0x1a7/0x3c0
tcp_v6_rcv+0xc82/0xcf0
ip6_input_finish+0x10d/0x690
ip6_input+0x45/0x1e0
? ip6_rcv_finish+0x1d0/0x1d0
ipv6_rcv+0x32b/0x880
? ip6_make_skb+0x1e0/0x1e0
__netif_receive_skb_core+0x6f2/0xdf0
? process_backlog+0x85/0x250
? process_backlog+0x85/0x250
? process_backlog+0xec/0x250
process_backlog+0xec/0x250
net_rx_action+0x153/0x480
__do_softirq+0xd9/0x4f7
do_softirq_own_stack+0x2a/0x40
...
While not present in the backtrace, ipv6_renew_option() ends up calling
access_ok() via the following chain:
access_ok()
_copy_from_user()
copy_from_user()
ipv6_renew_option()
The fix presented in this patch is to perform the userspace copy
earlier in the call chain such that it is only called when the option
data is actually coming from userspace; that place is
do_ipv6_setsockopt(). Not only does this solve the problem seen in
the backtrace above, it also allows us to simplify the code quite a
bit by removing ipv6_renew_options_kern() completely. We also take
this opportunity to cleanup ipv6_renew_options()/ipv6_renew_option()
a small amount as well.
This patch is heavily based on a rough patch by Al Viro. I've taken
his original patch, converted a kmemdup() call in do_ipv6_setsockopt()
to a memdup_user() call, made better use of the e_inval jump target in
the same function, and cleaned up the use ipv6_renew_option() by
ipv6_renew_options().
CC: Al Viro
Signed-off-by: Paul Moore
---
include/net/ipv6.h |9
net/ipv6/calipso.c |9 +---
net/ipv6/exthdrs.c | 108 --
net/ipv6/ipv6_sockglue.c | 27
4 files changed, 50 insertions(+), 103 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 16475c269749..d02881e4ad1f 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -355,14 +355,7 @@ struct ipv6_txoptions *ipv6_dup_options(struct sock *sk,
struct ipv6_txoptions *ipv6_renew_options(struct sock *sk,
struct ipv6_txoptions *opt,
int newtype,
- struct ipv6_opt_hdr __user *newopt,
- int newoptlen);
-struct ipv6_txoptions *
-ipv6_renew_options_kern(struct sock *sk,
- struct ipv6_txoptions *opt,
- int newtype,
- struct ipv6_opt_hdr *newopt,
- int newoptlen);
+ struct ipv6_opt_hdr *newopt);
struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,
struct ipv6_txoptions *opt);
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index 1323b9679cf7..1c0bb9fb76e6 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -799,8 +799,7 @@ static int calipso_opt_update(struct sock *sk, struct
ipv6_opt_hdr *hop)
{
struct ipv6_txoptions *old = txopt_get(inet6_sk(sk)), *txopts;
- txopts = ipv6_renew_options_kern(sk, old, IPV6_HOPOPTS,
-hop, hop ? ipv6_optlen(hop) : 0);
+ txopts = ipv6_renew_options(sk, old, IPV6_HOPOPTS, hop);
txopt_put(old);
if (IS_ERR(txopts))
return PTR_ERR(txopts);
@@ -1222,8 +1221,7 @@ static int calipso_req_setattr(struct request_sock *req,
if (IS_ERR(new))
return PTR_ERR(new);
- txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-new, new ? ipv6_optlen(new) : 0);
+ txopts = ipv6_renew_options(sk, req_inet->ipv6_opt, IPV6_HOPOPTS, new);
kfree(new);
@@ -1260,8 +1258,7 @@ static void calipso_req_delattr(struct request_sock *req)
if (calipso_opt_del(req_inet->ipv6_opt->hopopt, ))
return; /* Nothing to do */
- txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-new, new ? ipv6_optlen(new) : 0);
Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()
On Sun, Jun 24, 2018 at 3:48 AM David Miller wrote:
>
> From: Al Viro
> Date: Sat, 23 Jun 2018 23:21:07 +0100
>
> > BTW, I wonder if the life would be simpler with do_ipv6_setsockopt() doing
> > the copy-in and verifying ipv6_optlen(*hdr) <= newoptlen; that would've
> > simplified ipv6_renew_option{,s}() quite a bit and completely eliminated
> > ipv6_renew_options_kern()...
>
> I agree that this makes things a lot simpler.
I had looked at moving the userspace copy up, but feared it was a bit
too invasive. It sounds like you are open to the idea so I'll code
something up.
> One thing that drives me crazy though is this inherit stuff:
>
> > + ipv6_renew_option(newtype == IPV6_HOPOPTS ? newopt :
> > + opt ? opt->hopopt : NULL,
>
> Why don't we pass the type into ipv6_renew_option() and have it
> do this pointer dance instead?
>
> That's going to definitely be easier to read.
I agree, that struck me as a little odd. I'll rework that too. I'll
send you guys something this week to take a look at.
Thanks.
> I don't know enough about this code to give feedback about the
> option length handling wrt. copies, sorry.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 13/13] debug audit: read container ID of a process
On Tue, May 22, 2018 at 1:35 PM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2018-05-21 16:06, Paul Moore wrote: >> On Mon, May 21, 2018 at 3:19 PM, Eric W. Biederman <ebied...@xmission.com> >> wrote: >> > Steve Grubb <sgr...@redhat.com> writes: >> >> On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote: >> >>> Add support for reading the container ID from the proc filesystem. >> >> >> >> I think this could be useful in general. Please consider this to be part >> >> of >> >> the full patch set and not something merely used to debug the patches. >> > >> > Only with an audit specific name. >> > >> > As it is: >> > >> > Nacked-by: "Eric W. Biederman" <ebied...@xmission.com> >> > >> > The truth is the containerid name really stinks and is quite confusing >> > and does not imply that the label applies only to audit. And little >> > things like this make me extremely uncofortable with it. >> >> It also makes the audit container ID (notice how I *always* call it >> the *audit* container ID? that is not an accident) available for >> userspace applications to abuse. Perhaps in the future we can look at >> ways to make this more available to applications, but this patch is >> not the answer. > > Do you have a productive suggestion? I haven't given it much thought beyond our discussions and until we get the basic audit container ID support in place (all the other parts of this patchset) I doubt I'll be giving it much thought. -- paul moore www.paul-moore.com
Re: [RFC PATCH ghak32 V2 13/13] debug audit: read container ID of a process
On Mon, May 21, 2018 at 3:19 PM, Eric W. Biederman <ebied...@xmission.com> wrote: > Steve Grubb <sgr...@redhat.com> writes: > >> On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote: >>> Add support for reading the container ID from the proc filesystem. >> >> I think this could be useful in general. Please consider this to be part of >> the full patch set and not something merely used to debug the patches. > > Only with an audit specific name. > > As it is: > > Nacked-by: "Eric W. Biederman" <ebied...@xmission.com> > > The truth is the containerid name really stinks and is quite confusing > and does not imply that the label applies only to audit. And little > things like this make me extremely uncofortable with it. It also makes the audit container ID (notice how I *always* call it the *audit* container ID? that is not an accident) available for userspace applications to abuse. Perhaps in the future we can look at ways to make this more available to applications, but this patch is not the answer. -- paul moore www.paul-moore.com
Re: [PATCH ghak81 V3 3/3] audit: collect audit task parameters
- if (!context) > - return; > - > /* Check for system calls that do not go through the exit > * function (e.g., exit_group), then free context block. > * We use GFP_ATOMIC here because we might be doing this > * in the context of the idle thread */ > /* that can happen only if we are called from do_exit() */ > - if (context->in_syscall && context->current_state == > AUDIT_RECORD_CONTEXT) > + if (context && context->in_syscall && > + context->current_state == AUDIT_RECORD_CONTEXT) > audit_log_exit(context, 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); > + if (!context) > + return; > if (!list_empty(>killed_trees)) > audit_kill_trees(>killed_trees); > > @@ -2071,8 +2104,8 @@ int audit_set_loginuid(kuid_t loginuid) > sessionid = (unsigned > int)atomic_inc_return(_id); > } > > - task->sessionid = sessionid; > - task->loginuid = loginuid; > + task->audit->sessionid = sessionid; > + task->audit->loginuid = loginuid; > out: > audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, > sessionid, rc); > return rc; > diff --git a/kernel/fork.c b/kernel/fork.c > index cd18448..92ab849 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1713,7 +1713,7 @@ static __latent_entropy struct task_struct > *copy_process( > p->start_time = ktime_get_ns(); > p->real_start_time = ktime_get_boot_ns(); > p->io_context = NULL; > - audit_set_context(p, NULL); > + p->audit = NULL; > cgroup_fork(p); > #ifdef CONFIG_NUMA > p->mempolicy = mpol_dup(p->mempolicy); > -- > 1.8.3.1 -- paul moore www.paul-moore.com
Re: [PATCH ghak81 V3 1/3] audit: use new audit_context access funciton for seccomp_actions_logged
On Wed, May 16, 2018 at 7:55 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> On the rebase of the following commit on the new seccomp actions_logged
> function, one audit_context access was missed.
>
> commit cdfb6b341f0f2409aba24b84f3b4b2bba50be5c5
> ("audit: use inline function to get audit context")
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/auditsc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Merged into audit/next, thanks for the follow-up.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index cbab0da..f3d3dc6 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2497,7 +2497,7 @@ void audit_seccomp_actions_logged(const char *names,
> const char *old_names,
> if (!audit_enabled)
> return;
>
> - ab = audit_log_start(current->audit_context, GFP_KERNEL,
> + ab = audit_log_start(audit_context(), GFP_KERNEL,
> AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
Re: [PATCH ghak81 V3 2/3] audit: normalize loginuid read access
On Wed, May 16, 2018 at 7:55 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> Recognizing that the loginuid is an internal audit value, use an access
> function to retrieve the audit loginuid value for the task rather than
> reaching directly into the task struct to get it.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/auditsc.c | 24 +++-
> 1 file changed, 15 insertions(+), 9 deletions(-)
Also merged into audit/next.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f3d3dc6..ef3e189 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
> case AUDIT_COMPARE_EGID_TO_OBJ_GID:
> return audit_compare_gid(cred->egid, name, f, ctx);
> case AUDIT_COMPARE_AUID_TO_OBJ_UID:
> - return audit_compare_uid(tsk->loginuid, name, f, ctx);
> + return audit_compare_uid(audit_get_loginuid(tsk), name, f,
> ctx);
> case AUDIT_COMPARE_SUID_TO_OBJ_UID:
> return audit_compare_uid(cred->suid, name, f, ctx);
> case AUDIT_COMPARE_SGID_TO_OBJ_GID:
> @@ -385,7 +385,8 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_compare_gid(cred->fsgid, name, f, ctx);
> /* uid comparisons */
> case AUDIT_COMPARE_UID_TO_AUID:
> - return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
> + return audit_uid_comparator(cred->uid, f->op,
> + audit_get_loginuid(tsk));
> case AUDIT_COMPARE_UID_TO_EUID:
> return audit_uid_comparator(cred->uid, f->op, cred->euid);
> case AUDIT_COMPARE_UID_TO_SUID:
> @@ -394,11 +395,14 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
> /* auid comparisons */
> case AUDIT_COMPARE_AUID_TO_EUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> + cred->euid);
> case AUDIT_COMPARE_AUID_TO_SUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> + cred->suid);
> case AUDIT_COMPARE_AUID_TO_FSUID:
> - return audit_uid_comparator(tsk->loginuid, f->op,
> cred->fsuid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> + cred->fsuid);
> /* euid comparisons */
> case AUDIT_COMPARE_EUID_TO_SUID:
> return audit_uid_comparator(cred->euid, f->op, cred->suid);
> @@ -611,7 +615,8 @@ static int audit_filter_rules(struct task_struct *tsk,
> result = match_tree_refs(ctx, rule->tree);
> break;
> case AUDIT_LOGINUID:
> - result = audit_uid_comparator(tsk->loginuid, f->op,
> f->uid);
> + result = audit_uid_comparator(audit_get_loginuid(tsk),
> + f->op, f->uid);
> break;
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk),
> f->op, f->val);
> @@ -2278,14 +2283,15 @@ int audit_signal_info(int sig, struct task_struct *t)
> {
> struct audit_aux_data_pids *axp;
> struct audit_context *ctx = audit_context();
> - kuid_t uid = current_uid(), t_uid = task_uid(t);
> + kuid_t uid = current_uid(), auid, t_uid = task_uid(t);
>
> if (auditd_test_task(t) &&
> (sig == SIGTERM || sig == SIGHUP ||
> sig == SIGUSR1 || sig == SIGUSR2)) {
> audit_sig_pid = task_tgid_nr(current);
> - if (uid_valid(current->loginuid))
> - audit_sig_uid = current->loginuid;
> + auid = audit_get_loginuid(current);
> + if (uid_valid(auid))
> + audit_sig_uid = auid;
> else
> audit_sig_uid = uid;
> security_task_getsecid(current, _sig_sid);
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
Re: [PATCH ghak81 RFC V2 5/5] audit: collect audit task parameters
): */
> -struct audit_context;
> struct backing_dev_info;
> struct bio_list;
> struct blk_plug;
> @@ -832,10 +832,8 @@ struct task_struct {
>
> struct callback_head*task_works;
>
> - struct audit_context*audit_context;
> #ifdef CONFIG_AUDITSYSCALL
> - kuid_t loginuid;
> - unsigned intsessionid;
> + struct audit_task_info audit;
> #endif
> struct seccomp seccomp;
>
> diff --git a/init/init_task.c b/init/init_task.c
> index 74f60ba..d33260d 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -119,8 +119,11 @@ struct task_struct init_task
> .thread_group = LIST_HEAD_INIT(init_task.thread_group),
> .thread_node= LIST_HEAD_INIT(init_signals.thread_head),
> #ifdef CONFIG_AUDITSYSCALL
> - .loginuid = INVALID_UID,
> - .sessionid = AUDIT_SID_UNSET,
> + .audit = {
> + .loginuid = INVALID_UID,
> + .sessionid = AUDIT_SID_UNSET,
> + .ctx= NULL,
> + },
> #endif
> #ifdef CONFIG_PERF_EVENTS
> .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d441d68..4c1fd18 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -836,7 +836,7 @@ static inline struct audit_context
> *audit_take_context(struct task_struct *tsk,
> int return_valid,
> long return_code)
> {
> - struct audit_context *context = tsk->audit_context;
> + struct audit_context *context = tsk->audit.ctx;
>
> if (!context)
> return NULL;
> @@ -2066,8 +2066,8 @@ int audit_set_loginuid(kuid_t loginuid)
> sessionid = (unsigned
> int)atomic_inc_return(_id);
> }
>
> - task->sessionid = sessionid;
> - task->loginuid = loginuid;
> + task->audit.sessionid = sessionid;
> + task->audit.loginuid = loginuid;
> out:
> audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid,
> sessionid, rc);
> return rc;
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
Re: [PATCH ghak81 RFC V2 4/5] audit: use inline function to set audit context
On Sat, May 12, 2018 at 9:58 PM, Richard Guy Briggs <r...@redhat.com> wrote: > Recognizing that the audit context is an internal audit value, use an > access function to set the audit context pointer for the task > rather than reaching directly into the task struct to set it. > > Signed-off-by: Richard Guy Briggs <r...@redhat.com> > --- > include/linux/audit.h | 6 ++ > kernel/auditsc.c | 7 +++ > kernel/fork.c | 2 +- > 3 files changed, 10 insertions(+), 5 deletions(-) Merged with some minor fuzz. -- paul moore www.paul-moore.com
Re: [PATCH ghak81 RFC V2 3/5] audit: use inline function to get audit context
On Sat, May 12, 2018 at 9:58 PM, Richard Guy Briggs <r...@redhat.com> wrote: > Recognizing that the audit context is an internal audit value, use an > access function to retrieve the audit context pointer for the task > rather than reaching directly into the task struct to get it. > > Signed-off-by: Richard Guy Briggs <r...@redhat.com> > --- > include/linux/audit.h| 14 ++-- > include/net/xfrm.h | 2 +- > kernel/audit.c | 6 ++-- > kernel/audit_watch.c | 2 +- > kernel/auditsc.c | 64 > +--- > net/bridge/netfilter/ebtables.c | 2 +- > net/core/dev.c | 2 +- > net/netfilter/x_tables.c | 2 +- > net/netlabel/netlabel_user.c | 2 +- > security/integrity/ima/ima_api.c | 2 +- > security/integrity/integrity_audit.c | 2 +- > security/lsm_audit.c | 2 +- > security/selinux/hooks.c | 4 +-- > security/selinux/selinuxfs.c | 6 ++-- > security/selinux/ss/services.c | 12 +++ > 15 files changed, 64 insertions(+), 60 deletions(-) Merged, but there was some fuzz due to the missing 1/5 patch and a handfull of checkpatch.pl fixes. Please take a look at the commit in the audit/next branch and if anything looks awry please send a patch to fix it. -- paul moore www.paul-moore.com
Re: [PATCH ghak81 RFC V2 2/5] audit: convert sessionid unset to a macro
On Sat, May 12, 2018 at 9:58 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> Use a macro, "AUDIT_SID_UNSET", to replace each instance of
> initialization and comparison to an audit session ID.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> include/linux/audit.h | 2 +-
> include/net/xfrm.h | 2 +-
> include/uapi/linux/audit.h | 1 +
> init/init_task.c | 3 ++-
> kernel/auditsc.c | 4 ++--
> 5 files changed, 7 insertions(+), 5 deletions(-)
Merged, thanks.
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 75d5b03..5f86f7c 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -513,7 +513,7 @@ static inline kuid_t audit_get_loginuid(struct
> task_struct *tsk)
> }
> static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
> {
> - return -1;
> + return AUDIT_SID_UNSET;
> }
> static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> { }
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index a872379..fcce8ee 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -751,7 +751,7 @@ static inline void xfrm_audit_helper_usrinfo(bool
> task_valid,
> audit_get_loginuid(current) :
> INVALID_UID);
> const unsigned int ses = task_valid ? audit_get_sessionid(current) :
> - (unsigned int) -1;
> + AUDIT_SID_UNSET;
>
> audit_log_format(audit_buf, " auid=%u ses=%u", auid, ses);
> audit_log_task_context(audit_buf);
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 4e61a9e..04f9bd2 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -465,6 +465,7 @@ struct audit_tty_status {
> };
>
> #define AUDIT_UID_UNSET (unsigned int)-1
> +#define AUDIT_SID_UNSET ((unsigned int)-1)
>
> /* audit_rule_data supports filter rules with both integer and string
> * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> diff --git a/init/init_task.c b/init/init_task.c
> index 3ac6e75..74f60ba 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -119,7 +120,7 @@ struct task_struct init_task
> .thread_node= LIST_HEAD_INIT(init_signals.thread_head),
> #ifdef CONFIG_AUDITSYSCALL
> .loginuid = INVALID_UID,
> - .sessionid = (unsigned int)-1,
> + .sessionid = AUDIT_SID_UNSET,
> #endif
> #ifdef CONFIG_PERF_EVENTS
> .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 0d4e269..e157595 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2050,7 +2050,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid,
> kuid_t kloginuid,
> int audit_set_loginuid(kuid_t loginuid)
> {
> struct task_struct *task = current;
> - unsigned int oldsessionid, sessionid = (unsigned int)-1;
> + unsigned int oldsessionid, sessionid = AUDIT_SID_UNSET;
> kuid_t oldloginuid;
> int rc;
>
> @@ -2064,7 +2064,7 @@ int audit_set_loginuid(kuid_t loginuid)
> /* are we setting or clearing? */
> if (uid_valid(loginuid)) {
> sessionid = (unsigned int)atomic_inc_return(_id);
> - if (unlikely(sessionid == (unsigned int)-1))
> + if (unlikely(sessionid == AUDIT_SID_UNSET))
> sessionid = (unsigned
> int)atomic_inc_return(_id);
> }
>
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
Re: [PATCH ghak81 RFC V2 1/5] audit: normalize loginuid read access
On Sat, May 12, 2018 at 9:58 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> Recognizing that the loginuid is an internal audit value, use an access
> function to retrieve the audit loginuid value for the task rather than
> reaching directly into the task struct to get it.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/auditsc.c | 18 +-
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 479c031..0d4e269 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
> case AUDIT_COMPARE_EGID_TO_OBJ_GID:
> return audit_compare_gid(cred->egid, name, f, ctx);
> case AUDIT_COMPARE_AUID_TO_OBJ_UID:
> - return audit_compare_uid(tsk->loginuid, name, f, ctx);
> + return audit_compare_uid(audit_get_loginuid(tsk), name, f,
> ctx);
> case AUDIT_COMPARE_SUID_TO_OBJ_UID:
> return audit_compare_uid(cred->suid, name, f, ctx);
> case AUDIT_COMPARE_SGID_TO_OBJ_GID:
> @@ -385,7 +385,7 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_compare_gid(cred->fsgid, name, f, ctx);
> /* uid comparisons */
> case AUDIT_COMPARE_UID_TO_AUID:
> - return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
> + return audit_uid_comparator(cred->uid, f->op,
> audit_get_loginuid(tsk));
> case AUDIT_COMPARE_UID_TO_EUID:
> return audit_uid_comparator(cred->uid, f->op, cred->euid);
> case AUDIT_COMPARE_UID_TO_SUID:
> @@ -394,11 +394,11 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
> /* auid comparisons */
> case AUDIT_COMPARE_AUID_TO_EUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->euid);
> case AUDIT_COMPARE_AUID_TO_SUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->suid);
> case AUDIT_COMPARE_AUID_TO_FSUID:
> - return audit_uid_comparator(tsk->loginuid, f->op,
> cred->fsuid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->fsuid);
> /* euid comparisons */
> case AUDIT_COMPARE_EUID_TO_SUID:
> return audit_uid_comparator(cred->euid, f->op, cred->suid);
> @@ -611,7 +611,7 @@ static int audit_filter_rules(struct task_struct *tsk,
> result = match_tree_refs(ctx, rule->tree);
> break;
> case AUDIT_LOGINUID:
> - result = audit_uid_comparator(tsk->loginuid, f->op,
> f->uid);
> + result =
> audit_uid_comparator(audit_get_loginuid(tsk), f->op, f->uid);
> break;
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk),
> f->op, f->val);
> @@ -2281,14 +2281,14 @@ int audit_signal_info(int sig, struct task_struct *t)
> struct audit_aux_data_pids *axp;
> struct task_struct *tsk = current;
> struct audit_context *ctx = tsk->audit_context;
> - kuid_t uid = current_uid(), t_uid = task_uid(t);
> + kuid_t uid = current_uid(), auid, t_uid = task_uid(t);
>
> if (auditd_test_task(t) &&
> (sig == SIGTERM || sig == SIGHUP ||
> sig == SIGUSR1 || sig == SIGUSR2)) {
> audit_sig_pid = task_tgid_nr(tsk);
> - if (uid_valid(tsk->loginuid))
> - audit_sig_uid = tsk->loginuid;
> + if (uid_valid(auid = audit_get_loginuid(tsk)))
> + audit_sig_uid = auid;
> else
> audit_sig_uid = uid;
> security_task_getsecid(tsk, _sig_sid);
A gentle reminder that you should try to make you patches as
"checkpatch clean" as possible (see scripts/checkpatch.pl). There are
several 80-char warnings, which aren't fatal, but the big no-no is
below:
ERROR: do not use assignment in if condition
#72: FILE: kernel/auditsc.c:2290:
+ if (uid_valid(auid = audit_get_loginuid(tsk)))
... while I don't completely agree with everything checkpatch has to
say, I definitely agree with checkpatch when it comes to assignments
in if conditions.
--
paul moore
www.paul-moore.com
Re: [PATCH v2 1/3] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Fri, May 11, 2018 at 1:15 PM, Alexey Kodanev
<alexey.koda...@oracle.com> wrote:
> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
> with the old programs that can pass sockaddr_in structure with AF_UNSPEC
> and INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
> This was found with LTP/asapi_01 test.
>
> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
> case to AF_INET and make sure that the address is INADDR_ANY.
>
> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
> Signed-off-by: Alexey Kodanev <alexey.koda...@oracle.com>
> ---
>
> v2: As suggested by Paul:
> * return EINVAL for SCTP socket if sa_family is AF_UNSPEC and
> address is not INADDR_ANY
> * add new 'sa_family' variable so that it equals either AF_INET
> or AF_INET6. Besides, it it will be used in the next patch that
> fixes audit record.
>
> security/selinux/hooks.c | 29 +++--
> 1 file changed, 19 insertions(+), 10 deletions(-)
All three patches looked good to me. I've merged them into
selinux/stable-4.17 and assuming nothing breaks in the next day or two
I'll send it up to Linus mid-week.
Thanks everyone!
--
paul moore
www.paul-moore.com
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Thu, May 10, 2018 at 5:28 AM, Alexey Kodanev
<alexey.koda...@oracle.com> wrote:
> On 10.05.2018 01:02, Paul Moore wrote:
> ...
>> I just had a better look at this and I believe that Alexey and Stephen
>> are right: this is the best option. My apologies for the noise
>> earlier. However, while looking at the code I think there are some
>> additional necessary changes:
>>
>> * In the case of an SCTP socket, we should return -EINVAL, just as we
>> do with other address families.
>
> Right.
>
>> * While not strictly related to AF_UNSPEC, we really should be passing
>> the address family of the sockaddr, and not the socket, to functions
>> that need to interpret the bind address/port.
>
> That looks like a correct solution. I guess we need the same fix for
> sctp_connectx(), in selinux_socket_connect_helper().
Yes. I think we also need a small change to
selinux_sctp_bind_connect() to both not error out on AF_UNSPEC, and to
return EINVAL on a bad address family (some of the callers pass on the
return value, some don't).
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5f30045b2053..a8bac9b37ee7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5277,6 +5277,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int>
while (walk_size < addrlen) {
addr = addr_buf;
switch (addr->sa_family) {
+ case AF_UNSPEC:
case AF_INET:
len = sizeof(struct sockaddr_in);
break;
@@ -5284,7 +5285,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int>
len = sizeof(struct sockaddr_in6);
break;
default:
- return -EAFNOSUPPORT;
+ return -EINVAL;
}
err = -EINVAL;
>> I'm waiting for my kernel to compile so I haven't given this any
>> sanity testing, but the patch below is what I think we need ...
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 4cafe6a19167..5f30045b2053 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -4576,6 +4576,7 @@ static int selinux_socket_post_create(struct socket
>> *sock,
>> int family,
>> static int selinux_socket_bind(struct socket *sock, struct sockaddr
>> *address, i
>> nt addrlen)
>> {
>>struct sock *sk = sock->sk;
>> + struct sk_security_struct *sksec = sk->sk_security;
>>u16 family;
>>int err;
>>
>> @@ -4587,13 +4588,13 @@ static int selinux_socket_bind(struct socket *sock,
>> stru
>> ct sockaddr *address, in
>>family = sk->sk_family;
>>if (family == PF_INET || family == PF_INET6) {
>>char *addrp;
>> - struct sk_security_struct *sksec = sk->sk_security;
>>struct common_audit_data ad;
>>struct lsm_network_audit net = {0,};
>>struct sockaddr_in *addr4 = NULL;
>>struct sockaddr_in6 *addr6 = NULL;
>>unsigned short snum;
>>u32 sid, node_perm;
>> + u16 family_sa = address->sa_family;
>>
>>/*
>> * sctp_bindx(3) calls via selinux_sctp_bind_connect()
>> @@ -4601,11 +4602,19 @@ static int selinux_socket_bind(struct socket *sock,
>> stru
>> ct sockaddr *address, in
>> * need to check address->sa_family as it is possible to have
>> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>> */
>> - switch (address->sa_family) {
>> + switch (family_sa) {
>> + case AF_UNSPEC:
>>case AF_INET:
>>if (addrlen < sizeof(struct sockaddr_in))
>>return -EINVAL;
>>addr4 = (struct sockaddr_in *)address;
>> + if (family_sa == AF_UNSPEC) {
>> + /* see "__inet_bind()", we only want to allow
>> +* AF_UNSPEC if the address is INADDR_ANY */
>> + if (addr4->sin_addr.s_addr !=
>> htonl(INADDR_ANY))
>> + goto err_af;
>> + family_sa = AF_INET;
>> + }
>>snum = ntohs(addr4->sin_port);
>>addrp = (char *)>sin_addr.s_addr;
&g
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Wed, May 9, 2018 at 11:34 AM, Paul Moore <p...@paul-moore.com> wrote:
> On Wed, May 9, 2018 at 11:11 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
>> On 05/09/2018 11:01 AM, Paul Moore wrote:
>>> On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
>>>> On 05/08/2018 08:25 PM, Paul Moore wrote:
>>>>> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley <s...@tycho.nsa.gov>
>>>>> wrote:
>>>>>> On 05/08/2018 01:05 PM, Paul Moore wrote:
>>>>>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>>>>>>> <alexey.koda...@oracle.com> wrote:
>>>>>>>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>>>>>>>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>>>>>>>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>>>>>>>> It was found with LTP/asapi_01 test.
>>>>>>>>
>>>>>>>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>>>>>>>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>>>>>>>> case to AF_INET and make sure that the address is INADDR_ANY.
>>>>>>>>
>>>>>>>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>>>>>>>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>>>>>>>
>>>>>>>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>>>>>>>> Signed-off-by: Alexey Kodanev <alexey.koda...@oracle.com>
>>>>>>>> ---
>>>>>>>> security/selinux/hooks.c | 12 +---
>>>>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> Thanks for finding and reporting this regression.
>>>>>>>
>>>>>>> I think I would prefer to avoid having to duplicate the
>>>>>>> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
>>>>>>> it is a small bit of code and unlikely to change. I'm wondering if it
>>>>>>> would be better to check both the socket and sockaddr address family
>>>>>>> in the main if conditional inside selinux_socket_bind(), what do you
>>>>>>> think? Another option would be to go back to just checking the
>>>>>>> soackaddr address family; we moved away from that for a reason which
>>>>>>> escapes at the moment (code cleanliness?), but perhaps that was a
>>>>>>> mistake.
>>>>>>
>>>>>> We've always used the sk->sk_family there; it was only the recent code
>>>>>> from Richard that started
>>>>>> using the socket address family.
>>>>>
>>>>> Yes I know, I thought I was the one that suggested it at some point
>>>>> (I'll take the blame) ... although now that I'm looking at the git
>>>>> log, maybe I'm confusing it with something else.
>>>>>
>>>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>>>> index 4cafe6a19167..a3789b167667 100644
>>>>>>> --- a/security/selinux/hooks.c
>>>>>>> +++ b/security/selinux/hooks.c
>>>>>>> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket
>>>>>>> *sock, struc>
>>>>>>> {
>>>>>>>struct sock *sk = sock->sk;
>>>>>>>u16 family;
>>>>>>> + u16 family_sa;
>>>>>>>int err;
>>>>>>>
>>>>>>>err = sock_has_perm(sk, SOCKET__BIND);
>>>>>>> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket
>>>>>>> *sock, struc>
>>>>>>>
>>>>>>>/* If PF_INET or PF_INET6, check name_bind permission for the
>>>>>>> port. */
>>>>>>>family = sk->sk_family;
>>>>>>> - if (family == PF_INET || family == PF_INET6) {
>>>>>>> + family_sa = address->sa_family;
>>>>>>> + if ((family == PF_INET || family == PF_INET6) &&
>>>>>>> + (
Re: [PATCH ghak81 RFC V1 0/5] audit: group task params
On Fri, May 4, 2018 at 4:54 PM, Richard Guy Briggs <r...@redhat.com> wrote: > Group the audit parameters for each task into one structure. > In particular, remove the loginuid and sessionid values and the audit > context pointer from the task structure, replacing them with an audit > task information structure to contain them. Use access functions to > access audit values. > > Note: Use static allocation of the audit task information structure > initially. Dynamic allocation was considered and attempted, but isn't > ready yet. Static allocation has the limitation that future audit task > information structure changes would cause a visible change to the rest > of the kernel, whereas dynamic allocation would mostly hide any future > changes. > > The first four access normalization patches could stand alone. I agree that the first four patches have some standalone value, and since we are currently at -rc4, did you want to post another patchset of just those four patches with feedback incorporated? I imagine that should be quick work, and that way they aren't help up with any problems/discussion regarding the take_struct changes. > Passes audit-testsuite. > > Richard Guy Briggs (5): > audit: normalize loginuid read access > audit: convert sessionid unset to a macro > audit: use inline function to get audit context > audit: use inline function to set audit context > audit: collect audit task parameters > > MAINTAINERS | 2 +- > include/linux/audit.h| 30 ++--- > include/linux/audit_task.h | 31 ++ > include/linux/sched.h| 6 +-- > include/net/xfrm.h | 4 +- > include/uapi/linux/audit.h | 1 + > init/init_task.c | 8 +++- > kernel/audit.c | 4 +- > kernel/audit_watch.c | 2 +- > kernel/auditsc.c | 82 > ++-- > kernel/fork.c| 2 +- > net/bridge/netfilter/ebtables.c | 2 +- > net/core/dev.c | 2 +- > net/netfilter/x_tables.c | 2 +- > net/netlabel/netlabel_user.c | 2 +- > security/integrity/ima/ima_api.c | 2 +- > security/integrity/integrity_audit.c | 2 +- > security/lsm_audit.c | 2 +- > security/selinux/hooks.c | 4 +- > security/selinux/selinuxfs.c | 6 +-- > security/selinux/ss/services.c | 12 +++--- > 21 files changed, 129 insertions(+), 79 deletions(-) > create mode 100644 include/linux/audit_task.h > > -- > 1.8.3.1 > -- paul moore www.paul-moore.com
Re: [PATCH ghak81 RFC V1 5/5] audit: collect audit task parameters
if
Considering that the audit_context pointer is now in the
audit_task_info struct, should the audit_task_info struct be placed
outside the CONFIG_AUDITSYSCALL protections? Or rather, shouldn't the
CONFIG_AUDITSYSCALL protections be moved inside audit_task_info or
removed entirely?
> diff --git a/init/init_task.c b/init/init_task.c
> index c788f91..d33260d 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -118,8 +119,11 @@ struct task_struct init_task
> .thread_group = LIST_HEAD_INIT(init_task.thread_group),
> .thread_node= LIST_HEAD_INIT(init_signals.thread_head),
> #ifdef CONFIG_AUDITSYSCALL
> - .loginuid = INVALID_UID,
> - .sessionid = AUDIT_SID_UNSET,
> + .audit = {
> + .loginuid = INVALID_UID,
> + .sessionid = AUDIT_SID_UNSET,
> + .ctx= NULL,
> + },
> #endif
> #ifdef CONFIG_PERF_EVENTS
> .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f294e4a..b5d8bff 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2068,8 +2068,8 @@ int audit_set_loginuid(kuid_t loginuid)
> sessionid = (unsigned
> int)atomic_inc_return(_id);
> }
>
> - task->sessionid = sessionid;
> - task->loginuid = loginuid;
> + task->audit.sessionid = sessionid;
> + task->audit.loginuid = loginuid;
> out:
> audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid,
> sessionid, rc);
> return rc;
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Wed, May 9, 2018 at 11:11 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
> On 05/09/2018 11:01 AM, Paul Moore wrote:
>> On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
>>> On 05/08/2018 08:25 PM, Paul Moore wrote:
>>>> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
>>>>> On 05/08/2018 01:05 PM, Paul Moore wrote:
>>>>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>>>>>> <alexey.koda...@oracle.com> wrote:
>>>>>>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>>>>>>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>>>>>>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>>>>>>> It was found with LTP/asapi_01 test.
>>>>>>>
>>>>>>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>>>>>>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>>>>>>> case to AF_INET and make sure that the address is INADDR_ANY.
>>>>>>>
>>>>>>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>>>>>>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>>>>>>
>>>>>>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>>>>>>> Signed-off-by: Alexey Kodanev <alexey.koda...@oracle.com>
>>>>>>> ---
>>>>>>> security/selinux/hooks.c | 12 +---
>>>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> Thanks for finding and reporting this regression.
>>>>>>
>>>>>> I think I would prefer to avoid having to duplicate the
>>>>>> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
>>>>>> it is a small bit of code and unlikely to change. I'm wondering if it
>>>>>> would be better to check both the socket and sockaddr address family
>>>>>> in the main if conditional inside selinux_socket_bind(), what do you
>>>>>> think? Another option would be to go back to just checking the
>>>>>> soackaddr address family; we moved away from that for a reason which
>>>>>> escapes at the moment (code cleanliness?), but perhaps that was a
>>>>>> mistake.
>>>>>
>>>>> We've always used the sk->sk_family there; it was only the recent code
>>>>> from Richard that started
>>>>> using the socket address family.
>>>>
>>>> Yes I know, I thought I was the one that suggested it at some point
>>>> (I'll take the blame) ... although now that I'm looking at the git
>>>> log, maybe I'm confusing it with something else.
>>>>
>>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>>> index 4cafe6a19167..a3789b167667 100644
>>>>>> --- a/security/selinux/hooks.c
>>>>>> +++ b/security/selinux/hooks.c
>>>>>> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket
>>>>>> *sock, struc>
>>>>>> {
>>>>>>struct sock *sk = sock->sk;
>>>>>>u16 family;
>>>>>> + u16 family_sa;
>>>>>>int err;
>>>>>>
>>>>>>err = sock_has_perm(sk, SOCKET__BIND);
>>>>>> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket
>>>>>> *sock, struc>
>>>>>>
>>>>>>/* If PF_INET or PF_INET6, check name_bind permission for the
>>>>>> port. */
>>>>>>family = sk->sk_family;
>>>>>> - if (family == PF_INET || family == PF_INET6) {
>>>>>> + family_sa = address->sa_family;
>>>>>> + if ((family == PF_INET || family == PF_INET6) &&
>>>>>> + (family_sa == PF_INET || family_sa == PF_INET6)) {
>>>>>
>>>>> Wouldn't this allow bypassing the name_bind permission check by passing
>>>>> in AF_UNSPEC?
>>>>
>>>> I believe these name_bind permission checkis skipped for AF_UNSPEC
>>>> already, isn't it? The only way the name_bind check would
Re: [PATCH ghak81 RFC V1 3/5] audit: use inline function to get audit context
On Fri, May 4, 2018 at 4:54 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> Recognizing that the audit context is an internal audit value, use an
> access function to retrieve the audit context pointer for the task
> rather than reaching directly into the task struct to get it.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> include/linux/audit.h| 16 ---
> include/net/xfrm.h | 2 +-
> kernel/audit.c | 4 +--
> kernel/audit_watch.c | 2 +-
> kernel/auditsc.c | 52
> ++--
> net/bridge/netfilter/ebtables.c | 2 +-
> net/core/dev.c | 2 +-
> net/netfilter/x_tables.c | 2 +-
> net/netlabel/netlabel_user.c | 2 +-
> security/integrity/ima/ima_api.c | 2 +-
> security/integrity/integrity_audit.c | 2 +-
> security/lsm_audit.c | 2 +-
> security/selinux/hooks.c | 4 +--
> security/selinux/selinuxfs.c | 6 ++---
> security/selinux/ss/services.c | 12 -
> 15 files changed, 60 insertions(+), 52 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 5f86f7c..93e4c61 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -235,26 +235,30 @@ extern void __audit_inode_child(struct inode *parent,
> extern void __audit_seccomp(unsigned long syscall, long signr, int code);
> extern void __audit_ptrace(struct task_struct *t);
>
> +static inline struct audit_context *audit_context(struct task_struct *task)
> +{
> + return task->audit_context;
> +}
Another case where I think I agree with everything here on principle,
especially when one considers it in the larger context of the audit
container ID work. However, I think we might be able to somply this a
bit by eliminating the parameter to the new audit_context() helper and
making it always reference the current task_struct. Based on this
patch it would appear that this change would work for all callers
except for audit_take_context() and __audit_syscall_entry(), both of
which are contained within the core audit code and are enough of a
special case that I think it is acceptable for them to access the
context directly. I'm trying to think of reasons why a non-audit
kernel subsystem would ever need to access the audit context of a
process other than current and I can't think of any ... removing the
task_struct pointer might help prevent mistakes/abuse in the future.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6e3ceb9..a4bbdcc 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -836,7 +836,7 @@ static inline struct audit_context
> *audit_take_context(struct task_struct *tsk,
> int return_valid,
> long return_code)
> {
> - struct audit_context *context = tsk->audit_context;
> + struct audit_context *context = audit_context(tsk);
>
> if (!context)
> return NULL;
> @@ -1510,7 +1510,7 @@ void __audit_syscall_entry(int major, unsigned long a1,
> unsigned long a2,
>unsigned long a3, unsigned long a4)
> {
> struct task_struct *tsk = current;
> - struct audit_context *context = tsk->audit_context;
> + struct audit_context *context = audit_context(tsk);
> enum audit_state state;
>
> if (!audit_enabled || !context)
--
paul moore
www.paul-moore.com
Re: [PATCH ghak81 RFC V1 2/5] audit: convert sessionid unset to a macro
On Tue, May 8, 2018 at 9:34 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2018-05-04 16:54, Richard Guy Briggs wrote:
>> Use a macro, "AUDIT_SID_UNSET", to replace each instance of
>> initialization and comparison to an audit session ID.
>>
>> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
>
> There's a minor issue with this patch, adding a header include to
> init/init_task.c in this patch and removing it from patch 5. That'll be
> in the next revision.
Okay, thanks for the heads-up. FWIW, this patch looks reasonable in
principle; changing magic numbers to macros/constants is almost always
a step in the right direction.
> I have dynamic allocation working, so that has a good chance of
> appearing too.
I'll comment on that in your patch 0, I just want to get through the
rest of the patches first.
>> ---
>> include/linux/audit.h | 2 +-
>> include/net/xfrm.h | 2 +-
>> include/uapi/linux/audit.h | 1 +
>> init/init_task.c | 2 +-
>> kernel/auditsc.c | 4 ++--
>> 5 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 75d5b03..5f86f7c 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -513,7 +513,7 @@ static inline kuid_t audit_get_loginuid(struct
>> task_struct *tsk)
>> }
>> static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
>> {
>> - return -1;
>> + return AUDIT_SID_UNSET;
>> }
>> static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>> { }
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index a872379..fcce8ee 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -751,7 +751,7 @@ static inline void xfrm_audit_helper_usrinfo(bool
>> task_valid,
>> audit_get_loginuid(current) :
>> INVALID_UID);
>> const unsigned int ses = task_valid ? audit_get_sessionid(current) :
>> - (unsigned int) -1;
>> + AUDIT_SID_UNSET;
>>
>> audit_log_format(audit_buf, " auid=%u ses=%u", auid, ses);
>> audit_log_task_context(audit_buf);
>> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
>> index 4e61a9e..04f9bd2 100644
>> --- a/include/uapi/linux/audit.h
>> +++ b/include/uapi/linux/audit.h
>> @@ -465,6 +465,7 @@ struct audit_tty_status {
>> };
>>
>> #define AUDIT_UID_UNSET (unsigned int)-1
>> +#define AUDIT_SID_UNSET ((unsigned int)-1)
>>
>> /* audit_rule_data supports filter rules with both integer and string
>> * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> diff --git a/init/init_task.c b/init/init_task.c
>> index 3ac6e75..c788f91 100644
>> --- a/init/init_task.c
>> +++ b/init/init_task.c
>> @@ -119,7 +119,7 @@ struct task_struct init_task
>> .thread_node= LIST_HEAD_INIT(init_signals.thread_head),
>> #ifdef CONFIG_AUDITSYSCALL
>> .loginuid = INVALID_UID,
>> - .sessionid = (unsigned int)-1,
>> + .sessionid = AUDIT_SID_UNSET,
>> #endif
>> #ifdef CONFIG_PERF_EVENTS
>> .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index f3817d0..6e3ceb9 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -2050,7 +2050,7 @@ static void audit_log_set_loginuid(kuid_t
>> koldloginuid, kuid_t kloginuid,
>> int audit_set_loginuid(kuid_t loginuid)
>> {
>> struct task_struct *task = current;
>> - unsigned int oldsessionid, sessionid = (unsigned int)-1;
>> + unsigned int oldsessionid, sessionid = AUDIT_SID_UNSET;
>> kuid_t oldloginuid;
>> int rc;
>>
>> @@ -2064,7 +2064,7 @@ int audit_set_loginuid(kuid_t loginuid)
>> /* are we setting or clearing? */
>> if (uid_valid(loginuid)) {
>> sessionid = (unsigned int)atomic_inc_return(_id);
>> - if (unlikely(sessionid == (unsigned int)-1))
>> + if (unlikely(sessionid == AUDIT_SID_UNSET))
>> sessionid = (unsigned
>> int)atomic_inc_return(_id);
>> }
>>
>> --
>> 1.8.3.1
>>
>> --
>> Linux-audit mailing list
>> linux-au...@redhat.com
>> https://www.redhat.com/mailman/listinfo/linux-audit
>
> - RGB
>
> --
> Richard Guy Briggs <r...@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>
> --
> Linux-audit mailing list
> linux-au...@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
Re: [PATCH ghak81 RFC V1 1/5] audit: normalize loginuid read access
On Fri, May 4, 2018 at 4:54 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> Recognizing that the loginuid is an internal audit value, use an access
> function to retrieve the audit loginuid value for the task rather than
> reaching directly into the task struct to get it.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/auditsc.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 479c031..f3817d0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
> case AUDIT_COMPARE_EGID_TO_OBJ_GID:
> return audit_compare_gid(cred->egid, name, f, ctx);
> case AUDIT_COMPARE_AUID_TO_OBJ_UID:
> - return audit_compare_uid(tsk->loginuid, name, f, ctx);
> + return audit_compare_uid(audit_get_loginuid(tsk), name, f,
> ctx);
> case AUDIT_COMPARE_SUID_TO_OBJ_UID:
> return audit_compare_uid(cred->suid, name, f, ctx);
> case AUDIT_COMPARE_SGID_TO_OBJ_GID:
> @@ -385,7 +385,7 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_compare_gid(cred->fsgid, name, f, ctx);
> /* uid comparisons */
> case AUDIT_COMPARE_UID_TO_AUID:
> - return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
> + return audit_uid_comparator(cred->uid, f->op,
> audit_get_loginuid(tsk));
> case AUDIT_COMPARE_UID_TO_EUID:
> return audit_uid_comparator(cred->uid, f->op, cred->euid);
> case AUDIT_COMPARE_UID_TO_SUID:
> @@ -394,11 +394,11 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
> /* auid comparisons */
> case AUDIT_COMPARE_AUID_TO_EUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->euid);
> case AUDIT_COMPARE_AUID_TO_SUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->suid);
> case AUDIT_COMPARE_AUID_TO_FSUID:
> - return audit_uid_comparator(tsk->loginuid, f->op,
> cred->fsuid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->fsuid);
> /* euid comparisons */
> case AUDIT_COMPARE_EUID_TO_SUID:
> return audit_uid_comparator(cred->euid, f->op, cred->suid);
> @@ -611,7 +611,7 @@ static int audit_filter_rules(struct task_struct *tsk,
> result = match_tree_refs(ctx, rule->tree);
> break;
> case AUDIT_LOGINUID:
> - result = audit_uid_comparator(tsk->loginuid, f->op,
> f->uid);
> + result =
> audit_uid_comparator(audit_get_loginuid(tsk), f->op, f->uid);
> break;
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk),
> f->op, f->val);
> @@ -2287,8 +2287,8 @@ int audit_signal_info(int sig, struct task_struct *t)
> (sig == SIGTERM || sig == SIGHUP ||
> sig == SIGUSR1 || sig == SIGUSR2)) {
> audit_sig_pid = task_tgid_nr(tsk);
> - if (uid_valid(tsk->loginuid))
> - audit_sig_uid = tsk->loginuid;
> + if (uid_valid(audit_get_loginuid(tsk)))
> + audit_sig_uid = audit_get_loginuid(tsk);
I realize this comment is a little silly given the nature of loginuid,
but if we are going to abstract away loginuid accesses (which I think
is good), we should probably access it once, store it in a local
variable, perform the validity check on the local variable, then
commit the local variable to audit_sig_uid. I realize a TOCTOU
problem is unlikely here, but with this new layer of abstraction it
seems that some additional safety might be a good thing.
> else
> audit_sig_uid = uid;
> security_task_getsecid(tsk, _sig_sid);
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
> On 05/08/2018 08:25 PM, Paul Moore wrote:
>> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
>>> On 05/08/2018 01:05 PM, Paul Moore wrote:
>>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>>>> <alexey.koda...@oracle.com> wrote:
>>>>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>>>>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>>>>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>>>>> It was found with LTP/asapi_01 test.
>>>>>
>>>>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>>>>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>>>>> case to AF_INET and make sure that the address is INADDR_ANY.
>>>>>
>>>>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>>>>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>>>>
>>>>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>>>>> Signed-off-by: Alexey Kodanev <alexey.koda...@oracle.com>
>>>>> ---
>>>>> security/selinux/hooks.c | 12 +---
>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> Thanks for finding and reporting this regression.
>>>>
>>>> I think I would prefer to avoid having to duplicate the
>>>> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
>>>> it is a small bit of code and unlikely to change. I'm wondering if it
>>>> would be better to check both the socket and sockaddr address family
>>>> in the main if conditional inside selinux_socket_bind(), what do you
>>>> think? Another option would be to go back to just checking the
>>>> soackaddr address family; we moved away from that for a reason which
>>>> escapes at the moment (code cleanliness?), but perhaps that was a
>>>> mistake.
>>>
>>> We've always used the sk->sk_family there; it was only the recent code from
>>> Richard that started
>>> using the socket address family.
>>
>> Yes I know, I thought I was the one that suggested it at some point
>> (I'll take the blame) ... although now that I'm looking at the git
>> log, maybe I'm confusing it with something else.
>>
>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>> index 4cafe6a19167..a3789b167667 100644
>>>> --- a/security/selinux/hooks.c
>>>> +++ b/security/selinux/hooks.c
>>>> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket *sock,
>>>> struc>
>>>> {
>>>>struct sock *sk = sock->sk;
>>>>u16 family;
>>>> + u16 family_sa;
>>>>int err;
>>>>
>>>>err = sock_has_perm(sk, SOCKET__BIND);
>>>> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket *sock,
>>>> struc>
>>>>
>>>>/* If PF_INET or PF_INET6, check name_bind permission for the port.
>>>> */
>>>>family = sk->sk_family;
>>>> - if (family == PF_INET || family == PF_INET6) {
>>>> + family_sa = address->sa_family;
>>>> + if ((family == PF_INET || family == PF_INET6) &&
>>>> + (family_sa == PF_INET || family_sa == PF_INET6)) {
>>>
>>> Wouldn't this allow bypassing the name_bind permission check by passing in
>>> AF_UNSPEC?
>>
>> I believe these name_bind permission checkis skipped for AF_UNSPEC
>> already, isn't it? The only way the name_bind check would be
>> triggered is if the source port, snum, was non-zero and I didn't think
>> that was really legal for AF_UNSPEC/INADDR_ANY, is it?
>
> 1) What in inet_bind() prevents that from occurring?
> 2) Regardless, what about the node_bind check?
Fair enough. As mentioned above, perhaps the right fix is to move the
address family checking back to how it was pre-SCTP.
Alexey, is this something you want to do, or should we take care of that?
>>>>char *addrp;
>>>>struct sk_security_struct *sksec = sk->sk_security;
>>>>struct common_audit_data ad;
>>>> @@ -4601,7 +4604,7 @@ static int selinux_s
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
> On 05/08/2018 01:05 PM, Paul Moore wrote:
>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>> <alexey.koda...@oracle.com> wrote:
>>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>>> It was found with LTP/asapi_01 test.
>>>
>>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>>> case to AF_INET and make sure that the address is INADDR_ANY.
>>>
>>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>>
>>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>>> Signed-off-by: Alexey Kodanev <alexey.koda...@oracle.com>
>>> ---
>>> security/selinux/hooks.c | 12 +---
>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> Thanks for finding and reporting this regression.
>>
>> I think I would prefer to avoid having to duplicate the
>> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
>> it is a small bit of code and unlikely to change. I'm wondering if it
>> would be better to check both the socket and sockaddr address family
>> in the main if conditional inside selinux_socket_bind(), what do you
>> think? Another option would be to go back to just checking the
>> soackaddr address family; we moved away from that for a reason which
>> escapes at the moment (code cleanliness?), but perhaps that was a
>> mistake.
>
> We've always used the sk->sk_family there; it was only the recent code from
> Richard that started
> using the socket address family.
Yes I know, I thought I was the one that suggested it at some point
(I'll take the blame) ... although now that I'm looking at the git
log, maybe I'm confusing it with something else.
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 4cafe6a19167..a3789b167667 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket *sock,
>> struc>
>> {
>>struct sock *sk = sock->sk;
>>u16 family;
>> + u16 family_sa;
>>int err;
>>
>>err = sock_has_perm(sk, SOCKET__BIND);
>> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket *sock,
>> struc>
>>
>>/* If PF_INET or PF_INET6, check name_bind permission for the port. */
>>family = sk->sk_family;
>> - if (family == PF_INET || family == PF_INET6) {
>> + family_sa = address->sa_family;
>> + if ((family == PF_INET || family == PF_INET6) &&
>> + (family_sa == PF_INET || family_sa == PF_INET6)) {
>
> Wouldn't this allow bypassing the name_bind permission check by passing in
> AF_UNSPEC?
I believe these name_bind permission checkis skipped for AF_UNSPEC
already, isn't it? The only way the name_bind check would be
triggered is if the source port, snum, was non-zero and I didn't think
that was really legal for AF_UNSPEC/INADDR_ANY, is it?
>>char *addrp;
>>struct sk_security_struct *sksec = sk->sk_security;
>>struct common_audit_data ad;
>> @@ -4601,7 +4604,7 @@ static int selinux_socket_bind(struct socket *sock,
>> struc>
>> * need to check address->sa_family as it is possible to have
>> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>> */
>> - switch (address->sa_family) {
>> + switch (family_sa) {
>>case AF_INET:
>>if (addrlen < sizeof(struct sockaddr_in))
>>return -EINVAL;
>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 4cafe6a..649a3be 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -4602,10 +4602,16 @@ static int selinux_socket_bind(struct socket *sock,
>>> struct sockaddr *address, in
>>> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>>> */
>>> switch (address->sa_family) {
&g
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
<alexey.koda...@oracle.com> wrote:
> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
> with the old programs that can pass sockaddr_in with AF_UNSPEC and
> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
> It was found with LTP/asapi_01 test.
>
> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
> case to AF_INET and make sure that the address is INADDR_ANY.
>
> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>
> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
> Signed-off-by: Alexey Kodanev <alexey.koda...@oracle.com>
> ---
> security/selinux/hooks.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
Thanks for finding and reporting this regression.
I think I would prefer to avoid having to duplicate the
AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
it is a small bit of code and unlikely to change. I'm wondering if it
would be better to check both the socket and sockaddr address family
in the main if conditional inside selinux_socket_bind(), what do you
think? Another option would be to go back to just checking the
soackaddr address family; we moved away from that for a reason which
escapes at the moment (code cleanliness?), but perhaps that was a
mistake.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4cafe6a19167..a3789b167667 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket *sock, struc>
{
struct sock *sk = sock->sk;
u16 family;
+ u16 family_sa;
int err;
err = sock_has_perm(sk, SOCKET__BIND);
@@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket *sock, struc>
/* If PF_INET or PF_INET6, check name_bind permission for the port. */
family = sk->sk_family;
- if (family == PF_INET || family == PF_INET6) {
+ family_sa = address->sa_family;
+ if ((family == PF_INET || family == PF_INET6) &&
+ (family_sa == PF_INET || family_sa == PF_INET6)) {
char *addrp;
struct sk_security_struct *sksec = sk->sk_security;
struct common_audit_data ad;
@@ -4601,7 +4604,7 @@ static int selinux_socket_bind(struct socket *sock, struc>
* need to check address->sa_family as it is possible to have
* sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
*/
- switch (address->sa_family) {
+ switch (family_sa) {
case AF_INET:
if (addrlen < sizeof(struct sockaddr_in))
return -EINVAL;
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 4cafe6a..649a3be 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4602,10 +4602,16 @@ static int selinux_socket_bind(struct socket *sock,
> struct sockaddr *address, in
> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
> */
> switch (address->sa_family) {
> + case AF_UNSPEC:
> case AF_INET:
> if (addrlen < sizeof(struct sockaddr_in))
> return -EINVAL;
> addr4 = (struct sockaddr_in *)address;
> +
> + if (address->sa_family == AF_UNSPEC &&
> + addr4->sin_addr.s_addr != htonl(INADDR_ANY))
> + return -EAFNOSUPPORT;
> +
> snum = ntohs(addr4->sin_port);
> addrp = (char *)>sin_addr.s_addr;
> break;
> @@ -4681,10 +4687,10 @@ static int selinux_socket_bind(struct socket *sock,
> struct sockaddr *address, in
> ad.u.net->sport = htons(snum);
> ad.u.net->family = family;
>
> - if (address->sa_family == AF_INET)
> - ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
> - else
> + if (address->sa_family == AF_INET6)
> ad.u.net->v6info.saddr = addr6->sin6_addr;
> + else
> + ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
>
> err = avc_has_perm(_state,
>sksec->sid, sid,
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Tue, Apr 24, 2018 at 8:40 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2018-04-24 15:01, Paul Moore wrote:
>> On Mon, Apr 23, 2018 at 10:02 PM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > On 2018-04-23 19:15, Paul Moore wrote:
>> >> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs <r...@redhat.com>
>> >> wrote:
>> >> > On 2018-04-18 19:47, Paul Moore wrote:
>> >> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com>
>> >> >> wrote:
>> >> >> > Implement the proc fs write to set the audit container ID of a
>> >> >> > process,
>> >> >> > emitting an AUDIT_CONTAINER record to document the event.
>> >> >> >
>> >> >> > This is a write from the container orchestrator task to a proc entry
>> >> >> > of
>> >> >> > the form /proc/PID/containerid where PID is the process ID of the
>> >> >> > newly
>> >> >> > created task that is to become the first task in a container, or an
>> >> >> > additional task added to a container.
>> >> >> >
>> >> >> > The write expects up to a u64 value (unset: 18446744073709551615).
>> >> >> >
>> >> >> > This will produce a record such as this:
>> >> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
>> >> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
>> >> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615
>> >> >> > contid=123455 res=0
>> >> >> >
>> >> >> > The "op" field indicates an initial set. The "pid" to "ses" fields
>> >> >> > are
>> >> >> > the orchestrator while the "opid" field is the object's PID, the
>> >> >> > process
>> >> >> > being "contained". Old and new container ID values are given in the
>> >> >> > "contid" fields, while res indicates its success.
>> >> >> >
>> >> >> > It is not permitted to self-set, unset or re-set the container ID. A
>> >> >> > child inherits its parent's container ID, but then can be set only
>> >> >> > once
>> >> >> > after.
>> >> >> >
>> >> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >> >> >
>> >> >> > Signed-off-by: Richard Guy Briggs <r...@redhat.com>
>> >> >> > ---
>> >> >> > fs/proc/base.c | 37
>> >> >> > include/linux/audit.h | 16 +
>> >> >> > include/linux/init_task.h | 4 ++-
>> >> >> > include/linux/sched.h | 1 +
>> >> >> > include/uapi/linux/audit.h | 2 ++
>> >> >> > kernel/auditsc.c | 84
>> >> >> > ++
>> >> >> > 6 files changed, 143 insertions(+), 1 deletion(-)
>>
>> ...
>>
>> >> >> > /* audit_rule_data supports filter rules with both integer and
>> >> >> > string
>> >> >> > * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> >> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> >> > index 4e0a4ac..29c8482 100644
>> >> >> > --- a/kernel/auditsc.c
>> >> >> > +++ b/kernel/auditsc.c
>> >> >> > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
>> >> >> > return rc;
>> >> >> > }
>> >> >> >
>> >> >> > +static int audit_set_containerid_perm(struct task_struct *task, u64
>> >> >> > containerid)
>> >> >> > +{
>> >> >> > + struct task_struct *parent;
>> >> >> > + u64 pcontainerid, ccontainerid;
>> >> >> > +
>> >> >> > + /* Don't allow to set our own containerid */
>> >> >> > + if (current == task)
>> >> >> > + return -EPERM;
>> >> >>
>> >> >> Why not? Is there some obvious securit
Re: [PATCH 0/3] Introduce LSM-hook for socketpair(2)
On Wed, Apr 25, 2018 at 2:44 PM, James Morris <jmor...@namei.org> wrote: > On Mon, 23 Apr 2018, David Herrmann wrote: >> This patch series tries to close this gap and makes both behave the >> same. A new LSM-hook is added which allows LSMs to cache the correct >> peer information on newly created socket-pairs. > > Looks okay to me. > > Once it's respun with the Smack backend and maybe the hook name change, > I'll merge this unless DaveM wants it to go in via his networking tree. Note my objection to the hook placement in patch 2/3; I think we should move the hook out of the AF_UNIX layer and up into the socket layer. -- paul moore www.paul-moore.com
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Mon, Apr 23, 2018 at 10:02 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2018-04-23 19:15, Paul Moore wrote:
>> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > On 2018-04-18 19:47, Paul Moore wrote:
>> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com>
>> >> wrote:
>> >> > Implement the proc fs write to set the audit container ID of a process,
>> >> > emitting an AUDIT_CONTAINER record to document the event.
>> >> >
>> >> > This is a write from the container orchestrator task to a proc entry of
>> >> > the form /proc/PID/containerid where PID is the process ID of the newly
>> >> > created task that is to become the first task in a container, or an
>> >> > additional task added to a container.
>> >> >
>> >> > The write expects up to a u64 value (unset: 18446744073709551615).
>> >> >
>> >> > This will produce a record such as this:
>> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
>> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
>> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
>> >> > res=0
>> >> >
>> >> > The "op" field indicates an initial set. The "pid" to "ses" fields are
>> >> > the orchestrator while the "opid" field is the object's PID, the process
>> >> > being "contained". Old and new container ID values are given in the
>> >> > "contid" fields, while res indicates its success.
>> >> >
>> >> > It is not permitted to self-set, unset or re-set the container ID. A
>> >> > child inherits its parent's container ID, but then can be set only once
>> >> > after.
>> >> >
>> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >> >
>> >> > Signed-off-by: Richard Guy Briggs <r...@redhat.com>
>> >> > ---
>> >> > fs/proc/base.c | 37
>> >> > include/linux/audit.h | 16 +
>> >> > include/linux/init_task.h | 4 ++-
>> >> > include/linux/sched.h | 1 +
>> >> > include/uapi/linux/audit.h | 2 ++
>> >> > kernel/auditsc.c | 84
>> >> > ++
>> >> > 6 files changed, 143 insertions(+), 1 deletion(-)
...
>> >> > /* audit_rule_data supports filter rules with both integer and string
>> >> > * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> > index 4e0a4ac..29c8482 100644
>> >> > --- a/kernel/auditsc.c
>> >> > +++ b/kernel/auditsc.c
>> >> > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
>> >> > return rc;
>> >> > }
>> >> >
>> >> > +static int audit_set_containerid_perm(struct task_struct *task, u64
>> >> > containerid)
>> >> > +{
>> >> > + struct task_struct *parent;
>> >> > + u64 pcontainerid, ccontainerid;
>> >> > +
>> >> > + /* Don't allow to set our own containerid */
>> >> > + if (current == task)
>> >> > + return -EPERM;
>> >>
>> >> Why not? Is there some obvious security concern that I missing?
>> >
>> > We then lose the distinction in the AUDIT_CONTAINER record between the
>> > initiating PID and the target PID. This was outlined in the proposal.
>>
>> I just went back and reread the v3 proposal and I still don't see a
>> good explanation of this. Why is this bad? What's the security
>> concern?
>
> I don't remember, specifically. Maybe this has been addressed by the
> check for children/threads or identical parent container ID. So, I'm
> reluctantly willing to remove that check for now.
Okay. For the record, if someone can explain to me why this
restriction saves us from some terrible situation I'm all for leaving
it. I'm just opposed to restrictions without solid reasoning behind
them.
>> > Having said that, I'm still not sure we have protected sufficiently from
>> > a child turning around and setting it's parent's as yet unset or
Re: [PATCH 2/3] net/unix: hook unix_socketpair() into LSM
On Tue, Apr 24, 2018 at 1:56 PM, David Miller <da...@davemloft.net> wrote:
> From: Paul Moore <p...@paul-moore.com>
> Date: Tue, 24 Apr 2018 13:55:31 -0400
>
>> On Mon, Apr 23, 2018 at 9:30 AM, David Herrmann <dh.herrm...@gmail.com>
>> wrote:
>>> Use the newly created LSM-hook for unix_socketpair(). The default hook
>>> return-value is 0, so behavior stays the same unless LSMs start using
>>> this hook.
>>>
>>> Signed-off-by: David Herrmann <dh.herrm...@gmail.com>
>>> ---
>>> net/unix/af_unix.c | 5 +
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>>> index 68bb70a62afe..bc9705ace9b1 100644
>>> --- a/net/unix/af_unix.c
>>> +++ b/net/unix/af_unix.c
>>> @@ -1371,6 +1371,11 @@ static int unix_stream_connect(struct socket *sock,
>>> struct sockaddr *uaddr,
>>> static int unix_socketpair(struct socket *socka, struct socket *sockb)
>>> {
>>> struct sock *ska = socka->sk, *skb = sockb->sk;
>>> + int err;
>>> +
>>> + err = security_unix_stream_socketpair(ska, skb);
>>> + if (err)
>>> + return err;
>>
>> I recognize that AF_UNIX is really the only protocol that supports
>> socketpair(2) at the moment, but I like to avoid protocol specific LSM
>> hooks whenever possible. Unless someone can think of a good
>> objection, I would prefer to see the hook placed in __sys_socketpair()
>> instead (and obviously drop the "unix_stream" portion from the hook
>> name).
>
> The counterargument is that after 30 years no other protocol has grown
> usage of this operation. :-)
Call me a an optimist ;)
--
paul moore
www.paul-moore.com
Re: [PATCH 2/3] net/unix: hook unix_socketpair() into LSM
On Mon, Apr 23, 2018 at 9:30 AM, David Herrmann <dh.herrm...@gmail.com> wrote:
> Use the newly created LSM-hook for unix_socketpair(). The default hook
> return-value is 0, so behavior stays the same unless LSMs start using
> this hook.
>
> Signed-off-by: David Herrmann <dh.herrm...@gmail.com>
> ---
> net/unix/af_unix.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 68bb70a62afe..bc9705ace9b1 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1371,6 +1371,11 @@ static int unix_stream_connect(struct socket *sock,
> struct sockaddr *uaddr,
> static int unix_socketpair(struct socket *socka, struct socket *sockb)
> {
> struct sock *ska = socka->sk, *skb = sockb->sk;
> + int err;
> +
> + err = security_unix_stream_socketpair(ska, skb);
> + if (err)
> + return err;
I recognize that AF_UNIX is really the only protocol that supports
socketpair(2) at the moment, but I like to avoid protocol specific LSM
hooks whenever possible. Unless someone can think of a good
objection, I would prefer to see the hook placed in __sys_socketpair()
instead (and obviously drop the "unix_stream" portion from the hook
name).
> /* Join our sockets back to back */
> sock_hold(ska);
> --
> 2.17.0
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2018-04-18 19:47, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > Implement the proc fs write to set the audit container ID of a process,
>> > emitting an AUDIT_CONTAINER record to document the event.
>> >
>> > This is a write from the container orchestrator task to a proc entry of
>> > the form /proc/PID/containerid where PID is the process ID of the newly
>> > created task that is to become the first task in a container, or an
>> > additional task added to a container.
>> >
>> > The write expects up to a u64 value (unset: 18446744073709551615).
>> >
>> > This will produce a record such as this:
>> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
>> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0
>> > ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
>> >
>> > The "op" field indicates an initial set. The "pid" to "ses" fields are
>> > the orchestrator while the "opid" field is the object's PID, the process
>> > being "contained". Old and new container ID values are given in the
>> > "contid" fields, while res indicates its success.
>> >
>> > It is not permitted to self-set, unset or re-set the container ID. A
>> > child inherits its parent's container ID, but then can be set only once
>> > after.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >
>> > Signed-off-by: Richard Guy Briggs <r...@redhat.com>
>> > ---
>> > fs/proc/base.c | 37
>> > include/linux/audit.h | 16 +
>> > include/linux/init_task.h | 4 ++-
>> > include/linux/sched.h | 1 +
>> > include/uapi/linux/audit.h | 2 ++
>> > kernel/auditsc.c | 84
>> > ++
>> > 6 files changed, 143 insertions(+), 1 deletion(-)
...
>> > diff --git a/include/linux/sched.h b/include/linux/sched.h
>> > index d258826..1b82191 100644
>> > --- a/include/linux/sched.h
>> > +++ b/include/linux/sched.h
>> > @@ -796,6 +796,7 @@ struct task_struct {
>> > #ifdef CONFIG_AUDITSYSCALL
>> > kuid_t loginuid;
>> > unsigned intsessionid;
>> > + u64 containerid;
>>
>> This one line addition to the task_struct scares me the most of
>> anything in this patchset. Why? It's a field named "containerid" in
>> a perhaps one of the most widely used core kernel structures; the
>> possibilities for abuse are endless, and it's foolish to think we
>> would ever be able to adequately police this.
>
> Fair enough.
>
>> Unfortunately, we can't add the field to audit_context as things
>> currently stand because we don't always allocate an audit_context,
>> it's dependent on the system's configuration, and we need to track the
>> audit container ID for a given process, regardless of the audit
>> configuration. Pretty much the same reason why loginuid and sessionid
>> are located directly in task_struct now. As I stressed during the
>> design phase, I really want to keep this as an *audit* container ID
>> and not a general purpose kernel wide container ID. If the kernel
>> ever grows a general purpose container ID token, I'll be the first in
>> line to convert the audit code, but I don't want audit to be that
>> general purpose mechanism ... audit is hated enough as-is ;)
>
> When would we need an audit container ID when audit is not enabled
> enough to have an audit_context?
I'm thinking of the audit_alloc() case where audit_filter_task()
returns AUDIT_DISABLED.
I believe this is the same reason why loginuid and sessionid live
directly in the task_struct and not in the audit_context; they need to
persist for the lifetime of the task.
> If it is only used for audit, and audit is the only consumer, and audit
> can only use it when it is enabled, then we can just return success to
> any write to the proc filehandle, or not even present it. Nothing will
> be able to know that value wasn't used.
>
> When are loginuid and sessionid used now when audit is not enabled (or
> should I say, explicitly disabled)?
See above. I think that should answer these questions.
>> > diff --git a/include/uapi/linux/audit.h b/includ
Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces
On April 20, 2018 4:48:34 PM Richard Guy Briggs <r...@redhat.com> wrote: On 2018-04-20 16:22, Paul Moore wrote: On Fri, Apr 20, 2018 at 4:02 PM, Richard Guy Briggs <r...@redhat.com> wrote: On 2018-04-18 21:46, Paul Moore wrote: On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote: Audit events could happen in a network namespace outside of a task context due to packets received from the net that trigger an auditing rule prior to being associated with a running task. The network namespace could in use by multiple containers by association to the tasks in that network namespace. We still want a way to attribute these events to any potential containers. Keep a list per network namespace to track these container identifiiers. Add/increment the container identifier on: - initial setting of the container id via /proc - clone/fork call that inherits a container identifier - unshare call that inherits a container identifier - setns call that inherits a container identifier Delete/decrement the container identifier on: - an inherited container id dropped when child set - process exit - unshare call that drops a net namespace - setns call that drops a net namespace See: https://github.com/linux-audit/audit-kernel/issues/32 See: https://github.com/linux-audit/audit-testsuite/issues/64 Signed-off-by: Richard Guy Briggs <r...@redhat.com> --- include/linux/audit.h | 7 +++ include/net/net_namespace.h | 12 kernel/auditsc.c| 9 ++--- kernel/nsproxy.c| 6 ++ net/core/net_namespace.c| 45 + 5 files changed, 76 insertions(+), 3 deletions(-) ... diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c index f6c5d33..d9f1090 100644 --- a/kernel/nsproxy.c +++ b/kernel/nsproxy.c @@ -140,6 +140,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk) struct nsproxy *old_ns = tsk->nsproxy; struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns); struct nsproxy *new_ns; + u64 containerid = audit_get_containerid(tsk); if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC | CLONE_NEWPID | CLONE_NEWNET | @@ -167,6 +168,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk) return PTR_ERR(new_ns); tsk->nsproxy = new_ns; + net_add_audit_containerid(new_ns->net_ns, containerid); return 0; } Hopefully we can handle this in audit_net_init(), we just need to figure out where we can get the correct task_struct for the audit container ID (some backpointer in the net struct?). I don't follow. This needs to happen on every task startup. audit_net_init() is only called when a new network namespace starts up. Yep, sorry, my mistake. I must have confused myself when I was looking at the code. I'm thinking out loud here, bear with me ... Assuming we move the netns/audit-container-ID tracking to audit_net, and considering we already have an audit hook in copy_process() (it calls audit_alloc()), would this be better handled by the copy_process() hook? This ignores naming, audit_alloc() reuse, etc.; those can be easily fixed. I'm just thinking of ways to limit our impact on the core kernel and leverage our existing interaction points. The new namespace hasn't been cloned yet and this is the only function where we have access to both namespaces, so I don't see how that could work... I'll take another, closer look, with v3. paul moore - RGB -- Richard Guy Briggs <r...@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 -- paul moore www.paul-moore.com
Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces
On Fri, Apr 20, 2018 at 4:02 PM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2018-04-18 21:46, Paul Moore wrote: >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote: >> > Audit events could happen in a network namespace outside of a task >> > context due to packets received from the net that trigger an auditing >> > rule prior to being associated with a running task. The network >> > namespace could in use by multiple containers by association to the >> > tasks in that network namespace. We still want a way to attribute >> > these events to any potential containers. Keep a list per network >> > namespace to track these container identifiiers. >> > >> > Add/increment the container identifier on: >> > - initial setting of the container id via /proc >> > - clone/fork call that inherits a container identifier >> > - unshare call that inherits a container identifier >> > - setns call that inherits a container identifier >> > Delete/decrement the container identifier on: >> > - an inherited container id dropped when child set >> > - process exit >> > - unshare call that drops a net namespace >> > - setns call that drops a net namespace >> > >> > See: https://github.com/linux-audit/audit-kernel/issues/32 >> > See: https://github.com/linux-audit/audit-testsuite/issues/64 >> > Signed-off-by: Richard Guy Briggs <r...@redhat.com> >> > --- >> > include/linux/audit.h | 7 +++ >> > include/net/net_namespace.h | 12 >> > kernel/auditsc.c| 9 ++--- >> > kernel/nsproxy.c| 6 ++ >> > net/core/net_namespace.c| 45 >> > + >> > 5 files changed, 76 insertions(+), 3 deletions(-) ... >> > diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c >> > index f6c5d33..d9f1090 100644 >> > --- a/kernel/nsproxy.c >> > +++ b/kernel/nsproxy.c >> > @@ -140,6 +140,7 @@ int copy_namespaces(unsigned long flags, struct >> > task_struct *tsk) >> > struct nsproxy *old_ns = tsk->nsproxy; >> > struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns); >> > struct nsproxy *new_ns; >> > + u64 containerid = audit_get_containerid(tsk); >> > >> > if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC | >> > CLONE_NEWPID | CLONE_NEWNET | >> > @@ -167,6 +168,7 @@ int copy_namespaces(unsigned long flags, struct >> > task_struct *tsk) >> > return PTR_ERR(new_ns); >> > >> > tsk->nsproxy = new_ns; >> > + net_add_audit_containerid(new_ns->net_ns, containerid); >> > return 0; >> > } >> >> Hopefully we can handle this in audit_net_init(), we just need to >> figure out where we can get the correct task_struct for the audit >> container ID (some backpointer in the net struct?). > > I don't follow. This needs to happen on every task startup. > audit_net_init() is only called when a new network namespace starts up. Yep, sorry, my mistake. I must have confused myself when I was looking at the code. I'm thinking out loud here, bear with me ... Assuming we move the netns/audit-container-ID tracking to audit_net, and considering we already have an audit hook in copy_process() (it calls audit_alloc()), would this be better handled by the copy_process() hook? This ignores naming, audit_alloc() reuse, etc.; those can be easily fixed. I'm just thinking of ways to limit our impact on the core kernel and leverage our existing interaction points. -- paul moore www.paul-moore.com
Re: [RFC PATCH ghak32 V2 06/13] audit: add support for non-syscall auxiliary records
On Thu, Apr 19, 2018 at 9:23 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2018-04-18 20:39, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> 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.
>> >
>> > Signed-off-by: Richard Guy Briggs <r...@redhat.com>
>> > ---
>> > include/linux/audit.h | 8
>> > kernel/auditsc.c | 20 +++-
>> > 2 files changed, 27 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/linux/audit.h b/include/linux/audit.h
>> > index ed16bb6..c0b83cb 100644
>> > --- a/include/linux/audit.h
>> > +++ b/include/linux/audit.h
>> > @@ -227,7 +227,9 @@ static inline int audit_log_container_info(struct
>> > audit_context *context,
>> > /* These are defined in auditsc.c */
>> > /* Public API */
>> > extern int audit_alloc(struct task_struct *task);
>> > +extern struct audit_context *audit_alloc_local(void);
>> > extern void __audit_free(struct task_struct *task);
>> > +extern void audit_free_context(struct audit_context *context);
>> > extern void __audit_syscall_entry(int major, unsigned long a0, unsigned
>> > long a1,
>> > unsigned long a2, unsigned long a3);
>> > extern void __audit_syscall_exit(int ret_success, long ret_value);
>> > @@ -472,6 +474,12 @@ static inline int audit_alloc(struct task_struct
>> > *task)
>> > {
>> > return 0;
>> > }
>> > +static inline struct audit_context *audit_alloc_local(void)
>> > +{
>> > + return NULL;
>> > +}
>> > +static inline void audit_free_context(struct audit_context *context)
>> > +{ }
>> > static inline void audit_free(struct task_struct *task)
>> > { }
>> > static inline void audit_syscall_entry(int major, unsigned long a0,
>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 2932ef1..7103d23 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -959,8 +959,26 @@ int audit_alloc(struct task_struct *tsk)
>> > return 0;
>> > }
>> >
>> > -static inline void audit_free_context(struct audit_context *context)
>> > +struct audit_context *audit_alloc_local(void)
>> > {
>> > + struct audit_context *context;
>> > +
>> > + if (!audit_ever_enabled)
>> > + return NULL; /* Return if not auditing. */
>> > +
>> > + context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
>> > + if (!context)
>> > + return NULL;
>> > + context->serial = audit_serial();
>> > + context->ctime = current_kernel_time64();
>> > + context->in_syscall = 1;
>> > + return context;
>> > +}
>> > +
>> > +inline void audit_free_context(struct audit_context *context)
>> > +{
>> > + if (!context)
>> > + return;
>> > audit_free_names(context);
>> > unroll_tree_refs(context, NULL, 0);
>> > free_tree_refs(context);
>>
>> I'm reserving the option to comment on this idea further as I make my
>> way through the patchset, but audit_free_context() definitely
>> shouldn't be declared as an inline function.
>
> Ok, I think I follow. When it wasn't exported, inline was fine, but now
> that it has been exported, it should no longer be inlined ...
Pretty much. Based on a few comments I've seen by compiler folks over
the years, my current thinking is that we shouldn't worry about
explicit inlining static functions in C files (header files are a
different story). The basic idea being that the compiler almost
always does a better job than us stupid developers.
> ... or should use
> an intermediate function name to export so that local uses of it can
> remain inline.
Possibly, but my guess is that the compiler could (will?) do that by
itself for code that lives in the same file.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 05/13] audit: add containerid support for ptrace and signals
On Thu, Apr 19, 2018 at 9:03 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2018-04-18 20:32, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote:
...
>> > /*
>> > * audit_log_container_info - report container info
>> > - * @tsk: task to be recorded
>> > * @context: task or local context for record
>> > + * @op: containerid string description
>> > + * @containerid: container ID to report
>> > */
>> > -int audit_log_container_info(struct task_struct *tsk, struct
>> > audit_context *context)
>> > +int audit_log_container_info(struct audit_context *context,
>> > + char *op, u64 containerid)
>> > {
>> > struct audit_buffer *ab;
>> >
>> > - if (!audit_containerid_set(tsk))
>> > + if (!cid_valid(containerid))
>> > return 0;
>> > /* Generate AUDIT_CONTAINER_INFO with container ID */
>> > ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER_INFO);
>> > if (!ab)
>> > return -ENOMEM;
>> > - audit_log_format(ab, "contid=%llu", audit_get_containerid(tsk));
>> > + audit_log_format(ab, "op=%s contid=%llu", op, containerid);
>> > audit_log_end(ab);
>> > return 0;
>> > }
>>
>> Let's get these changes into the first patch where
>> audit_log_container_info() is defined. Why? This inserts a new field
>> into the record which is a no-no. Yes, it is one single patchset, but
>> they are still separate patches and who knows which patches a given
>> distribution and/or tree may decide to backport.
>
> Fair enough. That first thought went through my mind... Would it be
> sufficient to move that field addition to the first patch and leave the
> rest here to support trace and signals?
I should have been more clear ... yes, that's what I was thinking; the
record format is the important part as it's user visible.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 10/13] audit: add containerid support for seccomp and anom_abend records
On Thu, Apr 19, 2018 at 8:42 PM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2018-04-18 21:31, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > Add container ID auxiliary records to secure computing and abnormal end
>> > standalone records.
>> >
>> > Signed-off-by: Richard Guy Briggs <r...@redhat.com>
>> > ---
>> > kernel/auditsc.c | 10 --
>> > 1 file changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 7103d23..2f02ed9 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -2571,6 +2571,7 @@ static void audit_log_task(struct audit_buffer *ab)
>> > void audit_core_dumps(long signr)
>> > {
>> > struct audit_buffer *ab;
>> > + struct audit_context *context = audit_alloc_local();
>>
>> Looking quickly at do_coredump() I *believe* we can use current here.
>>
>> > if (!audit_enabled)
>> > return;
>> > @@ -2578,19 +2579,22 @@ void audit_core_dumps(long signr)
>> > if (signr == SIGQUIT) /* don't care for those */
>> > return;
>> >
>> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
>> > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_ANOM_ABEND);
>> > if (unlikely(!ab))
>> > return;
>> > audit_log_task(ab);
>> > audit_log_format(ab, " sig=%ld res=1", signr);
>> > audit_log_end(ab);
>> > + audit_log_container_info(context, "abend",
>> > audit_get_containerid(current));
>> > + audit_free_context(context);
>> > }
>> >
>> > void __audit_seccomp(unsigned long syscall, long signr, int code)
>> > {
>> > struct audit_buffer *ab;
>> > + struct audit_context *context = audit_alloc_local();
>>
>> We can definitely use current here.
>
> Ok, so both syscall aux records. That elimintes this patch from the
> set, can go in independently.
Yep. It should help shrink the audit container ID patchset and
perhaps more importantly it should put some distance between the
connected-record debate and the audit container ID debate.
I understand we are going to need a "local" context for some things,
the network packets are probably the best example, but whenever
possible I would like to connect these records back to a task's
context.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 12/13] audit: NETFILTER_PKT: record each container ID associated with a netNS
On Thu, Apr 19, 2018 at 8:45 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2018-04-18 22:10, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > Add container ID auxiliary record(s) to NETFILTER_PKT event standalone
>> > records. Iterate through all potential container IDs associated with a
>> > network namespace.
>> >
>> > Signed-off-by: Richard Guy Briggs <r...@redhat.com>
>> > ---
>> > kernel/audit.c | 1 +
>> > kernel/auditsc.c | 2 ++
>> > net/netfilter/xt_AUDIT.c | 15 ++-
>> > 3 files changed, 17 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/kernel/audit.c b/kernel/audit.c
>> > index 08662b4..3c77e47 100644
>> > --- a/kernel/audit.c
>> > +++ b/kernel/audit.c
>> > @@ -2102,6 +2102,7 @@ int audit_log_container_info(struct audit_context
>> > *context,
>> > audit_log_end(ab);
>> > return 0;
>> > }
>> > +EXPORT_SYMBOL(audit_log_container_info);
>> >
>> > void audit_log_key(struct audit_buffer *ab, char *key)
>> > {
>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 208da962..af68d01 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -975,6 +975,7 @@ struct audit_context *audit_alloc_local(void)
>> > context->in_syscall = 1;
>> > return context;
>> > }
>> > +EXPORT_SYMBOL(audit_alloc_local);
>> >
>> > inline void audit_free_context(struct audit_context *context)
>> > {
>> > @@ -989,6 +990,7 @@ inline void audit_free_context(struct audit_context
>> > *context)
>> > audit_proctitle_free(context);
>> > kfree(context);
>> > }
>> > +EXPORT_SYMBOL(audit_free_context);
>> >
>> > static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>> > kuid_t auid, kuid_t uid, unsigned int
>> > sessionid,
>> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
>> > index c502419..edaa456 100644
>> > --- a/net/netfilter/xt_AUDIT.c
>> > +++ b/net/netfilter/xt_AUDIT.c
>> > @@ -71,10 +71,14 @@ static bool audit_ip6(struct audit_buffer *ab, struct
>> > sk_buff *skb)
>> > {
>> > struct audit_buffer *ab;
>> > int fam = -1;
>> > + struct audit_context *context = audit_alloc_local();
>> > + struct audit_containerid *cont;
>> > + int i = 0;
>> > + struct net *net;
>> >
>> > if (audit_enabled == 0)
>> > goto errout;
>>
>> Do I need to say it? I probably should ... the allocation should
>> happen after the audit_enabled check.
>
> Already fixed in V3 in my tree a couple of weeks ago...
... which you never posted, at least not anywhere I've seen. Which
effectively means I wasted a good chunk of time reviewing this code
late last night. Awesome.
> More timely review please?
More patience on your part?
>> > - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
>> > + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
>> > if (ab == NULL)
>> > goto errout;
>> >
>> > @@ -104,7 +108,16 @@ static bool audit_ip6(struct audit_buffer *ab, struct
>> > sk_buff *skb)
>> >
>> > audit_log_end(ab);
>> >
>> > + net = sock_net(NETLINK_CB(skb).sk);
>> > + list_for_each_entry(cont, >audit_containerid, list) {
>> > + char buf[14];
>> > +
>> > + sprintf(buf, "net%u", i++);
>> > + audit_log_container_info(context, buf, cont->id);
>> > + }
>>
>> It seems like this could (should?) be hidden inside an audit function,
>> e.g. audit_log_net_containers() or something like that.
>
> Perhaps... It was open-coded since at this point there are no other
> users. That'll make this tidier though.
If the code was all contained within a single subsystem them I would
generally agree that open coding is preferable, but since we are
crossing a subsystem boundary I think it would be preferable to
abstract away the details into a separate function.
This will probably also be necessary once you change to using the
audit_net/net_generic mechanism.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records
On Thu, Apr 19, 2018 at 8:31 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2018-04-18 21:27, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > Add container ID auxiliary records to configuration change, feature set
>> > change
>> > and user generated standalone records.
>> >
>> > Signed-off-by: Richard Guy Briggs <r...@redhat.com>
>> > ---
>> > kernel/audit.c | 50
>> > --
>> > kernel/auditfilter.c | 5 -
>> > 2 files changed, 44 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/kernel/audit.c b/kernel/audit.c
>> > index b238be5..08662b4 100644
>> > --- a/kernel/audit.c
>> > +++ b/kernel/audit.c
>> > @@ -400,8 +400,9 @@ static int audit_log_config_change(char
>> > *function_name, u32 new, u32 old,
>> > {
>> > struct audit_buffer *ab;
>> > int rc = 0;
>> > + struct audit_context *context = audit_alloc_local();
>>
>> We should be able to use current->audit_context here right? If we
>> can't for every caller, perhaps we pass an audit_context as an
>> argument and only allocate a local context when the passed
>> audit_context is NULL.
>>
>> Also, if you're not comfortable always using current, just pass the
>> audit_context as you do with audit_log_common_recv_msg().
>
> As mentioned in the tree/watch/mark patch, this is all obsoleted by
> making the AUDIT_CONFIG_CHANGE record a SYSCALL auxiliary record.
You've known about my desire to connect records for quite some time.
> This review would have been more helpful a month and a half ago.
If you really want to sink to that level of discussion, better quality
patches from you would have been helpful too, that is the one of the
main reasons why it takes so long to review your code. Let's keep the
commentary focused on the code, discussions like this aren't likely to
be helpful to anyone.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 12/13] audit: NETFILTER_PKT: record each container ID associated with a netNS
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> Add container ID auxiliary record(s) to NETFILTER_PKT event standalone
> records. Iterate through all potential container IDs associated with a
> network namespace.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/audit.c | 1 +
> kernel/auditsc.c | 2 ++
> net/netfilter/xt_AUDIT.c | 15 ++-
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 08662b4..3c77e47 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2102,6 +2102,7 @@ int audit_log_container_info(struct audit_context
> *context,
> audit_log_end(ab);
> return 0;
> }
> +EXPORT_SYMBOL(audit_log_container_info);
>
> void audit_log_key(struct audit_buffer *ab, char *key)
> {
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 208da962..af68d01 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -975,6 +975,7 @@ struct audit_context *audit_alloc_local(void)
> context->in_syscall = 1;
> return context;
> }
> +EXPORT_SYMBOL(audit_alloc_local);
>
> inline void audit_free_context(struct audit_context *context)
> {
> @@ -989,6 +990,7 @@ inline void audit_free_context(struct audit_context
> *context)
> audit_proctitle_free(context);
> kfree(context);
> }
> +EXPORT_SYMBOL(audit_free_context);
>
> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> kuid_t auid, kuid_t uid, unsigned int
> sessionid,
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index c502419..edaa456 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -71,10 +71,14 @@ static bool audit_ip6(struct audit_buffer *ab, struct
> sk_buff *skb)
> {
> struct audit_buffer *ab;
> int fam = -1;
> + struct audit_context *context = audit_alloc_local();
> + struct audit_containerid *cont;
> + int i = 0;
> + struct net *net;
>
> if (audit_enabled == 0)
> goto errout;
Do I need to say it? I probably should ... the allocation should
happen after the audit_enabled check.
> - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (ab == NULL)
> goto errout;
>
> @@ -104,7 +108,16 @@ static bool audit_ip6(struct audit_buffer *ab, struct
> sk_buff *skb)
>
> audit_log_end(ab);
>
> + net = sock_net(NETLINK_CB(skb).sk);
> + list_for_each_entry(cont, >audit_containerid, list) {
> + char buf[14];
> +
> + sprintf(buf, "net%u", i++);
> + audit_log_container_info(context, buf, cont->id);
> + }
It seems like this could (should?) be hidden inside an audit function,
e.g. audit_log_net_containers() or something like that.
> errout:
> + audit_free_context(context);
> return XT_CONTINUE;
> }
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces
ET |
> @@ -167,6 +168,7 @@ int copy_namespaces(unsigned long flags, struct
> task_struct *tsk)
> return PTR_ERR(new_ns);
>
> tsk->nsproxy = new_ns;
> + net_add_audit_containerid(new_ns->net_ns, containerid);
> return 0;
> }
Hopefully we can handle this in audit_net_init(), we just need to
figure out where we can get the correct task_struct for the audit
container ID (some backpointer in the net struct?).
> @@ -217,6 +219,7 @@ int unshare_nsproxy_namespaces(unsigned long
> unshare_flags,
> void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> {
> struct nsproxy *ns;
> + u64 containerid = audit_get_containerid(p);
>
> might_sleep();
>
> @@ -224,6 +227,9 @@ void switch_task_namespaces(struct task_struct *p, struct
> nsproxy *new)
> ns = p->nsproxy;
> p->nsproxy = new;
> task_unlock(p);
> + net_del_audit_containerid(ns->net_ns, containerid);
> + if (new)
> + net_add_audit_containerid(new->net_ns, containerid);
Okay, we might need a hook here for switching namespaces, but I would
much rather it be a generic audit hook that calls directly into audit.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 10/13] audit: add containerid support for seccomp and anom_abend records
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> Add container ID auxiliary records to secure computing and abnormal end
> standalone records.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/auditsc.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 7103d23..2f02ed9 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2571,6 +2571,7 @@ static void audit_log_task(struct audit_buffer *ab)
> void audit_core_dumps(long signr)
> {
> struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
Looking quickly at do_coredump() I *believe* we can use current here.
> if (!audit_enabled)
> return;
> @@ -2578,19 +2579,22 @@ void audit_core_dumps(long signr)
> if (signr == SIGQUIT) /* don't care for those */
> return;
>
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_ANOM_ABEND);
> if (unlikely(!ab))
> return;
> audit_log_task(ab);
> audit_log_format(ab, " sig=%ld res=1", signr);
> audit_log_end(ab);
> + audit_log_container_info(context, "abend",
> audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> void __audit_seccomp(unsigned long syscall, long signr, int code)
> {
> struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
We can definitely use current here.
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_SECCOMP);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_SECCOMP);
> if (unlikely(!ab))
> return;
> audit_log_task(ab);
> @@ -2598,6 +2602,8 @@ void __audit_seccomp(unsigned long syscall, long signr,
> int code)
> signr, syscall_get_arch(), syscall,
> in_compat_syscall(), KSTK_EIP(current), code);
> audit_log_end(ab);
> + audit_log_container_info(context, "seccomp",
> audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> struct list_head *audit_killed_trees(void)
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records
(t & AUDIT_TTY_LOG_PASSWD); > > - audit_log_common_recv_msg(, AUDIT_CONFIG_CHANGE); > + audit_log_common_recv_msg(context, , AUDIT_CONFIG_CHANGE); > audit_log_format(ab, " op=tty_set old-enabled=%d > new-enabled=%d" > " old-log_passwd=%d new-log_passwd=%d > res=%d", > old.enabled, s.enabled, old.log_passwd, > s.log_passwd, !err); > audit_log_end(ab); > + audit_log_container_info(context, "config", > +audit_get_containerid(current)); > + audit_free_context(context); > break; > } > default: > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index c4c8746..5f7f4d6 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -1109,11 +1109,12 @@ static void audit_log_rule_change(char *action, > struct audit_krule *rule, int re > struct audit_buffer *ab; > uid_t loginuid = from_kuid(_user_ns, > audit_get_loginuid(current)); > unsigned int sessionid = audit_get_sessionid(current); > + struct audit_context *context = audit_alloc_local(); > > if (!audit_enabled) > return; Well, first I think we should be able to get rid of the local context, but if for some reason we can't use current->audit_context then do the allocation after the audit_enabled check. > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE); > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE); > if (!ab) > return; > audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid); > @@ -1122,6 +1123,8 @@ static void audit_log_rule_change(char *action, struct > audit_krule *rule, int re > audit_log_key(ab, rule->filterkey); > audit_log_format(ab, " list=%d res=%d", rule->listnr, res); > audit_log_end(ab); > + audit_log_container_info(context, "config", > audit_get_containerid(current)); > + audit_free_context(context); > } -- paul moore www.paul-moore.com
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Wed, Apr 18, 2018 at 8:41 PM, Casey Schaufler <ca...@schaufler-ca.com> wrote:
> On 4/18/2018 4:47 PM, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote:
>>> Implement the proc fs write to set the audit container ID of a process,
>>> emitting an AUDIT_CONTAINER record to document the event.
>>> ...
>>>
>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>> index d258826..1b82191 100644
>>> --- a/include/linux/sched.h
>>> +++ b/include/linux/sched.h
>>> @@ -796,6 +796,7 @@ struct task_struct {
>>> #ifdef CONFIG_AUDITSYSCALL
>>> kuid_t loginuid;
>>> unsigned intsessionid;
>>> + u64 containerid;
>> This one line addition to the task_struct scares me the most of
>> anything in this patchset. Why? It's a field named "containerid" in
>> a perhaps one of the most widely used core kernel structures; the
>> possibilities for abuse are endless, and it's foolish to think we
>> would ever be able to adequately police this.
>
> If we can get the LSM infrastructure managed task blobs from
> module stacking in ahead of this we could create a trivial security
> module to manage this. It's not as if there aren't all sorts of
> interactions between security modules and the audit system already.
While yes, there are plenty of interactions between the two, it is
possible to use audit without the LSMs and I would like to preserve
that. Further, I don't want to entangle two very complicated code
changes or make the audit container ID effort dependent on LSM
stacking.
You're a good salesman Casey, but you're not that good ;)
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 07/13] audit: add container aux record to watch/tree/mark
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> Add container ID auxiliary record to mark, watch and tree rule
> configuration standalone records.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> kernel/audit_fsnotify.c | 5 -
> kernel/audit_tree.c | 5 -
> kernel/audit_watch.c| 33 +++--
> 3 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index 52f368b..18c110d 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -124,10 +124,11 @@ static void audit_mark_log_rule_change(struct
> audit_fsnotify_mark *audit_mark, c
> {
> struct audit_buffer *ab;
> struct audit_krule *rule = audit_mark->rule;
> + struct audit_context *context = audit_alloc_local();
>
> if (!audit_enabled)
> return;
Move the audit_alloc_local() after the audit_enabled check.
> - ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> + ab = audit_log_start(context, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> audit_log_format(ab, "auid=%u ses=%u op=%s",
> @@ -138,6 +139,8 @@ static void audit_mark_log_rule_change(struct
> audit_fsnotify_mark *audit_mark, c
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=1", rule->listnr);
> audit_log_end(ab);
> + audit_log_container_info(context, "config",
> audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> void audit_remove_mark(struct audit_fsnotify_mark *audit_mark)
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 67e6956..7c085be 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -496,8 +496,9 @@ static int tag_chunk(struct inode *inode, struct
> audit_tree *tree)
> static void audit_tree_log_remove_rule(struct audit_krule *rule)
> {
> struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
Sort of independent of the audit container ID work, but shouldn't we
have an audit_enabled check here?
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> audit_log_format(ab, "op=remove_rule");
> @@ -506,6 +507,8 @@ static void audit_tree_log_remove_rule(struct audit_krule
> *rule)
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=1", rule->listnr);
> audit_log_end(ab);
> + audit_log_container_info(context, "config",
> audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> static void kill_rules(struct audit_tree *tree)
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 9eb8b35..60d75a2 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -238,20 +238,25 @@ static struct audit_watch *audit_dupe_watch(struct
> audit_watch *old)
>
> static void audit_watch_log_rule_change(struct audit_krule *r, struct
> audit_watch *w, char *op)
> {
> - if (audit_enabled) {
> - struct audit_buffer *ab;
> - ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> - if (unlikely(!ab))
> - return;
> - audit_log_format(ab, "auid=%u ses=%u op=%s",
> -from_kuid(_user_ns,
> audit_get_loginuid(current)),
> -audit_get_sessionid(current), op);
> - audit_log_format(ab, " path=");
> - audit_log_untrustedstring(ab, w->path);
> - audit_log_key(ab, r->filterkey);
> - audit_log_format(ab, " list=%d res=1", r->listnr);
> - audit_log_end(ab);
> - }
> + struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
> +
> + if (!audit_enabled)
> + return;
Same as above, do the allocation after the audit_enabled check.
> + ab = audit_log_start(context, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> + if (unlikely(!ab))
> + return;
> + audit_log_format(ab, "auid=%u ses=%u op=%s",
> +from_kuid(_user_ns,
> audit_get_loginuid(current)),
> +audit_get_sessionid(current), op);
> + audit_log_format(ab, " path=");
> + audit_log_untrustedstring(ab, w->path);
> + audit_log_key(ab, r->filterkey);
> + audit_log_format(ab, " list=%d res=1", r->listnr);
> + audit_log_end(ab);
> + audit_log_container_info(context, "config",
> audit_get_containerid(current));
> + audit_free_context(context);
> }
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 06/13] audit: add support for non-syscall auxiliary records
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> 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.
>
> Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> ---
> include/linux/audit.h | 8
> kernel/auditsc.c | 20 +++-
> 2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index ed16bb6..c0b83cb 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -227,7 +227,9 @@ static inline int audit_log_container_info(struct
> audit_context *context,
> /* These are defined in auditsc.c */
> /* Public API */
> extern int audit_alloc(struct task_struct *task);
> +extern struct audit_context *audit_alloc_local(void);
> extern void __audit_free(struct task_struct *task);
> +extern void audit_free_context(struct audit_context *context);
> extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long
> a1,
> unsigned long a2, unsigned long a3);
> extern void __audit_syscall_exit(int ret_success, long ret_value);
> @@ -472,6 +474,12 @@ static inline int audit_alloc(struct task_struct *task)
> {
> return 0;
> }
> +static inline struct audit_context *audit_alloc_local(void)
> +{
> + return NULL;
> +}
> +static inline void audit_free_context(struct audit_context *context)
> +{ }
> static inline void audit_free(struct task_struct *task)
> { }
> static inline void audit_syscall_entry(int major, unsigned long a0,
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 2932ef1..7103d23 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -959,8 +959,26 @@ int audit_alloc(struct task_struct *tsk)
> return 0;
> }
>
> -static inline void audit_free_context(struct audit_context *context)
> +struct audit_context *audit_alloc_local(void)
> {
> + struct audit_context *context;
> +
> + if (!audit_ever_enabled)
> + return NULL; /* Return if not auditing. */
> +
> + context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
> + if (!context)
> + return NULL;
> + context->serial = audit_serial();
> + context->ctime = current_kernel_time64();
> + context->in_syscall = 1;
> + return context;
> +}
> +
> +inline void audit_free_context(struct audit_context *context)
> +{
> + if (!context)
> + return;
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> free_tree_refs(context);
I'm reserving the option to comment on this idea further as I make my
way through the patchset, but audit_free_context() definitely
shouldn't be declared as an inline function.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 05/13] audit: add containerid support for ptrace and signals
> 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))
> + context->target_sid, context->target_comm)
> + && audit_log_container_info(context, "target",
> context->target_cid))
Same question.
> call_panic = 1;
>
> if (context->pwd.dentry && context->pwd.mnt) {
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 04/13] audit: add containerid filtering
CONTAINERID:
> + if (f->val != sizeof(u64))
> + goto exit_free;
> + str = audit_unpack_string(, , f->val);
> + if (IS_ERR(str))
> + goto exit_free;
> + f->val64 = ((u64 *)str)[0];
> + break;
> }
> }
>
> @@ -666,6 +675,11 @@ static struct audit_rule_data
> *audit_krule_to_data(struct audit_krule *krule)
> data->buflen += data->values[i] =
> audit_pack_string(,
> audit_mark_path(krule->exe));
> break;
> + case AUDIT_CONTAINERID:
> + data->buflen += data->values[i] = sizeof(u64);
> + for (i = 0; i < sizeof(u64); i++)
> + ((char *)bufp)[i] = ((char *)>val64)[i];
> + break;
> case AUDIT_LOGINUID_SET:
> if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val)
> {
> data->fields[i] = AUDIT_LOGINUID;
> @@ -752,6 +766,10 @@ static int audit_compare_rule(struct audit_krule *a,
> struct audit_krule *b)
> if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> return 1;
> break;
> + case AUDIT_CONTAINERID:
> + if (a->fields[i].val64 != b->fields[i].val64)
> + return 1;
> + break;
> default:
> if (a->fields[i].val != b->fields[i].val)
> return 1;
> @@ -1210,6 +1228,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> }
> }
>
> +int audit_comparator64(u64 left, u32 op, u64 right)
> +{
> + switch (op) {
> + case Audit_equal:
> + return (left == right);
> + case Audit_not_equal:
> + return (left != right);
> + case Audit_lt:
> + return (left < right);
> + case Audit_le:
> + return (left <= right);
> + case Audit_gt:
> + return (left > right);
> + case Audit_ge:
> + return (left >= right);
> + case Audit_bitmask:
> + return (left & right);
> + case Audit_bittest:
> + return ((left & right) == right);
> + default:
> + BUG();
> + return 0;
> + }
> +}
> +
> int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> {
> switch (op) {
> @@ -1348,6 +1391,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> result =
> audit_comparator(audit_loginuid_set(current),
> f->op, f->val);
> break;
> + case AUDIT_CONTAINERID:
> + result =
> audit_comparator64(audit_get_containerid(current),
> + f->op,
> f->val64);
> + break;
> case AUDIT_MSGTYPE:
> result = audit_comparator(msgtype, f->op,
> f->val);
> break;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 65be110..2bba324 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -614,6 +614,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk),
> f->op, f->val);
> break;
> + case AUDIT_CONTAINERID:
> + result =
> audit_comparator64(audit_get_containerid(tsk), f->op, f->val64);
> + break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> linux-au...@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 02/13] audit: check children and threading before allowing containerid
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <r...@redhat.com> wrote: > Check if a task has existing children or co-threads and refuse to set > the container ID if either are present. Failure to check this could > permit games where a child scratches its parent's back to work around > inheritance and double-setting policy. > > Signed-off-by: Richard Guy Briggs <r...@redhat.com> > --- > kernel/auditsc.c | 4 > 1 file changed, 4 insertions(+) I would just include this in patch 1/2 as I can't think of world where we wouldn't this check. > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 29c8482..a6b0a52 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2087,6 +2087,10 @@ static int audit_set_containerid_perm(struct > task_struct *task, u64 containerid) > /* if we don't have caps, reject */ > if (!capable(CAP_AUDIT_CONTROL)) > return -EPERM; > + /* if task has children or is not single-threaded, deny */ > + if (!list_empty(>children) || > + !(thread_group_leader(task) && thread_group_empty(task))) > + return -EPERM; > /* if containerid is unset, allow */ > if (!audit_containerid_set(task)) > return 0; > -- > 1.8.3.1 -- paul moore www.paul-moore.com
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
ontainerid)
> +{
> + struct task_struct *parent;
> + u64 pcontainerid, ccontainerid;
> +
> + /* Don't allow to set our own containerid */
> + if (current == task)
> + return -EPERM;
Why not? Is there some obvious security concern that I missing?
I ask because I suppose it might be possible for some container
runtime to do a fork, setup some of the environment and them exec the
container (before you answer the obvious "namespaces!" please remember
we're not trying to define containers).
> + /* Don't allow the containerid to be unset */
> + if (!cid_valid(containerid))
> + return -EINVAL;
> + /* if we don't have caps, reject */
> + if (!capable(CAP_AUDIT_CONTROL))
> + return -EPERM;
> + /* if containerid is unset, allow */
> + if (!audit_containerid_set(task))
> + return 0;
> + /* it is already set, and not inherited from the parent, reject */
> + ccontainerid = audit_get_containerid(task);
> + rcu_read_lock();
> + parent = rcu_dereference(task->real_parent);
> + rcu_read_unlock();
> + task_lock(parent);
> + pcontainerid = audit_get_containerid(parent);
> + task_unlock(parent);
> + if (ccontainerid != pcontainerid)
> + return -EPERM;
> + return 0;
> +}
> +
> +static void audit_log_set_containerid(struct task_struct *task, u64
> oldcontainerid,
> + u64 containerid, int rc)
> +{
> + struct audit_buffer *ab;
> + uid_t uid;
> + struct tty_struct *tty;
> +
> + if (!audit_enabled)
> + return;
> +
> + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONTAINER);
> + if (!ab)
> + return;
> +
> + uid = from_kuid(_user_ns, task_uid(current));
> + tty = audit_get_tty(current);
> +
> + audit_log_format(ab, "op=set pid=%d uid=%u", task_tgid_nr(current),
> uid);
> + audit_log_task_context(ab);
> + audit_log_format(ab, " auid=%u tty=%s ses=%u opid=%d old-contid=%llu
> contid=%llu res=%d",
> +from_kuid(_user_ns,
> audit_get_loginuid(current)),
> +tty ? tty_name(tty) : "(none)",
> audit_get_sessionid(current),
> +task_tgid_nr(task), oldcontainerid, containerid,
> !rc);
> +
> + audit_put_tty(tty);
> + audit_log_end(ab);
> +}
> +
> +/**
> + * audit_set_containerid - set current task's audit_context containerid
> + * @containerid: containerid value
> + *
> + * Returns 0 on success, -EPERM on permission failure.
> + *
> + * Called (set) from fs/proc/base.c::proc_containerid_write().
> + */
> +int audit_set_containerid(struct task_struct *task, u64 containerid)
> +{
> + u64 oldcontainerid;
> + int rc;
> +
> + oldcontainerid = audit_get_containerid(task);
> +
> + rc = audit_set_containerid_perm(task, containerid);
> + if (!rc) {
> + task_lock(task);
> + task->containerid = containerid;
> + task_unlock(task);
> + }
> +
> + audit_log_set_containerid(task, oldcontainerid, containerid, rc);
> + return rc;
Why are audit_set_containerid_perm() and audit_log_containerid()
separate functions?
--
paul moore
www.paul-moore.com
Re: linux-next: manual merge of the selinux tree with the net-next tree
len < sizeof(struct in6_addr))
> + goto free;
>
> - /* Label connection socket for first association 1-to-many
> - * style for client sequence socket()->sendmsg(). This
> - * needs to be done before sctp_assoc_add_peer() as that will
> - * set up the initial packet that needs to account for any
> - * security ip options (CIPSO/CALIPSO) added to the packet.
> - */
> - af = sctp_get_af_specific(to.sa.sa_family);
> - if (!af) {
> - err = -EINVAL;
> - goto out_unlock;
> + dlen = sizeof(struct in6_addr);
> + daddr->v6.sin6_family = AF_INET6;
> + daddr->v6.sin6_port = htons(asoc->peer.port);
> + memcpy(>v6.sin6_addr, CMSG_DATA(cmsg), dlen);
> }
> - err = security_sctp_bind_connect(sk, SCTP_SENDMSG_CONNECT,
> - (struct sockaddr *),
> - af->sockaddr_len);
> - if (err < 0)
> - goto out_unlock;
> + err = sctp_verify_addr(sk, daddr, sizeof(*daddr));
> + if (err)
> + goto free;
>
> - new_asoc = sctp_association_new(ep, sk, scope, GFP_KERNEL);
> - if (!new_asoc) {
> - err = -ENOMEM;
> - goto out_unlock;
> - }
> - asoc = new_asoc;
> - err = sctp_assoc_set_bind_addr_from_ep(asoc, scope,
> GFP_KERNEL);
> - if (err < 0) {
> - err = -ENOMEM;
> - goto out_free;
> + old = sctp_endpoint_lookup_assoc(ep, daddr, );
> + if (old && old != asoc) {
> + if (old->state >= SCTP_STATE_ESTABLISHED)
> + err = -EISCONN;
> + else
> + err = -EALREADY;
> + goto free;
> }
>
> - /* If the SCTP_INIT ancillary data is specified, set all
> - * the association init values accordingly.
> - */
> - if (sinit) {
> - if (sinit->sinit_num_ostreams) {
> - __u16 outcnt = sinit->sinit_num_ostreams;
> -
> - asoc->c.sinit_num_ostreams = outcnt;
> - /* outcnt has been changed, so re-init stream
> */
> - err = sctp_stream_init(>stream, outcnt,
> 0,
> - GFP_KERNEL);
> - if (err)
> - goto out_free;
> - }
> - if (sinit->sinit_max_instreams) {
> - asoc->c.sinit_max_instreams =
> - sinit->sinit_max_instreams;
> - }
> - if (sinit->sinit_max_attempts) {
> - asoc->max_init_attempts
> - = sinit->sinit_max_attempts;
> - }
> - if (sinit->sinit_max_init_timeo) {
> - asoc->max_init_timeo =
> -
> msecs_to_jiffies(sinit->sinit_max_init_timeo);
> - }
> + if (sctp_endpoint_is_peeled_off(ep, daddr)) {
> + err = -EADDRNOTAVAIL;
> + goto free;
> }
>
> - /* Prime the peer's transport structures. */
> - transport = sctp_assoc_add_peer(asoc, , GFP_KERNEL,
> SCTP_UNKNOWN);
> + transport = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL,
> + SCTP_UNKNOWN);
> if (!transport) {
> err = -ENOMEM;
> - goto out_free;
> + goto free;
> }
> }
>
--
paul moore
www.paul-moore.com
Re: linux-next: manual merge of the selinux tree with the net-next tree
On Wed, Mar 7, 2018 at 3:26 PM, David Miller <da...@davemloft.net> wrote: > From: Paul Moore <p...@paul-moore.com> > Date: Wed, 7 Mar 2018 15:20:33 -0500 > >>> So you would only have to wait until my tree went in before >>> sending your pull request. >> >> So you would want me to rebase selinux/next on top of Linus' tree in >> the middle of the merge window? I'm sure that isn't what you meant, >> but that's how I keep reading the above ... which can't be right, >> because in my experience that's one way to piss off Linus. Help me >> understand what you are saying. > > I never said you rebase anything. I wonder where you get that from. As I said, I was just trying to figure out what you were suggesting. Your email was not very clear in my opinion. > I'm saying, you just defer your pull request until Linus takes my > networking tree in. > > No changes or rebasing of your tree is necessary whatsoever. You just > ask him to pull your tree as-is. > > Again, this is what other smaller subsystem trees do when they have a > situation like this. Which gets us back to what I originally suggested in my first email of this thread: linux-next carries the fixup patch and when we send the pull requests to Linus we mention this fixup/thread. For what it's worth, if you mention the potential merge conflict, and the fixup that Stephen provided, it shouldn't matter when the pull requests are sent to Linus; he's a smart guy, he'll merge things in the order he wants. I've seen more than a few people get burned by deferring pull requests, I don't intend to have SELinux, or audit for that matter, run into the same problem. -- paul moore www.paul-moore.com
Re: linux-next: manual merge of the selinux tree with the net-next tree
On Wed, Mar 7, 2018 at 12:45 PM, David Miller <da...@davemloft.net> wrote: > From: Paul Moore <p...@paul-moore.com> > Date: Wed, 7 Mar 2018 12:27:52 -0500 > >> I'm not sure we could have cleanly separated the core network stack >> changes from the rest of the SELinux/SCTP enablement, regardless it's >> a bit late at this point. The only other thought would have been to >> simply push Xin Long's cleanup patches until after the next merge >> window, but that would only be worth considering if they truly were >> just cleanup patches, and even then it doesn't seem very fair to Xin >> Long to have to wait. > > I think you wanted to have more integration, rather than less. I'm not quite sure where you are going here, I think we *all* want integration - subtrees merge patches/trees, Linus merges subtrees, etc. - and I don't believe I've said anything to the contrary. > What others have done in the past, is they simply pull my networking > tree into their's. I only base the SELinux and audit trees on Linus' tree. Perhaps I'm wrong, but a quick look at net-next makes me believe you do the same. I think it is also worth mentioning that the SELinux/SCTP patches have been in the selinux/next branch for several days now; from what I can tell they predate these net-next cleanup patches. Not that it matters, I just don't believe that pulling net-next would have solved this problem; I suppose the right thing would have been for net-next to pull selinux/next, yes? > I never rebase, ever. I've learned that saying "never" (or "never X, ever" in this case) is a recipe for disaster, but if it works for you, go for it. FWIW, I try to avoid rebases as much as possible; it's the nuclear option as far as I'm concerned and the only time I regularly rebase the SELinux and audit trees is after the merge window (e.g. we need something in -rc1, or we are simply too far out of date). Looking quickly at net-next, it looks like net-next/master is refreshed/rebased on a regular basis too (it contains the selinux-pr-20180130 tag)... and perhaps rebase is a term you don't want to use, but I think we are on the same page here. > My tree often goes in reasonable early in the merge window. Generally speaking I send my pull request to Linus early in the merge window too. It obviously tends to vary on when he does the pull, but we generally haven't had any major problems. > So you would only have to wait until my tree went in before > sending your pull request. So you would want me to rebase selinux/next on top of Linus' tree in the middle of the merge window? I'm sure that isn't what you meant, but that's how I keep reading the above ... which can't be right, because in my experience that's one way to piss off Linus. Help me understand what you are saying. > That's really the way to handle something like this. -- paul moore www.paul-moore.com
Re: linux-next: manual merge of the selinux tree with the net-next tree
On Wed, Mar 7, 2018 at 11:41 AM, David Miller <da...@davemloft.net> wrote:
> From: Paul Moore <p...@paul-moore.com>
> Date: Wed, 7 Mar 2018 11:34:31 -0500
>> On Mon, Mar 5, 2018 at 2:03 AM, Xin Long <lucien@gmail.com> wrote:
>>> On Mon, Mar 5, 2018 at 9:40 AM, Stephen Rothwell <s...@canb.auug.org.au>
>>> wrote:
>>>> Hi Paul,
>>>>
>>>> Today's linux-next merge of the selinux tree got a conflict in:
>>>>
>>>> net/sctp/socket.c
>>>>
>>>> between several refactoring commits from the net-next tree and commit:
>>>>
>>>> 2277c7cd75e3 ("sctp: Add LSM hooks")
>>>>
>>>> from the selinux tree.
>>>>
>>>> I fixed it up (I think - see below) and can carry the fix as
>>> The fixup is great! the same as I mentioned in:
>>> https://patchwork.ozlabs.org/patch/879898/
>>> for net-next.git
>>>
>>>> necessary. This is now fixed as far as linux-next is concerned, but any
>>>> non trivial conflicts should be mentioned to your upstream maintainer
>>>> when your tree is submitted for merging. You may also want to consider
>>>> cooperating with the maintainer of the conflicting tree to minimise any
>>>> particularly complex conflicts.
>>>
>>> [net-next,0/9] sctp: clean up sctp_sendmsg, this patchset was just applied
>>> in net-next. So I just guess it might not yet be there when selinux tree was
>>> being submitted.
>>
>> The selinux/next branch is based on v4.16-rc1 and doesn't feed into
>> the netdev tree, it goes straight to Linus during the merge window so
>> unfortunately I think we may need to carry this for some time and
>> relay this fix-up patch up to Linus during the merge window.
>
> What a mess.
>
> The SCTP option changes should have gone through my tree in retrospect.
It's unfortunate.
I'm not sure we could have cleanly separated the core network stack
changes from the rest of the SELinux/SCTP enablement, regardless it's
a bit late at this point. The only other thought would have been to
simply push Xin Long's cleanup patches until after the next merge
window, but that would only be worth considering if they truly were
just cleanup patches, and even then it doesn't seem very fair to Xin
Long to have to wait.
Thankfully stuff like this is rare (at least from a netdev/SELinux POV).
--
paul moore
www.paul-moore.com
Re: linux-next: manual merge of the selinux tree with the net-next tree
On Mon, Mar 5, 2018 at 2:03 AM, Xin Long <lucien@gmail.com> wrote:
> On Mon, Mar 5, 2018 at 9:40 AM, Stephen Rothwell <s...@canb.auug.org.au>
> wrote:
>> Hi Paul,
>>
>> Today's linux-next merge of the selinux tree got a conflict in:
>>
>> net/sctp/socket.c
>>
>> between several refactoring commits from the net-next tree and commit:
>>
>> 2277c7cd75e3 ("sctp: Add LSM hooks")
>>
>> from the selinux tree.
>>
>> I fixed it up (I think - see below) and can carry the fix as
> The fixup is great! the same as I mentioned in:
> https://patchwork.ozlabs.org/patch/879898/
> for net-next.git
>
>> necessary. This is now fixed as far as linux-next is concerned, but any
>> non trivial conflicts should be mentioned to your upstream maintainer
>> when your tree is submitted for merging. You may also want to consider
>> cooperating with the maintainer of the conflicting tree to minimise any
>> particularly complex conflicts.
>
> [net-next,0/9] sctp: clean up sctp_sendmsg, this patchset was just applied
> in net-next. So I just guess it might not yet be there when selinux tree was
> being submitted.
The selinux/next branch is based on v4.16-rc1 and doesn't feed into
the netdev tree, it goes straight to Linus during the merge window so
unfortunately I think we may need to carry this for some time and
relay this fix-up patch up to Linus during the merge window.
--
paul moore
www.paul-moore.com
Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()
On Tue, Mar 6, 2018 at 6:59 PM, Stephen Hemminger
<step...@networkplumber.org> wrote:
> On Tue, 06 Mar 2018 17:27:44 -0500
> Paul Moore <pmo...@redhat.com> wrote:
>> From: Paul Moore <p...@paul-moore.com>
>>
>> Starting with v4.16-rc1 we've been seeing a higher than usual number
>> of requests for the kernel to load networking modules, even on events
>> which shouldn't trigger a module load (e.g. ioctl(TCGETS)). Stephen
>> Smalley suggested the problem may lie in commit 44c02a2c3dc5
>> ("dev_ioctl(): move copyin/copyout to callers") which moves changes
>> the network dev_ioctl() function to always call dev_load(),
>> regardless of the requested ioctl.
>>
>> This patch moves the dev_load() calls back into the individual ioctls
>> while preserving the rest of the original patch.
>>
>> Reported-by: Dominick Grift <dac.overr...@gmail.com>
>> Suggested-by: Stephen Smalley <s...@tycho.nsa.gov>
>> Signed-off-by: Paul Moore <p...@paul-moore.com>
>> ---
>> net/core/dev_ioctl.c |7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
>> index 0ab1af04296c..a04e1e88bf3a 100644
>> --- a/net/core/dev_ioctl.c
>> +++ b/net/core/dev_ioctl.c
>> @@ -402,8 +402,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
>> ifreq *ifr, bool *need_c
>> if (colon)
>> *colon = 0;
>>
>> - dev_load(net, ifr->ifr_name);
>
> Actually dev_load by ethernet name is really a legacy thing that should just
> die,
>
> It was kept around so that some very tunnel configuration using special names.
>
> # ifconfig sit0
>
> which probably several web pages still tell users to do...
> We have much better control now with ip commands so that this is just
> baggage.
In an effort to get this regression fixed quickly, and not get tangled
up in a user education issue, can we at least restore the old ioctl()
behavior and worry about removing dev_load() later?
--
paul moore
www.paul-moore.com
Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()
On Tue, Mar 6, 2018 at 5:27 PM, Paul Moore <pmo...@redhat.com> wrote:
> From: Paul Moore <p...@paul-moore.com>
>
> Starting with v4.16-rc1 we've been seeing a higher than usual number
> of requests for the kernel to load networking modules, even on events
> which shouldn't trigger a module load (e.g. ioctl(TCGETS)). Stephen
> Smalley suggested the problem may lie in commit 44c02a2c3dc5
> ("dev_ioctl(): move copyin/copyout to callers") which moves changes
> the network dev_ioctl() function to always call dev_load(),
> regardless of the requested ioctl.
>
> This patch moves the dev_load() calls back into the individual ioctls
> while preserving the rest of the original patch.
>
> Reported-by: Dominick Grift <dac.overr...@gmail.com>
> Suggested-by: Stephen Smalley <s...@tycho.nsa.gov>
> Signed-off-by: Paul Moore <p...@paul-moore.com>
> ---
> net/core/dev_ioctl.c |7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
In the interest of full disclosure, I've compiled this code but I
haven't booted it yet (test kernel building now). I just wanted to
post this sooner rather than later in case the networking folks, or
Al, had a different solution they would prefer.
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 0ab1af04296c..a04e1e88bf3a 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -402,8 +402,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> if (colon)
> *colon = 0;
>
> - dev_load(net, ifr->ifr_name);
> -
> /*
> * See which interface the caller is talking about.
> */
> @@ -423,6 +421,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> case SIOCGIFMAP:
> case SIOCGIFINDEX:
> case SIOCGIFTXQLEN:
> + dev_load(net, ifr->ifr_name);
> rcu_read_lock();
> ret = dev_ifsioc_locked(net, ifr, cmd);
> rcu_read_unlock();
> @@ -431,6 +430,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> return ret;
>
> case SIOCETHTOOL:
> + dev_load(net, ifr->ifr_name);
> rtnl_lock();
> ret = dev_ethtool(net, ifr);
> rtnl_unlock();
> @@ -447,6 +447,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> case SIOCGMIIPHY:
> case SIOCGMIIREG:
> case SIOCSIFNAME:
> + dev_load(net, ifr->ifr_name);
> if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> return -EPERM;
> rtnl_lock();
> @@ -494,6 +495,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> /* fall through */
> case SIOCBONDSLAVEINFOQUERY:
> case SIOCBONDINFOQUERY:
> + dev_load(net, ifr->ifr_name);
> rtnl_lock();
> ret = dev_ifsioc(net, ifr, cmd);
> rtnl_unlock();
> @@ -518,6 +520,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> cmd == SIOCGHWTSTAMP ||
> (cmd >= SIOCDEVPRIVATE &&
> cmd <= SIOCDEVPRIVATE + 15)) {
> + dev_load(net, ifr->ifr_name);
> rtnl_lock();
> ret = dev_ifsioc(net, ifr, cmd);
> rtnl_unlock();
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
paul moore
www.paul-moore.com
[PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()
From: Paul Moore <p...@paul-moore.com>
Starting with v4.16-rc1 we've been seeing a higher than usual number
of requests for the kernel to load networking modules, even on events
which shouldn't trigger a module load (e.g. ioctl(TCGETS)). Stephen
Smalley suggested the problem may lie in commit 44c02a2c3dc5
("dev_ioctl(): move copyin/copyout to callers") which moves changes
the network dev_ioctl() function to always call dev_load(),
regardless of the requested ioctl.
This patch moves the dev_load() calls back into the individual ioctls
while preserving the rest of the original patch.
Reported-by: Dominick Grift <dac.overr...@gmail.com>
Suggested-by: Stephen Smalley <s...@tycho.nsa.gov>
Signed-off-by: Paul Moore <p...@paul-moore.com>
---
net/core/dev_ioctl.c |7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 0ab1af04296c..a04e1e88bf3a 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -402,8 +402,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
if (colon)
*colon = 0;
- dev_load(net, ifr->ifr_name);
-
/*
* See which interface the caller is talking about.
*/
@@ -423,6 +421,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
case SIOCGIFMAP:
case SIOCGIFINDEX:
case SIOCGIFTXQLEN:
+ dev_load(net, ifr->ifr_name);
rcu_read_lock();
ret = dev_ifsioc_locked(net, ifr, cmd);
rcu_read_unlock();
@@ -431,6 +430,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
return ret;
case SIOCETHTOOL:
+ dev_load(net, ifr->ifr_name);
rtnl_lock();
ret = dev_ethtool(net, ifr);
rtnl_unlock();
@@ -447,6 +447,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
case SIOCGMIIPHY:
case SIOCGMIIREG:
case SIOCSIFNAME:
+ dev_load(net, ifr->ifr_name);
if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
return -EPERM;
rtnl_lock();
@@ -494,6 +495,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
/* fall through */
case SIOCBONDSLAVEINFOQUERY:
case SIOCBONDINFOQUERY:
+ dev_load(net, ifr->ifr_name);
rtnl_lock();
ret = dev_ifsioc(net, ifr, cmd);
rtnl_unlock();
@@ -518,6 +520,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
cmd == SIOCGHWTSTAMP ||
(cmd >= SIOCDEVPRIVATE &&
cmd <= SIOCDEVPRIVATE + 15)) {
+ dev_load(net, ifr->ifr_name);
rtnl_lock();
ret = dev_ifsioc(net, ifr, cmd);
rtnl_unlock();
Re: [RFC PATCH V1 01/12] audit: add container id
On Sat, Mar 3, 2018 at 4:19 AM, Serge E. Hallyn <se...@hallyn.com> wrote:
> On Thu, Mar 01, 2018 at 02:41:04PM -0500, Richard Guy Briggs wrote:
> ...
>> +static inline bool audit_containerid_set(struct task_struct *tsk)
>
> Hi Richard,
>
> the calls to audit_containerid_set() confused me. Could you make it
> is_audit_containerid_set() or audit_containerid_isset()?
I haven't gone through the entire patchset yet, but I wanted to
quickly comment on this ... I really dislike the
function-names-as-sentences approach and would would greatly prefer
audit_containerid_isset().
>> +{
>> + return audit_get_containerid(tsk) != INVALID_CID;
>> +}
--
paul moore
www.paul-moore.com
Re: [PATCH] selinux: Fix ltp test connect-syscall failure
On Fri, Mar 2, 2018 at 2:54 PM, Richard Haines
<richard_c_hai...@btinternet.com> wrote:
> Fix the following error when running regression tests using LTP as follows:
> cd /opt/ltp/
> cat runtest/syscalls |grep connect01>runtest/connect-syscall
> ./runltp -pq -f connect-syscall
>
> Running tests...
> connect011 TPASS : bad file descriptor successful
> connect012 TPASS : invalid socket buffer successful
> connect013 TPASS : invalid salen successful
> connect014 TPASS : invalid socket successful
> connect015 TPASS : already connected successful
> connect016 TPASS : connection refused successful
> connect017 TFAIL : connect01.c:146: invalid address family ;
> returned -1 (expected -1), errno 22 (expected 97)
> INFO: ltp-pan reported some tests FAIL
> LTP Version: 20180118
>
> Reported-by: Anders Roxell <anders.rox...@linaro.org>
> Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com>
> ---
> security/selinux/hooks.c | 42 ++
> 1 file changed, 30 insertions(+), 12 deletions(-)
Merged, thanks guys.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 28a5c4e..d614df1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4470,22 +4470,29 @@ static int selinux_socket_bind(struct socket *sock,
> struct sockaddr *address, in
> * need to check address->sa_family as it is possible to have
> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
> */
> - if (address->sa_family == AF_INET) {
> - if (addrlen < sizeof(struct sockaddr_in)) {
> - err = -EINVAL;
> - goto out;
> - }
> + switch (address->sa_family) {
> + case AF_INET:
> + if (addrlen < sizeof(struct sockaddr_in))
> + return -EINVAL;
> addr4 = (struct sockaddr_in *)address;
> snum = ntohs(addr4->sin_port);
> addrp = (char *)>sin_addr.s_addr;
> - } else {
> - if (addrlen < SIN6_LEN_RFC2133) {
> - err = -EINVAL;
> - goto out;
> - }
> + break;
> + case AF_INET6:
> + if (addrlen < SIN6_LEN_RFC2133)
> + return -EINVAL;
> addr6 = (struct sockaddr_in6 *)address;
> snum = ntohs(addr6->sin6_port);
> addrp = (char *)>sin6_addr.s6_addr;
> + break;
> + default:
> + /* Note that SCTP services expect -EINVAL, whereas
> +* others expect -EAFNOSUPPORT.
> +*/
> + if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> + return -EINVAL;
> + else
> + return -EAFNOSUPPORT;
> }
>
> if (snum) {
> @@ -4589,16 +4596,27 @@ static int selinux_socket_connect_helper(struct
> socket *sock,
> * need to check address->sa_family as it is possible to have
> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
> */
> - if (address->sa_family == AF_INET) {
> + switch (address->sa_family) {
> + case AF_INET:
> addr4 = (struct sockaddr_in *)address;
> if (addrlen < sizeof(struct sockaddr_in))
> return -EINVAL;
> snum = ntohs(addr4->sin_port);
> - } else {
> + break;
> + case AF_INET6:
> addr6 = (struct sockaddr_in6 *)address;
> if (addrlen < SIN6_LEN_RFC2133)
> return -EINVAL;
> snum = ntohs(addr6->sin6_port);
> + break;
> + default:
> + /* Note that SCTP services expect -EINVAL, whereas
> +* others expect -EAFNOSUPPORT.
> +*/
> + if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> + return -EINVAL;
> + else
> + return -EAFNOSUPPORT;
> }
>
> err = sel_netport_sid(sk->sk_protocol, snum, );
> --
> 2.14.3
>
--
paul moore
www.paul-moore.com
Re: [RFC PATCH V1 01/12] audit: add container id
On Fri, Mar 2, 2018 at 2:25 PM, Paul Moore <p...@paul-moore.com> wrote: > On Fri, Mar 2, 2018 at 1:23 PM, Matthew Wilcox <wi...@infradead.org> wrote: >> On Fri, Mar 02, 2018 at 10:48:42AM -0500, Paul Moore wrote: >>> On Thu, Mar 1, 2018 at 8:41 PM, Richard Guy Briggs <r...@redhat.com> wrote: >>> > On 2018-03-01 14:41, Richard Guy Briggs wrote: >>> FYI, I think you may have a problem with something in your outgoing >>> mail path; I didn't receive the original patchset you are referencing >>> and it doesn't appear in the mail archive either. >> >> I have those patches. Which mail archive is missing them? > > The archive run by the linux-audit mailing list: > > * https://www.redhat.com/archives/linux-audit After having my reply get bounced from the linux-audit list I realized that Richard had gotten a little overzealous with the number of recipients (note to Richard, you easily could have dropped some of those lists/people from the To/CC line). I was able to get in and free those patches from the moderation queue, they should be arriving on the linux-audit list shortly. -- paul moore www.paul-moore.com
Re: [RFC PATCH V1 01/12] audit: add container id
On Fri, Mar 2, 2018 at 1:23 PM, Matthew Wilcox <wi...@infradead.org> wrote: > On Fri, Mar 02, 2018 at 10:48:42AM -0500, Paul Moore wrote: >> On Thu, Mar 1, 2018 at 8:41 PM, Richard Guy Briggs <r...@redhat.com> wrote: >> > On 2018-03-01 14:41, Richard Guy Briggs wrote: >> FYI, I think you may have a problem with something in your outgoing >> mail path; I didn't receive the original patchset you are referencing >> and it doesn't appear in the mail archive either. > > I have those patches. Which mail archive is missing them? The archive run by the linux-audit mailing list: * https://www.redhat.com/archives/linux-audit -- paul moore www.paul-moore.com
Re: [RFC PATCH V1 01/12] audit: add container id
On Thu, Mar 1, 2018 at 8:41 PM, Richard Guy Briggs <r...@redhat.com> wrote: > On 2018-03-01 14:41, Richard Guy Briggs wrote: >> Implement the proc fs write to set the audit container ID of a process, >> emitting an AUDIT_CONTAINER record to document the event. >> >> This is a write from the container orchestrator task to a proc entry of >> the form /proc/PID/containerid where PID is the process ID of the newly >> created task that is to become the first task in a container, or an >> additional task added to a container. >> >> The write expects up to a u64 value (unset: 18446744073709551615). >> >> This will produce a record such as this: >> type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 >> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 >> ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0 >> >> The "op" field indicates an initial set. The "pid" to "ses" fields are >> the orchestrator while the "opid" field is the object's PID, the process >> being "contained". Old and new container ID values are given in the >> "contid" fields, while res indicates its success. >> >> It is not permitted to self-set, unset or re-set the container ID. A >> child inherits its parent's container ID, but then can be set only once >> after. > > There are more restrictions coming later: > - check that the child being set has no children or threads yet, or > forcibly set them all to the same container ID (assuming they all pass > the same tests). This will also prevent an orch from setting its > parent and other tit-for-tat games to circumvent the basic checks. FYI, I think you may have a problem with something in your outgoing mail path; I didn't receive the original patchset you are referencing and it doesn't appear in the mail archive either. -- paul moore www.paul-moore.com
Re: Regression found when running LTP connect01 on next-20180301
On Thu, Mar 1, 2018 at 3:01 PM, Anders Roxell <anders.rox...@linaro.org> wrote: > On 1 March 2018 at 14:42, Paul Moore <p...@paul-moore.com> wrote: >> On Thu, Mar 1, 2018 at 3:33 AM, Anders Roxell <anders.rox...@linaro.org> >> wrote: >>> Hi, >>> >>> I was running LTP's testcase connect01 [1] and found a regression in >>> linux-next >>> (next-20180301). Bisect gave me this patch as the problematic patch (sha >>> d452930fd3b9 "selinux: Add SCTP support") on a x86 target. >>> >>> Output from the test(LTP release 20180118): >>> $ cd /opt/ltp/ >>> $ cat runtest/syscalls |grep connect01>runtest/connect-syscall >>> $ ./runltp -pq -f connect-syscall >>> " >>> Running tests... >>> connect011 TPASS : bad file descriptor successful >>> connect012 TPASS : invalid socket buffer successful >>> connect013 TPASS : invalid salen successful >>> connect014 TPASS : invalid socket successful >>> connect015 TPASS : already connected successful >>> connect016 TPASS : connection refused successful >>> connect017 TFAIL : connect01.c:146: invalid address family ; >>> returned -1 (expected -1), errno 22 (expected 97) >>> INFO: ltp-pan reported some tests FAIL >>> LTP Version: 20180118 >>> " >>> >>> The output from the test expected 97 and we received 22, can you please >>> elaborate on what have been changed? >>> >>> Cheers, >>> Anders >>> [1] >>> https://github.com/linux-test-project/ltp/blob/20180118/testcases/kernel/syscalls/connect/connect01.c#L146 >> >> Hi Anders, >> >> Thanks for the report. Out of curiosity, we're you running the full >> LTP test suite and this was the only failure, or did you just run the >> connect01 test? > > Normally we run all syscalls, but when we saw this regression I did the > bisect and only ran test connect01. > On every new push we ran 19 different sets of LTP tests, where > connect01 is part of the syscalls test set. So this means that only the connect01 test experienced failures? >> Either answer is fine, I'm just trying to understand >> the scope of the regression. >> >> Richard, are you able to look into this? If not, let me know and I'll >> dig a bit deeper (I'll likely take a quick look today, but if the >> failure is subtle it might require some digging). >> >> -- >> paul moore >> www.paul-moore.com -- paul moore www.paul-moore.com
Re: Regression found when running LTP connect01 on next-20180301
On March 1, 2018 9:36:37 AM Richard Haines <richard_c_hai...@btinternet.com> wrote: > On Thu, 2018-03-01 at 08:42 -0500, Paul Moore wrote: >> On Thu, Mar 1, 2018 at 3:33 AM, Anders Roxell <anders.roxell@linaro.o >> rg> wrote: >> > Hi, >> > >> > I was running LTP's testcase connect01 [1] and found a regression >> > in linux-next >> > (next-20180301). Bisect gave me this patch as the problematic >> > patch (sha >> > d452930fd3b9 "selinux: Add SCTP support") on a x86 target. >> > >> > Output from the test(LTP release 20180118): >> > $ cd /opt/ltp/ >> > $ cat runtest/syscalls |grep connect01>runtest/connect-syscall >> > $ ./runltp -pq -f connect-syscall >> > " >> > Running tests... >> > connect011 TPASS : bad file descriptor successful >> > connect012 TPASS : invalid socket buffer successful >> > connect013 TPASS : invalid salen successful >> > connect014 TPASS : invalid socket successful >> > connect015 TPASS : already connected successful >> > connect016 TPASS : connection refused successful >> > connect017 TFAIL : connect01.c:146: invalid address family ; >> > returned -1 (expected -1), errno 22 (expected 97) >> > INFO: ltp-pan reported some tests FAIL >> > LTP Version: 20180118 >> > " >> > >> > The output from the test expected 97 and we received 22, can you >> > please >> > elaborate on what have been changed? >> > >> > Cheers, >> > Anders >> > [1] https://github.com/linux-test-project/ltp/blob/20180118/testcas >> > es/kernel/syscalls/connect/connect01.c#L146 >> >> Hi Anders, >> >> Thanks for the report. Out of curiosity, we're you running the full >> LTP test suite and this was the only failure, or did you just run the >> connect01 test? Either answer is fine, I'm just trying to understand >> the scope of the regression. >> >> Richard, are you able to look into this? If not, let me know and >> I'll >> dig a bit deeper (I'll likely take a quick look today, but if the >> failure is subtle it might require some digging). > > I'll have a look today. One more thing I forgot to mention earlier, if there is a patch to fix this, could you please base it on top of the existing SELinux/SCTP patches that have already been merged, and not respin an earlier patch? Thank you. -- paul moore www.paul-moore.com
Re: Regression found when running LTP connect01 on next-20180301
On Thu, Mar 1, 2018 at 9:36 AM, Richard Haines <richard_c_hai...@btinternet.com> wrote: > On Thu, 2018-03-01 at 08:42 -0500, Paul Moore wrote: >> On Thu, Mar 1, 2018 at 3:33 AM, Anders Roxell <anders.roxell@linaro.o >> rg> wrote: >> > Hi, >> > >> > I was running LTP's testcase connect01 [1] and found a regression >> > in linux-next >> > (next-20180301). Bisect gave me this patch as the problematic >> > patch (sha >> > d452930fd3b9 "selinux: Add SCTP support") on a x86 target. >> > >> > Output from the test(LTP release 20180118): >> > $ cd /opt/ltp/ >> > $ cat runtest/syscalls |grep connect01>runtest/connect-syscall >> > $ ./runltp -pq -f connect-syscall >> > " >> > Running tests... >> > connect011 TPASS : bad file descriptor successful >> > connect012 TPASS : invalid socket buffer successful >> > connect013 TPASS : invalid salen successful >> > connect014 TPASS : invalid socket successful >> > connect015 TPASS : already connected successful >> > connect016 TPASS : connection refused successful >> > connect017 TFAIL : connect01.c:146: invalid address family ; >> > returned -1 (expected -1), errno 22 (expected 97) >> > INFO: ltp-pan reported some tests FAIL >> > LTP Version: 20180118 >> > " >> > >> > The output from the test expected 97 and we received 22, can you >> > please >> > elaborate on what have been changed? >> > >> > Cheers, >> > Anders >> > [1] https://github.com/linux-test-project/ltp/blob/20180118/testcas >> > es/kernel/syscalls/connect/connect01.c#L146 >> >> Hi Anders, >> >> Thanks for the report. Out of curiosity, we're you running the full >> LTP test suite and this was the only failure, or did you just run the >> connect01 test? Either answer is fine, I'm just trying to understand >> the scope of the regression. >> >> Richard, are you able to look into this? If not, let me know and >> I'll >> dig a bit deeper (I'll likely take a quick look today, but if the >> failure is subtle it might require some digging). > > I'll have a look today. Thanks. Let me know if you get stuck. -- paul moore www.paul-moore.com
Re: Regression found when running LTP connect01 on next-20180301
On Thu, Mar 1, 2018 at 3:33 AM, Anders Roxell <anders.rox...@linaro.org> wrote: > Hi, > > I was running LTP's testcase connect01 [1] and found a regression in > linux-next > (next-20180301). Bisect gave me this patch as the problematic patch (sha > d452930fd3b9 "selinux: Add SCTP support") on a x86 target. > > Output from the test(LTP release 20180118): > $ cd /opt/ltp/ > $ cat runtest/syscalls |grep connect01>runtest/connect-syscall > $ ./runltp -pq -f connect-syscall > " > Running tests... > connect011 TPASS : bad file descriptor successful > connect012 TPASS : invalid socket buffer successful > connect013 TPASS : invalid salen successful > connect014 TPASS : invalid socket successful > connect015 TPASS : already connected successful > connect016 TPASS : connection refused successful > connect017 TFAIL : connect01.c:146: invalid address family ; returned > -1 (expected -1), errno 22 (expected 97) > INFO: ltp-pan reported some tests FAIL > LTP Version: 20180118 > " > > The output from the test expected 97 and we received 22, can you please > elaborate on what have been changed? > > Cheers, > Anders > [1] > https://github.com/linux-test-project/ltp/blob/20180118/testcases/kernel/syscalls/connect/connect01.c#L146 Hi Anders, Thanks for the report. Out of curiosity, we're you running the full LTP test suite and this was the only failure, or did you just run the connect01 test? Either answer is fine, I'm just trying to understand the scope of the regression. Richard, are you able to look into this? If not, let me know and I'll dig a bit deeper (I'll likely take a quick look today, but if the failure is subtle it might require some digging). -- paul moore www.paul-moore.com
Re: [PATCH V8 2/4] sctp: Add ip option support
.sockaddr_len = sizeof(struct sockaddr_in),
> + .ip_options_len= sctp_v4_ip_options_len,
> #ifdef CONFIG_COMPAT
> .compat_setsockopt = compat_ip_setsockopt,
> .compat_getsockopt = compat_ip_getsockopt,
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index bf271f8..eb55c63 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3138,6 +3138,7 @@ static int sctp_setsockopt_mappedv4(struct sock *sk,
> char __user *optval, unsign
> static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval,
> unsigned int optlen)
> {
> struct sctp_sock *sp = sctp_sk(sk);
> + struct sctp_af *af = sp->pf->af;
> struct sctp_assoc_value params;
> struct sctp_association *asoc;
> int val;
> @@ -3162,7 +3163,8 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char
> __user *optval, unsigned
> if (val) {
> int min_len, max_len;
>
> - min_len = SCTP_DEFAULT_MINSEGMENT -
> sp->pf->af->net_header_len;
> + min_len = SCTP_DEFAULT_MINSEGMENT - af->net_header_len;
> + min_len -= af->ip_options_len(sk);
> min_len -= sizeof(struct sctphdr) +
>sizeof(struct sctp_data_chunk);
>
> @@ -3175,7 +3177,8 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char
> __user *optval, unsigned
> asoc = sctp_id2assoc(sk, params.assoc_id);
> if (asoc) {
> if (val == 0) {
> - val = asoc->pathmtu - sp->pf->af->net_header_len;
> + val = asoc->pathmtu - af->net_header_len;
> + val -= af->ip_options_len(sk);
> val -= sizeof(struct sctphdr) +
>sctp_datachk_len(>stream);
> }
> @@ -5087,9 +5090,11 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id,
> struct socket **sockp)
> sctp_copy_sock(sock->sk, sk, asoc);
>
> /* Make peeled-off sockets more like 1-1 accepted sockets.
> -* Set the daddr and initialize id to something more random
> +* Set the daddr and initialize id to something more random and also
> +* copy over any ip options.
> */
> sp->pf->to_sk_daddr(>peer.primary_addr, sk);
> + sp->pf->copy_ip_options(sk, sock->sk);
>
> /* Populate the fields of the newsk from the oldsk and migrate the
> * asoc to the newsk.
> --
> 2.14.3
>
--
paul moore
www.paul-moore.com
Re: [PATCH V7 2/4] sctp: Add ip option support
On Thu, Feb 22, 2018 at 9:40 PM, Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> wrote: > On Thu, Feb 22, 2018 at 06:08:05PM -0500, Paul Moore wrote: >> On Wed, Feb 21, 2018 at 3:45 PM, Paul Moore <p...@paul-moore.com> wrote: >> > On February 21, 2018 9:33:51 AM Marcelo Ricardo Leitner >> > <marcelo.leit...@gmail.com> wrote: >> >> On Tue, Feb 20, 2018 at 07:15:27PM +, Richard Haines wrote: >> >>> Add ip option support to allow LSM security modules to utilise CIPSO/IPv4 >> >>> and CALIPSO/IPv6 services. >> >>> >> >>> Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com> >> >> >> >> LGTM too, thanks! >> >> >> >> Acked-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> >> > >> > I agree, thanks everyone for all the work, review, and patience behind >> > this patchset! I'll work on merging this into selinux/next and I'll send >> > a note when it's done. >> >> I just merged the four patches (1,3,4 from the v6 patchset, 2 from the >> v7 patchset) in selinux/next and did a quick sanity test on the kernel >> (booted, no basic SELinux regressions). Additional testing help is >> always appreciated ... > > I'll try it early next week. > > Any ideas on when this is going to appear on Dave's net-next tree? > We have a lot of SCTP changes to be posted on this cycle and would be > nice if we could avoid merge conflicts. It's merged into the SELinux tree, next branch; see the links below. Last I checked DaveM doesn't pull the selinux/next into his net-next tree (that would be a little funny for historical reasons). Any idea on how bad the merge conflicts are? >> * git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git >> * https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git -- paul moore www.paul-moore.com
Re: [PATCH V7 2/4] sctp: Add ip option support
On Wed, Feb 21, 2018 at 3:45 PM, Paul Moore <p...@paul-moore.com> wrote: > On February 21, 2018 9:33:51 AM Marcelo Ricardo Leitner > <marcelo.leit...@gmail.com> wrote: >> On Tue, Feb 20, 2018 at 07:15:27PM +, Richard Haines wrote: >>> Add ip option support to allow LSM security modules to utilise CIPSO/IPv4 >>> and CALIPSO/IPv6 services. >>> >>> Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com> >> >> LGTM too, thanks! >> >> Acked-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> > > I agree, thanks everyone for all the work, review, and patience behind this > patchset! I'll work on merging this into selinux/next and I'll send a note > when it's done. I just merged the four patches (1,3,4 from the v6 patchset, 2 from the v7 patchset) in selinux/next and did a quick sanity test on the kernel (booted, no basic SELinux regressions). Additional testing help is always appreciated ... * git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git * https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git -- paul moore www.paul-moore.com
Re: [PATCH V7 2/4] sctp: Add ip option support
On February 21, 2018 9:33:51 AM Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> wrote: > On Tue, Feb 20, 2018 at 07:15:27PM +, Richard Haines wrote: >> Add ip option support to allow LSM security modules to utilise CIPSO/IPv4 >> and CALIPSO/IPv6 services. >> >> Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com> > > LGTM too, thanks! > > Acked-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> I agree, thanks everyone for all the work, review, and patience behind this patchset! I'll work on merging this into selinux/next and I'll send a note when it's done. -- paul moore www.paul-moore.com
Re: [PATCH V6 0/4] Add SELinux SCTP protocol support
On Tue, Feb 13, 2018 at 3:52 PM, Richard Haines <richard_c_hai...@btinternet.com> wrote: > These patches have been built on Fedora 27 with kernel-4.16.0-0.rc1 plus > the following userspace patches to enable testing: > > 1) Updates to libsepol 2.7 to support the sctp portcon statement. >The patch is available from: > http://arctic.selinuxproject.org/~rhaines/selinux-sctp/ > selinux-Add-support-for-the-SCTP-portcon-keyword.patch > > 2) Updates to the SELinux Test Suite adding SCTP tests. Please read the >selinux-testsuite/README.sctp for details. The patch is available from: > http://arctic.selinuxproject.org/~rhaines/selinux-sctp/ > selinux-testsuite-Add-SCTP-test-support.patch > > 3) Updates to lksctp-tools that show SELinux info in sctp_darn and >sctp_test. It also contains a minor patch for test_1_to_1_connect.c >as when CIPSO/CALIPSO configured, NetLabel returns a different error >code for illegal addresses in test 5. The patch is available from: > http://arctic.selinuxproject.org/~rhaines/selinux-sctp/ > lksctp-tools-Add-SELinux-support-to-sctp_test-and-sc.patch > > All SCTP lksctp-tools/src/func_tests run correctly in enforcing mode. > > All SCTP regression tests "./sctp-tests run" run correctly in enforcing > mode. These tests are obtained from: https://github.com/sctp/sctp-tests > > The selinux-testsuite patch also adds remote tests (that need some manual > configuration). These are useful for testing CIPSO/CALIPSO over a network > with a number of categories to produce large ip option fields with various > message sizes forcing fragmentation etc.. > > Changes since RFC Patch: > Removed the NetLabel patch (was [RFC PATCH 4/5] netlabel: Add SCTP support) > as re-engineered. However this patchset will require the NetLabel > patch at [1] to fully run the SCTP selinux-testsuite. > > V1 Changes: > PATCH 1/4 > Remove unused parameter from security_sctp_assoc_request(). > Reformat and update LSM-sctp.rst documentation. > PATCH 2/4 > Add variables and RCU locks as requested in [2] to support IP options. > PATCH 3/4 > Added security_sctp_assoc_request() hook to sctp_sf_do_unexpected_init() > and sctp_sf_do_5_2_4_dupcook(). > Removed security_sctp_assoc_request() hook from sctp_sf_do_5_1C_ack() as > no longer required. > PATCH 4/4 > Reformat and update SELinux-sctp.rst documentation. > Remove bindx and connectx permissions. > Rework selinux_socket_connect() and selinux_netlbl_socket_connect() to > utilise helpers for code reuse. > Add spinlock to selinux_sctp_assoc_request(). > Remove unused parameter from security_sctp_assoc_request(). > Use address->sa_family == AF_INET in *_bind and *_connect to ensure > correct address type. > Minor cleanups. > > V2 Changes: > PATCH 4/4 - Remove spin lock from selinux_sctp_assoc_request() > PATCH 4/4 - Fix selinux_sctp_sk_clone() kbuild test robot catch [3] > > V3 Changes: > PATCH 2/4 - Account for IP options length in sctp.h sctp_frag_point() by > Marcelo > > V4 Changes: > PATCH 1/4 - Move specific SELinux descriptions from LSM-sctp.rst and > lsm_hooks.h to SELinux-sctp.rst in PATCH 4/4 > PATCH 4/4 - Rename selinux_netlbl_sctp_socket_connect() to > selinux_netlbl_socket_connect_locked() and move description comments to > selinux_sctp_bind_connect() > > V5 Change: Rework selinux_netlbl_socket_connect() and > selinux_netlbl_socket_connect_locked as requested by Paul. > > V6 Changes: > Rework SCTP patches 2/4 and 3/4 as there have been major SCTP updates since > kernel 4.14. > > [1] https://marc.info/?l=selinux=151061619115945=2 > [2] https://marc.info/?l=selinux=150962470215797=2 > [3] https://marc.info/?l=selinux=151198281817779=2 > > Richard Haines (4): > security: Add support for SCTP security hooks > sctp: Add ip option support > sctp: Add LSM hooks > selinux: Add SCTP support Marcelo, or any other SCTP folks, do the SCTP changes still look okay to you? I'd like to merge these into the selinux/next tree by the end of the week ... -- paul moore www.paul-moore.com
Re: [PATCH] netlabel: If PF_INET6, check sk_buff ip header version
On Mon, Nov 13, 2017 at 5:13 PM, Paul Moore <p...@paul-moore.com> wrote:
> On Mon, Nov 13, 2017 at 3:54 PM, Richard Haines
> <richard_c_hai...@btinternet.com> wrote:
>> When resolving a fallback label, check the sk_buff version as it
>> is possible (e.g. SCTP) to have family = PF_INET6 while
>> receiving ip_hdr(skb)->version = 4.
>>
>> Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com>
>> ---
>> net/netlabel/netlabel_unlabeled.c | 10 ++
>> 1 file changed, 10 insertions(+)
>
> Thanks Richard.
>
> Acked-by: Paul Moore <p...@paul-moore.com>
I don't believe the netdev folks picked this up, but I haven't heard
any objections (and I can't imagine there would be any) so I'm going
to go ahead and pull this into the selinux/next tree.
>> diff --git a/net/netlabel/netlabel_unlabeled.c
>> b/net/netlabel/netlabel_unlabeled.c
>> index 22dc1b9..c070dfc 100644
>> --- a/net/netlabel/netlabel_unlabeled.c
>> +++ b/net/netlabel/netlabel_unlabeled.c
>> @@ -1472,6 +1472,16 @@ int netlbl_unlabel_getattr(const struct sk_buff *skb,
>> iface = rcu_dereference(netlbl_unlhsh_def);
>> if (iface == NULL || !iface->valid)
>> goto unlabel_getattr_nolabel;
>> +
>> +#if IS_ENABLED(CONFIG_IPV6)
>> + /* When resolving a fallback label, check the sk_buff version as
>> +* it is possible (e.g. SCTP) to have family = PF_INET6 while
>> +* receiving ip_hdr(skb)->version = 4.
>> +*/
>> + if (family == PF_INET6 && ip_hdr(skb)->version == 4)
>> + family = PF_INET;
>> +#endif /* IPv6 */
>> +
>> switch (family) {
>> case PF_INET: {
>> struct iphdr *hdr4;
>> --
>> 2.13.6
--
paul moore
www.paul-moore.com
Re: [PATCH net-next 0/3] eBPF Seccomp filters
>>> program be able to read from them. > > This came up recently on the libseccomp mailing list. The map lookup > is faster than a linear search, but for large filters, the filter can > be written as a balanced tree (as Chrome does), or reordered by > syscall frequency (as is recommended by minijail), and that appears to > get a much larger improvement than even the map lookup. For reference, the current libseccomp approach is to put the shorter rules near the top of the filter (e.g. syscall only) with the longer rules (e.g. syscall + arguments) towards the end. The libseccomp API does allow for callers to influence the ordering via syscall priority hints. Someone is currently looking a tree-based ordering of syscalls for libseccomp, and I'm always open to new/better ideas. -- paul moore security @ redhat
Re: RFC(V3): Audit Kernel Container IDs
On Fri, Feb 2, 2018 at 5:19 PM, Simo Sorce <s...@redhat.com> wrote: > On Fri, 2018-02-02 at 16:24 -0500, Paul Moore wrote: >> On Wed, Jan 10, 2018 at 2:00 AM, Richard Guy Briggs <r...@redhat.com> wrote: >> > On 2018-01-09 11:18, Simo Sorce wrote: >> > > On Tue, 2018-01-09 at 07:16 -0500, Richard Guy Briggs wrote: ... >> > Paul, can you justify this somewhat larger inconvenience for some >> > relatively minor convenience on our part? >> >> Done in direct response to Simo. > > Sorry but your response sounds more like waving away then addressing > them, the excuse being: we can't please everyone, so we are going to > please no one. I obviously disagree with the take on my comments but you're free to your opinion. I believe saying we are pleasing no one isn't really fair now is it? Is there any type of audit container ID now? How would you go about associating audit events with containers now? (spoiler alert: it ain't pretty, and there are gaps I don't believe you can cover) This proposal provides a mechanism to do this in a way that isn't tied to any one particular concept of a container and is manageable inside the kernel. If you have a need to track audit events for containers, I find it extremely hard to believe that you are not at least partially pleased by the solutions presented here. It may not be everything on your wishlist, but when did you ever get *everything* on your wishlist? >> But to be clear Richard, we've talked about this a few times, it's not >> a "minor convenience" on our part, it's a pretty big convenience once >> we starting having to route audit events and make decisions based on >> the audit container ID information. Audit performance is less than >> awesome now, I'm working hard to not make it worse. > > Sounds like a security vs performance trade off to me. Welcome to software development. It's generally a pretty terrible hobby and/or occupation, but we make up for it with long hours and endless frustration. >> > u64 vs u128 is easy for us to >> > accomodate in terms of scalar comparisons. It doubles the information >> > in every container id field we print in audit records. >> >> ... and slows down audit container ID checks. > > Are you saying a cmp on a u128 is slower than a comparison on a u64 and > this is something that will be noticeable ? Do you have a 128 bit system? I don't. I've got a bunch of 64 bit systems, and a couple of 32 bit systems too. People that use audit have a tendency to really hammer on it, to the point that we get performance complaints on a not infrequent basis. I don't know the exact number of times we are going to need to check the audit container ID, but it's reasonable to think that we'll expose it as a filter-able field which adds a few checks, we'll use it for record routing so that's a few more, and if we're running multiple audit daemons we will probably want to include LSM checks which could result in a few more audit container ID checks. If it was one comparison I wouldn't be too worried about it, but the point I'm trying to make is that we don't know what the implementation is going to look like yet and I suspect this ID is going to be leveraged in several places in the audit subsystem and I would much rather start small to save headaches later. We can always expand the ID to a larger integer at a later date, but we can't make it smaller. >> > A c36 is a bigger step. >> >> Yeah, we're not doing that, no way. > > Ok, I can see your point though I do not agree with it. > > I can see why you do not want to have arbitrary length strings, but a > u128 sounded like a reasonable compromise to me as it has enough room > to be able to have unique cluster-wide IDs which a u64 definitely makes > a lot harder to provide w/o tight coordination. I originally wanted it to be a 32-bit integer, but Richard managed to talk me into 64-bits, that was my compromise :) As I said earlier, if you are doing container auditing you're going to need coordination with the orchestrator, regardless of the audit container ID size. -- paul moore www.paul-moore.com
Re: RFC(V3): Audit Kernel Container IDs
On Tue, Jan 9, 2018 at 7:16 AM, Richard Guy Briggs <r...@redhat.com> wrote: > Containers are a userspace concept. The kernel knows nothing of them. > > The Linux audit system needs a way to be able to track the container > provenance of events and actions. Audit needs the kernel's help to do > this. Two small comments below, but I tend to think we are at a point where you can start cobbling together some prototype/RFC patches. Surely there are going to be a few changes, and new comments, that come out once we see an initial implementation so let's see what those are. > The registration is a u64 representing the audit container identifier > written to a special file in a pseudo filesystem (proc, since PID tree > already exists) representing a process that will become a parent process > in that container. This write might place restrictions on mount > namespaces required to define a container, or at least careful checking > of namespaces in the kernel to verify permissions of the orchestrator so > it can't change its own container ID. A bind mount of nsfs may be > necessary in the container orchestrator's mount namespace. This write > can only happen once per process. > > Note: The justification for using a u64 is that it minimizes the > information printed in every audit record, reducing bandwidth and limits > comparisons to a single u64 which will be faster and less error-prone. I know Steve generally worries about audit record size, which is a perfectly valid concern in this case, I also worry about the additional overhead when we start routing audit records to multiple audit daemons (see my other emails in this thread). > ... > When a container ceases to exist because the last process in that > container has exited log the fact to balance the registration action. > (This is likely needed for certification accountability.) On the "container ceases to exist" point, I expect this "container dead" message to come from the orchestrator and not the kernel itself (I don't want the kernel to have to handle that level of bookkeeping). I imagine this should be similar to what is done for VM auditing with libvirt. -- paul moore www.paul-moore.com
Re: RFC(V3): Audit Kernel Container IDs
ESTROY] [clone(2), unshare(2), setns(2)] >> > Note: At this point it appears only network namespaces may need to track >> > container IDs apart from processes since incoming packets may cause an >> > auditable event before being associated with a process. Since a >> > namespace can be shared by processes in different containers, the >> > namespace will need to track all containers to which it has been >> > assigned. >> > >> > Upon registration, the target process' namespace IDs (in the form of a >> > nsfs device number and inode number tuple) will be recorded in an >> > AUDIT_NS_INFO auxilliary record. >> > >> > Log the destruction of every namespace that is no longer used by any >> > process, including the namespace IDs (device and inode number tuples). >> > [AUDIT_NS_DESTROY] [process exit, unshare(2), setns(2)] >> > >> > Issue a new auxilliary record AUDIT_NS_CHANGE listing (opt: op=$action) >> > the parent and child namespace IDs for any changes to a process' >> > namespaces. [setns(2)] >> > Note: It may be possible to combine AUDIT_NS_* record formats and >> > distinguish them with an op=$action field depending on the fields >> > required for each message type. >> > >> > The audit container identifier will need to be reaped from all >> > implicated namespaces upon the destruction of a container. >> > >> > This namespace information adds supporting information for tracking >> > events not attributable to specific processes. >> > >> > Changelog: >> > >> > (Upstream V3) >> > - switch back to u64 (from pmoore, can be expanded to u128 in future if >> > need arises without breaking API. u32 was originally proposed, up to >> > c36 discussed) >> > - write-once, but children inherit audit container identifier and can >> > then still be written once >> > - switch to CAP_AUDIT_CONTROL >> > - group namespace actions together, auxilliary records to namespace >> > operations. >> > >> > (Upstream V2) >> > - switch from u64 to u128 UUID >> > - switch from "signal" and "trigger" to "register" >> > - restrict registration to single process or force all threads and >> > children into same container >> >> I am trying to understand the back and forth on the ID size. >> >> From an orchestrator POV anything that requires tracking a node >> specific ID is not ideal. >> >> Orchestrators tend to span many nodes, and containers tend to have IDs >> that are either UUID or have a Hash (like SHA256) as identifier. >> >> The problem here is two-fold: >> >> a) Your auditing requires some mapping to be useful outside of the >> system. >> If you aggreggate audit logs outside of the system or you want to >> correlate the system audit logs with other components dealing with >> containers, now you need a place where you provide a mapping from your >> audit u64 to the ID a container has in the rest of the system. >> >> b) Now you need a mapping of some sort. The simplest way a container >> orchestrator can go about this is to just use the UUID or Hash >> representing their view of the container, truncate it to a u64 and use >> that for Audit. This means there are some chances there will be a >> collision and a duplicate u64 ID will be used by the orchestrator as >> the container ID. What happen in that case ? > > Paul, can you justify this somewhat larger inconvenience for some > relatively minor convenience on our part? Done in direct response to Simo. But to be clear Richard, we've talked about this a few times, it's not a "minor convenience" on our part, it's a pretty big convenience once we starting having to route audit events and make decisions based on the audit container ID information. Audit performance is less than awesome now, I'm working hard to not make it worse. > u64 vs u128 is easy for us to > accomodate in terms of scalar comparisons. It doubles the information > in every container id field we print in audit records. ... and slows down audit container ID checks. > A c36 is a bigger step. Yeah, we're not doing that, no way. -- paul moore www.paul-moore.com
Re: RFC(V3): Audit Kernel Container IDs
On Tue, Jan 9, 2018 at 11:18 AM, Simo Sorce <s...@redhat.com> wrote: > On Tue, 2018-01-09 at 07:16 -0500, Richard Guy Briggs wrote: ... >> Changelog: >> >> (Upstream V3) >> - switch back to u64 (from pmoore, can be expanded to u128 in future if >> need arises without breaking API. u32 was originally proposed, up to >> c36 discussed) >> - write-once, but children inherit audit container identifier and can >> then still be written once >> - switch to CAP_AUDIT_CONTROL >> - group namespace actions together, auxilliary records to namespace >> operations. >> >> (Upstream V2) >> - switch from u64 to u128 UUID >> - switch from "signal" and "trigger" to "register" >> - restrict registration to single process or force all threads and >> children into same container > > I am trying to understand the back and forth on the ID size. I'm just now getting a chance to read Richard's latest draft, but I wanted to comment on this quickly. There are two main reasons for keeping this a 32 or 64 bit integer: 1) After the initial "be able to associate audit events with a container" stage, we are going to look into supporting multiple audit daemons on the system so that you could run an audit daemon inside a container and it would collect events generated by the container (we're tentatively calling this "phase 2", feel free to insert your own "magic happens" joke). There are a lot things that need to happen in phase two, one of these things is the addition of an audit event routing mechanism that will send audit records to the right audit daemons (the "host" daemon will always see everything), in order to do this we will need to be able to quickly compare audit container IDs, this means an integer. 2) Whatever we pick for an audit container ID it is going to be wrong for at least one container orchestrator. There is no "one" solution here, so we are providing a small and flexible mechanism that higher level orchestrators can use to provide a more complete solution. > >From an orchestrator POV anything that requires tracking a node > specific ID is not ideal. > > Orchestrators tend to span many nodes, and containers tend to have IDs > that are either UUID or have a Hash (like SHA256) as identifier. You're helping me prove my reason #2. > The problem here is two-fold: > > a) Your auditing requires some mapping to be useful outside of the > system. > If you aggreggate audit logs outside of the system or you want to > correlate the system audit logs with other components dealing with > containers, now you need a place where you provide a mapping from your > audit u64 to the ID a container has in the rest of the system. Yep, see my reason #2. I want us to have something that "works" for a single system as well as something that can be leveraged by higher level tools for large networks of machines. I realize it's easy, and tempting, to expand the scope of this effort; but if we are to have any success it is only going to be through some discipline. We need to focus on a small solution which addresses the basic needs and hopefully remains flexible enough for any potential expansion while staying palatable to the audit folks and the general kernel community. > b) Now you need a mapping of some sort. The simplest way a container > orchestrator can go about this is to just use the UUID or Hash > representing their view of the container, truncate it to a u64 and use > that for Audit. This means there are some chances there will be a > collision and a duplicate u64 ID will be used by the orchestrator as > the container ID. What happen in that case ? That is a design decision left to the different container orchestrators. -- paul moore www.paul-moore.com
Re: PATCH V5 4/4] selinux: Add SCTP support
ctp_sk_clone(struct sock *sk, struct sock *newsk)
> +{
> + struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *newsksec = newsk->sk_security;
> +
> + newsksec->nlbl_state = sksec->nlbl_state;
> +}
> +
> /**
> * selinux_netlbl_socket_post_create - Label a socket using NetLabel
> * @sock: the socket to label
> @@ -470,7 +542,8 @@ int selinux_netlbl_socket_setsockopt(struct socket *sock,
> }
>
> /**
> - * selinux_netlbl_socket_connect - Label a client-side socket on connect
> + * selinux_netlbl_socket_connect_helper - Help label a client-side socket on
> + * connect
> * @sk: the socket to label
> * @addr: the destination address
> *
> @@ -479,18 +552,13 @@ int selinux_netlbl_socket_setsockopt(struct socket
> *sock,
> * Returns zero values on success, negative values on failure.
> *
> */
> -int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
> +static int selinux_netlbl_socket_connect_helper(struct sock *sk,
> + struct sockaddr *addr)
> {
> int rc;
> struct sk_security_struct *sksec = sk->sk_security;
> struct netlbl_lsm_secattr *secattr;
>
> - if (sksec->nlbl_state != NLBL_REQSKB &&
> - sksec->nlbl_state != NLBL_CONNLABELED)
> - return 0;
> -
> - lock_sock(sk);
> -
> /* connected sockets are allowed to disconnect when the address family
> * is set to AF_UNSPEC, if that is what is happening we want to reset
> * the socket */
> @@ -498,18 +566,61 @@ int selinux_netlbl_socket_connect(struct sock *sk,
> struct sockaddr *addr)
> netlbl_sock_delattr(sk);
> sksec->nlbl_state = NLBL_REQSKB;
> rc = 0;
> - goto socket_connect_return;
> + return rc;
> }
> secattr = selinux_netlbl_sock_genattr(sk);
> if (secattr == NULL) {
> rc = -ENOMEM;
> - goto socket_connect_return;
> + return rc;
> }
> rc = netlbl_conn_setattr(sk, addr, secattr);
> if (rc == 0)
> sksec->nlbl_state = NLBL_CONNLABELED;
>
> -socket_connect_return:
> + return rc;
> +}
> +
> +/**
> + * selinux_netlbl_socket_connect_locked - Label a client-side socket on
> + * connect
> + * @sk: the socket to label
> + * @addr: the destination address
> + *
> + * Description:
> + * Attempt to label a connected socket that already has the socket locked
> + * with NetLabel using the given address.
> + * Returns zero values on success, negative values on failure.
> + *
> + */
> +int selinux_netlbl_socket_connect_locked(struct sock *sk,
> +struct sockaddr *addr)
> +{
> + struct sk_security_struct *sksec = sk->sk_security;
> +
> + if (sksec->nlbl_state != NLBL_REQSKB &&
> + sksec->nlbl_state != NLBL_CONNLABELED)
> + return 0;
> +
> + return selinux_netlbl_socket_connect_helper(sk, addr);
> +}
> +
> +/**
> + * selinux_netlbl_socket_connect - Label a client-side socket on connect
> + * @sk: the socket to label
> + * @addr: the destination address
> + *
> + * Description:
> + * Attempt to label a connected socket with NetLabel using the given address.
> + * Returns zero values on success, negative values on failure.
> + *
> + */
> +int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
> +{
> + int rc;
> +
> + lock_sock(sk);
> + rc = selinux_netlbl_socket_connect_locked(sk, addr);
> release_sock(sk);
> +
> return rc;
> }
> --
> 2.14.3
>
--
paul moore
www.paul-moore.com
Re: [PATCH V4 0/4] Add SELinux SCTP protocol support
On Wed, Jan 10, 2018 at 1:51 PM, Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> wrote: > On Wed, Jan 10, 2018 at 11:39:45AM -0500, Paul Moore wrote: >> On Sat, Dec 30, 2017 at 12:18 PM, Richard Haines >> <richard_c_hai...@btinternet.com> wrote: >> > Note: Some conflicts are expected when merging with current net-next due to >> > Interleaving Data (I-DATA) sets of patches: >> > PATCH 2/4 - Where 'sctp_datachk_len(>stream)' has replaced >> > 'sizeof(struct sctp_data_chunk)' in include/net/sctp/sctp.h, >> > net/sctp/chunk.c and net/sctp/socket.c >> > PATCH 3/4 - Where include/uapi/linux/sctp.h requires a fix to update the >> > #define SCTP_SENDMSG_CONNECT to a higher number. >> > >> > These patches have been built on Fedora 27 with kernel 4.14.8 plus >> > the following userspace patches to enable testing: >> > >> > 1) Updates to libsepol 2.7 to support the sctp portcon statement. >> >The patch is available from: >> > http://arctic.selinuxproject.org/~rhaines/selinux-sctp/ >> > selinux-Add-support-for-the-SCTP-portcon-keyword.patch >> > >> > 2) Updates to the SELinux Test Suite adding SCTP tests. Please read the >> >selinux-testsuite/README.sctp for details. The patch is available from: >> > http://arctic.selinuxproject.org/~rhaines/selinux-sctp/ >> > selinux-testsuite-Add-SCTP-test-support.patch >> > >> > 3) Updates to lksctp-tools that show SELinux info in sctp_darn and >> >sctp_test. It also contains a minor patch for test_1_to_1_connect.c >> >as when CIPSO/CALIPSO configured, NetLabel returns a different error >> >code for illegal addresses in test 5. The patch is available from: >> > http://arctic.selinuxproject.org/~rhaines/selinux-sctp/ >> > lksctp-tools-Add-SELinux-support-to-sctp_test-and-sc.patch >> > >> > All SCTP lksctp-tools/src/func_tests run correctly in enforcing mode. >> > >> > All SCTP regression tests "./sctp-tests run" run correctly in enforcing >> > mode. These tests are obtained from: https://github.com/sctp/sctp-tests >> > >> > The selinux-testsuite patch also adds remote tests (that need some manual >> > configuration). These are useful for testing CIPSO/CALIPSO over a network >> > with a number of categories to produce large ip option fields with various >> > message sizes forcing fragmentation etc.. >> > >> > Changes since RFC Patch: >> > Removed the NetLabel patch (was [RFC PATCH 4/5] netlabel: Add SCTP support) >> > as re-engineered. However this patchset will require the NetLabel >> > patch at [1] to fully run the SCTP selinux-testsuite. >> > >> > V1 Changes: >> > PATCH 1/4 >> > Remove unused parameter from security_sctp_assoc_request(). >> > Reformat and update LSM-sctp.rst documentation. >> > PATCH 2/4 >> > Add variables and RCU locks as requested in [2] to support IP options. >> > PATCH 3/4 >> > Added security_sctp_assoc_request() hook to sctp_sf_do_unexpected_init() >> > and sctp_sf_do_5_2_4_dupcook(). >> > Removed security_sctp_assoc_request() hook from sctp_sf_do_5_1C_ack() as >> > no longer required. >> > PATCH 4/4 >> > Reformat and update SELinux-sctp.rst documentation. >> > Remove bindx and connectx permissions. >> > Rework selinux_socket_connect() and selinux_netlbl_socket_connect() to >> > utilise helpers for code reuse. >> > Add spinlock to selinux_sctp_assoc_request(). >> > Remove unused parameter from security_sctp_assoc_request(). >> > Use address->sa_family == AF_INET in *_bind and *_connect to ensure >> > correct address type. >> > Minor cleanups. >> > >> > V2 Changes: >> > PATCH 4/4 - Remove spin lock from selinux_sctp_assoc_request() >> > PATCH 4/4 - Fix selinux_sctp_sk_clone() kbuild test robot catch [3] >> > >> > V3 Changes: >> > PATCH 2/4 - Account for IP options length in sctp.h sctp_frag_point() by >> > Marcelo >> > >> > V4 Changes: >> > PATCH 1/4 - Move specific SELinux descriptions from LSM-sctp.rst and >> > lsm_hooks.h to SELinux-sctp.rst in PATCH 4/4 >> > PATCH 4/4 - Rename selinux_netlbl_sctp_socket_connect() to >> > selinux_netlbl_socket_connect_locked() and move description comments to >> > selinux_sctp_bind_connect() >> > >> > [1] https://marc.info/?l=selinux=151061619115945=2 >> > [2] https://marc.info/?l=selinux=150962470215797=2 >> > [3] https://marc.info/?l=selinux=151198281817779=2 >> >> SCTP folks, any objections? I'm planning on merging these into >> selinux-next after the next merge window so if you want to see any >> changes, please speak up ... > > No objections from my side. I figured not :) Thanks again for all the review/feedback. -- paul moore www.paul-moore.com
Re: [PATCH V4 0/4] Add SELinux SCTP protocol support
On Sat, Dec 30, 2017 at 12:18 PM, Richard Haines <richard_c_hai...@btinternet.com> wrote: > Note: Some conflicts are expected when merging with current net-next due to > Interleaving Data (I-DATA) sets of patches: > PATCH 2/4 - Where 'sctp_datachk_len(>stream)' has replaced > 'sizeof(struct sctp_data_chunk)' in include/net/sctp/sctp.h, > net/sctp/chunk.c and net/sctp/socket.c > PATCH 3/4 - Where include/uapi/linux/sctp.h requires a fix to update the > #define SCTP_SENDMSG_CONNECT to a higher number. > > These patches have been built on Fedora 27 with kernel 4.14.8 plus > the following userspace patches to enable testing: > > 1) Updates to libsepol 2.7 to support the sctp portcon statement. >The patch is available from: > http://arctic.selinuxproject.org/~rhaines/selinux-sctp/ > selinux-Add-support-for-the-SCTP-portcon-keyword.patch > > 2) Updates to the SELinux Test Suite adding SCTP tests. Please read the >selinux-testsuite/README.sctp for details. The patch is available from: > http://arctic.selinuxproject.org/~rhaines/selinux-sctp/ > selinux-testsuite-Add-SCTP-test-support.patch > > 3) Updates to lksctp-tools that show SELinux info in sctp_darn and >sctp_test. It also contains a minor patch for test_1_to_1_connect.c >as when CIPSO/CALIPSO configured, NetLabel returns a different error >code for illegal addresses in test 5. The patch is available from: > http://arctic.selinuxproject.org/~rhaines/selinux-sctp/ > lksctp-tools-Add-SELinux-support-to-sctp_test-and-sc.patch > > All SCTP lksctp-tools/src/func_tests run correctly in enforcing mode. > > All SCTP regression tests "./sctp-tests run" run correctly in enforcing > mode. These tests are obtained from: https://github.com/sctp/sctp-tests > > The selinux-testsuite patch also adds remote tests (that need some manual > configuration). These are useful for testing CIPSO/CALIPSO over a network > with a number of categories to produce large ip option fields with various > message sizes forcing fragmentation etc.. > > Changes since RFC Patch: > Removed the NetLabel patch (was [RFC PATCH 4/5] netlabel: Add SCTP support) > as re-engineered. However this patchset will require the NetLabel > patch at [1] to fully run the SCTP selinux-testsuite. > > V1 Changes: > PATCH 1/4 > Remove unused parameter from security_sctp_assoc_request(). > Reformat and update LSM-sctp.rst documentation. > PATCH 2/4 > Add variables and RCU locks as requested in [2] to support IP options. > PATCH 3/4 > Added security_sctp_assoc_request() hook to sctp_sf_do_unexpected_init() > and sctp_sf_do_5_2_4_dupcook(). > Removed security_sctp_assoc_request() hook from sctp_sf_do_5_1C_ack() as > no longer required. > PATCH 4/4 > Reformat and update SELinux-sctp.rst documentation. > Remove bindx and connectx permissions. > Rework selinux_socket_connect() and selinux_netlbl_socket_connect() to > utilise helpers for code reuse. > Add spinlock to selinux_sctp_assoc_request(). > Remove unused parameter from security_sctp_assoc_request(). > Use address->sa_family == AF_INET in *_bind and *_connect to ensure > correct address type. > Minor cleanups. > > V2 Changes: > PATCH 4/4 - Remove spin lock from selinux_sctp_assoc_request() > PATCH 4/4 - Fix selinux_sctp_sk_clone() kbuild test robot catch [3] > > V3 Changes: > PATCH 2/4 - Account for IP options length in sctp.h sctp_frag_point() by > Marcelo > > V4 Changes: > PATCH 1/4 - Move specific SELinux descriptions from LSM-sctp.rst and > lsm_hooks.h to SELinux-sctp.rst in PATCH 4/4 > PATCH 4/4 - Rename selinux_netlbl_sctp_socket_connect() to > selinux_netlbl_socket_connect_locked() and move description comments to > selinux_sctp_bind_connect() > > [1] https://marc.info/?l=selinux=151061619115945=2 > [2] https://marc.info/?l=selinux=150962470215797=2 > [3] https://marc.info/?l=selinux=151198281817779=2 SCTP folks, any objections? I'm planning on merging these into selinux-next after the next merge window so if you want to see any changes, please speak up ... -- paul moore www.paul-moore.com
Re: [PATCH V4 4/4] selinux: Add SCTP support
On Sat, Dec 30, 2017 at 12:20 PM, Richard Haines
<richard_c_hai...@btinternet.com> wrote:
> The SELinux SCTP implementation is explained in:
> Documentation/security/SELinux-sctp.rst
>
> Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com>
> ---
> Documentation/security/SELinux-sctp.rst | 157 ++
> security/selinux/hooks.c| 280
> +---
> security/selinux/include/classmap.h | 2 +-
> security/selinux/include/netlabel.h | 21 ++-
> security/selinux/include/objsec.h | 4 +
> security/selinux/netlabel.c | 138 ++--
> 6 files changed, 570 insertions(+), 32 deletions(-)
> create mode 100644 Documentation/security/SELinux-sctp.rst
...
> +/**
> + * selinux_netlbl_socket_connect - Label a client-side socket on connect
> + * @sk: the socket to label
> + * @addr: the destination address
> + *
> + * Description:
> + * Attempt to label a connected socket with NetLabel using the given address.
> + * Returns zero values on success, negative values on failure.
> + *
> + */
> +int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
> +{
> + int rc;
> + struct sk_security_struct *sksec = sk->sk_security;
> +
> + if (sksec->nlbl_state != NLBL_REQSKB &&
> + sksec->nlbl_state != NLBL_CONNLABELED)
> + return 0;
> +
> + lock_sock(sk);
> + rc = selinux_netlbl_socket_connect_helper(sk, addr);
> release_sock(sk);
> +
> return rc;
> }
> +
> +/**
> + * selinux_netlbl_socket_connect_locked - Label a client-side socket on
> + * connect
> + * @sk: the socket to label
> + * @addr: the destination address
> + *
> + * Description:
> + * Attempt to label a connected socket that already has the socket locked
> + * with NetLabel using the given address.
> + * Returns zero values on success, negative values on failure.
> + *
> + */
> +int selinux_netlbl_socket_connect_locked(struct sock *sk,
> +struct sockaddr *addr)
> +{
> + struct sk_security_struct *sksec = sk->sk_security;
> +
> + if (sksec->nlbl_state != NLBL_REQSKB &&
> + sksec->nlbl_state != NLBL_CONNLABELED)
> + return 0;
> +
> + return selinux_netlbl_socket_connect_helper(sk, addr);
> +}
[Sorry for the review delay, the holidays and some associated travel
made it hard to find some quiet time to look at the latest patches.]
I probably should have been a bit more clear in my last comment, but
what I had in mind was something like the following:
int selinux_netlbl_socket_connect_locked(...)
{
if (sksec->nlbl_state ...)
return 0;
return selinux_netlbl_socket_connect_helper();
}
int selinux_netlbl_socket_connect(...)
{
int rc;
lock_sock();
rc = selinux_netlbl_socket_connect_locked();
release_sock();
return rc;
}
Yes, you do end up checking nlbl_state while the socket lock is held,
but I believe the benefit of consolidating the code outweighs any
additional overhead (I believe it would be "noise" anyway).
Otherwise, this all looks good to me.
--
paul moore
www.paul-moore.com
Re: [PATCH v3 1/4] security: Add support for SCTP security hooks
On Wed, Dec 27, 2017 at 11:22 AM, Richard Haines <richard_c_hai...@btinternet.com> wrote: > On Fri, 2017-12-22 at 15:45 -0200, Marcelo Ricardo Leitner wrote: >> On Fri, Dec 22, 2017 at 09:20:45AM -0800, Casey Schaufler wrote: >> > On 12/22/2017 5:05 AM, Marcelo Ricardo Leitner wrote: >> > > From: Richard Haines <richard_c_hai...@btinternet.com> >> > > >> > > The SCTP security hooks are explained in: >> > > Documentation/security/LSM-sctp.rst >> >> Thanks Casey for your comments. However, I'm not that acquainted with >> these area of codes and I cannot work on them. I'll just wait for >> Richard then. > > I'm back online and will post a V4 set of patches within a week. These > will address Paul's comments as per [1] and Casey's regarding the > documentation. > Sorry for the delay No worries, thanks. -- paul moore www.paul-moore.com
Re: [trivial PATCH] treewide: Align function definition open/close braces
On Sun, Dec 17, 2017 at 7:28 PM, Joe Perches <j...@perches.com> wrote: > Some functions definitions have either the initial open brace and/or > the closing brace outside of column 1. > > Move those braces to column 1. > > This allows various function analyzers like gnu complexity to work > properly for these modified functions. > > Miscellanea: > > o Remove extra trailing ; and blank line from xfs_agf_verify > > Signed-off-by: Joe Perches <j...@perches.com> > --- > git diff -w shows no difference other than the above 'Miscellanea' > > (this is against -next, but it applies against Linus' tree > with a couple offsets) > > arch/x86/include/asm/atomic64_32.h | 2 +- > drivers/acpi/custom_method.c | 2 +- > drivers/acpi/fan.c | 2 +- > drivers/gpu/drm/amd/display/dc/core/dc.c | 2 +- > drivers/media/i2c/msp3400-kthreads.c | 2 +- > drivers/message/fusion/mptsas.c | 2 +- > drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c | 2 +- > drivers/net/wireless/ath/ath9k/xmit.c| 2 +- > drivers/platform/x86/eeepc-laptop.c | 2 +- > drivers/rtc/rtc-ab-b5ze-s3.c | 2 +- > drivers/scsi/dpt_i2o.c | 2 +- > drivers/scsi/sym53c8xx_2/sym_glue.c | 2 +- > fs/locks.c | 2 +- > fs/ocfs2/stack_user.c| 2 +- > fs/xfs/libxfs/xfs_alloc.c| 5 ++--- > fs/xfs/xfs_export.c | 2 +- > kernel/audit.c | 6 +++--- > kernel/trace/trace_printk.c | 4 ++-- > lib/raid6/sse2.c | 14 +++--- > sound/soc/fsl/fsl_dma.c | 2 +- > 20 files changed, 30 insertions(+), 31 deletions(-) For the audit bits ... Acked-by: Paul Moore <p...@paul-moore.com> -- paul moore www.paul-moore.com
Re: [PATCH 2/4] sctp: Add ip option support
On December 14, 2017 4:04:28 PM Marcelo Ricardo Leitner
<marcelo.leit...@gmail.com> wrote:
> On Tue, Dec 12, 2017 at 05:24:46PM -0500, Paul Moore wrote:
>> On Tue, Dec 12, 2017 at 4:56 PM, Marcelo Ricardo Leitner
>> <marcelo.leit...@gmail.com> wrote:
>> > On Tue, Dec 12, 2017 at 04:33:03PM -0500, Paul Moore wrote:
>> >> On Tue, Dec 12, 2017 at 11:08 AM, Marcelo Ricardo Leitner
>> >> <marcelo.leit...@gmail.com> wrote:
>> >> > Hi Richard,
>> >> >
>> >> > On Mon, Nov 27, 2017 at 07:31:21PM +, Richard Haines wrote:
>> >> > ...
>> >> >> --- a/net/sctp/socket.c
>> >> >> +++ b/net/sctp/socket.c
>> >> >> @@ -3123,8 +3123,10 @@ static int sctp_setsockopt_maxseg(struct sock
>> *sk, char __user *optval, unsigned
>> >> >>
>> >> >> if (asoc) {
>> >> >> if (val == 0) {
>> >> >> + struct sctp_af *af = sp->pf->af;
>> >> >> val = asoc->pathmtu;
>> >> >> - val -= sp->pf->af->net_header_len;
>> >> >> + val -= af->ip_options_len(asoc->base.sk);
>> >> >> + val -= af->net_header_len;
>> >> >> val -= sizeof(struct sctphdr) +
>> >> >> sizeof(struct sctp_data_chunk);
>> >> >> }
>> >> >
>> >> > Right below here there is a call to sctp_frag_point(). That function
>> >> > also needs this tweak.
>> >> >
>> >> > Yes, we should simplify all these calculations. I have a patch to use
>> >> > sctp_frag_point on where it is currently recalculating it on
>> >> > sctp_datamsg_from_user(), but probably should include other places as
>> >> > well.
>> >>
>> >> FYI: Richard let me know he is occupied with another project at the
>> >> moment and likely won't be able to do another respin until next week
>> >> at the earliest.
>> >
>> > Okay, thanks. I can do a follow-up patch if it helps.
>>
>> I'll leave that up to you, I think your comments are pretty
>> straightforward and should be easy for Richard to incorporate, and
>> there is a lot to be said for including the fix in the original patch,
>> but if you would prefer to send a separate patch I think that's fine
>> too.
>
> This patchset conflicts with the stream schedulers patchset (on
> sctp.h) and will also conflict with the stream interleaving patchsets
> from Xin (other files).
>
> I rebased the patchset upon current crypto tree but the last patchset
> on stream interleaving is still to be accepted via net-next.
>
> I'm unsure on how to proceed here. Please advise.
>
> Thanks,
> Marcelo
I still believe the right course of action is to merge this via the SELinux
tree (based on Linus' tree). Due to the nature of SELinux we often have to
deal with conflicts like this; I haven't seen the conflicts your talking
about (I'm currently traveling with limited access) but Linus should be
able to handle the trivial fixes, if it is more complex we can provide
guidance about how to resolve it.
--
paul moore
www.paul-moore.com
Re: [PATCH 2/4] sctp: Add ip option support
On Tue, Dec 12, 2017 at 4:56 PM, Marcelo Ricardo Leitner
<marcelo.leit...@gmail.com> wrote:
> On Tue, Dec 12, 2017 at 04:33:03PM -0500, Paul Moore wrote:
>> On Tue, Dec 12, 2017 at 11:08 AM, Marcelo Ricardo Leitner
>> <marcelo.leit...@gmail.com> wrote:
>> > Hi Richard,
>> >
>> > On Mon, Nov 27, 2017 at 07:31:21PM +, Richard Haines wrote:
>> > ...
>> >> --- a/net/sctp/socket.c
>> >> +++ b/net/sctp/socket.c
>> >> @@ -3123,8 +3123,10 @@ static int sctp_setsockopt_maxseg(struct sock *sk,
>> >> char __user *optval, unsigned
>> >>
>> >> if (asoc) {
>> >> if (val == 0) {
>> >> + struct sctp_af *af = sp->pf->af;
>> >> val = asoc->pathmtu;
>> >> - val -= sp->pf->af->net_header_len;
>> >> + val -= af->ip_options_len(asoc->base.sk);
>> >> + val -= af->net_header_len;
>> >> val -= sizeof(struct sctphdr) +
>> >> sizeof(struct sctp_data_chunk);
>> >> }
>> >
>> > Right below here there is a call to sctp_frag_point(). That function
>> > also needs this tweak.
>> >
>> > Yes, we should simplify all these calculations. I have a patch to use
>> > sctp_frag_point on where it is currently recalculating it on
>> > sctp_datamsg_from_user(), but probably should include other places as
>> > well.
>>
>> FYI: Richard let me know he is occupied with another project at the
>> moment and likely won't be able to do another respin until next week
>> at the earliest.
>
> Okay, thanks. I can do a follow-up patch if it helps.
I'll leave that up to you, I think your comments are pretty
straightforward and should be easy for Richard to incorporate, and
there is a lot to be said for including the fix in the original patch,
but if you would prefer to send a separate patch I think that's fine
too.
--
paul moore
www.paul-moore.com
Re: [PATCH 2/4] sctp: Add ip option support
On Tue, Dec 12, 2017 at 11:08 AM, Marcelo Ricardo Leitner
<marcelo.leit...@gmail.com> wrote:
> Hi Richard,
>
> On Mon, Nov 27, 2017 at 07:31:21PM +, Richard Haines wrote:
> ...
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -3123,8 +3123,10 @@ static int sctp_setsockopt_maxseg(struct sock *sk,
>> char __user *optval, unsigned
>>
>> if (asoc) {
>> if (val == 0) {
>> + struct sctp_af *af = sp->pf->af;
>> val = asoc->pathmtu;
>> - val -= sp->pf->af->net_header_len;
>> + val -= af->ip_options_len(asoc->base.sk);
>> + val -= af->net_header_len;
>> val -= sizeof(struct sctphdr) +
>> sizeof(struct sctp_data_chunk);
>> }
>
> Right below here there is a call to sctp_frag_point(). That function
> also needs this tweak.
>
> Yes, we should simplify all these calculations. I have a patch to use
> sctp_frag_point on where it is currently recalculating it on
> sctp_datamsg_from_user(), but probably should include other places as
> well.
FYI: Richard let me know he is occupied with another project at the
moment and likely won't be able to do another respin until next week
at the earliest.
--
paul moore
www.paul-moore.com
Re: [PATCH V2] selinux: Add SCTP support
On Wed, Dec 6, 2017 at 5:17 AM, Richard Haines
<richard_c_hai...@btinternet.com> wrote:
> The SELinux SCTP implementation is explained in:
> Documentation/security/SELinux-sctp.rst
>
> Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com>
> ---
> V2 Changes
> Remove lock from selinux_sctp_assoc_request()
> Fix selinux_sctp_sk_clone() kbuild test robot catch [1]
>
> [1] https://marc.info/?l=selinux=151198281817779=2
>
> Documentation/security/SELinux-sctp.rst | 104
> security/selinux/hooks.c| 270
> +---
> security/selinux/include/classmap.h | 2 +-
> security/selinux/include/netlabel.h | 20 ++-
> security/selinux/include/objsec.h | 4 +
> security/selinux/netlabel.c | 144 +++--
> 6 files changed, 512 insertions(+), 32 deletions(-)
> create mode 100644 Documentation/security/SELinux-sctp.rst
I just went through these patches again, and I think they look pretty
good. There is one small cosmetic nit (see below), but that should be
a quick fix.
As Dave already said, let's give the SCTP folks some time to look
things over before I merge anything. If we haven't heard anything by
next week, I'll ping Vlad to see if I can get him to take a look.
> +/**
> + * selinux_netlbl_socket_connect - Label a client-side socket on connect
> + * @sk: the socket to label
> + * @addr: the destination address
> + *
> + * Description:
> + * Attempt to label a connected socket with NetLabel using the given address.
> + * Returns zero values on success, negative values on failure.
> + *
> + */
> +int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
> +{
> + int rc;
> + struct sk_security_struct *sksec = sk->sk_security;
> +
> + if (sksec->nlbl_state != NLBL_REQSKB &&
> + sksec->nlbl_state != NLBL_CONNLABELED)
> + return 0;
> +
> + lock_sock(sk);
> + rc = selinux_netlbl_socket_connect_helper(sk, addr);
> release_sock(sk);
> +
> + return rc;
> +}
> +
> +/**
> + * selinux_netlbl_sctp_socket_connect - Label an SCTP client-side socket on a
> + * connect
> + * @sk: the socket to label
> + * @addr: the destination address
> + *
> + * Description:
> + * Attempt to label a connected socket with NetLabel using the given address
> + * when called by the SCTP protocol layer. The situations handled are:
> + * sctp_connectx(3), sctp_sendmsg(3), sendmsg(2), whenever a new IP address
> + * is added or when a new primary address is selected. Note that an SCTP
> + * connect(2) call happens before the SCTP protocol layer and is handled via
> + * selinux_netlbl_socket_connect()
> + * Returns zero values on success, negative values on failure.
> + *
> + */
> +int selinux_netlbl_sctp_socket_connect(struct sock *sk, struct sockaddr
> *addr)
> +{
> + int rc;
> + struct sk_security_struct *sksec = sk->sk_security;
> +
> + if (sksec->nlbl_state != NLBL_REQSKB &&
> + sksec->nlbl_state != NLBL_CONNLABELED)
> + return 0;
> +
> + rc = selinux_netlbl_socket_connect_helper(sk, addr);
> +
> return rc;
> }
My apologies, I should have noticed this sooner ... If the only
difference between selinux_netlbl_socket_connect() and
selinux_netlbl_sctp_socket_connect() is that the SCTP variant takes a
lock, why not simply rename selinux_netlbl_sctp_socket_connect() to
selinux_netlbl_socket_connect_locked()? There is nothing really SCTP
specific here, aside from the comment header, which should already be
covered elsewhere.
[NOTE TO MYSELF: pick shorter function names next time, oof.]
--
paul moore
www.paul-moore.com
Re: [PATCH V2] selinux: Add SCTP support
On Wed, Dec 6, 2017 at 7:46 AM, Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> wrote: > On Wed, Dec 06, 2017 at 10:17:36AM +, Richard Haines wrote: >> The SELinux SCTP implementation is explained in: >> Documentation/security/SELinux-sctp.rst >> >> Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com> >> --- >> V2 Changes >> Remove lock from selinux_sctp_assoc_request() >> Fix selinux_sctp_sk_clone() kbuild test robot catch [1] >> >> [1] https://marc.info/?l=selinux=151198281817779=2 >> > > Is this patchset going in via netdev tree or selinux one? > I see Dave is tracking the v1 > (http://patchwork.ozlabs.org/patch/841842/) so I'm thinking netdev but > for netdev it would require a repost of the entire patchset, even if > the first ones weren't changed. This should go in via the SELinux tree, although I would like to see ACKs from Dave and/or other netdev folks for the core stack bits. -- paul moore www.paul-moore.com
Re: [BUG] kernel stack corruption during/after Netlabel error
On Thu, Nov 30, 2017 at 7:47 AM, Paul Moore <p...@paul-moore.com> wrote:
> On Thu, Nov 30, 2017 at 5:50 AM, Eric Dumazet <eric.duma...@gmail.com> wrote:
>> On Wed, 2017-11-29 at 19:16 -0800, Casey Schaufler wrote:
>>> On 11/29/2017 4:31 PM, James Morris wrote:
>>> > On Wed, 29 Nov 2017, Casey Schaufler wrote:
>>> >
>>> > > I see that there is a proposed fix later in the thread, but I
>>> > > don't see
>>> > > the patch. Could you send it to me, so I can try it on my
>>> > > problem?
>>> >
>>> > Forwarded off-list.
>>>
>>> The patch does fix the problem I was seeing in Smack.
>>
>> Can you guys test the following more complete patch ?
>>
>> It should cover IPv4 and IPv6, and also the corner cases.
>>
>> ( Note that I squashed ipv6 fix in https://patchwork.ozlabs.org/patch/8
>> 42844/ that I spotted while cooking this patch )
>
> Building a test kernel now, although it make take me a few hours to
> test it due to some commitments this morning.
I just realized I forgot to enable KASAN in the build, but I can
verify that the patch doesn't break anything in the selinux-testsuite.
Tested-by: Paul Moore <p...@paul-moore.com>
>> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
>> index
>> c6bc0c4d19c624888b0d0b5a4246c7183edf63f5..77ea45da0fe9c746907a312989658af3ad3b198d
>> 100644
>> --- a/net/ipv4/tcp_ipv4.c
>> +++ b/net/ipv4/tcp_ipv4.c
>> @@ -1591,6 +1591,34 @@ int tcp_filter(struct sock *sk, struct sk_buff *skb)
>> }
>> EXPORT_SYMBOL(tcp_filter);
>>
>> +static void tcp_v4_restore_cb(struct sk_buff *skb)
>> +{
>> + memmove(IPCB(skb), _SKB_CB(skb)->header.h4,
>> + sizeof(struct inet_skb_parm));
>> +}
>> +
>> +static void tcp_v4_fill_cb(struct sk_buff *skb, const struct iphdr *iph,
>> + const struct tcphdr *th)
>> +{
>> + /* This is tricky : We move IPCB at its correct location into
>> TCP_SKB_CB()
>> +* barrier() makes sure compiler wont play fool^Waliasing games.
>> +*/
>> + memmove(_SKB_CB(skb)->header.h4, IPCB(skb),
>> + sizeof(struct inet_skb_parm));
>> + barrier();
>> +
>> + TCP_SKB_CB(skb)->seq = ntohl(th->seq);
>> + TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin
>> +
>> + skb->len - th->doff * 4);
>> + TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
>> + TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
>> + TCP_SKB_CB(skb)->tcp_tw_isn = 0;
>> + TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
>> + TCP_SKB_CB(skb)->sacked = 0;
>> + TCP_SKB_CB(skb)->has_rxtstamp =
>> + skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
>> +}
>> +
>> /*
>> * From tcp_input.c
>> */
>> @@ -1631,24 +1659,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
>>
>> th = (const struct tcphdr *)skb->data;
>> iph = ip_hdr(skb);
>> - /* This is tricky : We move IPCB at its correct location into
>> TCP_SKB_CB()
>> -* barrier() makes sure compiler wont play fool^Waliasing games.
>> -*/
>> - memmove(_SKB_CB(skb)->header.h4, IPCB(skb),
>> - sizeof(struct inet_skb_parm));
>> - barrier();
>> -
>> - TCP_SKB_CB(skb)->seq = ntohl(th->seq);
>> - TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin
>> +
>> - skb->len - th->doff * 4);
>> - TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
>> - TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
>> - TCP_SKB_CB(skb)->tcp_tw_isn = 0;
>> - TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
>> - TCP_SKB_CB(skb)->sacked = 0;
>> - TCP_SKB_CB(skb)->has_rxtstamp =
>> - skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
>> -
>> lookup:
>> sk = __inet_lookup_skb(_hashinfo, skb, __tcp_hdrlen(th),
>> th->source,
>>th->dest, sdif, );
>> @@ -1679,14 +1689,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
>> sock_hold(sk);
>> refcounted = true;
>> nsk = NULL;
>> - if (!tcp_filter(sk, skb))
>> + if (!tcp_filter(sk, skb)
Re: [BUG] kernel stack corruption during/after Netlabel error
v4_send_reset(nsk, skb);
> goto discard_and_relse;
> @@ -1712,6 +1727,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> goto discard_and_relse;
> th = (const struct tcphdr *)skb->data;
> iph = ip_hdr(skb);
> + tcp_v4_fill_cb(skb, iph, th);
>
> skb->dev = NULL;
>
> @@ -1742,6 +1758,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
> if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
> goto discard_it;
>
> + tcp_v4_fill_cb(skb, iph, th);
> +
> if (tcp_checksum_complete(skb)) {
> csum_error:
> __TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
> @@ -1768,6 +1786,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
> goto discard_it;
> }
>
> + tcp_v4_fill_cb(skb, iph, th);
> +
> if (tcp_checksum_complete(skb)) {
> inet_twsk_put(inet_twsk(sk));
> goto csum_error;
> @@ -1784,6 +1804,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
> if (sk2) {
> inet_twsk_deschedule_put(inet_twsk(sk));
> sk = sk2;
> + tcp_v4_restore_cb(skb);
> refcounted = false;
> goto process;
> }
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index
> 6bb98c93edfe2ed2f16fe5229605f8108cfc7f9a..1f04ec0e4a7aa2c11b8ee27cbdd4067b5bcf32e5
> 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1454,7 +1454,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
> struct sock *nsk;
>
> sk = req->rsk_listener;
> - tcp_v6_fill_cb(skb, hdr, th);
> if (tcp_v6_inbound_md5_hash(sk, skb)) {
> sk_drops_add(sk, skb);
> reqsk_put(req);
> @@ -1467,8 +1466,12 @@ static int tcp_v6_rcv(struct sk_buff *skb)
> sock_hold(sk);
> refcounted = true;
> nsk = NULL;
> - if (!tcp_filter(sk, skb))
> + if (!tcp_filter(sk, skb)) {
> + th = (const struct tcphdr *)skb->data;
> + hdr = ipv6_hdr(skb);
> + tcp_v6_fill_cb(skb, hdr, th);
> nsk = tcp_check_req(sk, skb, req, false);
> + }
> if (!nsk) {
> reqsk_put(req);
> goto discard_and_relse;
> @@ -1492,8 +1495,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
> if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
> goto discard_and_relse;
>
> - tcp_v6_fill_cb(skb, hdr, th);
> -
> if (tcp_v6_inbound_md5_hash(sk, skb))
> goto discard_and_relse;
>
> @@ -1501,6 +1502,7 @@ static int tcp_v6_rcv(struct sk_buff *skb)
> goto discard_and_relse;
> th = (const struct tcphdr *)skb->data;
> hdr = ipv6_hdr(skb);
> + tcp_v6_fill_cb(skb, hdr, th);
>
> skb->dev = NULL;
>
> @@ -1590,7 +1592,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
> tcp_v6_timewait_ack(sk, skb);
> break;
> case TCP_TW_RST:
> - tcp_v6_restore_cb(skb);
> tcp_v6_send_reset(sk, skb);
> inet_twsk_deschedule_put(inet_twsk(sk));
> goto discard_it;
>
>
>
--
paul moore
www.paul-moore.com
Re: [BUG] kernel stack corruption during/after Netlabel error
On Wed, Nov 29, 2017 at 12:34 PM, Eric Dumazet <eduma...@google.com> wrote: > On Wed, Nov 29, 2017 at 9:31 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote: >> On Wed, 2017-11-29 at 21:26 +1100, James Morris wrote: >>> I'm seeing a kernel stack corruption bug (detected via gcc) when >>> running >>> the SELinux testsuite on a 4.15-rc1 kernel, in the 2nd inet_socket >>> test: >>> >>> https://github.com/SELinuxProject/selinux-testsuite/blob/master/tests >>> /inet_socket/test >>> >>> # Verify that unauthorized client cannot communicate with the >>> server. >>> $result = system >>> "runcon -t test_inet_bad_client_t -- $basedir/client stream >>> 127.0.0.1 65535 2>&1"; >>> >>> This correctlly causes an access control error in the Netlabel code, >>> and >>> the bug seems to be triggered during the ICMP send: >>> >>> [ 339.806024] SELinux: failure in selinux_parse_skb(), unable to >>> parse packet >>> [ 339.822505] Kernel panic - not syncing: stack-protector: Kernel >>> stack is corrupted in: 81745af5 >>> [ 339.822505] >>> [ 339.852250] CPU: 4 PID: 3642 Comm: client Not tainted 4.15.0-rc1- >>> test #15 >>> [ 339.868498] Hardware name: LENOVO 10FGS0VA1L/30BC, BIOS >>> FWKT68A 01/19/2017 >>> [ 339.885060] Call Trace: >>> [ 339.896875] >>> [ 339.908103] dump_stack+0x63/0x87 >>> [ 339.920645] panic+0xe8/0x248 >>> [ 339.932668] ? ip_push_pending_frames+0x33/0x40 >>> [ 339.946328] ? icmp_send+0x525/0x530 >>> [ 339.958861] ? kfree_skbmem+0x60/0x70 >>> [ 339.971431] __stack_chk_fail+0x1b/0x20 >>> [ 339.984049] icmp_send+0x525/0x530 ... >>> This is mostly reliable, and I'm only seeing it on bare metal (not in >>> a >>> virtualbox vm). >>> >>> The SELinux skb parse error at the start only sometimes appears, and >>> looking at the code, I suspect some kind of memory corruption being >>> the >>> cause at that point (basic packet header checks). >>> >>> I bisected the bug down to the following change: >>> >>> commit bffa72cf7f9df842f0016ba03586039296b4caaf >>> Author: Eric Dumazet <eduma...@google.com> >>> Date: Tue Sep 19 05:14:24 2017 -0700 >>> >>> net: sk_buff rbnode reorg >>> ... >>> >>> >>> Anyone else able to reproduce this, or have any ideas on what's >>> happening? >> >> So far I haven't been able to reproduce with 4.15-rc1 or -linus. > > You might try adding KASAN in the picture ? ( CONFIG_KASAN=y ) As another data point, I have not hit this problem either, but I'm not currently building my test kernels with KASAN enabled. -- paul moore www.paul-moore.com
Re: [RFC PATCH 5/5] selinux: Add SCTP support
On Tue, Nov 14, 2017 at 4:52 PM, Richard Haines
<richard_c_hai...@btinternet.com> wrote:
> On Mon, 2017-11-13 at 17:40 -0500, Paul Moore wrote:
>> On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines
>> <richard_c_hai...@btinternet.com> wrote:
>> > On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote:
>> > > On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines
>> > > <richard_c_hai...@btinternet.com> wrote:
>> > > > The SELinux SCTP implementation is explained in:
>> > > > Documentation/security/SELinux-sctp.txt
>> > > >
>> > > > Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com>
>> > > > ---
>> > > > Documentation/security/SELinux-sctp.txt | 108 +
>> > > > security/selinux/hooks.c| 268
>> > > > ++--
>> > > > security/selinux/include/classmap.h | 3 +-
>> > > > security/selinux/include/netlabel.h | 9 +-
>> > > > security/selinux/include/objsec.h | 5 +
>> > > > security/selinux/netlabel.c | 52 ++-
>> > > > 6 files changed, 427 insertions(+), 18 deletions(-)
>> > > > create mode 100644 Documentation/security/SELinux-sctp.txt
>>
>> ...
>>
>> > > > +Policy Statements
>> > > > +==
>> > > > +The following class and permissions to support SCTP are
>> > > > available
>> > > > within the
>> > > > +kernel:
>> > > > +class sctp_socket inherits socket { node_bind }
>> > > > +
>> > > > +whenever the following policy capability is enabled:
>> > > > +policycap extended_socket_class;
>> > > > +
>> > > > +The SELinux SCTP support adds the additional permissions that
>> > > > are
>> > > > explained
>> > > > +in the sections below:
>> > > > +association bindx connectx
>> > >
>> > > Is the distinction between bind and bindx significant? The same
>> > > question applies to connect/connectx. I think we can probably
>> > > just
>> > > reuse bind and connect in these cases.
>> >
>> > This has been discussed before with Marcelo and keeping
>> > bindx/connectx
>> > is a useful distinction.
>>
>> My apologies, I must have forgotten/missed that discussion. Do you
>> have an archive pointer?
>
> No this was off list, however I've copied the relevant bits:
>
>> SCTP Socket Option Permissions
>> ===
>> Permissions that are validated on setsockopt(2) calls (note that the
>> sctp_socket SETOPT permission must be allowed):
>>
>> This option requires the BINDX_ADDR permission:
>> SCTP_SOCKOPT_BINDX_REM - Remove additional bind address.
>
> Can't see an usage for this one.
>
>>
>> These options require the SET_PARAMS permission:
>> SCTP_PEER_ADDR_PARAMS - Set heartbeats and address max
>> retransmissions.
>> SCTP_PEER_ADDR_THLDS - Set thresholds.
>> SCTP_ASSOCINFO- Set association / endpoint parameters.
>
> Also for these, considering we are not willing to go as deep as to only
> allow these if within a given threshold. But still even then, sounds
> like too much.
>
>>
>>
>> SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
>> ==
>> The hook security_sctp_addr_list() is called by SCTP when processing
>> various options (@optname) to check permissions required for the list
>> of ipv4/ipv6 addresses (@address) as follows:
>>
>> |sctp_socket BIND type permission checks |
>> |(The socket must also have the BIND permission) |
>> | @optname| Permission | @address |
>> |--|-|-|
>> |SCTP_SOCKOPT_BINDX_ADD|BINDX_ADDRS |One or more ipv4/ipv6 adr|
>
> This one can be useful, for that privilege-dropping case.
>
> Paul note: I later changed BINDX_ADDRS to just BINDX
>
>> |SCTP_PRIMARY_ADDR|SET_PRI_ADDR |Single ipv4 or ipv6 adr |
>> |SCTP_SET_PEER_PRIMARY_ADDR|SET_PEER_ADDR|Single ipv4 or ipv6 adr |
>
> But these, can't use an use-case.
>
>>
>> -
Re: [PATCH] net/netlabel: Add list_next_rcu() in rcu_dereference().
On Fri, Nov 17, 2017 at 8:35 PM, David Miller <da...@davemloft.net> wrote: > From: Tim Hansen <devtimhan...@gmail.com> > Date: Thu, 16 Nov 2017 12:03:34 -0500 > >> Add list_next_rcu() for fetching next list in rcu_deference safely. >> >> Found with sparse in linux-next tree on tag next-20171116. >> >> Signed-off-by: Tim Hansen <devtimhan...@gmail.com> > > Applied. Thanks guys. My apologies I wasn't able to ACK the patch sooner, I was traveling and had spotty network access. -- paul moore www.paul-moore.com
Re: [RFC PATCH 5/5] selinux: Add SCTP support
On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines
<richard_c_hai...@btinternet.com> wrote:
> On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote:
>> On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines
>> <richard_c_hai...@btinternet.com> wrote:
>> > The SELinux SCTP implementation is explained in:
>> > Documentation/security/SELinux-sctp.txt
>> >
>> > Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com>
>> > ---
>> > Documentation/security/SELinux-sctp.txt | 108 +
>> > security/selinux/hooks.c| 268
>> > ++--
>> > security/selinux/include/classmap.h | 3 +-
>> > security/selinux/include/netlabel.h | 9 +-
>> > security/selinux/include/objsec.h | 5 +
>> > security/selinux/netlabel.c | 52 ++-
>> > 6 files changed, 427 insertions(+), 18 deletions(-)
>> > create mode 100644 Documentation/security/SELinux-sctp.txt
...
>> > +Policy Statements
>> > +==
>> > +The following class and permissions to support SCTP are available
>> > within the
>> > +kernel:
>> > +class sctp_socket inherits socket { node_bind }
>> > +
>> > +whenever the following policy capability is enabled:
>> > +policycap extended_socket_class;
>> > +
>> > +The SELinux SCTP support adds the additional permissions that are
>> > explained
>> > +in the sections below:
>> > +association bindx connectx
>>
>> Is the distinction between bind and bindx significant? The same
>> question applies to connect/connectx. I think we can probably just
>> reuse bind and connect in these cases.
>
> This has been discussed before with Marcelo and keeping bindx/connectx
> is a useful distinction.
My apologies, I must have forgotten/missed that discussion. Do you
have an archive pointer?
>> > +SCTP Peer Labeling
>> > +===
>> > +An SCTP socket will only have one peer label assigned to it. This
>> > will be
>> > +assigned during the establishment of the first association. Once
>> > the peer
>> > +label has been assigned, any new associations will have the
>> > "association"
>> > +permission validated by checking the socket peer sid against the
>> > received
>> > +packets peer sid to determine whether the association should be
>> > allowed or
>> > +denied.
>> > +
>> > +NOTES:
>> > + 1) If peer labeling is not enabled, then the peer context will
>> > always be
>> > + SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
>> > +
>> > + 2) As SCTP supports multiple endpoints with multi-homing on a
>> > single socket
>> > + it is recommended that peer labels are consistent.
>>
>> My apologies if I'm confused, but I thought it was multiple
>> associations per-endpoint, with the associations providing the
>> multi-homing functionality, no?
>
> I've reworded to:
> As SCTP can support more than one transport address per endpoint
> (multi-homing) on a single socket, it is possible to configure policy
> and NetLabel to provide different peer labels for each of these. As the
> socket peer label is determined by the first associations transport
> address, it is recommended that all peer labels are consistent.
I'm still not sure this makes complete sense to me, but since I'm
still not 100% confident in my understanding of SCTP I'm willing to
punt on this for the moment.
>> > + 3) getpeercon(3) may be used by userspace to retrieve the
>> > sockets peer
>> > + context.
>> > +
>> > + 4) If using NetLabel be aware that if a label is assigned to a
>> > specific
>> > + interface, and that interface 'goes down', then the NetLabel
>> > service
>> > + will remove the entry. Therefore ensure that the network
>> > startup scripts
>> > + call netlabelctl(8) to set the required label (see netlabel-
>> > config(8)
>> > + helper script for details).
>>
>> Maybe this will be made clear as I work my way through this patch,
>> but
>> how is point #4 SCTP specific?
>
> It's not, I added this as a useful hint as I keep forgetting about it,
> I'll reword to: While not SCTP specific, be aware .
Okay. Better.
>> > +/* Called whenever SCTP receives an INIT or INIT_ACK chunk */
>> > +static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
>> > +
Re: [RFC PATCH 4/5] netlabel: Add SCTP support
On Mon, Nov 13, 2017 at 3:50 PM, Richard Haines
<richard_c_hai...@btinternet.com> wrote:
> On Mon, 2017-11-06 at 18:15 -0500, Paul Moore wrote:
>> On Tue, Oct 17, 2017 at 9:58 AM, Richard Haines
>> <richard_c_hai...@btinternet.com> wrote:
>> > Add support to label SCTP associations and cater for a situation
>> > where
>> > family = PF_INET6 with an ip_hdr(skb)->version = 4.
>> >
>> > Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com>
>> > ---
>> > include/net/netlabel.h| 3 ++
>> > net/netlabel/netlabel_kapi.c | 80
>> > +++
>> > net/netlabel/netlabel_unlabeled.c | 10 +
>> > 3 files changed, 93 insertions(+)
>> > /**
>> > + * netlbl_sctp_setattr - Label an incoming sctp association socket
>> > using
>> > + * the correct protocol
>> > + * @sk: the socket to label
>> > + * @skb: the packet
>> > + * @secattr: the security attributes
>> > + *
>> > + * Description:
>> > + * Attach the correct label to the given socket using the security
>> > attributes
>> > + * specified in @secattr. Returns zero on success, negative
>> > values on failure.
>> > + *
>> > + */
>> > +int netlbl_sctp_setattr(struct sock *sk,
>> > + struct sk_buff *skb,
>> > + const struct netlbl_lsm_secattr *secattr)
>> > +{
>> > + int ret_val = -EINVAL;
>> > + struct netlbl_dommap_def *entry;
>> > + struct iphdr *hdr4;
>> > +#if IS_ENABLED(CONFIG_IPV6)
>> > + struct ipv6hdr *hdr6;
>> > +#endif
>> > +
>> > + rcu_read_lock();
>> > + switch (sk->sk_family) {
>> > + case AF_INET:
>> > + hdr4 = ip_hdr(skb);
>> > +
>> > + entry = netlbl_domhsh_getentry_af4(secattr->domain,
>> > + hdr4->saddr);
>> > + if (entry == NULL) {
>> > + ret_val = -ENOENT;
>> > + goto sctp_setattr_return;
>> > + }
>> > + switch (entry->type) {
>> > + case NETLBL_NLTYPE_CIPSOV4:
>> > + ret_val = cipso_v4_sock_setattr(sk, entry-
>> > >cipso,
>> > + secattr);
>> > + break;
>> > + case NETLBL_NLTYPE_UNLABELED:
>> > + netlbl_sock_delattr(sk);
>> > + ret_val = 0;
>> > + break;
>> > + default:
>> > + ret_val = -ENOENT;
>> > + }
>> > + break;
>> > +#if IS_ENABLED(CONFIG_IPV6)
>> > + case AF_INET6:
>> > + hdr6 = ipv6_hdr(skb);
>> > + entry = netlbl_domhsh_getentry_af6(secattr->domain,
>> > + >saddr);
>> > + if (entry == NULL) {
>> > + ret_val = -ENOENT;
>> > + goto sctp_setattr_return;
>> > + }
>> > + switch (entry->type) {
>> > + case NETLBL_NLTYPE_CALIPSO:
>> > + ret_val = calipso_sock_setattr(sk, entry-
>> > >calipso,
>> > + secattr);
>> > + break;
>> > + case NETLBL_NLTYPE_UNLABELED:
>> > + netlbl_sock_delattr(sk);
>> > + ret_val = 0;
>> > + break;
>> > + default:
>> > + ret_val = -ENOENT;
>> > + }
>> > + break;
>> > +#endif /* IPv6 */
>> > + default:
>> > + ret_val = -EPROTONOSUPPORT;
>> > + }
>> > +
>> > +sctp_setattr_return:
>> > + rcu_read_unlock();
>> > + return ret_val;
>> > +}
>>
>> It seems like we should try to leverage the code in
>> netlbl_conn_setattr() a bit more. I would suggest either tweaking
>> the
>> callers to use a sockaddr struct and netlbl_conn_setattr(), or
>> implement netlbl_sctp_setattr(
Re: [PATCH] netlabel: If PF_INET6, check sk_buff ip header version
On Mon, Nov 13, 2017 at 3:54 PM, Richard Haines
<richard_c_hai...@btinternet.com> wrote:
> When resolving a fallback label, check the sk_buff version as it
> is possible (e.g. SCTP) to have family = PF_INET6 while
> receiving ip_hdr(skb)->version = 4.
>
> Signed-off-by: Richard Haines <richard_c_hai...@btinternet.com>
> ---
> net/netlabel/netlabel_unlabeled.c | 10 ++
> 1 file changed, 10 insertions(+)
Thanks Richard.
Acked-by: Paul Moore <p...@paul-moore.com>
> diff --git a/net/netlabel/netlabel_unlabeled.c
> b/net/netlabel/netlabel_unlabeled.c
> index 22dc1b9..c070dfc 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -1472,6 +1472,16 @@ int netlbl_unlabel_getattr(const struct sk_buff *skb,
> iface = rcu_dereference(netlbl_unlhsh_def);
> if (iface == NULL || !iface->valid)
> goto unlabel_getattr_nolabel;
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> + /* When resolving a fallback label, check the sk_buff version as
> +* it is possible (e.g. SCTP) to have family = PF_INET6 while
> +* receiving ip_hdr(skb)->version = 4.
> +*/
> + if (family == PF_INET6 && ip_hdr(skb)->version == 4)
> + family = PF_INET;
> +#endif /* IPv6 */
> +
> switch (family) {
> case PF_INET: {
> struct iphdr *hdr4;
> --
> 2.13.6
--
paul moore
www.paul-moore.com
Re: [RFC PATCH 5/5] selinux: Add SCTP support
one),
> + LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect),
> LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request),
> LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone),
> LSM_HOOK_INIT(inet_conn_established, selinux_inet_conn_established),
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index b9fe343..b4b10da 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -173,7 +173,8 @@ struct security_class_mapping secclass_map[] = {
> { COMMON_CAP2_PERMS, NULL } },
> { "sctp_socket",
> { COMMON_SOCK_PERMS,
> - "node_bind", NULL } },
> + "node_bind", "name_connect", "association", "bindx",
> + "connectx", NULL } },
> { "icmp_socket",
> { COMMON_SOCK_PERMS,
> "node_bind", NULL } },
> diff --git a/security/selinux/include/netlabel.h
> b/security/selinux/include/netlabel.h
> index 75686d5..835a0d6 100644
> --- a/security/selinux/include/netlabel.h
> +++ b/security/selinux/include/netlabel.h
> @@ -33,6 +33,7 @@
> #include
> #include
> #include
> +#include
>
> #include "avc.h"
> #include "objsec.h"
> @@ -53,7 +54,8 @@ int selinux_netlbl_skbuff_getsid(struct sk_buff *skb,
> int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
> u16 family,
> u32 sid);
> -
> +int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> +struct sk_buff *skb);
> int selinux_netlbl_inet_conn_request(struct request_sock *req, u16 family);
> void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family);
> int selinux_netlbl_socket_post_create(struct sock *sk, u16 family);
> @@ -114,6 +116,11 @@ static inline int selinux_netlbl_conn_setsid(struct sock
> *sk,
> return 0;
> }
>
> +static inline int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> + struct sk_buff *skb)
> +{
> + return 0;
> +}
> static inline int selinux_netlbl_inet_conn_request(struct request_sock *req,
>u16 family)
> {
> diff --git a/security/selinux/include/objsec.h
> b/security/selinux/include/objsec.h
> index 6ebc61e..660f270 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -130,6 +130,11 @@ struct sk_security_struct {
> u32 sid;/* SID of this object */
> u32 peer_sid; /* SID of peer */
> u16 sclass; /* sock security class */
> +
Extra vertical whitespace is not needed.
> + enum { /* SCTP association state */
> + SCTP_ASSOC_UNSET = 0,
> + SCTP_ASSOC_SET,
> + } sctp_assoc_state;
> };
>
> struct tun_security_struct {
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index aaba667..7d5aa15 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -250,6 +250,7 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
> sk = skb_to_full_sk(skb);
> if (sk != NULL) {
> struct sk_security_struct *sksec = sk->sk_security;
> +
> if (sksec->nlbl_state != NLBL_REQSKB)
> return 0;
> secattr = selinux_netlbl_sock_getattr(sk, sid);
> @@ -271,6 +272,41 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
> }
>
> /**
> + * selinux_netlbl_sctp_assoc_request - Label an incoming sctp association.
> + * @ep: incoming association endpoint.
> + * @skb: the packet.
> + *
> + * Description:
> + * A new incoming connection is represented by @ep, ..
> + * Returns zero on success, negative values on failure.
> + *
> + */
> +int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> +struct sk_buff *skb)
> +{
> + int rc;
> + struct netlbl_lsm_secattr secattr;
> + struct sk_security_struct *sksec = ep->base.sk->sk_security;
> +
> + if (ep->base.sk->sk_family != PF_INET &&
> + ep->base.sk->sk_family != PF_INET6)
> + return 0;
> +
> + netlbl_secattr_init();
> + rc = security_netlbl_sid_to_secattr(ep->secid, );
> + if (rc != 0)
> + goto assoc_request_return;
> +
> + rc = netlbl_sctp_setattr(ep->base.sk, skb, );
> + if (rc == 0)
> + sksec->nlbl_state = NLBL_LABELED;
> +
> +assoc_request_return:
> + netlbl_secattr_destroy();
> + return rc;
> +}
> +
> +/**
> * selinux_netlbl_inet_conn_request - Label an incoming stream connection
> * @req: incoming connection request socket
> *
> @@ -481,7 +517,7 @@ int selinux_netlbl_socket_setsockopt(struct socket *sock,
> */
> int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr)
> {
> - int rc;
> + int rc, already_owned_by_user = 0;
> struct sk_security_struct *sksec = sk->sk_security;
> struct netlbl_lsm_secattr *secattr;
>
> @@ -489,7 +525,16 @@ int selinux_netlbl_socket_connect(struct sock *sk,
> struct sockaddr *addr)
> sksec->nlbl_state != NLBL_CONNLABELED)
> return 0;
>
> - lock_sock(sk);
> + /* Note: When called via connect(2) this happens before the socket
> +* protocol layer connect operation and @sk is not locked, HOWEVER,
> +* when called by the SCTP protocol layer via sctp_connectx(3),
> +* sctp_sendmsg(3) or sendmsg(2), @sk is locked. Therefore check if
> +* @sk owned already.
> +*/
> + if (sock_owned_by_user(sk) && sksec->sclass == SECCLASS_SCTP_SOCKET)
> + already_owned_by_user = 1;
> + else
> + lock_sock(sk);
>
> /* connected sockets are allowed to disconnect when the address family
> * is set to AF_UNSPEC, if that is what is happening we want to reset
> @@ -510,6 +555,7 @@ int selinux_netlbl_socket_connect(struct sock *sk, struct
> sockaddr *addr)
> sksec->nlbl_state = NLBL_CONNLABELED;
>
> socket_connect_return:
> - release_sock(sk);
> + if (!already_owned_by_user)
> + release_sock(sk);
> return rc;
> }
> --
> 2.13.6
>
--
paul moore
www.paul-moore.com
