[iptables PATCH] xshared: Fix response to unprivileged users

[iptables PATCH] xshared: Fix response to unprivileged users

[Phil Sutter]

Expected behaviour in both variants is:

* Print help without error, append extension help if -m and/or -j
  options are present
* Indicate lack of permissions in an error message for anything else

With iptables-nft, this was broken basically from day 1. Shared use of
do_parse() then somewhat broke legacy: it started complaining about
inability to create a lock file.

Fix this by making iptables-nft assume extension revision 0 is present
if permissions don't allow to verify. This is consistent with legacy.

Second part is to exit directly after printing help - this avoids having
to make the following code "nop-aware" to prevent privileged actions.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c                                |  5 ++
 .../testcases/iptables/0008-unprivileged_0    | 60 +++++++++++++++++++
 iptables/xshared.c                            |  3 +-
 3 files changed, 66 insertions(+), 2 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/iptables/0008-unprivileged_0

diff --git a/iptables/nft.c b/iptables/nft.c
index 72f7cf1315661..b5de687c5c4cd 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -3312,6 +3312,11 @@ int nft_compatible_revision(const char *name, uint8_t rev, int opt)
 err:
 	mnl_socket_close(nl);
 
+	/* pretend revision 0 is valid if not permitted to check -
+	 * this is required for printing extension help texts as user */
+	if (ret < 0 && errno == EPERM && rev == 0)
+		return 1;
+
 	return ret < 0 ? 0 : 1;
 }
 
diff --git a/iptables/tests/shell/testcases/iptables/0008-unprivileged_0 b/iptables/tests/shell/testcases/iptables/0008-unprivileged_0
new file mode 100755
index 0000000000000..43e3bc8721dbd
--- /dev/null
+++ b/iptables/tests/shell/testcases/iptables/0008-unprivileged_0
@@ -0,0 +1,60 @@
+#!/bin/bash
+
+# iptables may print match/target specific help texts
+# help output should work for unprivileged users
+
+run() {
+	echo "running: $*" >&2
+	runuser -u nobody -- "$@"
+}
+
+grep_or_rc() {
+	declare -g rc
+	grep -q "$*" && return 0
+	echo "missing in output: $*" >&2
+	return 1
+}
+
+out=$(run $XT_MULTI iptables --help)
+let "rc+=$?"
+grep_or_rc "iptables -h (print this help information)" <<< "$out"
+let "rc+=$?"
+
+out=$(run $XT_MULTI iptables -m limit --help)
+let "rc+=$?"
+grep_or_rc "limit match options:" <<< "$out"
+let "rc+=$?"
+
+out=$(run $XT_MULTI iptables -p tcp --help)
+let "rc+=$?"
+grep_or_rc "tcp match options:" <<< "$out"
+let "rc+=$?"
+
+out=$(run $XT_MULTI iptables -j DNAT --help)
+let "rc+=$?"
+grep_or_rc "DNAT target options:" <<< "$out"
+let "rc+=$?"
+
+out=$(run $XT_MULTI iptables -p tcp -j DNAT --help)
+let "rc+=$?"
+grep_or_rc "tcp match options:" <<< "$out"
+let "rc+=$?"
+out=$(run $XT_MULTI iptables -p tcp -j DNAT --help)
+let "rc+=$?"
+grep_or_rc "DNAT target options:" <<< "$out"
+let "rc+=$?"
+
+
+run $XT_MULTI iptables -L 2>&1 | \
+	grep_or_rc "Permission denied"
+let "rc+=$?"
+
+run $XT_MULTI iptables -A FORWARD -p tcp --dport 123 2>&1 | \
+	grep_or_rc "Permission denied"
+let "rc+=$?"
+
+run $XT_MULTI iptables -A FORWARD -j DNAT --to-destination 1.2.3.4 2>&1 | \
+	grep_or_rc "Permission denied"
+let "rc+=$?"
+
+exit $rc
diff --git a/iptables/xshared.c b/iptables/xshared.c
index c5a93290be427..1fd7acc953ed7 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -1473,8 +1473,7 @@ void do_parse(int argc, char *argv[],
 					XTF_TRY_LOAD, &cs->matches);
 
 			xt_params->print_help(cs->matches);
