Re: [PATCH] netfilter: Record uid and gid in xt_AUDIT

Re: [PATCH] netfilter: Record uid and gid in xt_AUDIT

[Paul Moore]

On Wed, Oct 9, 2024 at 4:33 PM Richard Weinberger <richard@nod.at> wrote:
>
> When recording audit events for new outgoing connections,
> it is helpful to log the user info of the associated socket,
> if available.
> Therefore, check if the skb has a socket, and if it does,
> log the owning fsuid/fsgid.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  net/netfilter/xt_AUDIT.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index b6a015aee0cec..d88b5442beaa6 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -9,16 +9,19 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
>  #include <linux/audit.h>
> +#include <linux/cred.h>
> +#include <linux/file.h>
> +#include <linux/if_arp.h>
>  #include <linux/module.h>
>  #include <linux/skbuff.h>
>  #include <linux/tcp.h>
>  #include <linux/udp.h>
> -#include <linux/if_arp.h>
>  #include <linux/netfilter/x_tables.h>
>  #include <linux/netfilter/xt_AUDIT.h>
>  #include <linux/netfilter_bridge/ebtables.h>
> -#include <net/ipv6.h>
>  #include <net/ip.h>
> +#include <net/ipv6.h>
> +#include <net/sock.h>
>
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Thomas Graf <tgraf@redhat.com>");
> @@ -66,7 +69,9 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
>  static unsigned int
>  audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
>  {
> +       struct sock *sk = skb->sk;
>         struct audit_buffer *ab;
> +       bool got_uidgid = false;
>         int fam = -1;
>
>         if (audit_enabled == AUDIT_OFF)
> @@ -99,6 +104,24 @@ audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
>         if (fam == -1)
>                 audit_log_format(ab, " saddr=? daddr=? proto=-1");
>
> +       if (sk && sk_fullsock(sk)) {
> +               read_lock_bh(&sk->sk_callback_lock);
> +               if (sk->sk_socket && sk->sk_socket->file) {
> +                       const struct file *file = sk->sk_socket->file;
> +                       const struct cred *cred = file->f_cred;
> +
> +                       audit_log_format(ab, " uid=%u gid=%u",
> +                                        from_kuid(&init_user_ns, cred->fsuid),
> +                                        from_kgid(&init_user_ns, cred->fsgid));

[CC'ing the audit and LSM lists for obvious reasons]

If we're logging the subjective credentials of the skb's associated
socket, we really should also log the socket's LSM secctx similar to
what we do with audit_log_task() and audit_log_task_context().
Unfortunately, I don't believe we currently have a LSM interface that
return the secctx from a sock/socket, although we do have
security_inode_getsecctx() which *should* yield the same result using
SOCK_INODE(sk->sk_socket).

I should also mention that I'm currently reviewing a patchset which is
going to add proper support for multiple LSMs in audit which will
likely impact this work.

https://lore.kernel.org/linux-security-module/20241009173222.12219-1-casey@schaufler-ca.com/

> +                       got_uidgid = true;
> +               }
> +               read_unlock_bh(&sk->sk_callback_lock);
> +       }
> +
> +       if (!got_uidgid)
> +               audit_log_format(ab, " uid=? gid=?");
> +
>         audit_log_end(ab);
>
>  errout:
> --
> 2.35.3

-- 
paul-moore.com

Re: [PATCH] netfilter: Record uid and gid in xt_AUDIT

[Richard Weinberger]

Am Donnerstag, 10. Oktober 2024, 00:02:44 CEST schrieb Paul Moore:
> [CC'ing the audit and LSM lists for obvious reasons]
> 
> If we're logging the subjective credentials of the skb's associated
> socket, we really should also log the socket's LSM secctx similar to
> what we do with audit_log_task() and audit_log_task_context().
> Unfortunately, I don't believe we currently have a LSM interface that
> return the secctx from a sock/socket, although we do have
> security_inode_getsecctx() which *should* yield the same result using
> SOCK_INODE(sk->sk_socket).

Hm, I thought about that but saw 2173c519d5e91 ("audit: normalize NETFILTER_PKT").
It removed usage of audit_log_secctx() and many other, IMHO, useful fields.
What about skb->secctx?

> 
> I should also mention that I'm currently reviewing a patchset which is
> going to add proper support for multiple LSMs in audit which will
> likely impact this work.
> 
> https://lore.kernel.org/linux-security-module/20241009173222.12219-1-casey@schaufler-ca.com/

Ok!

Thanks,
//richard
 
-- 
​​​​​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
UID/VAT Nr: ATU 66964118 | FN: 374287y

Re: [PATCH] netfilter: Record uid and gid in xt_AUDIT

[Paul Moore]

On Thu, Oct 10, 2024 at 2:24 AM Richard Weinberger
<richard@sigma-star.at> wrote:
> Am Donnerstag, 10. Oktober 2024, 00:02:44 CEST schrieb Paul Moore:
> > [CC'ing the audit and LSM lists for obvious reasons]
> >
> > If we're logging the subjective credentials of the skb's associated
> > socket, we really should also log the socket's LSM secctx similar to
> > what we do with audit_log_task() and audit_log_task_context().
> > Unfortunately, I don't believe we currently have a LSM interface that
> > return the secctx from a sock/socket, although we do have
> > security_inode_getsecctx() which *should* yield the same result using
> > SOCK_INODE(sk->sk_socket).
>
> Hm, I thought about that but saw 2173c519d5e91 ("audit: normalize NETFILTER_PKT").
> It removed usage of audit_log_secctx() and many other, IMHO, useful fields.

The main motivation for that patch was getting rid of the inconsistent
usage of fields in the NETFILTER_PKT record (as mentioned in the
commit description).  There's a lot of history around this, and why we
are stuck with this pretty awful IMO, but one of the audit rules is
that if a field appears in one instance of an audit record, it must
appear in all instances of an audit record (which is why it is
important and good that you used the "?" values for UID/GID when they
are not able to be logged).

However, as part of that commit we also dropped a number of fields
because it wasn't clear that anyone cared about them and if we were
going to (re)normalize the NETFILTER_PKT record we figured it would be
best to start small and re-add fields as needed to satisfy user
requirements.  I'm working under the assumption that if you've taken
the time to draft a patch and test it, you have a legitimate need :)

> What about skb->secctx?

Heh, if there is anything with more history than the swinging fields
in an audit record, it would be that :)  We don't currently have an
explicit LSM blob/secid/secctx in a skb and I wouldn't hold your
breath waiting for one; we do have a secmark, but that is something
entirely different.  We've invented some mechanisms to somewhat mimic
a LSM security label for packets, but that's complicated and not
something we would want to deal with in the NETFILTER_PKT record at
this point in time.

-- 
paul-moore.com

Re: [PATCH] netfilter: Record uid and gid in xt_AUDIT

[Richard Weinberger]

Am Donnerstag, 10. Oktober 2024, 21:09:31 CEST schrieb Paul Moore:
> However, as part of that commit we also dropped a number of fields
> because it wasn't clear that anyone cared about them and if we were
> going to (re)normalize the NETFILTER_PKT record we figured it would be
> best to start small and re-add fields as needed to satisfy user
> requirements.  I'm working under the assumption that if you've taken
> the time to draft a patch and test it, you have a legitimate need :)

I'm currently exploring ways to log reliable what users/containers
create what network connections.
So, netfilter+conntrack+xt_AUDIT seemed legit to me.

Thanks,
//richard

-- 
​​​​​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
UID/VAT Nr: ATU 66964118 | FN: 374287y