[PATCH] xt_recent: Fix buffer overflow

[Andy Whitcroft]

On Fri, Feb 19, 2010 at 07:35:09AM -0700, Tim Gardner wrote:
> Colin Ian King wrote:
> > On Fri, 2010-02-19 at 12:42 +0200, Amit Kucheria wrote:
> >> On 10 Feb 18, Tim Gardner wrote:
> >>> If this looks right, then I'll send it upstream, and it should be a
> >>> pre-stable patch.
> >>>
> >>> rtg
> >>> -- 
> >>> Tim Gardner tim.gardner at canonical.com
> >>> From 478a6cbbd7646c78370da48677e99cc602076dd7 Mon Sep 17 00:00:00 2001
> >>> From: Tim Gardner <tim.gardner at canonical.com>
> >>> Date: Thu, 18 Feb 2010 20:04:51 -0700
> >>> Subject: [PATCH] xt_recent: Fix buffer overflow
> >>>
> >>> e->index overflows e->stamps[] every ip_pkt_list_tot
> >>> packets.
> >>>
> >>> Consider the case when ip_pkt_list_tot==1; the first packet received is stored
> >>> in e->stamps[0] and e->index is initialized to 1. The next received packet
> >>> timestamp is then stored at e->stamps[1] in recent_entry_update(),
> >>> a buffer overflow because the maximum e->stamps[] index is 0.
> >>>
> >>> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
> >>> Cc: stable at kernel.org
> >>> ---
> >>>  net/netfilter/xt_recent.c |    2 +-
> >>>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
> >>> index fc70a49..1bb0d6c 100644
> >>> --- a/net/netfilter/xt_recent.c
> >>> +++ b/net/netfilter/xt_recent.c
> >>> @@ -173,10 +173,10 @@ recent_entry_init(struct recent_table *t, const union nf_inet_addr *addr,
> >>>  
> >>>  static void recent_entry_update(struct recent_table *t, struct recent_entry *e)
> >>>  {
> >>> +	e->index %= ip_pkt_list_tot;
> >>>  	e->stamps[e->index++] = jiffies;
> >>>  	if (e->index > e->nstamps)
> >>>  		e->nstamps = e->index;
> >>> -	e->index %= ip_pkt_list_tot;
> >>>  	list_move_tail(&e->lru_list, &t->lru_list);
> >>>  }
> >>>  
> >>> -- 
> >>> 1.6.2.4
> >>>
> >> This is a little more tricky I thought.
> >>
> >> A brief look at the code tells me that e->stamps[] is supposed to store
> >> 'ip_pkt_list_tot' number of timestamps according to,
> >>
> >> 	e = kmalloc(sizeof(*e) + sizeof(e->stamps[0]) * ip_pkt_list_tot,
> >> 			    GFP_ATOMIC);
> >>
> >> And e->index is the index into the next slot to store a timestamp in. Is that
> >> correct?
> >>
> >> So, won't the kmalloc above actually assign 2 'unsigned longs' when
> >> ip_pkt_list_tot == 1, one due to sizeof(*e), the other due to
> >> sizeof(e->stamps[0]) * ip_pkt_list_tot ? If so, the original code is doing
> >> the right thing - of not letting index overflow for the _next_ call to
> >> recent_entry_update().
> > 
> > Not sure I'm with you on that Amit.  The struct contains a zero sized
> > array stamps[] - this array is exactly zero bytes in size. So the
> > kmalloc allocates just ip_pkt_list_tot number of unsigned longs.  Hence
> > when ip_pkt_list_tot == 1, only 1 unsigned long is allocated.
> > 
> > I like stefan's recommendations of:
> >  
> > e->stamps[e->index] = jiffies;
> > e->index = (e->index + 1) % ip_pkt_list_tot;
> > 
> > Colin
> >> /Amit
> >>
> 
> I wrote an example program last night while I was puzzling over this patch:
> 
> #include <stdio.h>
> struct s {
> 	int length;
> 	int array[0];
> };
> int main(int argc,char *argv[])
> {
> 	printf("sizeof(struct s) == %lu\n",sizeof(struct s));
> }
> 
> sizeof(struct s) == 4
> 
> I don't think Stefan's patch will do the right thing in
> recent_seq_show() which assumes e->index is out of bounds. I know, its
> not very robust code, but I'm only gonna change the overflow.

Note that the size is only the int at the top, there is (as one might
expect) no size associated with the array[0] element.

> >> 	e = kmalloc(sizeof(*e) + sizeof(e->stamps[0]) * ip_pkt_list_tot,
> >> 			    GFP_ATOMIC);

Now if that really is the allocation it seems wrong, though too big.  I
would have expected it to be 

	sizeof(*e) + (sizeof(e->stamps[0] * ip_pkt_list_tot))

-apw