| Message ID | 20220120101653.28280-1-phil@nwl.cc |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [iptables] xshared: Fix response to unprivileged users | expand |
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>
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.
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
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.
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
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.
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
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