net/netfilter/xt_AUDIT.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)
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));
+
+ 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.3On 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
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
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
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
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.
AFAIK audit isn't namespace aware at all (neither netns nor userns), so I
wonder how to handle this.
We can't reject adding a -j AUDIT rule for non-init-net (we could, but I'm sure
it'll break some setups...).
But I wonder if we should at least skip the uid if the user namespace is
'something else'.
> + if (sk && sk_fullsock(sk)) {
I.e. check net->user_ns == &init_user_ns too and don't log the uid
otherwise.
I don't think auditd can make sense of the uid otherwise, resp.
its misleading, no?
Alternatively, use this instead?
kuid = sock_net_uid(sock_net(sk), sk);
from_kuid_munged(sock_net(sk)->user_ns, kuid);
There is no need to follow ->file backpointer anymore, see
6acc5c2910689fc6ee181bf63085c5efff6a42bd and
86741ec25462e4c8cdce6df2f41ead05568c7d5e,
"net: core: Add a UID field to struct sock.".
I think we could streamline all the existing paths that fetch uid
from sock->file to not do that and use sock_net_uid() instead as well.
Am Mittwoch, 9. Oktober 2024, 23:33:45 CEST schrieb Florian Westphal: > There is no need to follow ->file backpointer anymore, see > 6acc5c2910689fc6ee181bf63085c5efff6a42bd and > 86741ec25462e4c8cdce6df2f41ead05568c7d5e, > "net: core: Add a UID field to struct sock.". Oh, neat! > I think we could streamline all the existing paths that fetch uid > from sock->file to not do that and use sock_net_uid() instead as well. Also xt_owner? Thanks, //richard -- sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT UID/VAT Nr: ATU 66964118 | FN: 374287y
Richard Weinberger <richard@sigma-star.at> wrote: > Am Mittwoch, 9. Oktober 2024, 23:33:45 CEST schrieb Florian Westphal: > > There is no need to follow ->file backpointer anymore, see > > 6acc5c2910689fc6ee181bf63085c5efff6a42bd and > > 86741ec25462e4c8cdce6df2f41ead05568c7d5e, > > "net: core: Add a UID field to struct sock.". > > Oh, neat! > > > I think we could streamline all the existing paths that fetch uid > > from sock->file to not do that and use sock_net_uid() instead as well. > > Also xt_owner? sk->sk_uid is already used e.g. for fib lookups so I think it makes sense to be consistent, so, yes, xt_owner, nfqueue, nft_meta.c, all can be converted.
Am Donnerstag, 10. Oktober 2024, 15:48:27 CEST schrieb Florian Westphal: > Richard Weinberger <richard@sigma-star.at> wrote: > > Am Mittwoch, 9. Oktober 2024, 23:33:45 CEST schrieb Florian Westphal: > > > There is no need to follow ->file backpointer anymore, see > > > 6acc5c2910689fc6ee181bf63085c5efff6a42bd and > > > 86741ec25462e4c8cdce6df2f41ead05568c7d5e, > > > "net: core: Add a UID field to struct sock.". > > > > Oh, neat! I gave sock_net_uid() a try, but I kinda fail. Maybe I have wrong expectations. e.g. I expected that sock_net_uid() will return 1000 when uid 1000 does something like: unshare -Umr followed by a veth connection to the host (initial user/net namespace). Shouldn't on the host side a forwarded skb have a ->dev that belongs uid 1000's net namespace? Thanks, //richard -- sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT UID/VAT Nr: ATU 66964118 | FN: 374287y
Richard Weinberger <richard@sigma-star.at> wrote: > Maybe I have wrong expectations. > e.g. I expected that sock_net_uid() will return 1000 when > uid 1000 does something like: unshare -Umr followed by a veth connection > to the host (initial user/net namespace). > Shouldn't on the host side a forwarded skb have a ->dev that belongs uid > 1000's net namespace? You mean skb->sk? dev doesn't make much sense in this context to me. Else, please clarify. ip stack orphans incoming skbs, i.e. skb->sk is gone, see skb_orphan() call in ip_rcv_core(). So when packet enters init_net prerouting hook, association with originating netns or sk is not present anymore.
Am Freitag, 11. Oktober 2024, 03:27:13 CEST schrieb Florian Westphal: > Richard Weinberger <richard@sigma-star.at> wrote: > > Maybe I have wrong expectations. > > e.g. I expected that sock_net_uid() will return 1000 when > > uid 1000 does something like: unshare -Umr followed by a veth connection > > to the host (initial user/net namespace). > > Shouldn't on the host side a forwarded skb have a ->dev that belongs uid > > 1000's net namespace? > > You mean skb->sk? dev doesn't make much sense in this context to me. > Else, please clarify. Well, this was a brain fart on my side. I wondered about the sock_net_uid(net, NULL) case and wrongly assumed that a skb I see in the outer namespace can have a skb->dev from another namespace. It would be awesome to have some information about the originating net namespace. Thanks, //richard -- sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT UID/VAT Nr: ATU 66964118 | FN: 374287y
On Thursday 2024-10-10 15:48, Florian Westphal wrote: >Richard Weinberger <richard@sigma-star.at> wrote: >> Am Mittwoch, 9. Oktober 2024, 23:33:45 CEST schrieb Florian Westphal: >> > There is no need to follow ->file backpointer anymore, see >> > 6acc5c2910689fc6ee181bf63085c5efff6a42bd and >> > 86741ec25462e4c8cdce6df2f41ead05568c7d5e, >> > "net: core: Add a UID field to struct sock.". >> >> Oh, neat! >> >> > I think we could streamline all the existing paths that fetch uid >> > from sock->file to not do that and use sock_net_uid() instead as well. >> >> Also xt_owner? > >sk->sk_uid is already used e.g. for fib lookups so I think it makes >sense to be consistent, so, yes, xt_owner, nfqueue, nft_meta.c, all can >be converted. I doubt it. We've been there before... if a process does setuid, some uid field doesn't change, and others do, so that's user-visible behavior you can't just change.
On Wed, Oct 9, 2024 at 5:34 PM Florian Westphal <fw@strlen.de> wrote: > 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. > > AFAIK audit isn't namespace aware at all (neither netns nor userns), so I > wonder how to handle this. > > We can't reject adding a -j AUDIT rule for non-init-net (we could, but I'm sure > it'll break some setups...). > > But I wonder if we should at least skip the uid if the user namespace is > 'something else'. This isn't unique to netfilter and the approach we take in the rest of audit is to always display UIDs/GIDs in the context of the init_user_ns; grep for from_kuid() in kernel/audit*.c. -- paul-moore.com
Paul Moore <paul@paul-moore.com> wrote: > On Wed, Oct 9, 2024 at 5:34 PM Florian Westphal <fw@strlen.de> wrote: > > 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. > > > > AFAIK audit isn't namespace aware at all (neither netns nor userns), so I > > wonder how to handle this. > > > > We can't reject adding a -j AUDIT rule for non-init-net (we could, but I'm sure > > it'll break some setups...). > > > > But I wonder if we should at least skip the uid if the user namespace is > > 'something else'. > > This isn't unique to netfilter and the approach we take in the rest of > audit is to always display UIDs/GIDs in the context of the > init_user_ns; grep for from_kuid() in kernel/audit*.c. Hmm, audit_netlink_ok() bails with -ECONNREFUSED for current_user_ns() != &init_user_ns, so audit_log_common_recv_msg() won't be called from tasks that reside in a different userns. If you say its fine and audit can figure out that the retuned uid is not related to the initial user namespace, then ok. I was worried audit records could blame wrong/bogus user id.
On Wed, Oct 9, 2024 at 6:34 PM Florian Westphal <fw@strlen.de> wrote: > Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Oct 9, 2024 at 5:34 PM Florian Westphal <fw@strlen.de> wrote: > > > 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. > > > > > > AFAIK audit isn't namespace aware at all (neither netns nor userns), so I > > > wonder how to handle this. > > > > > > We can't reject adding a -j AUDIT rule for non-init-net (we could, but I'm sure > > > it'll break some setups...). > > > > > > But I wonder if we should at least skip the uid if the user namespace is > > > 'something else'. > > > > This isn't unique to netfilter and the approach we take in the rest of > > audit is to always display UIDs/GIDs in the context of the > > init_user_ns; grep for from_kuid() in kernel/audit*.c. > > Hmm, audit_netlink_ok() bails with -ECONNREFUSED for current_user_ns() > != &init_user_ns, so audit_log_common_recv_msg() won't be called from > tasks that reside in a different userns. We have a requirement that the audit daemon and audit management tools run in the initial user namespace, but these are the audit collection and configuration mechanisms, not the audit record generation mechanisms. Regardless of the namespace limitations on auditd and auditctl, we want to collect audit records across the system, which is what we are doing in audit_tg(). > If you say its fine and audit can figure out that the retuned > uid is not related to the initial user namespace, then ok. > > I was worried audit records could blame wrong/bogus user id. Correct me if I'm wrong, but by using from_kXid(&init_user_ns, Xid) we get the ID number that is correct for the init namespace, yes? If so, that's what we want as right now all of the audit records, filters, etc. are intended to be set from the context of the initial namespace. -- paul-moore.com
Paul Moore <paul@paul-moore.com> wrote:
> Correct me if I'm wrong, but by using from_kXid(&init_user_ns, Xid) we
> get the ID number that is correct for the init namespace, yes? If so,
> that's what we want as right now all of the audit records, filters,
> etc. are intended to be set from the context of the initial namespace.
Seems to be the case, from_kuid() kdoc says
'There is always a mapping into the initial user_namespace.'.
I'm confused because of the various means of dealing with this:
9847371a84b0 ("netfilter: Allow xt_owner in any user namespace")
Does: make_kgid(net->user_ns, ... and also rejects rule-add if
net->user_ns != current_user_ns(). As this is for matching userids,
this makes sense to me, any userns will 'just work' for normal uid/gid
matching.
a6c6796c7127 ("userns: Convert cls_flow to work with user namespaces enabled")
Does: from_kuid(&init_user_ns, ... and rejects rule adds if sk_user_ns(NETLINK_CB(in_skb).ssk) != &init_user_ns)
Seems just a more conservative solution to the former one.
8c6e2a941ae7 ("userns: Convert xt_LOG to print socket kuids and kgids as uids and gids")
... which looks like the proposed xt_AUDIT change.
As I do not know what the use case is for xt_AUDIT rules residing in
another, possibly unprivileged network namespace not managed by root-root user,
I can't say if its right, but it should do the right thing.
Sorry for the noise.
On Thu, Oct 10, 2024 at 1:59 PM Florian Westphal <fw@strlen.de> wrote:
> Paul Moore <paul@paul-moore.com> wrote:
> > Correct me if I'm wrong, but by using from_kXid(&init_user_ns, Xid) we
> > get the ID number that is correct for the init namespace, yes? If so,
> > that's what we want as right now all of the audit records, filters,
> > etc. are intended to be set from the context of the initial namespace.
>
> Seems to be the case, from_kuid() kdoc says
> 'There is always a mapping into the initial user_namespace.'.
>
> I'm confused because of the various means of dealing with this:
> 9847371a84b0 ("netfilter: Allow xt_owner in any user namespace")
>
> Does: make_kgid(net->user_ns, ... and also rejects rule-add if
> net->user_ns != current_user_ns(). As this is for matching userids,
> this makes sense to me, any userns will 'just work' for normal uid/gid
> matching.
>
> a6c6796c7127 ("userns: Convert cls_flow to work with user namespaces enabled")
> Does: from_kuid(&init_user_ns, ... and rejects rule adds if sk_user_ns(NETLINK_CB(in_skb).ssk) != &init_user_ns)
>
> Seems just a more conservative solution to the former one.
>
> 8c6e2a941ae7 ("userns: Convert xt_LOG to print socket kuids and kgids as uids and gids")
> ... which looks like the proposed xt_AUDIT change.
>
> As I do not know what the use case is for xt_AUDIT rules residing in
> another, possibly unprivileged network namespace not managed by root-root user,
> I can't say if its right, but it should do the right thing.
>
> Sorry for the noise.
No worries, it was a fair question and discussion about this is rarely
a bad thing as it can get rather complicated somewhat quickly. With
audit our current philosophy is that we try to do our logging and run
the filters within the context of the host/initial namespace for the
sake of consistency. Of course this introduces some limitations and
"hide" some details specific to a child namespace, but it's the only
solution we could think of that allows the current kernel audit
implementation to behave in a comprehensive and sane manner across all
the various namespace/container solutions.
--
paul-moore.com
© 2016 - 2024 Red Hat, Inc.