| Message ID | 1465850579-23431-1-git-send-email-cernekee@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
Kevin Cernekee <cernekee@chromium.org> writes: > From: "Eric W. Biederman" <ebiederm@xmission.com> > > Making this work is a little tricky as it really isn't kosher to > change the xt_owner_match_info in a check function. > > Without changing xt_owner_match_info we need to know the user > namespace the uids and gids are specified in. In the common case > net->user_ns == current_user_ns(). Verify net->user_ns == > current_user_ns() in owner_check so we can later assume it in > owner_mt. > > In owner_check also verify that all of the uids and gids specified are > in net->user_ns and that the expected min/max relationship exists > between the uids and gids in xt_owner_match_info. > > In owner_mt get the network namespace from the outgoing socket, as this > must be the same network namespace as the netfilter rules, and use that > network namespace to find the user namespace the uids and gids in > xt_match_owner_info are encoded in. Then convert from their encoded > from into the kernel internal format for uids and gids and perform the > owner match. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > Signed-off-by: Kevin Cernekee <cernekee@chromium.org> > --- > > > Original post: > https://lists.linuxfoundation.org/pipermail/containers/2014-May/034514.html > > V1->V2: > > - Change (max <= min) check to (max < min), because it is legal for > min == max. I suspect this was the root cause of this error: > > https://lists.linuxfoundation.org/pipermail/containers/2014-June/034661.html > > - Fix checkpatch warnings. > > > This was tested on a system running Linux 3.14, then compile-tested on > Linus' master branch. > > Similar to ping_group_range, this code does not try to detect > noncontiguous UID/GID ranges. Acked-by: "Eric W. Biederman" <ebiederm@xmission.com> Would it be worth including this changelog in with the text? > > > net/netfilter/xt_owner.c | 42 ++++++++++++++++++++++++++++++++++++------ > 1 file changed, 36 insertions(+), 6 deletions(-) > > diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c > index 1302b475abcb..e1c1f31ef1e9 100644 > --- a/net/netfilter/xt_owner.c > +++ b/net/netfilter/xt_owner.c > @@ -21,11 +21,39 @@ > static int owner_check(const struct xt_mtchk_param *par) > { > struct xt_owner_match_info *info = par->matchinfo; > + struct net *net = par->net; > > - /* For now only allow adding matches from the initial user namespace */ > + /* Only allow the common case where the userns of the writer > + * matches the userns of the network namespace. > + */ > if ((info->match & (XT_OWNER_UID|XT_OWNER_GID)) && > - (current_user_ns() != &init_user_ns)) > + (current_user_ns() != net->user_ns)) > return -EINVAL; > + > + /* Ensure the uids are valid */ > + 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_valid(uid_min) || !uid_valid(uid_max) || > + (info->uid_max < info->uid_min) || > + uid_lt(uid_max, uid_min)) { > + return -EINVAL; > + } > + } > + > + /* Ensure the gids are valid */ > + if (info->match & XT_OWNER_GID) { > + kgid_t gid_min = make_kgid(net->user_ns, info->gid_min); > + kgid_t gid_max = make_kgid(net->user_ns, info->gid_max); > + > + if (!gid_valid(gid_min) || !gid_valid(gid_max) || > + (info->gid_max < info->gid_min) || > + gid_lt(gid_max, gid_min)) { > + return -EINVAL; > + } > + } > + > return 0; > } > > @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) > const struct xt_owner_match_info *info = par->matchinfo; > const struct file *filp; > struct sock *sk = skb_to_full_sk(skb); > + const struct net *net; > > if (sk == NULL || sk->sk_socket == NULL) > return (info->match ^ info->invert) == 0; > @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) > return ((info->match ^ info->invert) & > (XT_OWNER_UID | XT_OWNER_GID)) == 0; > > + net = sock_net(skb->sk); > if (info->match & XT_OWNER_UID) { > - kuid_t uid_min = make_kuid(&init_user_ns, info->uid_min); > - kuid_t uid_max = make_kuid(&init_user_ns, info->uid_max); > + 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)) ^ > !(info->invert & XT_OWNER_UID)) > @@ -60,8 +90,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) > } > > if (info->match & XT_OWNER_GID) { > - kgid_t gid_min = make_kgid(&init_user_ns, info->gid_min); > - kgid_t gid_max = make_kgid(&init_user_ns, info->gid_max); > + kgid_t gid_min = make_kgid(net->user_ns, info->gid_min); > + kgid_t gid_max = make_kgid(net->user_ns, info->gid_max); > if ((gid_gte(filp->f_cred->fsgid, gid_min) && > gid_lte(filp->f_cred->fsgid, gid_max)) ^ > !(info->invert & XT_OWNER_GID)) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Kevin Cernekee <cernekee@chromium.org> wrote: > @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) > const struct xt_owner_match_info *info = par->matchinfo; > const struct file *filp; > struct sock *sk = skb_to_full_sk(skb); > + const struct net *net; > > if (sk == NULL || sk->sk_socket == NULL) > return (info->match ^ info->invert) == 0; > @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) > return ((info->match ^ info->invert) & > (XT_OWNER_UID | XT_OWNER_GID)) == 0; > > + net = sock_net(skb->sk); I think you need to use sock_net(sk) as skb_to_full_sk(skb) can return something other than skb->sk. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2016-06-14 at 02:02 +0200, Florian Westphal wrote: > Kevin Cernekee <cernekee@chromium.org> wrote: > > @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) > > const struct xt_owner_match_info *info = par->matchinfo; > > const struct file *filp; > > struct sock *sk = skb_to_full_sk(skb); > > + const struct net *net; > > > > if (sk == NULL || sk->sk_socket == NULL) > > return (info->match ^ info->invert) == 0; > > @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) > > return ((info->match ^ info->invert) & > > (XT_OWNER_UID | XT_OWNER_GID)) == 0; > > > > + net = sock_net(skb->sk); > > I think you need to use sock_net(sk) as skb_to_full_sk(skb) can return something > other than skb->sk. They should share same network namespace ? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Florian Westphal <fw@strlen.de> wrote: > Kevin Cernekee <cernekee@chromium.org> wrote: > > @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) > > const struct xt_owner_match_info *info = par->matchinfo; > > const struct file *filp; > > struct sock *sk = skb_to_full_sk(skb); > > + const struct net *net; > > > > if (sk == NULL || sk->sk_socket == NULL) > > return (info->match ^ info->invert) == 0; > > @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) > > return ((info->match ^ info->invert) & > > (XT_OWNER_UID | XT_OWNER_GID)) == 0; > > > > + net = sock_net(skb->sk); > > I think you need to use sock_net(sk) as skb_to_full_sk(skb) can return something > other than skb->sk. I was worried about layout but sk_net is part of sock_common so its fine; comment withdrawn. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Florian Westphal <fw@strlen.de> writes: > Kevin Cernekee <cernekee@chromium.org> wrote: >> @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) >> const struct xt_owner_match_info *info = par->matchinfo; >> const struct file *filp; >> struct sock *sk = skb_to_full_sk(skb); >> + const struct net *net; >> >> if (sk == NULL || sk->sk_socket == NULL) >> return (info->match ^ info->invert) == 0; >> @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) >> return ((info->match ^ info->invert) & >> (XT_OWNER_UID | XT_OWNER_GID)) == 0; >> >> + net = sock_net(skb->sk); > > I think you need to use sock_net(sk) as skb_to_full_sk(skb) can return something > other than skb->sk. Actually this should be "par->net". That did not exist a few years ago when the patch was written but it does now, and that should simplify things a little bit, and remove any guess work or uncertainty. Eric -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 13, 2016 at 09:06:55PM -0500, Eric W. Biederman wrote: > Florian Westphal <fw@strlen.de> writes: > > > Kevin Cernekee <cernekee@chromium.org> wrote: > >> @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) > >> const struct xt_owner_match_info *info = par->matchinfo; > >> const struct file *filp; > >> struct sock *sk = skb_to_full_sk(skb); > >> + const struct net *net; > >> > >> if (sk == NULL || sk->sk_socket == NULL) > >> return (info->match ^ info->invert) == 0; > >> @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) > >> return ((info->match ^ info->invert) & > >> (XT_OWNER_UID | XT_OWNER_GID)) == 0; > >> > >> + net = sock_net(skb->sk); > > > > I think you need to use sock_net(sk) as skb_to_full_sk(skb) can return something > > other than skb->sk. > > Actually this should be "par->net". That did not exist a few years ago > when the patch was written but it does now, and that should simplify > things a little bit, and remove any guess work or uncertainty. Right. BTW, could you also send a follow up patch to update net/netfilter/nft_meta.c? We have similar support for socket owner in nf_tables as well (actually it will be a more simple patch that this, I would expect). Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c index 1302b475abcb..e1c1f31ef1e9 100644 --- a/net/netfilter/xt_owner.c +++ b/net/netfilter/xt_owner.c @@ -21,11 +21,39 @@ static int owner_check(const struct xt_mtchk_param *par) { struct xt_owner_match_info *info = par->matchinfo; + struct net *net = par->net; - /* For now only allow adding matches from the initial user namespace */ + /* Only allow the common case where the userns of the writer + * matches the userns of the network namespace. + */ if ((info->match & (XT_OWNER_UID|XT_OWNER_GID)) && - (current_user_ns() != &init_user_ns)) + (current_user_ns() != net->user_ns)) return -EINVAL; + + /* Ensure the uids are valid */ + 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_valid(uid_min) || !uid_valid(uid_max) || + (info->uid_max < info->uid_min) || + uid_lt(uid_max, uid_min)) { + return -EINVAL; + } + } + + /* Ensure the gids are valid */ + if (info->match & XT_OWNER_GID) { + kgid_t gid_min = make_kgid(net->user_ns, info->gid_min); + kgid_t gid_max = make_kgid(net->user_ns, info->gid_max); + + if (!gid_valid(gid_min) || !gid_valid(gid_max) || + (info->gid_max < info->gid_min) || + gid_lt(gid_max, gid_min)) { + return -EINVAL; + } + } + return 0; } @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) const struct xt_owner_match_info *info = par->matchinfo; const struct file *filp; struct sock *sk = skb_to_full_sk(skb); + const struct net *net; if (sk == NULL || sk->sk_socket == NULL) return (info->match ^ info->invert) == 0; @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) return ((info->match ^ info->invert) & (XT_OWNER_UID | XT_OWNER_GID)) == 0; + net = sock_net(skb->sk); if (info->match & XT_OWNER_UID) { - kuid_t uid_min = make_kuid(&init_user_ns, info->uid_min); - kuid_t uid_max = make_kuid(&init_user_ns, info->uid_max); + 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)) ^ !(info->invert & XT_OWNER_UID)) @@ -60,8 +90,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par) } if (info->match & XT_OWNER_GID) { - kgid_t gid_min = make_kgid(&init_user_ns, info->gid_min); - kgid_t gid_max = make_kgid(&init_user_ns, info->gid_max); + kgid_t gid_min = make_kgid(net->user_ns, info->gid_min); + kgid_t gid_max = make_kgid(net->user_ns, info->gid_max); if ((gid_gte(filp->f_cred->fsgid, gid_min) && gid_lte(filp->f_cred->fsgid, gid_max)) ^ !(info->invert & XT_OWNER_GID))