Florian Westphal [Mon, 12 Nov 2018 13:40:41 +0000 (14:40 +0100)]
xtables: add 'printf' attribute to xlate_add
This allows gcc to check format string vs. passed arguments.
Fix the fallout from this as well, typical warning produced is:
libebt_mark_m.c:112:28: warning: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'long unsigned int' [-Wformat=]
xt_xlate_add(xl, "and 0x%x %s0 ", info->mask, ...
~^ ~~~~~~~~~~
so add the required casts or fixup format strings as needed.
libxt_conntrack also passed an unneeded argument (port), so remove that.
In arptables-classic, the kernel will add dev->addr_len to the
arp header base address to obtain the correct location, but we cannot
do this in nf_tables, at least not at this time (we need a fixed offset
value).
but if user did not provide "--h-length 6" argument, then this won't
work even for ethernet, as the payload expression will be told to load
the first 4 bytes of arp header source mac address (sender hw address).
Fix this by pre-initialising arhlen to 6.
We also need to set up arhrd. Otherwise, src/dst mac can't be used:
arptables -A INPUT -i lo --destination-mac 11:22:33:44:55:66
arptables v1.8.1 (nf_tables): RULE_APPEND failed (Invalid argument): rule in chain INPUT
This means that matching won't work for AX25, NETROM etc, however,
arptables "classic" can't parse non-ethernet addresses, and makes
ETH_ALEN assumptions in several spots, so this should be fine from
compatibility point of view.
1. check both address and mask, not just first byte of mac
2. use add_addr() for this so mask is also handled via bitwise expr.
3. use the correct offsets.
4. add dissector so we can reverse translate the payload expressions
generated for this.
arptables classic doesn't have arptables-save, it only has a perl
script that attempts to emulate iptables-save. It supports no options,
and thus has no way to dump counters. Add -c option, like iptables to
enable this.
Florian Westphal [Mon, 12 Nov 2018 11:49:11 +0000 (12:49 +0100)]
ebtables: use extrapositioned negation consistently
in the iptables universe, we enforce extrapositioned negation:
! -i foo
"-i ! foo" is not even supported anymore.
At least make sure that ebtables prints the former syntax everywhere as
well so we don't have a mix of both ways.
Parsing of --option ! 42 will still work for backwards compat reasons.
iptables-tests: do not append xtables-multi to external commands
Lines starting by @ can be used to invoke an external command of any
kind. Do not add xtables-multi here since we may want to execute a
non-iptables command.
-j CONTINUE can be added, but it can't be removed:
extensions/libebt_standard.t: ERROR: line 5 (cannot find: ebtables -I INPUT -d de:ad:be:ef:00:00 -j CONTINUE)
This problem stems from silly ambiguity in ebtables-nft vs. iptables.
In iptables, you can do
iptables -A INPUT
(no -j)
in ebtables, you can do either
ebtables -A INPUT
or
ebtables -A INPUT -j CONTINUE
both are *supposed* to be the same (and they do the same even
in ebtables-nft on netlink side).
However, the temprary binary representation within ebtables-nft is not
the same: when parsing -j CONTINUE, we add a standard target, then omit
it later in _add_target().
When translating netlink representation to ebt binary one,
we do not add a standard target and instead just print '-j CONTINUE'
when listing rules.
So when doing
-I INPUT -j CONTINUE
-D INPUT -j CONTINUE
the -D operation fails because it has a standard target in the binary
representation, whereas the rule we obtained from translating
nftables netlink back to ebtables' binary represenation doesn't.
Phil Sutter [Wed, 31 Oct 2018 19:13:34 +0000 (20:13 +0100)]
xtables: Fix for matching rules with wildcard interfaces
Due to xtables_parse_interface() and parse_ifname() being misaligned
regarding interface mask setting, rules containing a wildcard interface
added with iptables-nft could neither be checked nor deleted.
As suggested, introduce extensions/iptables.t to hold checks for
built-in selectors. This file is picked up by iptables-test.py as-is.
The only limitation is that iptables is being used for it, so no
ip6tables-specific things can be tested with it (for now).
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Phil Sutter [Thu, 11 Oct 2018 11:30:38 +0000 (13:30 +0200)]
xtables: Remove target_maxnamelen field
This is a partial revert of commit 9f075031a1973 ("Combine
parse_target() and command_jump() implementations"): Upstream prefers to
reduce max chain name length of arptables by two characters instead of
the introduced struct xtables_globals field which requires to bump
library API version.
Fixes: 9f075031a1973 ("Combine parse_target() and command_jump() implementations") Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Chenbo Feng [Tue, 2 Oct 2018 01:23:07 +0000 (18:23 -0700)]
extensions: libxt_quota: Allow setting the remaining quota
The current xt_quota module cannot track the current remaining quota
of a specific rule. Everytime an unrelated rule is updated in the same
iptables table, the quota will be reset. This is not a very useful
function for iptables that get changed at run time. This patch fixes the
above problem by adding a new field in the struct that records the
current remaining quota.
Fixed a print out bug in verbose print out wrt. inversion.
Signed-off-by: Chenbo Feng <fengc@google.com> Suggested-by: Maciej Żenczykowski <maze@google.com> Reviewed-by: Maciej Żenczykowski <maze@google.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Phil Sutter [Mon, 24 Sep 2018 17:25:24 +0000 (19:25 +0200)]
Combine parse_target() and command_jump() implementations
Merge these two functions from xtables, iptables, ip6tables and
arptables. Both functions were basically identical in the first three,
only the last one required a bit more attention.
To eliminate access to 'invflags' in variant-specific location, move the
call to set_option() into callers. This is actually consistent with
parsing of other options in them.
As with command_match(), use xt_params instead of the different
*_globals objects to refer to 'opts' and 'orig_opts'.
It was necessary to rename parse_target() as it otherwise clashes with a
static function of same name in libxt_SET.
In arptables, the maximum allowed target name is a bit larger, so
introduce xtables_globals.target_maxnamelen defining the value. It is
used in the shared xt_parse_target() implementation.
Implementation of command_jump() in arptables diverted from the others
for no obvious reason. The call to parse_target() was done outside of it
and a pointer to cs->arp was passed but not used inside.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Mon, 24 Sep 2018 17:25:23 +0000 (19:25 +0200)]
Combine command_match() implementations
This merges the basically identical implementations of command_match()
from xtables, iptables and ip6tables into one. The only required
adjustment was to make use of xt_params instead of the different
*_globals objects.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Mon, 24 Sep 2018 17:25:22 +0000 (19:25 +0200)]
libiptc: NULL-terminate errorname
In struct chain_head, field 'name' is of size TABLE_MAXNAMELEN, hence
copying its content into 'error_name' field of struct xt_error_target
which is two bytes shorter may overflow. Make sure this doesn't happen
by using strncpy() and set the last byte to zero.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:17:07 +0000 (15:17 +0200)]
arptables: Fix incorrect strcmp() in nft_arp_rule_find()
Since nft_arp_rule_to_cs() may not set cs->jumpto, later call to
strcmp() may be passed a NULL pointer. Therefore check if the pointer is
valid before doing so.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:17:06 +0000 (15:17 +0200)]
xtables: Don't read garbage in nft_ipv4_parse_payload()
The problem here is that get_frag() does not set 'inv' in any case, so
when later checking its value, garbage may be read. Sanitize this case
by setting 'inv' to false before calling get_frag().
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:17:05 +0000 (15:17 +0200)]
libxtables: Use posix_spawn() instead of vfork()
According to covscan, vfork() may lead to a deadlock in the parent
process. It suggests to use posix_spawn() instead. Since the latter
combines vfork() and exec() calls, use it for xtables_insmod().
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:17:03 +0000 (15:17 +0200)]
extensions: libebt_ip{, 6}: Drop pointless error checking
Since info->protocol is of type __u8, its value will never become -1.
Apart from that, xtables_parse_protocol() calls xt_params->exit_err() in
case of error, so this code is dead anyway.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:17:00 +0000 (15:17 +0200)]
iptables: Use print_ifaces() from xtables
Move the function to xshared.c for common use between legacy and xtables
sources. While being at it, silence a covscan warning triggered by that
function as it couldn't verify input buffers won't exceed IFNAMSIZ.
Therefore use snprintf() when writing to the local buffer.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:16:59 +0000 (15:16 +0200)]
Share print_ipv{4,6}_addr() from xtables
These functions contain code which occurs in legacy's print_firewall()
functions, so use them there.
Rename them to at least make clear they print more than a single
address.
Also introduce ipv{4,6}_addr_to_string() which take care of converting
an address/netmask pair into string representation in a way which
doesn't upset covscan (since that didn't detect that 'buf' may not be
exceeded by the strings written into it.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:16:55 +0000 (15:16 +0200)]
libxtables: Don't read garbage in xtables_strtoui()
If xtables_strtoul() fails, it returns false and data pointed to by
parameter 'value' is undefined. Hence avoid copying that data in
xtables_strtoui() if the call failed.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:16:54 +0000 (15:16 +0200)]
libxtables: Avoid calling memcpy() with NULL source
Both affected functions check if 'oldopts' is NULL once but later seem
to ignore that possibility. To catch up on that, increment the pointer
only if it isn't NULL, also don't copy its content into the merged
options buffer in that case.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:16:53 +0000 (15:16 +0200)]
libiptc: Simplify alloc_handle() function signature
This change originated from covscan complaining about the strcpy() call
with an unknown size source buffer. But in fact, the size is known (and
equal to the destination size), so pass a pointer to STRUCT_GETINFO to
alloc_handle() instead of it's fields separately. Hopefully this will
silence covscan.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:16:52 +0000 (15:16 +0200)]
libxt_time: Drop initialization of variable 'year'
The variable is not read before being assigned the return value of
strtoul(), thefore the initialization is useless. And since after this
change parameter 'end' becomes unused, drop it as well.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:16:51 +0000 (15:16 +0200)]
libxt_ipvs: Avoid potential buffer overrun
Just like with libxt_conntrack, get rid of the temporary buffer. The
comment even states that it was copied from there, so just make them
identical again.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:16:50 +0000 (15:16 +0200)]
libxt_conntrack: Avoid potential buffer overrun
In print_addr(), a resolved hostname is written into a buffer without
size check. Since BUFSIZ is typically 8192 bytes, this shouldn't be an
issue, though covscan complained about it. Fix the code by using
conntrack_dump_addr() as an example.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:16:49 +0000 (15:16 +0200)]
libxt_conntrack: Version 0 does not support XT_CONNTRACK_DIRECTION
Since sinfo->flags is only 8 bytes large, checking for
XT_CONNTRACK_DIRECTION bit (which has value 1 << 12) will always return
false, so drop this dead code.
Phil Sutter [Wed, 19 Sep 2018 13:16:46 +0000 (15:16 +0200)]
ip{, 6}tables-restore: Fix for uninitialized array 'curtable'
When reading sufficiently malformed input, parser might hit end of
loop without having written the current table name into curtable and
therefore calling strcmp() with uninitialized buffer. Avoid this by
setting curtable to zero upon declaration.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:16:45 +0000 (15:16 +0200)]
Mark fall through cases in switch() statements
Typical covscan complaint, non-empty fall throughs should be marked as
such. There was but a single case which should break instead, namely in
libebt_log.c: It is not critical, since the next case merely asserts
'invert' being zero (which can't be as it was checked before). But while
being at it, introduce log_chk_inv() to consolidate the semantically
equal cases for the various log types.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:16:44 +0000 (15:16 +0200)]
libxtables: Integrate getethertype.c from xtables core
This moves getethertype.c into libxtables so that both extensions and
xtables-nft-multi may use the implementations therein. New users are
libebt_arp and libebt_vlan which drop their own duplicated
implementations of getethertypebyname() for the shared one.
This change originated from a covscan report of extensions'
implementations not checking fopen() return value which should be
implicitly fixed by this as well.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 19 Sep 2018 13:16:43 +0000 (15:16 +0200)]
xtables: Fix for wrong assert() in __nft_table_flush()
The code obviously tries to assert that nft_table_builtin_find()
returned a valid pointer before dereferencing it, but the wrong argument
was given. Assume this is just a typo and insert the missing underscore.
Phil Sutter [Wed, 19 Sep 2018 13:16:42 +0000 (15:16 +0200)]
nfnl_osf: Drop pointless check in xt_osf_strchr()
Although it remains unclear what the original intention behind the
affected code was, but 'tmp + 1' always evaluates true since 'tmp' is a
pointer value.
Phil Sutter [Mon, 17 Sep 2018 11:38:33 +0000 (13:38 +0200)]
libxt_string: Fix array out of bounds check
Commit 56d7ab42f3782 ("libxt_string: Avoid potential array out of bounds
access") tried to fix parse_hex_string() for overlong strings but the
change still allowed for 'sindex' to become XT_STRING_MAX_PATTERN_SIZE
which leads to access of first byte after info->pattern. This is not
really a problem because it merely overwrites info->patlen before
calling xtables_error() later, but covscan still detects it so it's
still worth fixing.
The crucial bit here is that 'sindex' has to be incremented at end of
the last iteration since its value is used for info->patlen. Hence just
move the overflow check to the beginning of the loop.
Fixes: 56d7ab42f3782 ("libxt_string: Avoid potential array out of bounds access") Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Phil Sutter [Mon, 10 Sep 2018 21:32:34 +0000 (23:32 +0200)]
xtables-save: Ignore uninteresting tables
When running iptables-nft-save with other tables present, the dump
succeeded but the tool complained about those other tables. In an
environment where iptables-nft and nftables are uses in parallel, this
is an expected situation, so only complain about incompatible builtin
tables.
While being at it, move the table existence check from __do_output()
into do_output() since the former may be called from
nft_for_each_table() in which case the table is guaranteed to exist.
Also use nft_table_builtin_find() in nft_is_table_compatible() instead
of open-coding the search by name in h->tables.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Phil Sutter [Mon, 10 Sep 2018 21:35:17 +0000 (23:35 +0200)]
extensions: REJECT: Merge reject tables
Initial motivation for this was a covscan report for potential array out
of bounds access in REJECT_xlate (a false-positive, because all possible
values of reject->with occur in reject_table_xlate).
Use reject types as array indices of reject_table so that reject->with
serves as array index. Also merge reject_table_xlate into it.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Mon, 10 Sep 2018 21:35:16 +0000 (23:35 +0200)]
libxt_string: Avoid potential array out of bounds access
The pattern index variable 'sindex' is bounds checked before
incrementing it, which means in the next loop iteration it might already
match the bounds check condition but is used anyway.
Fix this by incrementing the index before performing the bounds check.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Mon, 10 Sep 2018 21:35:14 +0000 (23:35 +0200)]
libiptc: Avoid side-effect in memset() calls
These calls to memset() are passed a length argument which exceeds
t->target.u.user.name's length by one byte and hence overwrite
t->target.u.user.revision as well (relying upon no padding to happen
between both).
Avoid this obscure behaviour by passing the correct field size and
explicitly overwriting 'revision' field.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Wed, 5 Sep 2018 17:14:40 +0000 (19:14 +0200)]
xtables: Accept --wait in iptables-nft-restore
Passing --wait option to iptables-nft-restore led to program abort
because the flag parameter was not skipped. Mimick iptables-restore
behaviour when encountering --wait or --wait-interval options (but still
ignore the parameter).
Fixes: b9d7b49d84bc2 ("xtables-compat: restore: sync options with iptables-restore") Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Fri, 7 Sep 2018 15:06:21 +0000 (17:06 +0200)]
xtables: Don't check all rules for being compatible
Commit f8e29a13fed8d ("xtables: avoid bogus 'is incompatible' warning")
fixed for compatibility checking to extend over all chains, not just the
relevant ones. This patch does the same for rules: Make sure only rules
belonging to the relevant table are being considered.
Note that comparing the rule's table name is sufficient here since the
table family is already considered when populating the rule cache.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
The logic to replicate 'iptables-restore --noflush' behaviour of
flushing custom chains if listed in the dump was broken for chains being
referenced. A minimal dump reproducing the issue is:
With --noflush, this can be restored just once in iptables-nft-restore.
Consecutive attempts return an error since xtables tries to delete the
referenced chain and recreate it instead of performing a real flush.
Fix this by really flushing the custom chain in 'chain_user_flush'
callback and running 'chain_user_add' callback only if the chain doesn't
exist already.
Fixes: df3d92bec6007 ("xtables-compat-restore: flush user-defined chains with -n") Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Fri, 31 Aug 2018 20:30:58 +0000 (22:30 +0200)]
xtables: Drop use of IP6T_F_PROTO
Setting this bit in cs->fw6.ipv6.flags was done only for rules parsed
from command line, not for those read from kernel. As a result,
appropriate rules could not be deleted. A simple test case is:
| # ip6tables-nft -A INPUT -p tcp -j ACCEPT
| # ip6tables-nft -D INPUT -p tcp -j ACCEPT
| iptables: Bad rule (does a matching rule exist in that chain?).
Since the flag is not used anywhere in xtables-nft, dropping its use fixes
the bug as well as setting it in both cases.
Fixes: 5ee03e6df4172 ("xtables: Use meta l4proto for -p match") Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Tue, 28 Aug 2018 19:44:16 +0000 (21:44 +0200)]
xtables: Fix for deleting rules with comment
Comment match allocation in command_match() and
nft_rule_to_iptables_command_state() were misaligned in that the latter
set match_size to just what is required instead of what the match needs
at maximum like the further. This led to failure when comparing them
later and therefore a rule with a comment could not be deleted.
For comments of a specific length, the udata buffer is padded by
libnftnl so nftnl_rule_get_data() returns a length value which is larger
than the string (including NULL-byte). The trailing data is supposed to
be ignored, but compare_matches() can't not know about that detail and
therefore returns a false-negative if trailing data contains junk. To
overcome this, use strncpy() when populating match data in
nft_rule_to_iptables_command_state(). While being at it, make sure
comment match allocation in that function is identical to what
command_match() does with regards to data allocation size. Also use
xtables_calloc() which does the required error checking.
Signed-off-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Florian Westphal <fw@strlen.de>
projects / thirdparty / iptables.git / log
