diff mbox series

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

Message ID 20211223070642.499278-1-zenczykowski@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [netfilter] netfilter: xt_owner: use sk->sk_uid for owner lookup | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers fail 1 blamed authors not CCed: davem@davemloft.net; 4 maintainers not CCed: davem@davemloft.net coreteam@netfilter.org kuba@kernel.org kadlec@netfilter.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 10 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Maciej Żenczykowski Dec. 23, 2021, 7:06 a.m. UTC 
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(-)

Comments

Jan Engelhardt Dec. 23, 2021, 10:35 a.m. UTC | #1 
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.
Maciej Żenczykowski Dec. 23, 2021, 6:36 p.m. UTC | #2 
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...
diff mbox series

Patch

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;
 	}