| Message ID | 20160602002354.GG1644@akamai.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On Wed, Jun 01, 2016 at 08:23:54PM -0400, Vishwanath Pai wrote: > netfilter/nflog: nflog-range does not truncate packets > > The --nflog-range parameter from userspace is ignored in the kernel and > the entire packet is sent to the userspace. The per-instance parameter > copy_range still works, with this change --nflog-range will have > preference over copy_range. I think it's reasonable to assume that --nflog-range from the rule applies globally to any instance. However, per-instance copy_range has prevailed over --nflog-range since the beginning, so I would follow a more conservative approach, ie. remain copy_range in preference over --nflog-range. So I'd suggest you invert this logic. Let me know, thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/06/2016 06:31 PM, Pablo Neira Ayuso wrote: > On Wed, Jun 01, 2016 at 08:23:54PM -0400, Vishwanath Pai wrote: >> netfilter/nflog: nflog-range does not truncate packets >> >> The --nflog-range parameter from userspace is ignored in the kernel and >> the entire packet is sent to the userspace. The per-instance parameter >> copy_range still works, with this change --nflog-range will have >> preference over copy_range. > > I think it's reasonable to assume that --nflog-range from the rule > applies globally to any instance. > > However, per-instance copy_range has prevailed over --nflog-range > since the beginning, so I would follow a more conservative approach, > ie. remain copy_range in preference over --nflog-range. > > So I'd suggest you invert this logic. > > Let me know, thanks. > Thanks for reviewing this. I think my comment on the patch was misleading, we do give preference to copy_range and that is what we default to. --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. Thanks, Vishwanath -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 07, 2016 at 07:06:15PM -0400, Vishwanath Pai wrote: > On 06/06/2016 06:31 PM, Pablo Neira Ayuso wrote: > > On Wed, Jun 01, 2016 at 08:23:54PM -0400, Vishwanath Pai wrote: > >> netfilter/nflog: nflog-range does not truncate packets > >> > >> The --nflog-range parameter from userspace is ignored in the kernel and > >> the entire packet is sent to the userspace. The per-instance parameter > >> copy_range still works, with this change --nflog-range will have > >> preference over copy_range. > > > > I think it's reasonable to assume that --nflog-range from the rule > > applies globally to any instance. > > > > However, per-instance copy_range has prevailed over --nflog-range > > since the beginning, so I would follow a more conservative approach, > > ie. remain copy_range in preference over --nflog-range. > > > > So I'd suggest you invert this logic. > > > > Let me know, thanks. > > > > Thanks for reviewing this. I think my comment on the patch was > misleading, we do give preference to copy_range and that is what we > default to. 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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/15/2016 08:39 AM, Pablo Neira Ayuso wrote: > 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? The value of len here is how many bytes were returned by recv. We do send the entire nlmsg_len to userspace, but recv cannot copy the full packet because the buffer is not big enough to hold this. They only allocate the buffer assuming that the packet won't be bigger than their snap len, but we send more data than their snap len and they don't handle this condition well. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Vish, Pablo, I wonder about the value of sending more data than a client is willing to consume (setting aside the important fact that the client code crashes due to the extra data). It seems that we should either drop the nflog-range parameter from nflog altogether (and just use the len from the client) or allow nflog-range to further *restrict* the number of bytes sent to the client. The "further restrict" logic would make it easier to build iptables rules that vary nflog-range based on some match conditions, so a single client would get different packet length depending on what rules matched. Thoughts? - Igor -----Original Message----- From: Vishwanath Pai [mailto:vpai@akamai.com] Sent: Wednesday, June 15, 2016 10:55 AM To: Pablo Neira Ayuso <pablo@netfilter.org> Cc: kaber@trash.net; kadlec@blackhole.kfki.hu; netfilter-devel@vger.kernel.org; coreteam@netfilter.org; Hunt, Joshua <johunt@akamai.com>; netdev@vger.kernel.org; pai.vishwain@gmail.com; Lubashev, Igor <ilubashe@akamai.com> Subject: Re: [PATCH] netfilter/nflog: nflog-range does not truncate packets On 06/15/2016 08:39 AM, Pablo Neira Ayuso wrote: > 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? The value of len here is how many bytes were returned by recv. We do send the entire nlmsg_len to userspace, but recv cannot copy the full packet because the buffer is not big enough to hold this. They only allocate the buffer assuming that the packet won't be bigger than their snap len, but we send more data than their snap len and they don't handle this condition well. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 15, 2016 at 03:13:15PM +0000, Lubashev, Igor wrote: > Vish, Pablo, > > I wonder about the value of sending more data than a client is > willing to consume (setting aside the important fact that the client > code crashes due to the extra data). > > It seems that we should either drop the nflog-range parameter from > nflog altogether (and just use the len from the client) or allow > nflog-range to further *restrict* the number of bytes sent to the > client. > > The "further restrict" logic would make it easier to build iptables > rules that vary nflog-range based on some match conditions, so a > single client would get different packet length depending on what > rules matched. Now I understand your usecase. Restricting the size based on match conditions sound reasonable to me. Why don't you add a new userspace option, eg. --nflog-size, that specifies this "further restrict" logic? What I'm proposing is: 1) If --nflog-range is used, print a message telling: "--nflog-range has never worked, ignoring this option." 2) If --nflog-size is used, set the size in the structure that is passed to the kernel, and apply this "further restrict" logic. 3) Add the flag to the kernel that I suggested. This flag is only set via --nflog-size. Just to clarify: What I'm trying to avoid is breaking the thing for users that are using this --nflog-range (even if it doesn't work) and then change the behaviour for them. With the new option, we really validate that the user is exactly asking for this "further restrict" logic that you need. let me know, thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/17/2016 07:22 AM, Pablo Neira Ayuso wrote: > On Wed, Jun 15, 2016 at 03:13:15PM +0000, Lubashev, Igor wrote: >> Vish, Pablo, >> >> I wonder about the value of sending more data than a client is >> willing to consume (setting aside the important fact that the client >> code crashes due to the extra data). >> >> It seems that we should either drop the nflog-range parameter from >> nflog altogether (and just use the len from the client) or allow >> nflog-range to further *restrict* the number of bytes sent to the >> client. >> >> The "further restrict" logic would make it easier to build iptables >> rules that vary nflog-range based on some match conditions, so a >> single client would get different packet length depending on what >> rules matched. > > Now I understand your usecase. Restricting the size based on match > conditions sound reasonable to me. > > Why don't you add a new userspace option, eg. --nflog-size, that > specifies this "further restrict" logic? > > What I'm proposing is: > > 1) If --nflog-range is used, print a message telling: "--nflog-range > has never worked, ignoring this option." > > 2) If --nflog-size is used, set the size in the structure that is > passed to the kernel, and apply this "further restrict" logic. > > 3) Add the flag to the kernel that I suggested. This flag is only set > via --nflog-size. > > Just to clarify: What I'm trying to avoid is breaking the thing for > users that are using this --nflog-range (even if it doesn't work) and > then change the behaviour for them. > > With the new option, we really validate that the user is exactly > asking for this "further restrict" logic that you need. > > let me know, thanks. > Sounds good to me, yes it will definitely change the behavior for users who are using that parameter (whether intentional or not). I'm OK with adding a new parameter instead of using --nflog-range. I will send a patch with these changes. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c index 4ef1fae..f40ddba 100644 --- a/net/netfilter/nfnetlink_log.c +++ b/net/netfilter/nfnetlink_log.c @@ -680,7 +680,6 @@ nfulnl_log_packet(struct net *net, if (qthreshold > li->u.ulog.qthreshold) qthreshold = li->u.ulog.qthreshold; - switch (inst->copy_mode) { case NFULNL_COPY_META: case NFULNL_COPY_NONE: @@ -688,10 +687,12 @@ nfulnl_log_packet(struct net *net, break; 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; + + if (data_len > skb->len) data_len = skb->len; - else - data_len = inst->copy_range; size += nla_total_size(data_len); break;