* Re: [Bugme-new] [Bug 12753] New: /proc/net/xt_recent/: +IP / -IP commands broken for IPv4
[not found] <bug-12753-10286@http.bugzilla.kernel.org/>
@ 2009-02-24 20:58 ` Andrew Morton
2009-02-25 5:04 ` Patrick McHardy
0 siblings, 1 reply; 2+ messages in thread
From: Andrew Morton @ 2009-02-24 20:58 UTC (permalink / raw)
To: netdev, netfilter-devel; +Cc: bugme-daemon, joe-lk
(switched to email. Please respond via emailed reply-to-all, not via the
bugzilla web interface).
On Sun, 22 Feb 2009 15:11:56 -0800 (PST)
bugme-daemon@bugzilla.kernel.org wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=12753
>
> Summary: /proc/net/xt_recent/: +IP / -IP commands broken for IPv4
> Product: Networking
> Version: 2.5
> KernelVersion: 2.6.28.7
> Platform: All
> OS/Version: Linux
> Tree: Mainline
> Status: NEW
> Severity: normal
> Priority: P1
> Component: Netfilter/Iptables
> AssignedTo: networking_netfilter-iptables@kernel-bugs.osdl.org
> ReportedBy: joe-lk@ttdpatch.net
>
>
> Latest working kernel version: up to at least 2.6.24
> Earliest failing kernel version: first noticed with upgrade to 2.6.28
So it's a regression?
> Distribution: Debian lenny, Ubuntu 8.04
> Hardware Environment: x86
>
> Problem Description:
> An uninitialized buffer causes IPv4 addresses added manually (via the +IP
> command to the proc interface) to never match any packets. Similarly, the -IP
> command fails to remove IPv4 addresses.
>
> Details:
> In the function recent_entry_lookup, the xt_recent module does comparisons of
> the entire nf_inet_addr union value, both for IPv4 and IPv6 addresses. For
> addresses initialized from actual packets the remaining 12 bytes not occupied
> by the IPv4 are zeroed so this works correctly. However when setting the
> nf_inet_addr addr variable in the recent_mt_proc_write function, only the IPv4
> bytes are initialized and the remaining 12 bytes contain garbage.
>
> Hence addresses added in this way never match any packets, unless these
> uninitialized 12 bytes happened to be zero by coincidence. Similarly, addresses
> cannot consistently be removed using the proc interface due to mismatch of the
> garbage bytes (although it will sometimes work to remove an address that was
> added manually).
>
> Reading the /proc/net/xt_recent/ entries hides this problem because this only
> uses the first 4 bytes when displaying IPv4 addresses.
OK.
> Steps to reproduce:
> $ iptables -I INPUT -m recent --rcheck -j LOG
> $ echo +169.254.156.239 > /proc/net/xt_recent/DEFAULT
> $ cat /proc/net/xt_recent/DEFAULT
> src=169.254.156.239 ttl: 0 last_seen: 119910 oldest_pkt: 1 119910
>
> [At this point no packets from 169.254.156.239 are being logged.]
>
> $ iptables -I INPUT -s 169.254.156.239 -m recent --set
> $ cat /proc/net/xt_recent/DEFAULT
> src=169.254.156.239 ttl: 0 last_seen: 119910 oldest_pkt: 1 119910
> src=169.254.156.239 ttl: 255 last_seen: 126184 oldest_pkt: 4 125434, 125684,
> 125934, 126184
>
> [At this point, adding the address via an iptables rule, packets are being
> logged correctly.]
>
> $ echo -169.254.156.239 > /proc/net/xt_recent/DEFAULT
> $ cat /proc/net/xt_recent/DEFAULT
> src=169.254.156.239 ttl: 0 last_seen: 119910 oldest_pkt: 1 119910
> src=169.254.156.239 ttl: 255 last_seen: 126992 oldest_pkt: 10 125434, 125684,
> 125934, 126184, 126434, 126684, 126934, 126991, 126991, 126992
> $ echo -169.254.156.239 > /proc/net/xt_recent/DEFAULT
> $ cat /proc/net/xt_recent/DEFAULT
> src=169.254.156.239 ttl: 0 last_seen: 119910 oldest_pkt: 1 119910
> src=169.254.156.239 ttl: 255 last_seen: 126992 oldest_pkt: 10 125434, 125684,
> 125934, 126184, 126434, 126684, 126934, 126991, 126991, 126992
>
> [Removing the address via /proc interface failed evidently.]
>
>
> Possible solutions:
> - initialize the addr variable in recent_mt_proc_write
> - compare only 4 bytes for IPv4 addresses in recent_entry_lookup
>
> Simplest fix:
> --- linux-2.6.28.7/net/netfilter/xt_recent.c.org 2009-02-22
> 17:34:19.000000000 +0100
> +++ linux-2.6.28.7/net/netfilter/xt_recent.c 2009-02-22 17:34:21.000000000
> +0100
> @@ -544,7 +544,7 @@
> struct recent_entry *e;
> char buf[sizeof("+b335:1d35:1e55:dead:c0de:1715:5afe:c0de")];
> const char *c = buf;
> - union nf_inet_addr addr;
> + union nf_inet_addr addr = {};
> u_int16_t family;
> bool add, succ;
>
hm, that function does some pretty ugly things.
I wonder if the same bug exists elsewhere (or might do so in the
future). A more general fix would be to write a new
in6_to_nf_inet_addr() and in4_to_nf_inet_addr() which correctly
initialise the whole union.
^ permalink raw reply [flat|nested] 2+ messages in thread