[PATCH 1/3] netfilter: Fix copy_to_user too small size parametre.

[PATCH 1/3] netfilter: Fix copy_to_user too small size parametre.

[santosh nayak]

From: Santosh Nayak <santoshprasadnayak@gmail.com>

While copying to userspace, the size of source is 29byte where as
size parametre is 32 byte.  Its leaking extra-information from
kernel space to user space.
Replace EBT_FUNCTION_MAXNAMELEN by XT_EXTENSION_MAXNAMELEN.

Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
---
 net/bridge/netfilter/ebtables.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 5864cc4..f3fcbd9 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1335,7 +1335,7 @@ static inline int ebt_make_matchname(const struct ebt_entry_match *m,
     const char *base, char __user *ubase)
 {
 	char __user *hlp = ubase + ((char *)m - base);
-	if (copy_to_user(hlp, m->u.match->name, EBT_FUNCTION_MAXNAMELEN))
+	if (copy_to_user(hlp, m->u.match->name, XT_EXTENSION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }
@@ -1344,7 +1344,7 @@ static inline int ebt_make_watchername(const struct ebt_entry_watcher *w,
     const char *base, char __user *ubase)
 {
 	char __user *hlp = ubase + ((char *)w - base);
-	if (copy_to_user(hlp , w->u.watcher->name, EBT_FUNCTION_MAXNAMELEN))
+	if (copy_to_user(hlp , w->u.watcher->name, XT_EXTENSION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }
@@ -1368,7 +1368,7 @@ ebt_make_names(struct ebt_entry *e, const char *base, char __user *ubase)
 	ret = EBT_WATCHER_ITERATE(e, ebt_make_watchername, base, ubase);
 	if (ret != 0)
 		return ret;
-	if (copy_to_user(hlp, t->u.target->name, EBT_FUNCTION_MAXNAMELEN))
+	if (copy_to_user(hlp, t->u.target->name, XT_EXTENSION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }
-- 
1.7.4.4

Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre.

[Pablo Neira Ayuso]

On Thu, Mar 01, 2012 at 02:46:30PM +0530, santosh nayak wrote:
> From: Santosh Nayak <santoshprasadnayak@gmail.com>
> 
> While copying to userspace, the size of source is 29byte where as
> size parametre is 32 byte.  Its leaking extra-information from
> kernel space to user space.
> Replace EBT_FUNCTION_MAXNAMELEN by XT_EXTENSION_MAXNAMELEN.

There's no information leak.

Let me clarify this. Have a look at /linux/netfilter/x_tables.h, then
you find:

#define XT_FUNCTION_MAXNAMELEN 30
#define XT_EXTENSION_MAXNAMELEN 29
#define XT_TABLE_MAXNAMELEN 32

For iptables, everything has been 30 bytes, but we stole one
byte to store the revision field for matches/targets.

For ebtables, there's no revision field and the length of the
table name is different.

But linux/netfilter/in ebtables.h, you'll find:

#define EBT_TABLE_MAXNAMELEN 32
#define EBT_CHAIN_MAXNAMELEN EBT_TABLE_MAXNAMELEN
#define EBT_FUNCTION_MAXNAMELEN EBT_TABLE_MAXNAMELEN

Note that someone decided to use 32 bytes for the ebtables
tables/match/target name instead of 30 bytes in iptables.

Yes, it sucks a bit we have to live with these interfaces until
we have some netlink interface for all these things.

> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
> ---
>  net/bridge/netfilter/ebtables.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 5864cc4..f3fcbd9 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1335,7 +1335,7 @@ static inline int ebt_make_matchname(const struct ebt_entry_match *m,
>      const char *base, char __user *ubase)
>  {
>  	char __user *hlp = ubase + ((char *)m - base);
> -	if (copy_to_user(hlp, m->u.match->name, EBT_FUNCTION_MAXNAMELEN))
> +	if (copy_to_user(hlp, m->u.match->name, XT_EXTENSION_MAXNAMELEN))
>  		return -EFAULT;
>  	return 0;
>  }
> @@ -1344,7 +1344,7 @@ static inline int ebt_make_watchername(const struct ebt_entry_watcher *w,
>      const char *base, char __user *ubase)
>  {
>  	char __user *hlp = ubase + ((char *)w - base);
> -	if (copy_to_user(hlp , w->u.watcher->name, EBT_FUNCTION_MAXNAMELEN))
> +	if (copy_to_user(hlp , w->u.watcher->name, XT_EXTENSION_MAXNAMELEN))
>  		return -EFAULT;
>  	return 0;
>  }
> @@ -1368,7 +1368,7 @@ ebt_make_names(struct ebt_entry *e, const char *base, char __user *ubase)
>  	ret = EBT_WATCHER_ITERATE(e, ebt_make_watchername, base, ubase);
>  	if (ret != 0)
>  		return ret;
> -	if (copy_to_user(hlp, t->u.target->name, EBT_FUNCTION_MAXNAMELEN))
> +	if (copy_to_user(hlp, t->u.target->name, XT_EXTENSION_MAXNAMELEN))
>  		return -EFAULT;
>  	return 0;
>  }
> -- 
> 1.7.4.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre.

[santosh prasad nayak]

Hi Pablo.

copy_to_user( dest, source, length)

Normally,  'length'  is equal to  'sizeof (source) '.

In this case "length"   =   32
       "sizeof(source)"  =   29.

Is it intentional ?
Won't it copy extra 3 bytes of kernel data to userspace ?



regards
santosh



On Thu, Mar 1, 2012 at 3:48 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Mar 01, 2012 at 02:46:30PM +0530, santosh nayak wrote:
>> From: Santosh Nayak <santoshprasadnayak@gmail.com>
>>
>> While copying to userspace, the size of source is 29byte where as
>> size parametre is 32 byte.  Its leaking extra-information from
>> kernel space to user space.
>> Replace EBT_FUNCTION_MAXNAMELEN by XT_EXTENSION_MAXNAMELEN.
>
> There's no information leak.
>
> Let me clarify this. Have a look at /linux/netfilter/x_tables.h, then
> you find:
>
> #define XT_FUNCTION_MAXNAMELEN 30
> #define XT_EXTENSION_MAXNAMELEN 29
> #define XT_TABLE_MAXNAMELEN 32
>
> For iptables, everything has been 30 bytes, but we stole one
> byte to store the revision field for matches/targets.
>
> For ebtables, there's no revision field and the length of the
> table name is different.
>
> But linux/netfilter/in ebtables.h, you'll find:
>
> #define EBT_TABLE_MAXNAMELEN 32
> #define EBT_CHAIN_MAXNAMELEN EBT_TABLE_MAXNAMELEN
> #define EBT_FUNCTION_MAXNAMELEN EBT_TABLE_MAXNAMELEN
>
> Note that someone decided to use 32 bytes for the ebtables
> tables/match/target name instead of 30 bytes in iptables.
>
> Yes, it sucks a bit we have to live with these interfaces until
> we have some netlink interface for all these things.
>
>> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
>> ---
>>  net/bridge/netfilter/ebtables.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
>> index 5864cc4..f3fcbd9 100644
>> --- a/net/bridge/netfilter/ebtables.c
>> +++ b/net/bridge/netfilter/ebtables.c
>> @@ -1335,7 +1335,7 @@ static inline int ebt_make_matchname(const struct ebt_entry_match *m,
>>      const char *base, char __user *ubase)
>>  {
>>       char __user *hlp = ubase + ((char *)m - base);
>> -     if (copy_to_user(hlp, m->u.match->name, EBT_FUNCTION_MAXNAMELEN))
>> +     if (copy_to_user(hlp, m->u.match->name, XT_EXTENSION_MAXNAMELEN))
>>               return -EFAULT;
>>       return 0;
>>  }
>> @@ -1344,7 +1344,7 @@ static inline int ebt_make_watchername(const struct ebt_entry_watcher *w,
>>      const char *base, char __user *ubase)
>>  {
>>       char __user *hlp = ubase + ((char *)w - base);
>> -     if (copy_to_user(hlp , w->u.watcher->name, EBT_FUNCTION_MAXNAMELEN))
>> +     if (copy_to_user(hlp , w->u.watcher->name, XT_EXTENSION_MAXNAMELEN))
>>               return -EFAULT;
>>       return 0;
>>  }
>> @@ -1368,7 +1368,7 @@ ebt_make_names(struct ebt_entry *e, const char *base, char __user *ubase)
>>       ret = EBT_WATCHER_ITERATE(e, ebt_make_watchername, base, ubase);
>>       if (ret != 0)
>>               return ret;
>> -     if (copy_to_user(hlp, t->u.target->name, EBT_FUNCTION_MAXNAMELEN))
>> +     if (copy_to_user(hlp, t->u.target->name, XT_EXTENSION_MAXNAMELEN))
>>               return -EFAULT;
>>       return 0;
>>  }
>> --
>> 1.7.4.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre.

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 317 bytes --]

Actually we had this discussion before.
http://www.spinics.net/lists/netfilter-devel/msg13860.html

Here was how we ended the discussion:
http://www.spinics.net/lists/netfilter-devel/msg14083.html

I was supposed to send a patch, but I lost track.  Sorry about that.
Perhaps you can send it?

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre.

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]

On Thu, Mar 01, 2012 at 11:18:09AM +0100, Pablo Neira Ayuso wrote:
> On Thu, Mar 01, 2012 at 02:46:30PM +0530, santosh nayak wrote:
> > From: Santosh Nayak <santoshprasadnayak@gmail.com>
> > 
> > While copying to userspace, the size of source is 29byte where as
> > size parametre is 32 byte.  Its leaking extra-information from
> > kernel space to user space.
> > Replace EBT_FUNCTION_MAXNAMELEN by XT_EXTENSION_MAXNAMELEN.
> 
> There's no information leak.
> 

Where do we clear "m"? 

include/linux/netfilter/x_tables.h
   287  struct xt_match {
   288          struct list_head list;
   289  
   290          const char name[XT_EXTENSION_MAXNAMELEN];
   291          u_int8_t revision;
   292  

There is a 2 byte holes here between "revision" and "match()".  We
copy three bytes past the end of name, so we include revision and
the hole.

But maybe we memset it somewhere?  I'm not sure.

   293          /* Return true or false: return FALSE and set *hotdrop = 1 to
   294             force immediate packet drop. */
   295          /* Arguments changed since 2.6.9, as this must now handle
   296             non-linear skb, using skb_header_pointer and
   297             skb_ip_make_writable. */
   298          bool (*match)(const struct sk_buff *skb,
   299                        struct xt_action_param *);

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre.

[Pablo Neira Ayuso]

On Thu, Mar 01, 2012 at 04:15:05PM +0530, santosh prasad nayak wrote:
> Hi Pablo.
> 
> copy_to_user( dest, source, length)
> 
> Normally,  'length'  is equal to  'sizeof (source) '.
> 
> In this case "length"   =   32
>        "sizeof(source)"  =   29.
> 
> Is it intentional ?

ebtables expects 32 bytes names.

> Won't it copy extra 3 bytes of kernel data to userspace ?

You're right. We have to copy 29 bytes but we have to fill the
remaining bytes with zeroes. I think something like:

char name[EBT_FUNCTION_MAXNAMELEN] = {};

/* user-space ebtables expects 32 bytes-long names, but xt_match uses
 * 29 bytes for that. */
sprintf(name, "%s", m->u.match->name);
if (copy_to_user(hlp, name, EBT_FUNCTION_MAXNAMELEN))
...

will resolve this issue.

Would you resend a new patch?

Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre.

[Pablo Neira Ayuso]

On Thu, Mar 01, 2012 at 02:37:36PM +0300, Dan Carpenter wrote:
> On Thu, Mar 01, 2012 at 11:18:09AM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Mar 01, 2012 at 02:46:30PM +0530, santosh nayak wrote:
> > > From: Santosh Nayak <santoshprasadnayak@gmail.com>
> > > 
> > > While copying to userspace, the size of source is 29byte where as
> > > size parametre is 32 byte.  Its leaking extra-information from
> > > kernel space to user space.
> > > Replace EBT_FUNCTION_MAXNAMELEN by XT_EXTENSION_MAXNAMELEN.
> > 
> > There's no information leak.
> > 
> 
> Where do we clear "m"? 
> 
> include/linux/netfilter/x_tables.h
>    287  struct xt_match {
>    288          struct list_head list;
>    289  
>    290          const char name[XT_EXTENSION_MAXNAMELEN];
>    291          u_int8_t revision;
>    292  
> 
> There is a 2 byte holes here between "revision" and "match()".  We
> copy three bytes past the end of name, so we include revision and
> the hole.
> 
> But maybe we memset it somewhere?  I'm not sure.

xt_match instances are declared as static for each module so it's
allocated in the BSS (already zeroed), is that what you mean?

>    293          /* Return true or false: return FALSE and set *hotdrop = 1 to
>    294             force immediate packet drop. */
>    295          /* Arguments changed since 2.6.9, as this must now handle
>    296             non-linear skb, using skb_header_pointer and
>    297             skb_ip_make_writable. */
>    298          bool (*match)(const struct sk_buff *skb,
>    299                        struct xt_action_param *);
> 
> regards,
> dan carpenter
> 

Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre.

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

On Thu, Mar 01, 2012 at 02:06:37PM +0100, Pablo Neira Ayuso wrote:
> > Where do we clear "m"? 
> > 
> > include/linux/netfilter/x_tables.h
> >    287  struct xt_match {
> >    288          struct list_head list;
> >    289  
> >    290          const char name[XT_EXTENSION_MAXNAMELEN];
> >    291          u_int8_t revision;
> >    292  
> > 
> > There is a 2 byte holes here between "revision" and "match()".  We
> > copy three bytes past the end of name, so we include revision and
> > the hole.
> > 
> > But maybe we memset it somewhere?  I'm not sure.
> 
> xt_match instances are declared as static for each module so it's
> allocated in the BSS (already zeroed), is that what you mean?
> 

Yeah.  I didn't know how that worked.  Thanks.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre.

[santosh prasad nayak]

Thanks Pablo for clarification.
I will resend the patch.


regards
santosh

On Thu, Mar 1, 2012 at 6:33 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Mar 01, 2012 at 04:15:05PM +0530, santosh prasad nayak wrote:
>> Hi Pablo.
>>
>> copy_to_user( dest, source, length)
>>
>> Normally,  'length'  is equal to  'sizeof (source) '.
>>
>> In this case "length"   =   32
>>        "sizeof(source)"  =   29.
>>
>> Is it intentional ?
>
> ebtables expects 32 bytes names.
>
>> Won't it copy extra 3 bytes of kernel data to userspace ?
>
> You're right. We have to copy 29 bytes but we have to fill the
> remaining bytes with zeroes. I think something like:
>
> char name[EBT_FUNCTION_MAXNAMELEN] = {};
>
> /* user-space ebtables expects 32 bytes-long names, but xt_match uses
>  * 29 bytes for that. */
> sprintf(name, "%s", m->u.match->name);
> if (copy_to_user(hlp, name, EBT_FUNCTION_MAXNAMELEN))
> ...
>
> will resolve this issue.
>
> Would you resend a new patch?