[PATCH 1/1] iptables-xml: Fix segfault on jump without a target

[Oliver Ford]

As reported in Bugzilla #1152, a segfault occurs in iptables-xml if a
jump or goto argument lacks a target argument. The following input will
segfault:
*filter
:INPUT ACCEPT [0:0]
-A INPUT -p tcp --dport 2200 -j

Problem occurs in do_rule_part, where the existsChain() function is called
with argv[arg + 1]. If the jump/goto argument is the last argument, then
arg + 1 is out of the array bounds. The fix ensures that arg + 1 is within
the array bounds before the call to existsChain() is made.

Signed-off-by: Oliver Ford 
---
 iptables/iptables-xml.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/iptables/iptables-xml.c b/iptables/iptables-xml.c
index 56b8372..80de2de 100644
--- a/iptables/iptables-xml.c
+++ b/iptables/iptables-xml.c
@@ -427,11 +427,8 @@ do_rule_part(char *leveltag1, char *leveltag2, int part, 
int argc,
printf("%s%s", spacer, argv[arg]);
spacer = " ";
} else if (!argvattr[arg] && isTarget(argv[arg])
-  && existsChain(argv[arg + 1])
-  && (2 + arg >= argc)) {
-   if (!((1 + arg) < argc))
-   // no args to -j, -m or -g, ignore & finish loop
-   break;
+  && (arg + 1 < argc)
+  && existsChain(argv[arg + 1])) {
CLOSE_LEVEL(2);
if (level1)
printf("%s", leveli1);
-- 
2.11.0


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6 RFC] netfilter: add audit netns ID

[Paul Moore]

On Wed, May 24, 2017 at 3:44 PM, Eric W. Biederman
 wrote:
> Richard Guy Briggs  writes:
>
>> On 2017-05-24 19:31, Pablo Neira Ayuso wrote:
>>> Cc'ing Eric Biederman.
>>>
>>> On Thu, May 18, 2017 at 01:21:52PM -0400, Richard Guy Briggs wrote:
>>> > diff --git a/net/bridge/netfilter/ebtables.c 
>>> > b/net/bridge/netfilter/ebtables.c
>>> > index 59b63a8..0f77b2a 100644
>>> > --- a/net/bridge/netfilter/ebtables.c
>>> > +++ b/net/bridge/netfilter/ebtables.c
>>> > @@ -27,6 +27,7 @@
>>> >  #include 
>>> >  #include 
>>> >  #include 
>>> > +#define PROC_DYNAMIC_FIRST 0xF000U
>>> >  #include 
>>> >  /* needed for logical [in,out]-dev filtering */
>>> >  #include "../br_private.h"
>>> > @@ -1075,7 +1076,8 @@ static int do_replace_finish(struct net *net, 
>>> > struct ebt_replace *repl,
>>> >ab = audit_log_start(current->audit_context, 
>>> > GFP_KERNEL,
>>> > AUDIT_NETFILTER_CFG);
>>> >if (ab) {
>>> > -  audit_log_format(ab, "op=replace family=%u 
>>> > table=%s entries=%u",
>>> > +  audit_log_format(ab, "op=replace net=%u 
>>> > family=%u table=%s entries=%u",
>>> > +   net->ns.inum - 
>>> > PROC_DYNAMIC_FIRST,
>>>
>>> IIRC, there was a discussion on exposing netns i-node number to
>>> userspace time ago on netdev and Eric Biederman was not happy about
>>> this?
>>
>> He was not happy about it being exposed in the /proc filesystem.  We've
>> been talking since then and while we've not come to a definitive
>> conclusion there is a communication channel open.
>>
>> This is more of an RFC patch than the rest of this set and I didn't
>> seriously expect this one to be accepted, I did want to present the idea
>> to see if there were concerns or better ideas generated how to
>> differentiate this record from a seemingly identical one.  The only
>> other ID would be the network namespace' struct pointer.
>>
>> At this stage, one thing that is missing is a device number to qualify
>> this namespace ID.
>>
>> Once I started printing the namespace proc inode number (minus the
>> starting offset) in decimal, it was very clear what was happenning and
>> seemed worth sharing that debugging tool patch.
>
> If the appropriate device number and full inode number is included I
> don't have any deep problems with the idea.  I don't like the bare inode
> number as we have had in the past and may in the future have these inode
> numbers in multiple filesystems so the inode number by itself is not
> unique.

Let's punt on this netfilter record specific patch until we sort out
the general problem of recording namespace/container information in
the audit records.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6 RFC] netfilter: add audit operation field

[Paul Moore]

On Thu, May 18, 2017 at 1:21 PM, Richard Guy Briggs  wrote:
> Add the operation performed (register or replace) to the NETFILTER_CFG and
> NETFILTER_CFGSOLO records.
>
> Here are sample records for accompanied:
>   type=NETFILTER_CFG msg=audit(1494981627.248:9764): op=replace family=7 
> table=broute entries=0
>
> and unaccompanied cases:
>   type=UNKNOWN[1331] msg=audit(1494815998.178:167): auid=4294967295 uid=0 
> gid=0 ses=4294967295 subj=system_u:system_r:iptables_t:s0 pid=598 
> comm="ip6tables-resto" exe="/usr/sbin/xtables-multi" op=replace family=10 
> table=filter entries=4

A reminder to add new fields to the end of the record.

> See: https://github.com/linux-audit/audit-kernel/issues/25
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  net/bridge/netfilter/ebtables.c |8 
>  net/netfilter/x_tables.c|5 +++--
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 7499232..59b63a8 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1075,7 +1075,7 @@ static int do_replace_finish(struct net *net, struct 
> ebt_replace *repl,
> ab = audit_log_start(current->audit_context, 
> GFP_KERNEL,
>  AUDIT_NETFILTER_CFG);
> if (ab) {
> -   audit_log_format(ab, "family=%u table=%s 
> entries=%u",
> +   audit_log_format(ab, "op=replace family=%u 
> table=%s entries=%u",
>  AF_BRIDGE, repl->name,
>  repl->nentries);
> audit_log_end(ab);
> @@ -1085,7 +1085,7 @@ static int do_replace_finish(struct net *net, struct 
> ebt_replace *repl,
>  AUDIT_NETFILTER_CFGSOLO);
> if (ab) {
> audit_log_task(ab);
> -   audit_log_format(ab, " family=%u table=%s 
> entries=%u",
> +   audit_log_format(ab, " op=replace family=%u 
> table=%s entries=%u",
>  AF_BRIDGE, repl->name,
>  repl->nentries);
> audit_log_end(ab);
> @@ -1259,7 +1259,7 @@ struct ebt_table * ebt_register_table(struct net *net,
> ab = audit_log_start(current->audit_context, 
> GFP_KERNEL,
>  AUDIT_NETFILTER_CFG);
> if (ab) {
> -   audit_log_format(ab, "family=%u table=%s 
> entries=%u",
> +   audit_log_format(ab, "op=register family=%u 
> table=%s entries=%u",
>  AF_BRIDGE, repl->name,
>  repl->nentries);
> audit_log_end(ab);
> @@ -1269,7 +1269,7 @@ struct ebt_table * ebt_register_table(struct net *net,
>  AUDIT_NETFILTER_CFGSOLO);
> if (ab) {
> audit_log_task(ab);
> -   audit_log_format(ab, " family=%u table=%s 
> entries=%u",
> +   audit_log_format(ab, " op=register family=%u 
> table=%s entries=%u",
>  AF_BRIDGE, repl->name,
>  repl->nentries);
> audit_log_end(ab);
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index 8d28fff..395ebd3 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -1199,7 +1199,8 @@ struct xt_table_info *xt_replace_table(struct xt_table 
> *table,
> ab = audit_log_start(current->audit_context, 
> GFP_KERNEL,
>  AUDIT_NETFILTER_CFG);
> if (ab) {
> -   audit_log_format(ab, "family=%u table=%s 
> entries=%u",
> +   audit_log_format(ab, "op=%s family=%u 
> table=%s entries=%u",
> +private->number ? "replace" 
> : "register",
>  table->af, table->name,
>  private->number);
> audit_log_end(ab);
> @@ -1209,7 +1210,7 @@ struct xt_table_info *xt_replace_table(struct xt_table 
> *table,
>  AUDIT_NETFILTER_CFGSOLO);
> if (ab) {
> audit_log_task(ab);
> -   audit_log_format(ab, " 

Re: [PATCH 4/6 RFC] netfilter: ebtables: audit table registration

[Paul Moore]

On Thu, May 18, 2017 at 1:21 PM, Richard Guy Briggs  wrote:
> Generate audit NETFILTER_CFG records on ebtables table registration.
>
> Previously this was only being done for all x_tables operations and ebtables
> table replacement.
>
> Audit only when there is an existing syscall audit rule, otherwise issue a
> standalone record only on table modification rather than empty table creation.
> Include subject attributes to the new standalone NETFILTER_CFGSOLO record 
> using
> audit_log_task().
>
> Here is a sample accompanied record:
>   type=NETFILTER_CFG msg=audit(1494907217.558:5403): family=7 table=filter 
> entries=0
>
> and unaccompanied case:
>   type=UNKNOWN[1331] msg=audit(1494723394.832:111): auid=4294967295 uid=0 
> gid=0 ses=4294967295 subj=system_u:system_r:iptables_t:s0 pid=556 
> comm="ebtables-restor" exe="/usr/sbin/ebtables-restore" family=7 table=broute 
> entries=1
>
> See: https://github.com/linux-audit/audit-kernel/issues/43
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  net/bridge/netfilter/ebtables.c |   26 ++
>  1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 743f9e6..7499232 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1251,6 +1251,32 @@ struct ebt_table * ebt_register_table(struct net *net,
> }
> list_add(>list, >xt.tables[NFPROTO_BRIDGE]);
> mutex_unlock(_mutex);
> +#ifdef CONFIG_AUDIT
> +   if (audit_enabled) {
> +   struct audit_buffer *ab;
> +
> +   if(!audit_dummy_context()) {
> +   ab = audit_log_start(current->audit_context, 
> GFP_KERNEL,
> +AUDIT_NETFILTER_CFG);
> +   if (ab) {
> +   audit_log_format(ab, "family=%u table=%s 
> entries=%u",
> +AF_BRIDGE, repl->name,
> +repl->nentries);
> +   audit_log_end(ab);
> +   }
> +   } else if(repl->nentries) {
> +   ab = audit_log_start(NULL, GFP_KERNEL,
> +AUDIT_NETFILTER_CFGSOLO);
> +   if (ab) {
> +   audit_log_task(ab);
> +   audit_log_format(ab, " family=%u table=%s 
> entries=%u",
> +AF_BRIDGE, repl->name,
> +repl->nentries);
> +   audit_log_end(ab);
> +   }
> +   }
> +   }
> +#endif

Similar comments from patch 3/6 apply here, let's stick with a single
audit record type.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6 RFC] netfilter: audit only on xtables and ebtables syscall rule or standalone

[Paul Moore]

On Wed, May 24, 2017 at 2:09 PM, Richard Guy Briggs  wrote:
> On 2017-05-24 19:36, Pablo Neira Ayuso wrote:
>> On Thu, May 18, 2017 at 01:21:49PM -0400, Richard Guy Briggs wrote:
>> > There were syscall events unsolicited by any audit rule caused by a missing
>> > !audit_dummy_context() check before creating an
>> > iptables/ip6tables/arptables/ebtables NETFILTER_CFG record.  Check
>> > !audit_dummy_context() before creating the NETFILTER_CFG record.
>> >
>> > The vast majority of observed unaccompanied records are caused by the 
>> > fedora
>> > default rule: "-a never,task" and the occasional early startup one is I 
>> > believe
>> > caused by the iptables filter table module hard linked into the kernel 
>> > rather
>> > than a loadable module. The !audit_dummy_context() check above should avoid
>> > them.  Audit only when there is an existing syscall audit rule, otherwise 
>> > issue
>> > a standalone record only on table modification rather than empty table
>> > creation.
>> >
>> > Add subject attributes to the new standalone NETFILTER_CFGSOLO record using
>> > a newly exported audit_log_task().
>>
>> This new NETFILTER_CFGSOLO looks like audit infra is missing some way
>> to export a revision / context to userspace? It's duplicating quite a
>> bit of the code from what I can see in this patch.
>
> Interesting you brought that up.  I did another revision that stores
> this information in a struct audit_context and greatly simplifies the
> code in netfilter and re-uses code in audit itself, which may be a
> better way to go, but that idea needed to settle a bit more before
> seeing peer review.
>
> I'm also having doubts about two record types.

Richard and I had a discussion about this a week (or two?) ago and I'm
currently of the opinion that two record types are a mistake.  I agree
that we need to add the audit_dummy_context() check but the other
changes in this patch I'm less excited about.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf-next] netns: add and use net_ns_barrier

[Florian Westphal]

David Laight  wrote:
> From: Florian Westphal
> > Sent: 30 May 2017 10:38
> > 
> > Quoting Joe Stringer:
> >   If a user loads nf_conntrack_ftp, sends FTP traffic through a network
> >   namespace, destroys that namespace then unloads the FTP helper module,
> >   then the kernel will crash.
> > 
> > Events that lead to the crash:
> > 1. conntrack is created with ftp helper in netns x
> > 2. This netns is destroyed
> > 3. netns destruction is scheduled
> > 4. netns destruction wq starts, removes netns from global list
> > 5. ftp helper is unloaded, which resets all helpers of the conntracks
> > via for_each_net()
> > 
> > but because netns is already gone from list the for_each_net() loop
> > doesn't include it, therefore all of these conntracks are unaffected.
> > 
> > 6. helper module unload finishes
> > 7. netns wq invokes destructor for rmmod'ed helper
> > 
> ...
> >  void
> >  nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data), void 
> > *data)
> > @@ -1734,6 +1736,13 @@ nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, 
> > void *data), void *data)
> > }
> > rtnl_unlock();
> > 
> > +   /* Need to wait for netns cleanup worker to finish, if its
> > +* running -- it might have deleted a net namespace from
> > +* the global list, so our __nf_ct_unconfirmed_destroy() might
> > +* not have affected all namespaces.
> > +*/
> > +   net_ns_barrier();
> > +
> 
> A problem I see is that nothing obvious guarantees that the cleanup worker
> has actually started.

If it hasn't even started, the earlier for_each_net() has seen all
net namespaces and we managed to clear helper extensions of all conntracks.

Same in case it has finished already: netns cleanup work queue has
free'd all the affected conntracks we might have missed.

We only are in trouble if netns work queue is running concurrently:
netns cleanup first removes net namespaces from the global list,
so nf_ct_iterate_destroy might have missed these.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH nf-next] netns: add and use net_ns_barrier

[David Laight]

From: Florian Westphal
> Sent: 30 May 2017 10:38
> 
> Quoting Joe Stringer:
>   If a user loads nf_conntrack_ftp, sends FTP traffic through a network
>   namespace, destroys that namespace then unloads the FTP helper module,
>   then the kernel will crash.
> 
> Events that lead to the crash:
> 1. conntrack is created with ftp helper in netns x
> 2. This netns is destroyed
> 3. netns destruction is scheduled
> 4. netns destruction wq starts, removes netns from global list
> 5. ftp helper is unloaded, which resets all helpers of the conntracks
> via for_each_net()
> 
> but because netns is already gone from list the for_each_net() loop
> doesn't include it, therefore all of these conntracks are unaffected.
> 
> 6. helper module unload finishes
> 7. netns wq invokes destructor for rmmod'ed helper
> 
...
>  void
>  nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data), void *data)
> @@ -1734,6 +1736,13 @@ nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, 
> void *data), void *data)
>   }
>   rtnl_unlock();
> 
> + /* Need to wait for netns cleanup worker to finish, if its
> +  * running -- it might have deleted a net namespace from
> +  * the global list, so our __nf_ct_unconfirmed_destroy() might
> +  * not have affected all namespaces.
> +  */
> + net_ns_barrier();
> +

A problem I see is that nothing obvious guarantees that the cleanup worker
has actually started.

David

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] netfilter, kbuild: use canonical method to specify objs.

[Jike Song]

Should use ":=" instead of "+=".

Signed-off-by: Jike Song 
---
 net/netfilter/Makefile | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile
index c9b78e7..9133809 100644
--- a/net/netfilter/Makefile
+++ b/net/netfilter/Makefile
@@ -70,10 +70,9 @@ obj-$(CONFIG_NETFILTER_SYNPROXY) += nf_synproxy_core.o
 obj-$(CONFIG_NF_DUP_NETDEV)+= nf_dup_netdev.o
 
 # nf_tables
-nf_tables-objs += nf_tables_core.o nf_tables_api.o nf_tables_trace.o
-nf_tables-objs += nft_immediate.o nft_cmp.o nft_range.o
-nf_tables-objs += nft_bitwise.o nft_byteorder.o nft_payload.o
-nf_tables-objs += nft_lookup.o nft_dynset.o
+nf_tables-objs := nf_tables_core.o nf_tables_api.o nf_tables_trace.o \
+ nft_immediate.o nft_cmp.o nft_range.o nft_bitwise.o \
+ nft_byteorder.o nft_payload.o nft_lookup.o nft_dynset.o
 
 obj-$(CONFIG_NF_TABLES)+= nf_tables.o
 obj-$(CONFIG_NF_TABLES_INET)   += nf_tables_inet.o
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html