-			p->command = CMD_NONE;
-			return;
+			exit(0);
 
 			/*
 			 * Option selection
-- 
2.34.1

Re: [iptables PATCH] xshared: Fix response to unprivileged users

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Expected behaviour in both variants is:
> 
> * Print help without error, append extension help if -m and/or -j
>   options are present
> * Indicate lack of permissions in an error message for anything else
> 
> With iptables-nft, this was broken basically from day 1. Shared use of
> do_parse() then somewhat broke legacy: it started complaining about
> inability to create a lock file.
> 
> Fix this by making iptables-nft assume extension revision 0 is present
> if permissions don't allow to verify. This is consistent with legacy.
> 
> Second part is to exit directly after printing help - this avoids having
> to make the following code "nop-aware" to prevent privileged actions.

Thanks!

Reviewed-by: Florian Westphal <fw@strlen.de>

Re: [iptables PATCH] xshared: Fix response to unprivileged users

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

Hi Phil,

On Thu, Jan 20, 2022 at 11:16:53AM +0100, Phil Sutter wrote:
> Expected behaviour in both variants is:
> 
> * Print help without error, append extension help if -m and/or -j
>   options are present
> * Indicate lack of permissions in an error message for anything else
> 
> With iptables-nft, this was broken basically from day 1. Shared use of
> do_parse() then somewhat broke legacy: it started complaining about
> inability to create a lock file.
> 
> Fix this by making iptables-nft assume extension revision 0 is present
> if permissions don't allow to verify. This is consistent with legacy.
> 
> Second part is to exit directly after printing help - this avoids having
> to make the following code "nop-aware" to prevent privileged actions.

On top of this patch, it should be possible to allow for some
nfnetlink command to be used from unpriviledged process.

I'm attaching a sketch patch, it skips module autoload which is should
not be triggered by an unpriviledged process.

This should allow for better help with -m/-j if the module is present.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 3313 bytes --]

diff --git a/include/linux/netfilter/nfnetlink.h b/include/linux/netfilter/nfnetlink.h
index 241e005f290a..1e10bf19af93 100644
--- a/include/linux/netfilter/nfnetlink.h
+++ b/include/linux/netfilter/nfnetlink.h
@@ -28,6 +28,7 @@ struct nfnl_callback {
 	const struct nla_policy	*policy;
 	enum nfnl_callback_type	type;
 	__u16			attr_count;
+	bool			unpriviledged;
 };
 
 enum nfnl_abort_action {
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 7e2c8dd01408..0f450c8a43ac 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -211,21 +211,24 @@ EXPORT_SYMBOL_GPL(nfnetlink_broadcast);
 static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 			     struct netlink_ext_ack *extack)
 {
+	const struct nfnetlink_subsystem *ss;
 	struct net *net = sock_net(skb->sk);
 	const struct nfnl_callback *nc;
-	const struct nfnetlink_subsystem *ss;
+	bool cap_net_admin;
 	int type, err;
 
 	/* All the messages must at least contain nfgenmsg */
 	if (nlmsg_len(nlh) < sizeof(struct nfgenmsg))
 		return 0;
 
+	cap_net_admin = netlink_net_capable(skb, CAP_NET_ADMIN);
+
 	type = nlh->nlmsg_type;
 replay:
 	rcu_read_lock();
 
 	ss = nfnetlink_get_subsys(type);
