Patchwork [v2] ipv4: netfilter: always let NUL terminated string ended by '\0'

login
register
mail settings
Submitter Chen Gang
Date May 23, 2013, 11:50 a.m.
Message ID <519E0296.6010601@asianux.com>
Download mbox | patch
Permalink /patch/245924/
State Accepted
Headers show

Comments

Chen Gang - May 23, 2013, 11:50 a.m.
For NUL terminated string, need always be sure of ended by '\0'.

'prefix' max length is 128 (NF_LOG_PREFIXLEN), and 'pm->prefix' max
length is 32 (ULOG_PREFIX_LEN), so really need notice it.

'pm' is 'struct ulog_packet_msg_t' which may be copied to user mode
(defined in "include/uapi/..."), so can not use strlcpy() instead of.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 net/ipv4/netfilter/ipt_ULOG.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Pablo Neira - May 23, 2013, 12:26 p.m.
On Thu, May 23, 2013 at 07:50:46PM +0800, Chen Gang wrote:
> 
> For NUL terminated string, need always be sure of ended by '\0'.
> 
> 'prefix' max length is 128 (NF_LOG_PREFIXLEN), and 'pm->prefix' max
> length is 32 (ULOG_PREFIX_LEN), so really need notice it.
> 
> 'pm' is 'struct ulog_packet_msg_t' which may be copied to user mode
> (defined in "include/uapi/..."), so can not use strlcpy() instead of.

That's fixing a real bug. We're passing strings that are longer than
32 bytes from nf_conntrack_tcp via nf_log, if ipt_ULOG is used, it
will pass a non-null terminated string.

I'm going to rework the patch description to include this and apply
this patch.

Thanks Chen.
--
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
Chen Gang - May 24, 2013, 1:11 a.m.
On 05/23/2013 08:26 PM, Pablo Neira Ayuso wrote:
> On Thu, May 23, 2013 at 07:50:46PM +0800, Chen Gang wrote:
>> > 
>> > For NUL terminated string, need always be sure of ended by '\0'.
>> > 
>> > 'prefix' max length is 128 (NF_LOG_PREFIXLEN), and 'pm->prefix' max
>> > length is 32 (ULOG_PREFIX_LEN), so really need notice it.
>> > 
>> > 'pm' is 'struct ulog_packet_msg_t' which may be copied to user mode
>> > (defined in "include/uapi/..."), so can not use strlcpy() instead of.
> That's fixing a real bug. We're passing strings that are longer than
> 32 bytes from nf_conntrack_tcp via nf_log, if ipt_ULOG is used, it
> will pass a non-null terminated string.
> 
> I'm going to rework the patch description to include this and apply
> this patch.
> 
> Thanks Chen.
> 
> 

Thank you too.

Patch

diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
index cf08218..ff4b781 100644
--- a/net/ipv4/netfilter/ipt_ULOG.c
+++ b/net/ipv4/netfilter/ipt_ULOG.c
@@ -231,8 +231,10 @@  static void ipt_ulog_packet(struct net *net,
 	put_unaligned(tv.tv_usec, &pm->timestamp_usec);
 	put_unaligned(skb->mark, &pm->mark);
 	pm->hook = hooknum;
-	if (prefix != NULL)
-		strncpy(pm->prefix, prefix, sizeof(pm->prefix));
+	if (prefix != NULL) {
+		strncpy(pm->prefix, prefix, sizeof(pm->prefix) - 1);
+		pm->prefix[sizeof(pm->prefix) - 1] = '\0';
+	}
 	else if (loginfo->prefix[0] != '\0')
 		strncpy(pm->prefix, loginfo->prefix, sizeof(pm->prefix));
 	else