[PATCH 1/3] extra: use inet_ntop instead of inet_ntoa

[PATCH 1/3] extra: use inet_ntop instead of inet_ntoa

[lantw44]

From: Ting-Wei Lan <lantw44@gmail.com>

The result of inet_ntoa() will be overwritten by the next call to
inet_ntoa(), so using it twice in the same snprintf() call causes
wrong result.
---
 src/extra/ipv4.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/extra/ipv4.c b/src/extra/ipv4.c
index 0fe716b..a93d113 100644
--- a/src/extra/ipv4.c
+++ b/src/extra/ipv4.c
@@ -134,9 +134,13 @@ int nfq_ip_snprintf(char *buf, size_t size, const struct iphdr *iph)
 	struct in_addr src = { iph->saddr };
 	struct in_addr dst = { iph->daddr };
 
+	char src_str[INET_ADDRSTRLEN];
+	char dst_str[INET_ADDRSTRLEN];
+
 	ret = snprintf(buf, size, "SRC=%s DST=%s LEN=%u TOS=0x%X "
 				  "PREC=0x%X TTL=%u ID=%u PROTO=%u ",
-			inet_ntoa(src), inet_ntoa(dst),
+			inet_ntop(AF_INET, &src, src_str, INET_ADDRSTRLEN),
+			inet_ntop(AF_INET, &dst, dst_str, INET_ADDRSTRLEN),
 			ntohs(iph->tot_len), IPTOS_TOS(iph->tos),
 			IPTOS_PREC(iph->tos), iph->ttl, ntohs(iph->id),
 			iph->protocol);
-- 
1.9.3

[PATCH 2/3] extra: fix wrong implementation in nfq_udp_get_payload

[lantw44]

From: Ting-Wei Lan <lantw44@gmail.com>

---
 src/extra/udp.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/extra/udp.c b/src/extra/udp.c
index eee732e..2a17a2f 100644
--- a/src/extra/udp.c
+++ b/src/extra/udp.c
@@ -56,13 +56,17 @@ EXPORT_SYMBOL(nfq_udp_get_hdr);
  */
 void *nfq_udp_get_payload(struct udphdr *udph, struct pkt_buff *pktb)
 {
-	unsigned int doff = udph->len;
+	uint16_t len = ntohs (udph->len);
 
-	/* malformed UDP data offset. */
-	if (pktb->transport_header + doff > pktb->tail)
+	/* the UDP packet is too short. */
+	if (len < sizeof(struct udphdr))
 		return NULL;
 
-	return pktb->transport_header + doff;
+	/* malformed UDP packet. */
+	if (pktb->transport_header + len > pktb->tail)
+		return NULL;
+
+	return pktb->transport_header + sizeof(struct udphdr);
 }
 EXPORT_SYMBOL(nfq_udp_get_payload);
 
-- 
1.9.3

Re: [PATCH 2/3] extra: fix wrong implementation in nfq_udp_get_payload

[Pablo Neira Ayuso]

On Fri, Jun 20, 2014 at 06:27:00PM +0800, lantw44@gmail.com wrote:
> From: Ting-Wei Lan <lantw44@gmail.com>
> 
> ---
>  src/extra/udp.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/extra/udp.c b/src/extra/udp.c
> index eee732e..2a17a2f 100644
> --- a/src/extra/udp.c
> +++ b/src/extra/udp.c
> @@ -56,13 +56,17 @@ EXPORT_SYMBOL(nfq_udp_get_hdr);
>   */
>  void *nfq_udp_get_payload(struct udphdr *udph, struct pkt_buff *pktb)
>  {
> -	unsigned int doff = udph->len;
> +	uint16_t len = ntohs (udph->len);
                            ^

removed this space.

Applied, thanks.

Re: [PATCH 2/3] extra: fix wrong implementation in nfq_udp_get_payload

[藍挺瑋]

於 一，2014-06-30 於 12:02 +0200，Pablo Neira Ayuso 提到：
> On Fri, Jun 20, 2014 at 06:27:00PM +0800, lantw44@gmail.com wrote:
> > From: Ting-Wei Lan <lantw44@gmail.com>
> > 
> > ---
> >  src/extra/udp.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/extra/udp.c b/src/extra/udp.c
> > index eee732e..2a17a2f 100644
> > --- a/src/extra/udp.c
> > +++ b/src/extra/udp.c
> > @@ -56,13 +56,17 @@ EXPORT_SYMBOL(nfq_udp_get_hdr);
> >   */
> >  void *nfq_udp_get_payload(struct udphdr *udph, struct pkt_buff *pktb)
> >  {
> > -	unsigned int doff = udph->len;
> > +	uint16_t len = ntohs (udph->len);
>                             ^
> 
> removed this space.
> 
> Applied, thanks.

but I cannot find this patch in libnetfilter_queue git log ...


--
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

[PATCH 3/3] extra: fix wrong implementation in nfq_udp_get_payload_len

[lantw44]

From: Ting-Wei Lan <lantw44@gmail.com>

---
 src/extra/udp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/extra/udp.c b/src/extra/udp.c
index 2a17a2f..64d3db4 100644
--- a/src/extra/udp.c
+++ b/src/extra/udp.c
@@ -76,7 +76,7 @@ EXPORT_SYMBOL(nfq_udp_get_payload);
  */
 unsigned int nfq_udp_get_payload_len(struct udphdr *udph, struct pkt_buff *pktb)
 {
-	return pktb->tail - pktb->transport_header;
+	return pktb->tail - pktb->transport_header - sizeof(struct udphdr);
 }
 EXPORT_SYMBOL(nfq_udp_get_payload_len);
 
-- 
1.9.3

Re: [PATCH 3/3] extra: fix wrong implementation in nfq_udp_get_payload_len

[Pablo Neira Ayuso]

On Fri, Jun 20, 2014 at 06:27:01PM +0800, lantw44@gmail.com wrote:
> From: Ting-Wei Lan <lantw44@gmail.com>
> 
> ---
>  src/extra/udp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/extra/udp.c b/src/extra/udp.c
> index 2a17a2f..64d3db4 100644
> --- a/src/extra/udp.c
> +++ b/src/extra/udp.c
> @@ -76,7 +76,7 @@ EXPORT_SYMBOL(nfq_udp_get_payload);
>   */
>  unsigned int nfq_udp_get_payload_len(struct udphdr *udph, struct pkt_buff *pktb)
>  {
> -	return pktb->tail - pktb->transport_header;
> +	return pktb->tail - pktb->transport_header - sizeof(struct udphdr);

I agree that this should substract the UDP header size, but:

1) This underflows the unsigned int if the UDP header is truncated.
2) This breaks the current behaviour, but we don't have any client in
   the tree for this code yet though.

Let's do it well, deprecate this interface and introduce a new one:

int nfq_udp_get_payload_length(struct udphdr *udph, struct pkt_buff *pktb)

Same thing for the tcp variant.

The deprecation can be achieved through this:

in __attribute__((deprecated) nfq_udp_get_payload_len(...)

in the header file.

Would you send me a patch for that, please?

BTW, please Signed-off-by your userspace patches too. Thanks.

Re: [PATCH 1/3] extra: use inet_ntop instead of inet_ntoa

[Pablo Neira Ayuso]

On Fri, Jun 20, 2014 at 06:26:59PM +0800, lantw44@gmail.com wrote:
> From: Ting-Wei Lan <lantw44@gmail.com>
> 
> The result of inet_ntoa() will be overwritten by the next call to
> inet_ntoa(), so using it twice in the same snprintf() call causes
> wrong result.

Applied, thanks Lan.