| Message ID | 20241009203218.26329-1-richard@nod.at (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netfilter: Record uid and gid in xt_AUDIT | expand |
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.
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.
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 <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.
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
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
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.
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.
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 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.
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.
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
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
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
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:
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(-)