[PATCH netfilter] netfilter: xt_owner: use sk->sk_uid for owner lookup

[PATCH netfilter] netfilter: xt_owner: use sk->sk_uid for owner lookup

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

this makes fchown() affect '-m owner --uid-owner'

Cc: Lorenzo Colitti <lorenzo@google.com>
Fixes: 86741ec25462 ('net: core: Add a UID field to struct sock.')
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/netfilter/xt_owner.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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)) ^
 		    !(info->invert & XT_OWNER_UID))
 			return false;
 	}
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH netfilter] netfilter: xt_owner: use sk->sk_uid for owner lookup

[Jan Engelhardt]

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.

Re: [PATCH netfilter] netfilter: xt_owner: use sk->sk_uid for owner lookup

[Maciej Żenczykowski]

On Thu, Dec 23, 2021 at 2:35 AM Jan Engelhardt <jengelh@inai.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...