Re: [PATCH nf] netfilter: nf_nat: fix buffer overflow in IRC NAT helper

[Daniel Borkmann <dborkman@redhat.com>]

On 01/06/2014 02:04 PM, Pablo Neira Ayuso wrote:
> Hi Daniel,
>
> On Tue, Dec 31, 2013 at 04:28:39PM +0100, Daniel Borkmann wrote:
>> diff --git a/net/netfilter/nf_nat_irc.c b/net/netfilter/nf_nat_irc.c
>> index f02b360..fbbb1e6 100644
>> --- a/net/netfilter/nf_nat_irc.c
>> +++ b/net/netfilter/nf_nat_irc.c
>> @@ -34,10 +34,14 @@ static unsigned int help(struct sk_buff *skb,
>>   			 struct nf_conntrack_expect *exp)
>>   {
>>   	char buffer[sizeof("4294967296 65635")];
>> +	struct nf_conn *ct = exp->master;
>> +	union nf_inet_addr newaddr;
>>   	u_int16_t port;
>>   	unsigned int ret;
>>
>>   	/* Reply comes from server. */
>> +	newaddr = ct->tuplehash[IP_CT_DIR_REPLY].tuple.dst.u3;
>> +
>>   	exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
>>   	exp->dir = IP_CT_DIR_REPLY;
>>   	exp->expectfn = nf_nat_follow_master;
>> @@ -57,17 +61,41 @@ static unsigned int help(struct sk_buff *skb,
>>   	}
>>
>>   	if (port == 0) {
>> -		nf_ct_helper_log(skb, exp->master, "all ports in use");
>> +		nf_ct_helper_log(skb, ct, "all ports in use");
>>   		return NF_DROP;
>>   	}
>>
>> -	ret = nf_nat_mangle_tcp_packet(skb, exp->master, ctinfo,
>> -				       protoff, matchoff, matchlen, buffer,
>> -				       strlen(buffer));
>> +	/* strlen("\1DCC CHAT chat AAAAAAAA P\1\n")=27
>> +	 * strlen("\1DCC SCHAT chat AAAAAAAA P\1\n")=28
>> +	 * strlen("\1DCC SEND F AAAAAAAA P S\1\n")=26
>> +	 * strlen("\1DCC MOVE F AAAAAAAA P S\1\n")=26
>> +	 * strlen("\1DCC TSEND F AAAAAAAA P S\1\n")=27
>> +	 *
>> +	 * AAAAAAAAA: bound addr (1.0.0.0==16777216, min 8 digits,
>> +	 *                        255.255.255.255==4294967296, 10 digits)
>> +	 * P:         bound port (min 1 d, max 5d (65635))
>> +	 * F:         filename   (min 1 d )
>> +	 * S:         size       (min 1 d )
>> +	 * 0x01, \n:  terminators
>> +	 */
>> +	/* AAA = "us", ie. where server normally talks to. */
>> +	if (nf_ct_l3num(ct) == NFPROTO_IPV4) {
>> +		snprintf(buffer, sizeof(buffer), "%u %u",
>> +			 ntohl(newaddr.ip), port);
>> +		pr_debug("nf_nat_irc: inserting '%s' == %pI4, port %u\n",
>> +			 buffer, &newaddr.ip, port);
>> +	} else {
>> +		nf_ct_helper_log(skb, ct, "IPv6 DCC unsupported for now");
>> +		return NF_DROP;
>> +	}
>
> I have mangled your patch (see attachment) to remove this branch since
> there is real IPv6 support for IRC yet in master. We'll need to
> revisit this anyway when finishing IPv6 support to the IRC helper. Let
> me know if you have any concern with this. Thanks.

Thanks, that's fine.

+	 * 0x01, \n:  terminators
+	 */
+	/* AAA = "us", ie. where server normally talks to. */
+	snprintf(buffer, sizeof(buffer), "%u %u", ntohl(newaddr.ip), port);
+	pr_debug("nf_nat_irc: inserting '%s' == %pI4, port %u\n",
+		 buffer, &newaddr.ip, port);

That should be good for now. The thing I've noticed so far is that for
DCC and IPv6 there doesn't seem to be a standard and clients try to
parse "%u %u" and expect IPv4 here.

But anyway, that fixes the bug, feel free to apply, thanks.

+	ret = nf_nat_mangle_tcp_packet(skb, ct, ctinfo, protoff, matchoff,
+				       matchlen, buffer, strlen(buffer));