Re: [PATCH] netfilter/nflog: nflog-range does not truncate packets

[Vishwanath Pai]

On 06/08/2016 08:16 AM, Pablo Neira Ayuso wrote:
> Looking again at your code:
> 
> case NFULNL_COPY_PACKET:
> -   if (inst->copy_range > skb->len)
> +   data_len = inst->copy_range;
> +   if (li->u.ulog.copy_len < data_len)
> +   data_len = li->u.ulog.copy_len;
> 
> data_len is set to instance's copy_range.
> 
> But then, if the NFLOG rule indicates smaller copy_len, you use this
> value. So to my understanding, NFLOG rule prevails over instance's
> copy_range in what matters, which is to shrink the copy range.
> 
>> > --nflog-range will not override the per-instance default,
>> > the only time it would get preference is when its value is lesser than
>> > the per-instance value. If copy_range is lesser than --nflog-range then
>> > we retain copy_range.
>> >
>> > So basically what we are doing is min(copy_range, nflog-range).
>> > Just wanted to clarify this, if this is not how it's meant to be
>> > please let me know.
>> >
>> > Also, there is a bug in my patch, li->u.ulog.copy_len can be set to "0"
>> > from userspace (if --nflog-range is not specified), so we have to check
>> > for this condition before using the value. I will send a V2 of the patch
>> > based on your reply.
> Currently, li->u.ulog.copy_len is set to "0" by default when not
> specified.
> 
> But copy_len = 0 is a valid possibility, so this looks a bit more
> tricky to me to fix since we may need to get flags here to know when
> this is set.
> 
> Probably something like:
> 
> if (li->flags & NF_LOG_F_COPY_RANGE)
> data_len = li->u.ulog.copy_len;
> /* Per-instance copy range prevails over global per-rule option. */
> if (data_len < inst->copy_range)
> data_len = inst->copy_range;
> if (data_len > skb->len)
> data_len = skb->len;
> 
> Although this would require a bit more code to introduce these flags.
> 
> You will also need a new flag for xt_NFLOG:
> 
> #define XT_NFLOG_COPY_LEN   0x2
> 
> it seems other XT_NFLOG_* flags were already in place.
> 
> Interesting that nobody noticed this for so long BTW.

I tried this out, I added two flags: one for XT_NFLOG to notify the
kernel when --nflog-range is set by the user, and another flag for
nfnetlink_log to pass this on to nfulnl_log_packet. This design works
fine but while testing this I found a problem.

Lets say --nflog-range is set to 200, and the instance copy_range is set
to 100. According to the code above the final value of data_len will be
200 so we try to pack 200 bytes into the skb. But somewhere between
nfnetlink_log to dumpcap the packet is getting truncated and dumpcap
doesn't like this:

$> dumpcap -y NFLOG -i nflog:5 -s 100
Capturing on 'nflog:5'
File: /tmp/wireshark_pcapng_nflog-5_20160609133531_pi6MrS
dumpcap: Error while capturing packets: Message truncated: (got: 228)
(nlmsg_len: 320)
Please report this to the Wireshark developers.
https://bugs.wireshark.org/
(This is not a crash; please do not report it as such.)
Packets captured: 0
Packets received/dropped on interface 'nflog:5': 0/0
(pcap:0/dumpcap:0/flushed:0/ps_ifdrop:0) (0.0%)

I'm trying to figure out where the packet is getting truncated.
--
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] extensions: libxt_cgroup: Add translation to nft

[Laura Garcia Liebana]

Add translation for cgroup to nft. Path parameter not supported in nft
yet.

Examples:

$ sudo iptables-translate -t filter -A INPUT -m cgroup --cgroup 0 -j ACCEPT
nft add rule ip filter INPUT meta cgroup 0 counter accept

$ sudo iptables-translate -t filter -A INPUT -m cgroup ! --cgroup 0 -j ACCEPT
nft add rule ip filter INPUT meta cgroup != 0 counter accept

Signed-off-by: Laura Garcia Liebana 
---
 extensions/libxt_cgroup.c | 28 
 1 file changed, 28 insertions(+)

diff --git a/extensions/libxt_cgroup.c b/extensions/libxt_cgroup.c
index 3be42ad..1191815 100644
--- a/extensions/libxt_cgroup.c
+++ b/extensions/libxt_cgroup.c
@@ -121,6 +121,32 @@ static void cgroup_save_v1(const void *ip, const struct 
xt_entry_match *match)
   info->classid);
 }
 
