netdev - Re: [PATCH netfilter] netfilter: xt_owner: use sk->sk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANP3RGct11+Cu0z-ksEMcpQGyFp5Ek-99+z6qEFc1FFh0xUt7Q@mail.gmail.com>
Date:   Thu, 23 Dec 2021 10:36:38 -0800
From:   Maciej Żenczykowski <zenczykowski@...il.com>
To:     Jan Engelhardt <jengelh@...i.de>
Cc:     Pablo Neira Ayuso <pablo@...filter.org>,
        Florian Westphal <fw@...len.de>,
        Linux Network Development Mailing List 
        <netdev@...r.kernel.org>,
        Netfilter Development Mailing List 
        <netfilter-devel@...r.kernel.org>,
        Lorenzo Colitti <lorenzo@...gle.com>
Subject: Re: [PATCH netfilter] netfilter: xt_owner: use sk->sk_uid for owner lookup

On Thu, Dec 23, 2021 at 2:35 AM Jan Engelhardt <jengelh@...i.de> wrote:
> On Thursday 2021-12-23 08:06, Maciej Żenczykowski wrote:
>
> >diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
> >index e85ce69924ae..3eebd9c7ea4b 100644
> >--- a/net/netfilter/xt_owner.c
> >+++ b/net/netfilter/xt_owner.c
> >@@ -84,8 +84,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
> >       if (info->match & XT_OWNER_UID) {
> >               kuid_t uid_min = make_kuid(net->user_ns, info->uid_min);
> >               kuid_t uid_max = make_kuid(net->user_ns, info->uid_max);
> >-              if ((uid_gte(filp->f_cred->fsuid, uid_min) &&
> >-                   uid_lte(filp->f_cred->fsuid, uid_max)) ^
> >+              if ((uid_gte(sk->sk_uid, uid_min) &&
> >+                   uid_lte(sk->sk_uid, uid_max)) ^
>
> I have a "déjà rencontré" moment about these lines...
>
> filp->f_cred->fsuid should be the EUID which performed the access (after
> peeling away the setfsuid(2) logic...), and sk_uid has a value that the
> original author of ipt_owner did not find useful. I think that was the
> motivation. listen(80) then drop privileges by set(e)uid. sk_uid would be 0,
> and thus not useful.

Ugh!  Well, that's certainly interesting to hear...

There's like 6 different uids associated with a socket (sk_uid, inode
uid, f_cred->uid/euid/suid/fsuid)
- and I guess it might also matter whether we're talking about at
socket() [or accept()] creation time, or currently...
it's a mess.  [and 5 gids + supplemental groups]

I'm not really certain which of these have which meaning.  I don't
really understand the meaning of filp->f_cred.

I guess it's back to the drawing board.  The Android DNS resolver uses
fchown() on the dns sockets it creates
to 'impersonate' the clients on whose behalf it's doing dns queries.
This works for bpf, because:

bpf_get_socket_uid(skb) returns (roughly) skb->sk->sk_uid
[and there's simply no bpf helper that deals with gids]

but this of course results in -m owner --uid-owner seeing root while
bpf sees something else.

I wonder if the solution is to add -m owner --sk-uid X (or
--socket-uid) syntax instead... ?!?

I'm not sure if it would be safe (or even desirable) to get fchown()
to modify the existing f_cred->fsuid field...