: Netfilter fixes
Subsystems (2)
cover : Netfilter fixes
Hi Dave,
these two patches fix a missing bit on conntrack entries with master connections created through ctnetlink and some brokeness in the iptables compat code, causing it to use pointers dumped to userspace and copied back again to the kernel without any checks for validity.
Pleasy apply, thanks.
net/ipv4/netfilter/ip_tables.c | 57 +++++++-------------------------- net/netfilter/nf_conntrack_netlink.c | 4 ++- net/netfilter/x_tables.c | 8 +++- 3 files changed, 21 insertions(+), 48 deletions(-)
Pablo Neira Ayuso (1):
[NETFILTER]: ctnetlink: set expected bit for related conntracks
Patrick McHardy (1):
[NETFILTER]: ip_tables: fix compat copy race
PATCH 1/2 : ctnetlink: set expected bit for related conntracks
[NETFILTER]: ctnetlink: set expected bit for related conntracks
This patch is a fix. It sets IPS_EXPECTED for related conntracks.
---
commit 4e5080caf7901c74983fcd47af4b2789bcf9fb1c
tree b240fe271290ba1a6912abc317238385187d548f
parent 82d29bf6dc7317aeb0a3a13c2348ca8591965875
author Pablo Neira Ayuso <[email protected]> Tue, 11 Dec 2007 18:24:41 +0100 committer Patrick McHardy <[email protected]> Tue, 11 Dec 2007 18:24:41 +0100
net/netfilter/nf_conntrack_netlink.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
net/netfilter/nf_conntrack_netlink.c +3 -1
@@ -1024,8 +1024,10 @@ ctnetlink_create_conntrack(struct nlattr *cda[], }/* setup master conntrack: this is a confirmed expectation */ -if (master_ct) +if (master_ct) { +__set_bit (IPS_EXPECTED_BIT , &ct->status); ct->master = master_ct; + }add_timer (&ct->timeout);nf_conntrack_hash_insert (ct);
Re: [NETFILTER 01/02]: ctnetlink: set expected bit for related conntracks
From: Patrick McHardy <[email protected]>
Date: Tue, 11 Dec 2007 18:42:09 +0100 (MET)
Quoted 6 lines
[NETFILTER]: ctnetlink: set expected bit for related conntracks
This patch is a fix. It sets
IPS_EXPECTEDfor related conntracks.Signed-off-by Pablo Neira Ayuso <[email protected]>Signed-off-by Patrick McHardy <[email protected]>
Applied to net-2.6
PATCH 2/2 : ip_tables: fix compat copy race
In reply to Patrick McHardy[NETFILTER]: ip_tables: fix compat copy race
When copying entries to user, the kernel makes two passes through the data, first copying all the entries, then fixing up names and counters. On the second pass it copies the kernel and match data from userspace to the kernel again to find the corresponding structures, expecting that kernel pointers contained in the data are still valid.
This is obviously broken, fix by avoiding the second pass completely and fixing names and counters while dumping the ruleset, using the kernel-internal data structures.
---
commit 060d71fa0ce1d805708890701e55a40b1f15d4a7
tree ca0488fb9ec5b7c3a1f22dc851ab26f989641138
parent 4e5080caf7901c74983fcd47af4b2789bcf9fb1c
author Patrick McHardy <[email protected]> Tue, 11 Dec 2007 18:24:45 +0100 committer Patrick McHardy <[email protected]> Tue, 11 Dec 2007 18:24:45 +0100
net/ipv4/netfilter/ip_tables.c | 57 ++++++++-------------------------------- net/netfilter/x_tables.c | 8 ++++-- 2 files changed, 18 insertions(+), 47 deletions(-)
net/ipv4/netfilter/ip_tables.c +12 -45
@@ -1492,8 +1492,10 @@ static inline int compat_copy_match_to_user(struct ipt_entry_match *m,return xt_compat_match_to_user (m, dstptr, size); } -static int compat_copy_entry_to_user (struct ipt_entry *e, -void __user **dstptr,compat_uint_t *size) +static int +compat_copy_entry_to_user (struct ipt_entry *e,void __user **dstptr, +compat_uint_t *size,struct xt_counters *counters, +unsigned int *i) {struct ipt_entry_target *t;struct compat_ipt_entry __user *ce;
@@ -1507,6 +1509,9 @@ static int compat_copy_entry_to_user(struct ipt_entry *e,if (copy_to_user (ce, e,sizeof (struct ipt_entry ))) goto out; +if (copy_to_user (&ce->counters, &counters[*i],sizeof (counters[*i]))) + goto out; + *dstptr +=sizeof (struct compat_ipt_entry ); ret =IPT_MATCH_ITERATE (e, compat_copy_match_to_user, dstptr, size); target_offset = e->target_offset - (origsize - *size);
@@ -1522,6 +1527,8 @@ static int compat_copy_entry_to_user(struct ipt_entry *e, goto out;if (put_user (next_offset, &ce->next_offset)) goto out; + + (*i)++;return 0 ; out:return ret;
@@ -1937,14 +1944,13 @@ struct compat_ipt_get_entriesstatic int compat_copy_entries_to_user (unsigned int total_size,struct xt_table *table,void __user *userptr) { -unsigned int off, num; -struct compat_ipt_entry e;struct xt_counters *counters;struct xt_table_info *private = table->private;void __user *pos;unsigned int size;int ret =0 ;void *loc_cpu_entry; +unsigned int i =0 ; counters =alloc_counters (table);if (IS_ERR (counters))
@@ -1958,48 +1964,9 @@ static int compat_copy_entries_to_user(unsigned int total_size, pos = userptr; size = total_size; ret =IPT_ENTRY_ITERATE (loc_cpu_entry, total_size, - compat_copy_entry_to_user, &pos, &size); -if (ret) - goto free_counters; - -/* ... then go back and fix counters and names */ -for (off =0 , num =0 ; off < size; off += e.next_offset, num++) { -unsigned int i; -struct ipt_entry_match m; -struct ipt_entry_target t; + compat_copy_entry_to_user, + &pos, &size, counters, &i); - ret = -EFAULT ; -if (copy_from_user (&e, userptr + off, -sizeof (struct compat_ipt_entry ))) - goto free_counters; -if (copy_to_user (userptr + off + - offsetof(struct compat_ipt_entry , counters), - &counters[num],sizeof (counters[num]))) - goto free_counters; - -for (i =sizeof (struct compat_ipt_entry ); - i < e.target_offset; i += m.u.match_size) { -if (copy_from_user (&m, userptr + off + i, -sizeof (struct ipt_entry_match ))) - goto free_counters; -if (copy_to_user (userptr + off + i + - offsetof(struct ipt_entry_match, u.user.name), - m.u.kernel.match->name, - strlen(m.u.kernel.match->name) + 1)) - goto free_counters; - } - -if (copy_from_user (&t, userptr + off + e.target_offset, -sizeof (struct ipt_entry_target ))) - goto free_counters; -if (copy_to_user (userptr + off + e.target_offset + - offsetof(struct ipt_entry_target, u.user.name), - t.u.kernel.target->name, - strlen(t.u.kernel.target->name) + 1)) - goto free_counters; - } - ret =0 ; -free_counters:vfree (counters);return ret; }
net/netfilter/x_tables.c +6 -2
@@ -377,7 +377,9 @@ int xt_compat_match_to_user(struct xt_entry_match *m, void __user **dstptr,u_int16_t msize = m->u.user.match_size - off;if (copy_to_user (cm, m,sizeof (*cm)) || -put_user (msize, &cm->u.user.match_size)) +put_user (msize, &cm->u.user.match_size) || +copy_to_user (cm->u.user.name, m->u.kernel.match->name, +strlen (m->u.kernel.match->name) +1 ))return -EFAULT ;if (match->compat_to_user) {
@@ -468,7 +470,9 @@ int xt_compat_target_to_user(struct xt_entry_target *t, void __user **dstptr,u_int16_t tsize = t->u.user.target_size - off;if (copy_to_user (ct, t,sizeof (*ct)) || -put_user (tsize, &ct->u.user.target_size)) +put_user (tsize, &ct->u.user.target_size) || +copy_to_user (ct->u.user.name, t->u.kernel.target->name, +strlen (t->u.kernel.target->name) +1 ))return -EFAULT ;if (target->compat_to_user) {
David Miller
maintainerRe: [NETFILTER 02/02]: ip_tables: fix compat copy race
From: Patrick McHardy <[email protected]>
Date: Tue, 11 Dec 2007 18:42:11 +0100 (MET)
Quoted 8 lines
[NETFILTER]:
ip_tables: fix compat copy raceWhen copying entries to user, the kernel makes two passes through the data, first copying all the entries, then fixing up names and counters. On the second pass it copies the kernel and match data from userspace to the kernel again to find the corresponding structures, expecting that kernel pointers contained in the data are still valid.
Quoted 5 lines
This is obviously broken, fix by avoiding the second pass completely and fixing names and counters while dumping the ruleset, using the kernel-internal data structures.
Signed-off-by Patrick McHardy <[email protected]>
Also applied to net-2.6, thanks Patrick!