+static int cgroup_xlate_v0(const void *ip, const struct xt_entry_match *match,
+  struct xt_xlate *xl, int numeric)
+{
+   const struct xt_cgroup_info_v0 *info = (void *)match->data;
+
+   xt_xlate_add(xl, "meta cgroup %s%u ", info->invert ? "!= " : "",
+info->id);
+   return 1;
+}
+
+static int cgroup_xlate_v1(const void *ip, const struct xt_entry_match *match,
+  struct xt_xlate *xl, int numeric)
+{
+   const struct xt_cgroup_info_v1 *info = (void *)match->data;
+
+   if (info->has_path)
+   return 0;
+
+   if (info->has_classid)
+   xt_xlate_add(xl, "meta cgroup %s%u ",
+info->invert_classid ? "!= " : "",
+info->classid);
+
+   return 1;
+}
+
 static struct xtables_match cgroup_match[] = {
{
.family = NFPROTO_UNSPEC,
@@ -134,6 +160,7 @@ static struct xtables_match cgroup_match[] = {
.save   = cgroup_save_v0,
.x6_parse   = cgroup_parse_v0,
.x6_options = cgroup_opts_v0,
+   .xlate  = cgroup_xlate_v0,
},
{
.family = NFPROTO_UNSPEC,
@@ -147,6 +174,7 @@ static struct xtables_match cgroup_match[] = {
.save   = cgroup_save_v1,
.x6_parse   = cgroup_parse_v1,
.x6_options = cgroup_opts_v1,
+   .xlate  = cgroup_xlate_v1,
},
 };
 
-- 
2.7.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 net-next] nfnetlink_queue: enable PID info retrieval

[Daniel Borkmann]

On 06/10/2016 12:21 AM, Daniel Borkmann wrote:

On 06/09/2016 11:35 PM, Florian Westphal wrote:

Saeed Mahameed  wrote:

index a1bd161..67de200 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -382,6 +382,7 @@ struct file *sock_alloc_file(struct socket *sock, int 
flags, const char *dname)
  }

  sock->file = file;
+file->f_owner.sock_pid  = find_get_pid(task_pid_nr(current));
  file->f_flags = O_RDWR | (flags & O_NONBLOCK);
  file->private_data = sock;
  return file;


This looks like this leaks sock_pid reference...?

(find_get_pid -> get_pid -> atomic_inc() , I don't see a put_pid in the
  patch)

Can't comment further than this since I'm not familiar with vfs; e.g.
I can't say if fown_struct is right place or not, or if this approach
even works when creating process has exited after fork, etc.


Or ... if you xmit the fd via unix domain socket to a different process
and initial owner terminates, which should give you invalid information
then; afaik, this would just increase struct file's refcnt and hand out
an unused fdnum ( get_unused_fd_flags() + fd_install(), etc).
For extending 'struct fown_struct', you probably also need to Cc fs folks.


[ Cc'ing John, Daniel, et al ]

Btw, while I just looked at scm_detach_fds(), I think commits ...

 * 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set 
correctly")
 * d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set 
correctly")

... might not be correct, maybe I'm missing something ...? Lets say process A
has a socket fd that it sends via SCM_RIGHTS to process B. Process A was the
one that called sk_alloc() originally. Now in scm_detach_fds() we install a new
fd for process B pointing to the same sock (file's private_data) and above 
commits
update the cached socket cgroup data for net_cls/net_prio to the new process B.
So, if process A for example still sends data over that socket, skbs will then
wrongly match on B's cgroup membership instead of A's, no?

Thanks,
Daniel
--
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 1/3] bridge: netfilter: checkpatch whitespace fixes

[Joe Perches]

On Wed, 2016-06-08 at 19:38 +0200, Pablo Neira Ayuso wrote:
> On Wed, Jun 08, 2016 at 07:31:21PM +0200, Pablo Neira Ayuso wrote:
> > Then you can follow up with a patch to add this function.
> > 
> > Just a suggestion, let me know if this is fine with you.
> Forget this idea.
> 
> Actually your patch from: Date: Tue, 07 Jun 2016 11:02:30 -0700
> 
> looks easier to readable than original Tobin's, so I'll wait for you
> to resubmit.

Well, maybe next week after the other couple patches are in -next.

--
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 net-next] nfnetlink_queue: enable PID info retrieval

[Eric Dumazet]

On Thu, 2016-06-09 at 23:50 +0300, Saeed Mahameed wrote:
> From: Matthew Finlay 


> diff --git a/net/socket.c b/net/socket.c
> index a1bd161..67de200 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -382,6 +382,7 @@ struct file *sock_alloc_file(struct socket *sock, int 
> flags, const char *dname)
>   }
>  
>   sock->file = file;
> + file->f_owner.sock_pid  = find_get_pid(task_pid_nr(current));
>   file->f_flags = O_RDWR | (flags & O_NONBLOCK);
>   file->private_data = sock;
>   return file;

Wow, that is a serious memory leak weapon (of struct pid)

Why don't you store the pid value, instead of a pointer ?


--
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 net-next] nfnetlink_queue: enable PID info retrieval

[Daniel Borkmann]

On 06/09/2016 11:35 PM, Florian Westphal wrote:

Saeed Mahameed  wrote:

index a1bd161..67de200 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -382,6 +382,7 @@ struct file *sock_alloc_file(struct socket *sock, int 
flags, const char *dname)
}

sock->file = file;
+   file->f_owner.sock_pid  = find_get_pid(task_pid_nr(current));
file->f_flags = O_RDWR | (flags & O_NONBLOCK);
file->private_data = sock;
return file;


This looks like this leaks sock_pid reference...?

(find_get_pid -> get_pid -> atomic_inc() , I don't see a put_pid in the
  patch)

Can't comment further than this since I'm not familiar with vfs; e.g.
I can't say if fown_struct is right place or not, or if this approach
even works when creating process has exited after fork, etc.


Or ... if you xmit the fd via unix domain socket to a different process
and initial owner terminates, which should give you invalid information
then; afaik, this would just increase struct file's refcnt and hand out
an unused fdnum ( get_unused_fd_flags() + fd_install(), etc).
For extending 'struct fown_struct', you probably also need to Cc fs folks.
--
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