-	if (!ss) {
+	if (!ss && cap_net_admin) {
 #ifdef CONFIG_MODULES
 		rcu_read_unlock();
 		request_module("nfnetlink-subsys-%d", NFNL_SUBSYS_ID(type));
@@ -245,6 +248,11 @@ static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EINVAL;
 	}
 
+	if (cap_net_admin && !nc->unpriviledged) {
+		rcu_read_unlock();
+		return -EPERM;
+	}
+
 	{
 		int min_len = nlmsg_total_size(sizeof(struct nfgenmsg));
 		struct nfnl_net *nfnlnet = nfnl_pernet(net);
@@ -607,6 +615,11 @@ static void nfnetlink_rcv_skb_batch(struct sk_buff *skb, struct nlmsghdr *nlh)
 	u32 gen_id = 0;
 	u16 res_id;
 
+	if (!netlink_net_capable(skb, CAP_NET_ADMIN)) {
+		netlink_ack(skb, nlh, -EPERM, NULL);
+		return;
+	}
+
 	msglen = NLMSG_ALIGN(nlh->nlmsg_len);
 	if (msglen > skb->len)
 		msglen = skb->len;
@@ -643,11 +656,6 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 	    skb->len < nlh->nlmsg_len)
 		return;
 
-	if (!netlink_net_capable(skb, CAP_NET_ADMIN)) {
-		netlink_ack(skb, nlh, -EPERM, NULL);
-		return;
-	}
-
 	if (nlh->nlmsg_type == NFNL_MSG_BATCH_BEGIN)
 		nfnetlink_rcv_skb_batch(skb, nlh);
 	else
diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c
index f69cc73c5813..a3a23707fcb3 100644
--- a/net/netfilter/nft_compat.c
+++ b/net/netfilter/nft_compat.c
@@ -677,10 +677,13 @@ static int nfnl_compat_get_rcu(struct sk_buff *skb,
 		return -EINVAL;
 
 	rcu_read_unlock();
-	try_then_request_module(xt_find_revision(family, name, rev, target, &ret),
-				fmt, name);
-	if (ret < 0)
-		goto out_put;
+	if (netlink_net_capable(skb, CAP_NET_ADMIN)) {
+		try_then_request_module(xt_find_revision(family, name, rev,
+							 target, &ret),
+					fmt, name);
+		if (ret < 0)
+			goto out_put;
+	}
 
 	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (skb2 == NULL) {
@@ -718,7 +721,8 @@ static const struct nfnl_callback nfnl_nft_compat_cb[NFNL_MSG_COMPAT_MAX] = {
 		.call		= nfnl_compat_get_rcu,
 		.type		= NFNL_CB_RCU,
 		.attr_count	= NFTA_COMPAT_MAX,
-		.policy		= nfnl_compat_policy_get
+		.policy		= nfnl_compat_policy_get,
+		.unpriviledged	= true,
 	},
 };
 

Re: [iptables PATCH] xshared: Fix response to unprivileged users

[Phil Sutter]

Hi Pablo,

On Thu, Jan 27, 2022 at 05:28:39PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 20, 2022 at 11:16:53AM +0100, Phil Sutter wrote:
> > Expected behaviour in both variants is:
> > 
> > * Print help without error, append extension help if -m and/or -j
> >   options are present
> > * Indicate lack of permissions in an error message for anything else
> > 
> > With iptables-nft, this was broken basically from day 1. Shared use of
> > do_parse() then somewhat broke legacy: it started complaining about
> > inability to create a lock file.
> > 
> > Fix this by making iptables-nft assume extension revision 0 is present
> > if permissions don't allow to verify. This is consistent with legacy.
> > 
> > Second part is to exit directly after printing help - this avoids having
> > to make the following code "nop-aware" to prevent privileged actions.
> 
> On top of this patch, it should be possible to allow for some
> nfnetlink command to be used from unpriviledged process.
> 
> I'm attaching a sketch patch, it skips module autoload which is should
> not be triggered by an unpriviledged process.
> 
> This should allow for better help with -m/-j if the module is present.

That's interesting. What's the use-case? With my patch, extension help
text printing works fine as unprivileged user. Does it allow to drop the
"revision == 0 && EPERM" hack?

Thanks, Phil

Re: [iptables PATCH] xshared: Fix response to unprivileged users

[Pablo Neira Ayuso]

On Thu, Jan 27, 2022 at 06:08:31PM +0100, Phil Sutter wrote:
> Hi Pablo,
> 
> On Thu, Jan 27, 2022 at 05:28:39PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Jan 20, 2022 at 11:16:53AM +0100, Phil Sutter wrote:
> > > Expected behaviour in both variants is:
> > > 
> > > * Print help without error, append extension help if -m and/or -j
> > >   options are present
> > > * Indicate lack of permissions in an error message for anything else
> > > 
> > > With iptables-nft, this was broken basically from day 1. Shared use of
> > > do_parse() then somewhat broke legacy: it started complaining about
> > > inability to create a lock file.
> > > 
> > > Fix this by making iptables-nft assume extension revision 0 is present
> > > if permissions don't allow to verify. This is consistent with legacy.
> > > 
> > > Second part is to exit directly after printing help - this avoids having
> > > to make the following code "nop-aware" to prevent privileged actions.
> > 
> > On top of this patch, it should be possible to allow for some
> > nfnetlink command to be used from unpriviledged process.
> > 
> > I'm attaching a sketch patch, it skips module autoload which is should
> > not be triggered by an unpriviledged process.
> > 
> > This should allow for better help with -m/-j if the module is present.
> 
> That's interesting. What's the use-case? With my patch, extension help
> text printing works fine as unprivileged user. Does it allow to drop the
> "revision == 0 && EPERM" hack?

Your patch is needed because we have to deal with older kernels.

You assume revision 0 in case of EPERM. My patch provides better help
if the module is present since there is no need to assume revision 0.

Anyway, I think your approach is fine for the unpriviledged scenario
you describe. I just wanted to write here that there is room to extend
nfnetlink to support for unpriviledged requests.

Re: [iptables PATCH] xshared: Fix response to unprivileged users

[Phil Sutter]

On Thu, Jan 27, 2022 at 06:21:10PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 27, 2022 at 06:08:31PM +0100, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Thu, Jan 27, 2022 at 05:28:39PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Jan 20, 2022 at 11:16:53AM +0100, Phil Sutter wrote:
> > > > Expected behaviour in both variants is:
> > > > 
> > > > * Print help without error, append extension help if -m and/or -j
> > > >   options are present
> > > > * Indicate lack of permissions in an error message for anything else
> > > > 
> > > > With iptables-nft, this was broken basically from day 1. Shared use of
> > > > do_parse() then somewhat broke legacy: it started complaining about
> > > > inability to create a lock file.
> > > > 
> > > > Fix this by making iptables-nft assume extension revision 0 is present
> > > > if permissions don't allow to verify. This is consistent with legacy.
> > > > 
> > > > Second part is to exit directly after printing help - this avoids having
> > > > to make the following code "nop-aware" to prevent privileged actions.
> > > 
> > > On top of this patch, it should be possible to allow for some
> > > nfnetlink command to be used from unpriviledged process.
> > > 
> > > I'm attaching a sketch patch, it skips module autoload which is should
> > > not be triggered by an unpriviledged process.
> > > 
> > > This should allow for better help with -m/-j if the module is present.
> > 
> > That's interesting. What's the use-case? With my patch, extension help
> > text printing works fine as unprivileged user. Does it allow to drop the
> > "revision == 0 && EPERM" hack?
> 
> Your patch is needed because we have to deal with older kernels.
> 
> You assume revision 0 in case of EPERM. My patch provides better help
> if the module is present since there is no need to assume revision 0.

Ah, that's a good point. Users always see rev0 help texts, which are
naturally the most limited ones.

> Anyway, I think your approach is fine for the unpriviledged scenario
> you describe. I just wanted to write here that there is room to extend
> nfnetlink to support for unpriviledged requests.

I see, thanks. Yet your approach works only if the module is loaded
already, right?

Unless it's useful elsewhere as well, I don't think it's worth the
effort for iptables alone - requesting extension help as non-root is
quite a corner-case IMHO.

Cheers, Phil

Re: [iptables PATCH] xshared: Fix response to unprivileged users

[Pablo Neira Ayuso]

On Thu, Jan 27, 2022 at 06:29:26PM +0100, Phil Sutter wrote:
> On Thu, Jan 27, 2022 at 06:21:10PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Jan 27, 2022 at 06:08:31PM +0100, Phil Sutter wrote:
> > > Hi Pablo,
> > > 
> > > On Thu, Jan 27, 2022 at 05:28:39PM +0100, Pablo Neira Ayuso wrote:
> > > > On Thu, Jan 20, 2022 at 11:16:53AM +0100, Phil Sutter wrote:
> > > > > Expected behaviour in both variants is:
> > > > > 
> > > > > * Print help without error, append extension help if -m and/or -j
> > > > >   options are present
> > > > > * Indicate lack of permissions in an error message for anything else
> > > > > 
> > > > > With iptables-nft, this was broken basically from day 1. Shared use of
> > > > > do_parse() then somewhat broke legacy: it started complaining about
> > > > > inability to create a lock file.
> > > > > 
> > > > > Fix this by making iptables-nft assume extension revision 0 is present
> > > > > if permissions don't allow to verify. This is consistent with legacy.
> > > > > 
> > > > > Second part is to exit directly after printing help - this avoids having
> > > > > to make the following code "nop-aware" to prevent privileged actions.
> > > > 
> > > > On top of this patch, it should be possible to allow for some
> > > > nfnetlink command to be used from unpriviledged process.
> > > > 
> > > > I'm attaching a sketch patch, it skips module autoload which is should
> > > > not be triggered by an unpriviledged process.
> > > > 
> > > > This should allow for better help with -m/-j if the module is present.
> > > 
> > > That's interesting. What's the use-case? With my patch, extension help
> > > text printing works fine as unprivileged user. Does it allow to drop the
> > > "revision == 0 && EPERM" hack?
> > 
> > Your patch is needed because we have to deal with older kernels.
> > 
> > You assume revision 0 in case of EPERM. My patch provides better help
> > if the module is present since there is no need to assume revision 0.
> 
> Ah, that's a good point. Users always see rev0 help texts, which are
> naturally the most limited ones.
> 
> > Anyway, I think your approach is fine for the unpriviledged scenario
> > you describe. I just wanted to write here that there is room to extend
> > nfnetlink to support for unpriviledged requests.
> 
> I see, thanks. Yet your approach works only if the module is loaded
> already, right?

Yes, I don't think we should allow to autoload modules for non-CAP_NET_ADMIN.

> Unless it's useful elsewhere as well, I don't think it's worth the
> effort for iptables alone - requesting extension help as non-root is
> quite a corner-case IMHO.

Agreed.