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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Sun, Jun 12, 2016 at 11:40:57PM -0400, Vishwanath Pai wrote:
> On 06/09/2016 01:57 PM, Vishwanath Pai wrote:
> > 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.
> > 
> 
> I found where the problem is. This is the userspace code for libpcap:
> 
>         do {
>                 len = recv(handle->fd, handle->buffer, handle->bufsize, 0);
>                 if (handle->break_loop) {
>                         handle->break_loop = 0;
>                         return -2;
>                 }
>         } while ((len == -1) && (errno == EINTR));
> 
>        ...
> 
>         buf = handle->buffer;
>         while (len >= NLMSG_SPACE(0)) {
>                 const struct nlmsghdr *nlh = (const struct nlmsghdr *) buf;
>                 u_int32_t msg_len;
>                 nftype_t type = OTHER;
> 
>                 if (nlh->nlmsg_len < sizeof(struct nlmsghdr) || len <
> nlh->nlmsg_len) {
>                         snprintf(handle->errbuf, PCAP_ERRBUF_SIZE,
> "Message truncated: (got: %d) (nlmsg_len: %u)", len, nlh->nlmsg_len);
>                         return -1;
>                 }
>
> handle->bufsize is only big enough to accommodate the snaplen specified
> by the user in dumpcap. So if we send more data than that then we will
> break userspace. One way around this is to send min(li->u.ulog.copy_len,
> inst->copy_range). If this is OK then I can send a patch for this,
> please suggest.

But nlmsg_len should match len in this.

If we're just sending a part of the packet to userspace, then we
should adjust nlmsg_len to indicate exactly the netlink message length
that we're sending to userspace.

Is your patch triggering this nlmsg_len != len?